linux-kernel - Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yg802fTm0n31FC+T@B-P7TQMD6M-0146.local>
Date:   Fri, 18 Feb 2022 13:55:37 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     xfs <linux-xfs@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

Hi Darrick,

On Thu, Feb 17, 2022 at 09:40:32PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 17, 2022 at 05:55:42PM +0800, Gao Xiang wrote:
> > COW extents are already converted into written real extents after
> > xfs_reflink_convert_cow_locked(), therefore cmap->br_state should
> > reflect it.
> > 
> > Otherwise, there is another necessary unwritten convertion
> > triggered in xfs_dio_write_end_io() for direct I/O cases.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@...ux.alibaba.com>
> 
> I /think/ this looks ok.  Does it test ok too?  AFAICT nothing in the
> iomap/writeback machinery cares the incorrect state because we always
> set IOMAP_F_SHARED (which triggers COW) so I think this is simply a fix
> for directio, like you said?

Yeah, I think so, from the code logic buffered i/o seems no impacted...
And the unnecessary unwritten convertion under direct i/o takes
noticeable extra overhead in our workloads...

I checked my last night xfstests, it seems it stops unexpectedly (maybe
due to some environmental problem). I will rerun tests today and
feedback later.

Thanks,
Gao Xiang

> 
> --D
> 
> > ---
> > From the logic itself and runtime tracing, IMO, it seems true.
> > Kindly correct me here if my understanding is wrong.
> > 
> >  fs/xfs/xfs_reflink.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 75bd2e03cd5b..5f0a364739a5 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -424,7 +424,10 @@ xfs_reflink_allocate_cow(
> >  	if (!convert_now || cmap->br_state == XFS_EXT_NORM)
> >  		return 0;
> >  	trace_xfs_reflink_convert_cow(ip, cmap);
> > -	return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> > +	error = xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> > +	if (!error)
> > +		cmap->br_state = XFS_EXT_NORM;
> > +	return error;
> >  
> >  out_trans_cancel:
> >  	xfs_trans_cancel(tp);
> > -- 
> > 2.24.4
> >