linux-kernel - Re: [PATCH] xfs: make xfs_log_iovec independent from xfs_log

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240626050715.25210-1-alexjlzheng@tencent.com>
Date: Wed, 26 Jun 2024 13:07:15 +0800
From: Jinliang Zheng <alexjlzheng@...il.com>
To: hch@...radead.org
Cc: alexjlzheng@...il.com,
	alexjlzheng@...cent.com,
	chandan.babu@...cle.com,
	djwong@...nel.org,
	linux-kernel@...r.kernel.org,
	linux-xfs@...r.kernel.org
Subject: Re: [PATCH] xfs: make xfs_log_iovec independent from xfs_log_vec and release it early

On Tue, 25 Jun 2024 04:33:00 -0700, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -2526,6 +2526,8 @@ xlog_write(
> >  			xlog_write_full(lv, ticket, iclog, &log_offset,
> >  					 &len, &record_cnt, &data_cnt);
> >  		}
> > +		if (lv->lv_flags & XFS_LOG_VEC_DYNAMIC)
> > +			kvfree(lv->lv_iovecp);
> 
> This should porbably be a function paramter to xlog_write, with
> xlog_cil_write_chain asking for the iovecs to be freed because they
> are dynamically allocated, and the other two not becaue the iovecs
> are on-stack.  With that we don't need to grow a new field in
> struct xfs_log_vec.

xlog_write() will write all xfs_log_iovec on the lv chain linked list to iclog.
We seem to have no way to distinguish whether the xfs_log_iovec on the lv_chain
list is on the stack by adding new parameters to xlog_write().

> 
> >  	list_for_each_entry(lip, &tp->t_items, li_trans) {
> >  		struct xfs_log_vec *lv;
> > +		struct xfs_log_iovec *lvec;
> >  		int	niovecs = 0;
> >  		int	nbytes = 0;
> >  		int	buf_size;
> > @@ -339,18 +339,23 @@ xlog_cil_alloc_shadow_bufs(
> >  			 * the buffer, only the log vector header and the iovec
> >  			 * storage.
> >  			 */
> > -			kvfree(lip->li_lv_shadow);
> > -			lv = xlog_kvmalloc(buf_size);
> > -
> > -			memset(lv, 0, xlog_cil_iovec_space(niovecs));
> > +			if (lip->li_lv_shadow) {
> > +				kvfree(lip->li_lv_shadow->lv_iovecp);
> > +				kvfree(lip->li_lv_shadow);
> > +			}
> > +			lv = xlog_kvmalloc(sizeof(struct xfs_log_vec));
> > +			memset(lv, 0, sizeof(struct xfs_log_vec));
> > +			lvec = xlog_kvmalloc(buf_size);
> > +			memset(lvec, 0, xlog_cil_iovec_space(niovecs));
> 
> This area can use quite a bit of a redo.  The xfs_log_vec is tiny,
> so it doesn't really need a vmalloc fallback but can simply use
> kmalloc.
> 
> But more importantly there is no need to really it, you just
> need to allocate it.  So this should probably become:
> 
> 	lv = lip->li_lv_shadow;
> 	if (!lv) {
> 		/* kmalloc and initialize, set lv_size to zero */
> 	}
> 
> 	if (buf_size > lv->lv_size) {
> 		/* grow case that rallocates ->lv_iovecp */
> 	} else {
> 		/* same or smaller, optimise common overwrite case */
> 		..
> 	}

If we take the memory allocation of xfs_log_vec out of the if branch below, we
have to face the corner case of buf_size = 0.

But the release and reallocation of xfs_log_vec in this patch is indeed
redundant. I've optimized it in [PATCH v2] and [PATCH v3].

Link to [PATCH v3]:
- https://lore.kernel.org/linux-xfs/20240626044909.15060-1-alexjlzheng@tencent.com/T/#t

Thank you for your suggestion. :)
Jinliang Zheng