Re: [PATCH v3 7/7] xfs: error tag to force zeroing on debug kernels

[Brian Foster <bfoster@redhat.com>]

On Tue, Jul 15, 2025 at 07:30:41AM -0700, Darrick J. Wong wrote:
> On Tue, Jul 15, 2025 at 08:39:03AM -0400, Brian Foster wrote:
> > On Mon, Jul 14, 2025 at 10:24:44PM -0700, Darrick J. Wong wrote:
> > > On Mon, Jul 14, 2025 at 04:41:22PM -0400, Brian Foster wrote:
> > > > iomap_zero_range() has to cover various corner cases that are
> > > > difficult to test on production kernels because it is used in fairly
> > > > limited use cases. For example, it is currently only used by XFS and
> > > > mostly only in partial block zeroing cases.
> > > > 
> > > > While it's possible to test most of these functional cases, we can
> > > > provide more robust test coverage by co-opting fallocate zero range
> > > > to invoke zeroing of the entire range instead of the more efficient
> > > > block punch/allocate sequence. Add an errortag to occasionally
> > > > invoke forced zeroing.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_errortag.h |  4 +++-
> > > >  fs/xfs/xfs_error.c           |  3 +++
> > > >  fs/xfs/xfs_file.c            | 26 ++++++++++++++++++++------
> > > >  3 files changed, 26 insertions(+), 7 deletions(-)
> > > > 
> > ...
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index 0b41b18debf3..c865f9555b77 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -27,6 +27,8 @@
> > > >  #include "xfs_file.h"
> > > >  #include "xfs_aops.h"
> > > >  #include "xfs_zone_alloc.h"
> > > > +#include "xfs_error.h"
> > > > +#include "xfs_errortag.h"
> > > >  
> > > >  #include <linux/dax.h>
> > > >  #include <linux/falloc.h>
> > > > @@ -1269,13 +1271,25 @@ xfs_falloc_zero_range(
> > > >  	if (error)
> > > >  		return error;
> > > >  
> > > > -	error = xfs_free_file_space(XFS_I(inode), offset, len, ac);
> > > > -	if (error)
> > > > -		return error;
> > > > +	/*
> > > > +	 * Zero range implements a full zeroing mechanism but is only used in
> > > > +	 * limited situations. It is more efficient to allocate unwritten
> > > > +	 * extents than to perform zeroing here, so use an errortag to randomly
> > > > +	 * force zeroing on DEBUG kernels for added test coverage.
> > > > +	 */
> > > > +	if (XFS_TEST_ERROR(false, XFS_I(inode)->i_mount,
> > > > +			   XFS_ERRTAG_FORCE_ZERO_RANGE)) {
> > > > +		error = xfs_zero_range(XFS_I(inode), offset, len, ac, NULL);
> > > 
> > > Isn't this basically the ultra slow version fallback version of
> > > FALLOC_FL_WRITE_ZEROES ?
> > > 
> > 
> > ~/linux$ git grep FALLOC_FL_WRITE_ZEROES
> > ~/linux$ 
> > 
> > IIRC write zeroes is intended to expose fast hardware (physical) zeroing
> > (i.e. zeroed written extents)..? If so, I suppose you could consider
> > this a fallback of sorts. I'm not sure what write zeroes is expected to
> > do in the unwritten extent case, whereas iomap zero range is happy to
> > skip those mappings unless they're already dirty in pagecache.
> 
> Sorry, forgot that they weren't wiring anything up in xfs so it never
> showed up here:
> https://lore.kernel.org/linux-fsdevel/20250619111806.3546162-1-yi.zhang@huaweicloud.com/
> 
> Basically they want to avoid the unwritten extent conversion overhead by
> providing a way to preallocate written zeroed extents and sending magic
> commands to hardware that unmaps LBAs in such a way that rereads return
> zero.
> 

Ack.. I'd seen that before, but hadn't looked too closely and wasn't
sure what the status was.

> > Regardless, the purpose of this patch is not to add support for physical
> > zeroing, but rather to increase test coverage for the additional code on
> > debug kernels because the production use case tends to be more limited.
> > This could easily be moved/applied to write zeroes if it makes sense in
> > the future and test infra grows support for it.
> 
> <nod> On second look, I don't think the new fallocate flag allows for
> letting the kernel do pagecache zeroing + flush.  Admittedly that would
> be beside the point (and userspaces already do that anyway).
> 

Ok. Thanks for the reviews.

Brian

> Anyway enough mumbling from me,
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> --D
> 
> > Brian
> > 
> > > --D
> > > 
> > > > +	} else {
> > > > +		error = xfs_free_file_space(XFS_I(inode), offset, len, ac);
> > > > +		if (error)
> > > > +			return error;
> > > >  
> > > > -	len = round_up(offset + len, blksize) - round_down(offset, blksize);
> > > > -	offset = round_down(offset, blksize);
> > > > -	error = xfs_alloc_file_space(XFS_I(inode), offset, len);
> > > > +		len = round_up(offset + len, blksize) -
> > > > +			round_down(offset, blksize);
> > > > +		offset = round_down(offset, blksize);
> > > > +		error = xfs_alloc_file_space(XFS_I(inode), offset, len);
> > > > +	}
> > > >  	if (error)
> > > >  		return error;
> > > >  	return xfs_falloc_setsize(file, new_size);
> > > > -- 
> > > > 2.50.0
> > > > 
> > > > 
> > > 
> > 
> > 
>