Re: [PATCH v6 11/16] xfs: Check for -ENOATTR or -EEXIST

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sun, Jan 26, 2020 at 05:20:16PM -0700, Allison Collins wrote:
> 
> 
> On 1/26/20 3:28 PM, Darrick J. Wong wrote:
> > On Sat, Jan 25, 2020 at 09:41:47AM -0700, Allison Collins wrote:
> > > On 1/21/20 9:29 PM, Allison Collins wrote:
> > > > 
> > > > 
> > > > On 1/21/20 4:15 PM, Darrick J. Wong wrote:
> > > > > On Sat, Jan 18, 2020 at 03:50:30PM -0700, Allison Collins wrote:
> > > > > > Delayed operations cannot return error codes.  So we must check for
> > > > > > these conditions first before starting set or remove operations
> > > > > 
> > > > > Answering my own question from earlier -- I see here you actually /are/
> > > > > checking the attr existence w.r.t. ATTR_{CREATE,REPLACE} right after we
> > > > > allocate a transaction and ILOCK the inode, so
> > > > > 
> > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > Alrighty, thank you!
> > > > 
> > > > > 
> > > > > Though I am wondering if you could discard the predicates from the
> > > > > second patch in favor of doing a normal lookup of the attr with a zero
> > > > > valuelen to determine if there's already an attribute?
> > > > I think I likely answered this in the response to that patch.  Because
> > > > it's used as part of the remove procedures, we still need it.  We could
> > > > make a simpler version just for this application I suppose, but it seems
> > > > like it'd just be extra code since we still need the former.
> > > > 
> > > > Thank you for the reviews!
> > > > Allison
> > > > 
> > > > > 
> > > > > --D
> > > > > 
> > > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > > > > ---
> > > > > >    fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
> > > > > >    1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > > > index a2673fe..e9d22c1 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > > > @@ -457,6 +457,14 @@ xfs_attr_set(
> > > > > >            goto out_trans_cancel;
> > > > > >        xfs_trans_ijoin(args.trans, dp, 0);
> > > > > > +
> > > > > > +    error = xfs_has_attr(&args);
> > > > > > +    if (error == -EEXIST && (name->type & ATTR_CREATE))
> > > > > > +        goto out_trans_cancel;
> > > > > > +
> > > > > > +    if (error == -ENOATTR && (name->type & ATTR_REPLACE))
> > > > > > +        goto out_trans_cancel;
> > > > > > +
> > > > > >        error = xfs_attr_set_args(&args);
> > > So I was thinking of adding this one to a smaller 3 patch series I mentioned
> > > earlier.  I was also thinking of adding in some asserts here:
> > > 
> > > ASSERT(error != -EEXIST)
> > > ASSERT(error != -ENOATTR)
> > > 
> > > Just to make sure the changes are enforcing the behavioral changes that we
> > > want.  I thought this might be a good stabilizer to the rest of the delayed
> > > attr series.  Because chasing this bug back up through the log replay is a
> > > much bigger PITA than catching it here.  Thoughts?
> > 
> > Er, are the asserts to check that xfs_attr_set_args never returns
> > EEXIST/ENOATTR?  I'm not sure why you'd have to chase this through log
> > replay?
> 
> Yes, the idea is that EEXIST and ENOATTR are supposed to be found and
> returned by the xfs_has_attr routine above. If they happen at this point, it
> would actually be more of an internal error.  For example: if we're renaming
> an attr, and xfs_has_attr finds that it exists, but then xfs_attr_set_args
> comes back with ENOATTR, clearly something unexpected happened.
> 
> The motivation for this is just that if it does happen, it's easier to work
> out this out now rather than later when we bring in the rest of the delayed
> attribute code.  Because in that case, the error wont happen here, it will
> happen later as part of a finish_item (or a log replay).
> 
> It's not a requirement I suppose, just more of a pro-active check really.

Ok, it's purely a defensive check so that you'll notice problems early
before the fs goes bonkers, not a problem that mysteriously appears but
only after some random unexpected shutdown.

> > 
> > /me is in this funny place where he thinks that in general adding
> > asserts (or WARN_ON) to check assumptions is a good idea, but not sure
> > what's going on here.
> Did that answer your question then?

Yep.

--D

> > 
> > --D
> > 
> > > > > >        if (error)
> > > > > >            goto out_trans_cancel;
> > > > > > @@ -545,6 +553,10 @@ xfs_attr_remove(
> > > > > >         */
> > > > > >        xfs_trans_ijoin(args.trans, dp, 0);
> > > > > > +    error = xfs_has_attr(&args);
> > > > > > +    if (error != -EEXIST)
> > > > > > +        goto out;
> > > > > > +
> > > Here too:
> > > ASSERT(error != -EEXIST)
> > > 
> > > Let me know what folks think.  Thanks!
> > > 
> > > Allison
> > > 
> > > > > >        error = xfs_attr_remove_args(&args);
> > > > > >        if (error)
> > > > > >            goto out;
> > > > > > -- 
> > > > > > 2.7.4
> > > > > >