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

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

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

> +	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?

> +
> +	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?

And can we restructure this to follow the usual pattern:

error = xfs_blabhablhablah(...);
if (error)
	bail out;

<keep going>

> +
> +	return error;
> +}
> +
>  int
>  xfs_attr_set(
>  	struct xfs_inode	*dp,
> @@ -281,30 +317,13 @@ xfs_attr_set(
>  		 * Try to add the attr to the attribute list in
>  		 * the inode.
>  		 */
> -		error = xfs_attr_shortform_addname(&args);
> +		error = xfs_attr_try_sf_addname(dp, &args);
>  		if (error != -ENOSPC) {
> -			/*
> -			 * Commit the shortform mods, and we're done.
> -			 * NOTE: this is also the error path (EEXIST, etc).
> -			 */
> -			ASSERT(args.trans != NULL);
> -
> -			/*
> -			 * 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);
> -
> -			if (!error && (flags & ATTR_KERNOTIME) == 0) {
> -				xfs_trans_ichgtime(args.trans, dp,
> -							XFS_ICHGTIME_CHG);
> -			}
>  			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?

/me runs away

--D

>  		}
>  
>  		/*
> -- 
> 2.7.4
>