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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Allison Collins <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v6 11/16] xfs: Check for -ENOATTR or -EEXIST
Date: Sun, 26 Jan 2020 14:28:20 -0800	[thread overview]
Message-ID: <20200126222820.GL3447196@magnolia> (raw)
In-Reply-To: <26a49cf4-52df-55bd-67bb-9c0c981a860d@oracle.com>

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?

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