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@xxxxxxxxxx>
> > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx/
>
> 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@xxxxxxxxxx>
>
> --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
> > > >
> > > >
> > >
> >
> >
>
