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

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

[Christoph Hellwig]

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));
+			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

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

[Carlos Maiolino]

> 
> +/*
> + * For various operations we need to zero up to one block each at each end of

							^^^
					Is this correct? Or should have it been
					"one block at each end..." ?

Not native English speaker so just double checking. I can update it when
applying if it is not correct.

Other than that:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> + * 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));
> +			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
> 

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

[Christoph Hellwig]

On Tue, Dec 16, 2025 at 09:03:42AM +0100, Carlos Maiolino wrote:
> > 
> > +/*
> > + * For various operations we need to zero up to one block each at each end of
> 
> 							^^^
> 					Is this correct? Or should have it been
> 					"one block at each end..." ?
> 
> Not native English speaker so just double checking. I can update it when
> applying if it is not correct.

Your version sounds correct.

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

[Carlos Maiolino]

On Tue, Dec 16, 2025 at 09:06:18AM +0100, Christoph Hellwig wrote:
> On Tue, Dec 16, 2025 at 09:03:42AM +0100, Carlos Maiolino wrote:
> > >
> > > +/*
> > > + * For various operations we need to zero up to one block each at each end of
> >
> > 							^^^
> > 					Is this correct? Or should have it been
> > 					"one block at each end..." ?
> >
> > Not native English speaker so just double checking. I can update it when
> > applying if it is not correct.
> 
> Your version sounds correct.
> 

o>

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

[Darrick J. Wong]

On Tue, Dec 16, 2025 at 09:11:13AM +0100, Carlos Maiolino wrote:
> On Tue, Dec 16, 2025 at 09:06:18AM +0100, Christoph Hellwig wrote:
> > On Tue, Dec 16, 2025 at 09:03:42AM +0100, Carlos Maiolino wrote:
> > > >
> > > > +/*
> > > > + * For various operations we need to zero up to one block each at each end of
> > >
> > > 							^^^
> > > 					Is this correct? Or should have it been
> > > 					"one block at each end..." ?
> > >
> > > Not native English speaker so just double checking. I can update it when
> > > applying if it is not correct.
> > 
> > Your version sounds correct.
> > 
> 
> o>

The original version is correct.  If you have 4k fsblocks and a
zerorange request for 8GB - 2 bytes starting at 4097, you'll have to
actually zero 4097-8191 and 8589930496 - 8589934591.  (Almost) one block
each, at each end.

That said, "one block at each end" conveys that adquately in modern day
English usage and people think I'm old fashioned even for uttering the
first version. ;)

(Old fashioned being 1960s :P)

--D

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

[Christoph Hellwig]

On Tue, Dec 16, 2025 at 07:54:31AM -0800, Darrick J. Wong wrote:
> The original version is correct.  If you have 4k fsblocks and a
> zerorange request for 8GB - 2 bytes starting at 4097, you'll have to
> actually zero 4097-8191 and 8589930496 - 8589934591.  (Almost) one block
> each, at each end.
> 
> That said, "one block at each end" conveys that adquately in modern day
> English usage and people think I'm old fashioned even for uttering the
> first version. ;)
> 
> (Old fashioned being 1960s :P)

You're the only native speaker, so I'll defer to you.  A little
old-school is always good anyway.

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

[Brian Foster]

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

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

[Christoph Hellwig]

On Tue, Dec 16, 2025 at 01:57:43PM -0500, Brian Foster wrote:
> > +	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.

I really think that anyone who modidifies this area should run a debug
kernel to test.  And if they the usual automated runs will catch it.
Having allocation beahvior modified based on CONFIG_XFS_DEBUG, and only
for a case that isn't supposed to happen seems weird and in would cause
weird heisenbugs if it ever hit.

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

[Brian Foster]

On Fri, Dec 19, 2025 at 06:28:03AM +0100, Christoph Hellwig wrote:
> On Tue, Dec 16, 2025 at 01:57:43PM -0500, Brian Foster wrote:
> > > +	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.
> 
> I really think that anyone who modidifies this area should run a debug
> kernel to test.  And if they the usual automated runs will catch it.
> Having allocation beahvior modified based on CONFIG_XFS_DEBUG, and only
> for a case that isn't supposed to happen seems weird and in would cause
> weird heisenbugs if it ever hit.
> 

I agree on testing but XFS_DEBUG has always included some quirks that
alter algorithms and such to improve test coverage in this way. The
subtlety here is that this will only "catch it" on XFS_WARN (i.e.
!XFS_DEBUG). Otherwise this will quietly do forced zeroing. The
userspace tests should ideally pass because iomap zeroing is generally
expected to work, so there's no presumed test failure to rely on to
detect this. The assert won't fire unless XFS_WARN because either
XFS_DEBUG is enabled (so the assert passes) or if on a production
kernel, then the assert is compiled out.

I think if we broke it we'd eventually catch it one way or another, such
as if anybody is testing XFS_WARN regularly(?), but that loose end was
my reasoning for at least enforcing that production kernels run as
expected no matter what. Alternatively if we changed it to WARN_ON_ONCE
then that might provide more of a guarantee. But again just my .02.

Brian