Re: [PATCH 05/18] xfs: separate out initial attr_set states

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 05/18] xfs: separate out initial attr_set states
Date: Wed, 11 May 2022 11:06:51 +1000	[thread overview]
Message-ID: <20220511010651.GZ1098723@dread.disaster.area> (raw)
In-Reply-To: <20220510231234.GI27195@magnolia>

On Tue, May 10, 2022 at 04:12:34PM -0700, Darrick J. Wong wrote:
> On Mon, May 09, 2022 at 10:41:25AM +1000, Dave Chinner wrote:
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index c9c867e3406c..ad52b5dc59e4 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -530,4 +553,35 @@ void xfs_attri_destroy_cache(void);
> >  int __init xfs_attrd_init_cache(void);
> >  void xfs_attrd_destroy_cache(void);
> >  
> > +/*
> > + * Check to see if the attr should be upgraded from non-existent or shortform to
> > + * single-leaf-block attribute list.
> > + */
> > +static inline bool
> > +xfs_attr_is_shortform(
> > +	struct xfs_inode    *ip)
> > +{
> > +	return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL ||
> > +	       (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS &&
> > +		ip->i_afp->if_nextents == 0);
> > +}
> > +
> > +static inline enum xfs_delattr_state
> > +xfs_attr_init_add_state(struct xfs_da_args *args)
> > +{
> > +	if (!args->dp->i_afp)
> > +		return XFS_DAS_DONE;
> 
> If we're in add/replace attr call without an attr fork, why do we go
> straight to finished?

I suspect I've fixed all the issues that triggered crashes here
because args->dp->i_afp was null. THere were transient states in a
replace operaiton when the remove takes away the last attr, removes
the attr fork, then calls the ADD operation. The add operation
assumes that the attr fork has already been set up, and so bad
things happened here.

This also occurred when setting up recovery operations - recovery of
an add/replace could start from that same "there's no attr fork"
condition, and so calling xfs_inode_has_attr() or
xfs_attr_is_shortform() direct from the reocovery setup code would
go splat because ip->i_afp was null.

I'm going to leave this for the moment (cleanup note made) because I
don't want to have to find out that I missed a corner case somewhere
they hard way right now. It's basically there to stop log recovery
crashing hard, which only occurs when the experimental larp code is
running, so I think this is safe to leave for a later cleanup.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com