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@xxxxxxxxxxxxxxxxx> > > 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 > >
