Re: [PATCH v7 13/19] xfs: Add delay ready attr remove routines

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 25, 2020 at 04:57:46PM -0800, Allison Collins wrote:
> On 2/25/20 1:57 AM, Dave Chinner wrote:
> > On Sat, Feb 22, 2020 at 07:06:05PM -0700, Allison Collins wrote:
> > > +out:
> > > +	return error;
> > > +}
> > 
> > Brian commented on the structure of this loop better than I could.
> > 
> > > +
> > > +/*
> > > + * Remove the attribute specified in @args.
> > > + *
> > > + * This function may return -EAGAIN to signal that the transaction needs to be
> > > + * rolled.  Callers should continue calling this function until they receive a
> > > + * return value other than -EAGAIN.
> > > + */
> > > +int
> > > +xfs_attr_remove_iter(
> > >   	struct xfs_da_args      *args)
> > >   {
> > >   	struct xfs_inode	*dp = args->dp;
> > >   	int			error;
> > > +	/* State machine switch */
> > > +	switch (args->dac.dela_state) {
> > > +	case XFS_DAS_RM_SHRINK:
> > > +	case XFS_DAS_RMTVAL_REMOVE:
> > > +		goto node;
> > > +	default:
> > > +		break;
> > > +	}
> > 
> > Why separate out the state machine? Doesn't this shortcut the
> > xfs_inode_hasattr() check? Shouldn't that come first?
> Well, the idea is that when we first start the routine, we come in with
> neither state set, and we fall through to the break.  So we execute the
> check the first time through.
> 
> Though now that you point it out, I should probably go back and put the
> explicit numbering back in the enum (starting with 1) or they will default
> to zero, which would be incorrect.  I had pulled it out in one of the last
> reviews thinking it would be ok, but it should go back in.
> 
> > 
> > As it is:
> > 
> > 	case XFS_DAS_RM_SHRINK:
> > 	case XFS_DAS_RMTVAL_REMOVE:
> > 		return xfs_attr_node_removename(args);
> > 	default:
> > 		break;
> > 
> > would be nicer, and if this is the only way we can get to
> > xfs_attr_node_removename(c, getting rid of it from the code
> > below could be done, too.
> Well, the remove path is a lot simpler than the set path, so that trick does
> work here :-)
> 
> The idea though was to establish "jump points" with the "XFS_DAS_*" states.
> Based on the state, we jump back to where we were.  We could break this
> pattern for the remove path, but I dont think we'd want to do the same for
> the others.  The set routine is a really big function that would end up
> being inside a really big switch!

Right, which is why I think it should be factored into function
calls first, then the switch statement simply becomes a small set of
function calls.

We use that pattern quite a bit in the da_btree code to call
the correct dir/attr function based on the type of block we are
manipulating (i.e. based on da_state context). e.g. xfs_da3_split(),
xfs_da3_join(), etc.

> > >   	struct xfs_da_geometry *geo;	/* da block geometry */
> > >   	struct xfs_name	name;		/* name, length and argument  flags*/
> > >   	uint8_t		filetype;	/* filetype of inode for directories */
> > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > > index 1887605..9a649d1 100644
> > > --- a/fs/xfs/scrub/common.c
> > > +++ b/fs/xfs/scrub/common.c
> > > @@ -24,6 +24,8 @@
> > >   #include "xfs_rmap_btree.h"
> > >   #include "xfs_log.h"
> > >   #include "xfs_trans_priv.h"
> > > +#include "xfs_da_format.h"
> > > +#include "xfs_da_btree.h"
> > >   #include "xfs_attr.h"
> > >   #include "xfs_reflink.h"
> > >   #include "scrub/scrub.h"
> > 
> > Hmmm - why are these new includes necessary? You didn't add anything
> > new to these files or common header files to make the includes
> > needed....
> 
> Because the delayed attr context uses things from those headers.  And we put
> the context in xfs_da_args.  Now everything that uses xfs_da_args needs
> those includes.  But maybe if we do what you suggest above, we wont need to.
> :-)

put:

struct xfs_da_state;

and whatever other forward declarations are require for the pointer
types used in the delayed attr context at the top of xfs_attr.h.

These are just pointers in the structure, so we don't need the full
structure definitions if the pointers aren't actually dereferenced
by the code that includes the header file.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com