Re: [PATCH v4 15/17] xfs: Check for -ENOATTR or -EEXIST

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

On Fri, Nov 08, 2019 at 02:42:52PM -0700, Allison Collins wrote:
> On 11/8/19 2:28 PM, Darrick J. Wong wrote:
> > On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote:
> > > Delayed operations cannot return error codes.  So we must check for
> > > these conditions first before starting set or remove operations
> > > 
> > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > ---
> > >   fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++
> > >   1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 5dcb19f..626d4a98 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -458,6 +458,27 @@ xfs_attr_set(
> > >   		goto out_trans_cancel;
> > >   	xfs_trans_ijoin(args.trans, dp, 0);
> > > +
> > > +	error = xfs_has_attr(&args);
> > > +	if (error == -EEXIST) {
> > > +		if (name->type & ATTR_CREATE)
> > > +			goto out_trans_cancel;
> > > +		else
> > > +			name->type |= ATTR_REPLACE;
> > > +	}
> > > +
> > > +	if (error == -ENOATTR && (name->type & ATTR_REPLACE))
> > > +		goto out_trans_cancel;
> > > +
> > > +	if (name->type & ATTR_REPLACE) {
> > > +		name->type &= ~ATTR_REPLACE;
> > > +		error = xfs_attr_remove_args(&args);
> > > +		if (error)
> > > +			goto out_trans_cancel;
> > > +
> > > +		name->type |= ATTR_CREATE;
> > 
> > I thought _set_args already handled the remove part of replacing an
> > attr?
> No, IIRC in one of the other reviews we decided to break the rename into a
> set and then a remove.

But this looks like we remove the old attr before setting the new one,
which means that if we crash right here we'll come back up with no attr
at all.  The INCOMPLETE flag flipping trick only works for ATTR_REPLACE
if you add the new attr before removing the old one.

(Or am I misreading something here?)

> That way the error handling moves up here instead of
> trying to deal with it in the middle of the delayed operation

Sounds good.

> > And I thought that it did this with an atomic rename?  Won't this
> > break the atomicity of attr replacement?

> Hmm, think this worked for delayed operations, but not anymore since we're
> going back to supporting both delayed and inline in one code path.  I think
> what this means is that the flip flag has to get moved in here, right?  We
> flip on the incomplete flag before the remove and then set it when the
> rename is done?

Yeah.

--D

> 
> > 
> > --D
> > 
> > > +	}
> > > +
> > >   	error = xfs_attr_set_args(&args);
> > >   	if (error)
> > >   		goto out_trans_cancel;
> > > @@ -543,6 +564,10 @@ xfs_attr_remove(
> > >   	 */
> > >   	xfs_trans_ijoin(args.trans, dp, 0);
> > > +	error = xfs_has_attr(&args);
> > > +	if (error == -ENOATTR)
> > > +		goto out;
> > > +
> > >   	error = xfs_attr_remove_args(&args);
> > >   	if (error)
> > >   		goto out;
> > > -- 
> > > 2.7.4
> > >