Re: [PATCH 04/18] xfs: rework deferred attribute operation setup

[Dave Chinner <david@fromorbit.com>]

On Tue, May 10, 2022 at 04:04:51PM -0700, Darrick J. Wong wrote:
> On Mon, May 09, 2022 at 10:41:24AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Logged attribute intents only have set and remove types - there is
> > no type of the replace operation. We should ahve a separate type for
> 
> "..no separate intent type for a replace operation."?
> 
> Also, "ahve" -> "have".
> 
> > a replace operation, as it needs to perform operations that neither
> > SET or REMOVE can perform.
> > 
> > Add this type to the intent items and rearrange the deferred
> > operation setup to reflect the different operations we are
> > performing.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       | 165 +++++++++++++++++++--------------
> >  fs/xfs/libxfs/xfs_attr.h       |   2 -
> >  fs/xfs/libxfs/xfs_log_format.h |   1 +
> >  fs/xfs/xfs_attr_item.c         |   9 +-
> >  fs/xfs/xfs_trace.h             |   4 +
> >  5 files changed, 110 insertions(+), 71 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index a4b0b20a3bab..817e15740f9c 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -671,6 +671,81 @@ xfs_attr_lookup(
> >  	return xfs_attr_node_hasname(args, NULL);
> >  }
> >  
> > +static int
> > +xfs_attr_item_init(
> > +	struct xfs_da_args	*args,
> > +	unsigned int		op_flags,	/* op flag (set or remove) */
> > +	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
> > +{
> > +
> > +	struct xfs_attr_item	*new;
> > +
> > +	new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
> 
> Can this fail?
> 
> > +	new->xattri_op_flags = op_flags;
> > +	new->xattri_da_args = args;
> > +
> > +	*attr = new;
> > +	return 0;
> 
> And if it can't, then there's no need for a return value, AFAICT.  I
> looked at the end of xfs-5.19-compose and there's no other return
> statements in this function.
> 
> And then you don't need any of the error handling in this patch at all,
> right?
> 
> *OH*, this is just a hoist of the stuff at the end.  Could someone add a
> patch on the end of ... whatever patchset there is to clean that up?
> 
> In the meantime, with the commit message typos cleaned up,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks.

I'm making notes of all the things that need cleaning up. there's a
few things already, that probably won't happen until the next cycle
at this point...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com