Re: [PATCH v9 02/28] xfs: Add helper function xfs_attr_try_sf_addname

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 05, 2018 at 08:01:19PM -0700, Darrick J. Wong wrote:
> On Wed, Sep 26, 2018 at 03:14:41AM -0700, Allison Henderson wrote:
> > This patch adds a subroutine xfs_attr_try_sf_addname
> > used by xfs_attr_set.  This subrotine will attempt to
> > add the attribute name specified in args in shortform,
> > as well and perform error handling previously done in
> > xfs_attr_set.
> > 
> > This patch helps to pre-simplify xfs_attr_set for reviewing
> > purposes and reduce indentation.  New function will be added
> > in the next patch.
> > 
> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c | 61 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 40 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1e671d4..f0675be 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -191,6 +191,42 @@ xfs_attr_calc_size(
> >  	return nblks;
> >  }
> >  
> > +STATIC int
> > +xfs_attr_try_sf_addname(
> > +	struct xfs_inode	*dp,
> > +	struct xfs_da_args	*args)
> > +{
> > +
> > +	struct xfs_mount	*mp = dp->i_mount;
> > +	int			error;
> > +
> > +	error = xfs_attr_shortform_addname(args);
> 
> I wonder, what error codes can come out of this function?  -ENOSPC,
> -EEXIST, zero...?

-ENOATTR, -EFSCORRUPTED, -EIO, etc.

ENOSPC here says "attr doesn't fit in shortform, try leaf format",
while every other error needs to be handled as a shortform addition
error.

> 
> > +	if (error == -ENOSPC)
> > +		return error;
> > +
> > +	/*
> > +	 * Commit the shortform mods, and we're done.
> > +	 * NOTE: this is also the error path
> > +	 * (EEXIST, etc).
> > +	 */
> > +	ASSERT(args->trans != NULL);
> 
> <urk> Um... I get that this is just mechanical code refactoring, but...
> 
> > +
> > +	/*
> > +	 * If this is a synchronous mount, make sure
> > +	 * that the transaction goes to disk before
> > +	 * returning to the user.
> > +	 */
> > +	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > +		xfs_trans_set_sync(args->trans);
> 
> ...let's say we're on the error path.  Why would we care about making the
> transaction synchronous?

Because we've already modified the inode to add an attr fork, and
we have to obey WSYNC rules for inode changes whether the attr was
added or not.

> > +	if (!error && (args->flags & ATTR_KERNOTIME) == 0) {
> > +		xfs_trans_ichgtime(args->trans, dp,
> > +				XFS_ICHGTIME_CHG);
> > +	}
> 
> And what's the point of updating ctime if we do?

This is only run if there wasn't an error - it's skipped if we
didn't add the attribute to the inode.

> And can we restructure this to follow the usual pattern:
> 
> error = xfs_blabhablhablah(...);
> if (error)
> 	bail out;
> 
> <keep going>

Not quite that simple, some errors still require us to commit the
transaction. And if we've got a fatal error, then the commit will
fail anyway.

> >  			err2 = xfs_trans_commit(args.trans);
> > -			xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > +			error = error ? error : err2;
> >  
> > -			return error ? error : err2;
> > +			xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > +			return error;
> 
> Huh, so we commit the transaction on error?? Why?

So we don't shut down the filesystem by cancelling a dirty
transaction on an -EEXIST or -ENOATTR error after we added the attr
fork to the inode.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com