Re: [PATCH] xfs: fix XFS_ERRTAG_FORCE_ZERO_RANGE for zoned file system

[Brian Foster <bfoster@redhat.com>]

On Fri, Dec 12, 2025 at 08:39:37AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 10, 2025 at 12:14:35PM -0500, Brian Foster wrote:
> > Well yeah, it would look something like this at the current site:
> > 
> > 	if (!is_inode_zoned() && XFS_TEST_ERROR(...) ||
> > 	    ac->reserved_blocks == magic_default_res + len)
> > 		xfs_zero_range(...);
> > 	else
> > 		xfs_free_file_space(...);
> > 
> > ... and the higher level zoned code would clone the XFS_TEST_ERROR() to
> > create the block reservation condition to trigger it.
> > 
> > Alternatively perhaps you could make that check look something like:
> > 
> > 	if (XFS_TEST_ERROR() && (!ac || ac->res > len))
> > 		...
> > 	else
> > 		...
> 
> I had to juggle this a bit to not trigger the wrong way and add a
> helper.  The changes are a bit bigger than the original version,
> but I guess you'll probably prefer it because it keeps things more
> contained in the zoned code?
> 

Thanks for taking a stab at this. I agree that the whole indirect logic
trigger based on res thing is a wart/tradeoff, but even still I think I
like this better probably for the reasons you stated. It feels more
encapsulated, and is still limited to DEBUG mode so doesn't worry me as
much.

A few minor comments below, but otherwise if this works for you and
there aren't strong opinions to the contrary:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 6108612182e2..d70c8e0d802b 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1240,6 +1240,28 @@ xfs_falloc_insert_range(
>  	return xfs_insert_file_space(XFS_I(inode), offset, len);
>  }
>  
> +#define XFS_ZONED_ZERO_RANGE_SPACE_RES		2
> +

This 2 block res isn't purely a zero range thing, right? It looks like
it's for a few different falloc ops.. perhaps ZONED_FALLOC_SPACE_RES (or
whatever else that is less zero specific)..?

> +/*
> + * 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.
> + *
> + * On zoned file systems, the error is already injected by
> + * xfs_file_zoned_fallocate, which then reserves the additional space needed.
> + * We only check for this extra space reservation here.
> + */
> +static inline bool
> +xfs_falloc_force_zero(
> +	struct xfs_inode		*ip,
> +	struct xfs_zone_alloc_ctx	*ac)
> +{
> +	if (ac)
> +		return ac->reserved_blocks > XFS_ZONED_ZERO_RANGE_SPACE_RES;

Random thought: I wonder if doing something like:

	if (!IS_ENABLED(CONFIG_XFS_DEBUG))
		return false;

... in this helper would shore up the logic a bit? Just a bit of
defensive logic against the indirection since the helper already exists.
I also wonder if that would help the compiler optimize this out on
!DEBUG builds.

> +	return XFS_TEST_ERROR(ip->i_mount, XFS_ERRTAG_FORCE_ZERO_RANGE);
> +}
> +
>  /*
>   * Punch a hole and prealloc the range.  We use a hole punch rather than
>   * unwritten extent conversion for two reasons:
...
> @@ -1423,13 +1438,26 @@ xfs_file_zoned_fallocate(
>  {
>  	struct xfs_zone_alloc_ctx ac = { };
>  	struct xfs_inode	*ip = XFS_I(file_inode(file));
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_filblks_t		count_fsb;
>  	int			error;
>  
> -	error = xfs_zoned_space_reserve(ip->i_mount, 2, XFS_ZR_RESERVED, &ac);
> +	/*
> +	 * If full zeroing is forced by the error injection nob, we need a space

s/nob/knob/ ;)

Thanks!

Brian