[PATCH] xfs: fix recovery of partial attr removals

[PATCH] xfs: fix recovery of partial attr removals

[Christoph Hellwig]

When running xfs/753 on parent pointer-enabled file systems with DEBUG
enabled, log recovery will eventually crash after a few dozen iterations
due to an ASSERT that XFS_ATTR_INCOMPLETE is not already set when
xfs_attr3_leaf_setflag is called:

XFS: Assertion failed: (entry->flags & XFS_ATTR_INCOMPLETE) == 0, file: fs/xfs/libxfs/xfs_attr_leaf.c, line: 3045

This is because parent pointer use logged attributes, and the attribute
intents do not track the work that has been done as the transaction
rolls, and the recovery mechanism is to run the operation from scratch.
Because of this various places check for XFS_DA_OP_RECOVERY skip work
already done.  But while recovery tolerates an attribute already fully
removed, it currently trips the above assert for an attribute that
already has the INCOMPLETE bit set, but still exists.  Fix this by
handling the case like all the others.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---

This really wants a Fixes tag and/or a cc stable, but I have a really
hard time coming up with a good one.  The logged attrs are a messy
history with tons of refactoring, but this actually code goes back way
beyond that, but wasn't a problem before the attr item recovery.  Logged
attrs themselves are still only a debug option and not used by default
except for the parent pointer.  Any good suggestions?

 fs/xfs/libxfs/xfs_attr_leaf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 6068fad9dc9e..82932b5c02fd 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -3042,7 +3042,14 @@ xfs_attr3_leaf_setflag(
 #endif
 	entry = &xfs_attr3_leaf_entryp(leaf)[args->index];
 
-	ASSERT((entry->flags & XFS_ATTR_INCOMPLETE) == 0);
+	if (entry->flags & XFS_ATTR_INCOMPLETE) {
+		if (!(args->op_flags & XFS_DA_OP_RECOVERY)) {
+			ASSERT(args->op_flags & XFS_DA_OP_RECOVERY);
+			return -EFSCORRUPTED;
+		}
+		return 0;
+	}
+
 	entry->flags |= XFS_ATTR_INCOMPLETE;
 	xfs_trans_log_buf(args->trans, bp,
 			XFS_DA_LOGRANGE(leaf, entry, sizeof(*entry)));
-- 
2.53.0

Re: [PATCH] xfs: fix recovery of partial attr removals

[Carlos Maiolino]

On Wed, Oct 28, 2026 at 09:09:39AM +0100, Christoph Hellwig wrote:
> When running xfs/753 on parent pointer-enabled file systems with DEBUG
> enabled, log recovery will eventually crash after a few dozen iterations
> due to an ASSERT that XFS_ATTR_INCOMPLETE is not already set when
> xfs_attr3_leaf_setflag is called:
> 
> XFS: Assertion failed: (entry->flags & XFS_ATTR_INCOMPLETE) == 0, file: fs/xfs/libxfs/xfs_attr_leaf.c, line: 3045
> 
> This is because parent pointer use logged attributes, and the attribute
> intents do not track the work that has been done as the transaction
> rolls, and the recovery mechanism is to run the operation from scratch.
> Because of this various places check for XFS_DA_OP_RECOVERY skip work
> already done.  But while recovery tolerates an attribute already fully
> removed, it currently trips the above assert for an attribute that
> already has the INCOMPLETE bit set, but still exists.  Fix this by
> handling the case like all the others.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> This really wants a Fixes tag and/or a cc stable, but I have a really
> hard time coming up with a good one.  The logged attrs are a messy
> history with tons of refactoring, but this actually code goes back way
> beyond that, but wasn't a problem before the attr item recovery.  Logged
> attrs themselves are still only a debug option and not used by default
> except for the parent pointer.  Any good suggestions?

Uff.. I think this goes waaaay back, even before CRC?!

Giving this wasn't a proble before LARP, I'd probably be inclined to set
a Fixes pointing towards 1d08e11d04d2.
Not because it imminently fixes that patch, but that the broken behavior
kind of started after that. Cc stable probably v5.19. I don't think
there is any point before that.

For the patch though:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
>  fs/xfs/libxfs/xfs_attr_leaf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 6068fad9dc9e..82932b5c02fd 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -3042,7 +3042,14 @@ xfs_attr3_leaf_setflag(
>  #endif
>  	entry = &xfs_attr3_leaf_entryp(leaf)[args->index];
>  
> -	ASSERT((entry->flags & XFS_ATTR_INCOMPLETE) == 0);
> +	if (entry->flags & XFS_ATTR_INCOMPLETE) {
> +		if (!(args->op_flags & XFS_DA_OP_RECOVERY)) {
> +			ASSERT(args->op_flags & XFS_DA_OP_RECOVERY);
> +			return -EFSCORRUPTED;
> +		}
> +		return 0;
> +	}
> +
>  	entry->flags |= XFS_ATTR_INCOMPLETE;
>  	xfs_trans_log_buf(args->trans, bp,
>  			XFS_DA_LOGRANGE(leaf, entry, sizeof(*entry)));
> -- 
> 2.53.0
> 
> 

Re: [PATCH] xfs: fix recovery of partial attr removals

[Christoph Hellwig]

On Mon, Nov 02, 2026 at 12:52:38PM +0100, Carlos Maiolino wrote:
> Uff.. I think this goes waaaay back, even before CRC?!

The logic yes.  I don't think the bug was there.  The other interesting
bit is that back in the day the INCOMPLETE flag was only ever set when
removing a remote attribute.  I think this made sense, as there's no
point in setting it for a local form one.

> Giving this wasn't a proble before LARP, I'd probably be inclined to set
> a Fixes pointing towards 1d08e11d04d2.
>
> Not because it imminently fixes that patch, but that the broken behavior
> kind of started after that. Cc stable probably v5.19. I don't think
> there is any point before that.

Fine with me if no one chouts.  I assume you'll just add it?

Re: [PATCH] xfs: fix recovery of partial attr removals

[Carlos Maiolino]

On Tue, Nov 03, 2026 at 08:49:53AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 02, 2026 at 12:52:38PM +0100, Carlos Maiolino wrote:
> > Uff.. I think this goes waaaay back, even before CRC?!
> 
> The logic yes.  I don't think the bug was there.  The other interesting
> bit is that back in the day the INCOMPLETE flag was only ever set when
> removing a remote attribute.  I think this made sense, as there's no
> point in setting it for a local form one.
> 
> > Giving this wasn't a proble before LARP, I'd probably be inclined to set
> > a Fixes pointing towards 1d08e11d04d2.
> >
> > Not because it imminently fixes that patch, but that the broken behavior
> > kind of started after that. Cc stable probably v5.19. I don't think
> > there is any point before that.
> 
> Fine with me if no one chouts.  I assume you'll just add it?

Yeah, I can do that for you assuming nobody else chimes in.

Re: [PATCH] xfs: fix recovery of partial attr removals

[Carlos Maiolino]

On Wed, 28 Oct 2026 09:09:39 +0100, Christoph Hellwig wrote:
> When running xfs/753 on parent pointer-enabled file systems with DEBUG
> enabled, log recovery will eventually crash after a few dozen iterations
> due to an ASSERT that XFS_ATTR_INCOMPLETE is not already set when
> xfs_attr3_leaf_setflag is called:
> 
> XFS: Assertion failed: (entry->flags & XFS_ATTR_INCOMPLETE) == 0, file: fs/xfs/libxfs/xfs_attr_leaf.c, line: 3045
> 
> [...]

Applied to for-next, thanks!

[1/1] xfs: fix recovery of partial attr removals
      commit: 30bb61651d2da5a502cf504bcdad16c093c1bb08

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>