[PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[Gao Xiang]

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(-)

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

Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[Darrick J. Wong]

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
> 

Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[Gao Xiang]

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

Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[Gao Xiang]

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

Re: [PATCH] xfs: add missing cmap->br_state = XFS_EXT_NORM update

[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