diff mbox series

[3/6] xfs: distinguish extra split from real ENOSPC from xfs_attr3_leaf_split

Message ID 20240820170517.528181-4-hch@lst.de (mailing list archive)
State Superseded, archived
Headers show
Series [1/6] xfs: merge xfs_attr_leaf_try_add into xfs_attr_leaf_addname | expand

Commit Message

Christoph Hellwig Aug. 20, 2024, 5:04 p.m. UTC
xfs_attr3_leaf_split propagates the need for an extra btree split as
-ENOSPC to it's only caller, but the same return value can also be
returned from xfs_da_grow_inode when it fails to find free space.

Distinguish the two cases by returning 1 for the extra split case instead
of overloading -ENOSPC.

This can be triggered relatively easily with the pending realtime group
support and a file system with a lot of small zones that use metadata
space on the main device.  In this case every about 5-10th run of
xfs/538 runs into the following assert:

	ASSERT(oldblk->magic == XFS_ATTR_LEAF_MAGIC);

in xfs_attr3_leaf_split caused by an allocation failure.  Note that
the allocation failure is caused by another bug that will be fixed
subsequently, but this commit at least sorts out the error handling.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++++-
 fs/xfs/libxfs/xfs_da_btree.c  | 5 +++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Darrick J. Wong Aug. 21, 2024, 3:57 p.m. UTC | #1
On Tue, Aug 20, 2024 at 07:04:54PM +0200, Christoph Hellwig wrote:
> xfs_attr3_leaf_split propagates the need for an extra btree split as
> -ENOSPC to it's only caller, but the same return value can also be
> returned from xfs_da_grow_inode when it fails to find free space.
> 
> Distinguish the two cases by returning 1 for the extra split case instead
> of overloading -ENOSPC.
> 
> This can be triggered relatively easily with the pending realtime group
> support and a file system with a lot of small zones that use metadata
> space on the main device.  In this case every about 5-10th run of
> xfs/538 runs into the following assert:
> 
> 	ASSERT(oldblk->magic == XFS_ATTR_LEAF_MAGIC);
> 
> in xfs_attr3_leaf_split caused by an allocation failure.  Note that
> the allocation failure is caused by another bug that will be fixed
> subsequently, but this commit at least sorts out the error handling.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice cleanup
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++++-
>  fs/xfs/libxfs/xfs_da_btree.c  | 5 +++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index bcaf28732bfcae..92acef51876e4b 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1334,6 +1334,9 @@ xfs_attr3_leaf_create(
>  
>  /*
>   * Split the leaf node, rebalance, then add the new entry.
> + *
> + * Returns 0 if the entry was added, 1 if a further split is needed or a
> + * negative error number otherwise.
>   */
>  int
>  xfs_attr3_leaf_split(
> @@ -1390,7 +1393,7 @@ xfs_attr3_leaf_split(
>  	oldblk->hashval = xfs_attr_leaf_lasthash(oldblk->bp, NULL);
>  	newblk->hashval = xfs_attr_leaf_lasthash(newblk->bp, NULL);
>  	if (!added)
> -		return -ENOSPC;
> +		return 1;
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 16a529a8878083..17d9e6154f1978 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -593,9 +593,8 @@ xfs_da3_split(
>  		switch (oldblk->magic) {
>  		case XFS_ATTR_LEAF_MAGIC:
>  			error = xfs_attr3_leaf_split(state, oldblk, newblk);
> -			if ((error != 0) && (error != -ENOSPC)) {
> +			if (error < 0)
>  				return error;	/* GROT: attr is inconsistent */
> -			}
>  			if (!error) {
>  				addblk = newblk;
>  				break;
> @@ -617,6 +616,8 @@ xfs_da3_split(
>  				error = xfs_attr3_leaf_split(state, newblk,
>  							    &state->extrablk);
>  			}
> +			if (error == 1)
> +				return -ENOSPC;
>  			if (error)
>  				return error;	/* GROT: attr inconsistent */
>  			addblk = newblk;
> -- 
> 2.43.0
> 
>
Christoph Hellwig Aug. 22, 2024, 3:40 a.m. UTC | #2
On Wed, Aug 21, 2024 at 08:57:36AM -0700, Darrick J. Wong wrote:
> > in xfs_attr3_leaf_split caused by an allocation failure.  Note that
> > the allocation failure is caused by another bug that will be fixed
> > subsequently, but this commit at least sorts out the error handling.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Nice cleanup

It's not even supposed to clean things up, just to fix the incorrect
error handling..
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index bcaf28732bfcae..92acef51876e4b 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1334,6 +1334,9 @@  xfs_attr3_leaf_create(
 
 /*
  * Split the leaf node, rebalance, then add the new entry.
+ *
+ * Returns 0 if the entry was added, 1 if a further split is needed or a
+ * negative error number otherwise.
  */
 int
 xfs_attr3_leaf_split(
@@ -1390,7 +1393,7 @@  xfs_attr3_leaf_split(
 	oldblk->hashval = xfs_attr_leaf_lasthash(oldblk->bp, NULL);
 	newblk->hashval = xfs_attr_leaf_lasthash(newblk->bp, NULL);
 	if (!added)
-		return -ENOSPC;
+		return 1;
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 16a529a8878083..17d9e6154f1978 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -593,9 +593,8 @@  xfs_da3_split(
 		switch (oldblk->magic) {
 		case XFS_ATTR_LEAF_MAGIC:
 			error = xfs_attr3_leaf_split(state, oldblk, newblk);
-			if ((error != 0) && (error != -ENOSPC)) {
+			if (error < 0)
 				return error;	/* GROT: attr is inconsistent */
-			}
 			if (!error) {
 				addblk = newblk;
 				break;
@@ -617,6 +616,8 @@  xfs_da3_split(
 				error = xfs_attr3_leaf_split(state, newblk,
 							    &state->extrablk);
 			}
+			if (error == 1)
+				return -ENOSPC;
 			if (error)
 				return error;	/* GROT: attr inconsistent */
 			addblk = newblk;