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

[Brian Foster <bfoster@redhat.com>]

On Mon, Dec 15, 2025 at 07:05:46AM +0100, Christoph Hellwig wrote:
> The new XFS_ERRTAG_FORCE_ZERO_RANGE error tag added by commit
> ea9989668081 ("xfs: error tag to force zeroing on debug kernels") fails
> to account for the zoned space reservation rules and this reliably fails
> xfs/131 because the zeroing operation returns -EIO.
> 
> Fix this by reserving enough space to zero the entire range, which
> requires a bit of (fairly ugly) reshuffling to do the error injection
> early enough to affect the space reservation.
> 
> Fixes: ea9989668081 ("xfs: error tag to force zeroing on debug kernels")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Changes since v1:
>  - only do early injection for zoned mode to declutter the non-zoned
>    path a bit
> 
>  fs/xfs/xfs_file.c | 58 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 6108612182e2..8f753ad284a0 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1240,6 +1240,38 @@ xfs_falloc_insert_range(
>  	return xfs_insert_file_space(XFS_I(inode), offset, len);
>  }
>  
> +/*
> + * For various operations we need to zero up to one block each at each end of
> + * the affected range.  For zoned file systems this will require a space
> + * allocation, for which we need a reservation ahead of time.
> + */
> +#define XFS_ZONED_ZERO_EDGE_SPACE_RES		2
> +
> +/*
> + * 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 (xfs_is_zoned_inode(ip)) {
> +		if (ac->reserved_blocks > XFS_ZONED_ZERO_EDGE_SPACE_RES) {
> +			ASSERT(IS_ENABLED(CONFIG_XFS_DEBUG));

JFYI the reason I suggested a config check was as a safeguard against
forced zeroing on production kernels. The assert here would compile out
in that case, so won't necessarily provide that benefit (unless you
wanted to use ASSERT_ALWAYS() or WARN() or something..).

A warning on WARN && !DEBUG is still useful so I don't really care if
you leave it as is or tweak it. I just wanted to point that out.

Brian

> +			return true;
> +		}
> +		return false;
> +	}
> +	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:
> @@ -1268,14 +1300,7 @@ xfs_falloc_zero_range(
>  	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(ip->i_mount,
> -			   XFS_ERRTAG_FORCE_ZERO_RANGE)) {
> +	if (xfs_falloc_force_zero(ip, ac)) {
>  		error = xfs_zero_range(ip, offset, len, ac, NULL);
>  	} else {
>  		error = xfs_free_file_space(ip, offset, len, ac);
> @@ -1423,13 +1448,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 knob, we need a
> +	 * space reservation that covers the entire range.  See the comment in
> +	 * xfs_zoned_write_space_reserve for the rationale for the calculation.
> +	 * Otherwise just reserve space for the two boundary blocks.
> +	 */
> +	count_fsb = XFS_ZONED_ZERO_EDGE_SPACE_RES;
> +	if ((mode & FALLOC_FL_MODE_MASK) == FALLOC_FL_ZERO_RANGE &&
> +	    XFS_TEST_ERROR(mp, XFS_ERRTAG_FORCE_ZERO_RANGE))
> +		count_fsb += XFS_B_TO_FSB(mp, len) + 1;
> +
> +	error = xfs_zoned_space_reserve(mp, count_fsb, XFS_ZR_RESERVED, &ac);
>  	if (error)
>  		return error;
>  	error = __xfs_file_fallocate(file, mode, offset, len, &ac);
> -	xfs_zoned_space_unreserve(ip->i_mount, &ac);
> +	xfs_zoned_space_unreserve(mp, &ac);
>  	return error;
>  }
>  
> -- 
> 2.47.3
> 
>