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?
/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.
--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
> > > >
