| Message ID | 20220217095542.68085-1-hsiangkao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: add missing cmap->br_state = XFS_EXT_NORM update | expand |
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@linux.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? --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 >
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@linux.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 > >
On Fri, Feb 18, 2022 at 01:55:37PM +0800, Gao Xiang wrote: > 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@linux.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. On my test environment machine, with linux 5.17-rc4, both w/ and w/o the patch, it fails: generic/594 generic/600 xfs/158 xfs/160 I think no issue with this patch. Thanks, Gao Xiang
On Fri, Feb 18, 2022 at 07:30:42PM +0800, Gao Xiang wrote: > On Fri, Feb 18, 2022 at 01:55:37PM +0800, Gao Xiang wrote: > > 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@linux.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. > > On my test environment machine, with linux 5.17-rc4, > both w/ and w/o the patch, it fails: > > generic/594 generic/600 xfs/158 xfs/160 > > I think no issue with this patch. ping.. it seems a simple fix for direct I/O.. Thanks, Gao Xiang > > Thanks, > Gao Xiang
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);
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@linux.alibaba.com> --- 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(-)