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@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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
> > > > > >
