Re: [PATCH v4 09/17] xfs: Factor up commit from xfs_attr_try_sf_addname

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

On Wed, Nov 06, 2019 at 06:27:53PM -0700, Allison Collins wrote:
> New delayed attribute routines cannot handle transactions,
> so factor this up to the calling function.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index dda2eba..e0a38a2 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -227,8 +227,7 @@ xfs_attr_try_sf_addname(
>  	struct xfs_da_args	*args)
>  {
>  
> -	struct xfs_mount	*mp = dp->i_mount;
> -	int			error, error2;
> +	int			error;
>  
>  	error = xfs_attr_shortform_addname(args);
>  	if (error == -ENOSPC)
> @@ -241,12 +240,7 @@ xfs_attr_try_sf_addname(
>  	if (!error && (args->name.type & ATTR_KERNOTIME) == 0)
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);

What if you moved this part (the conditional ichgtime) into
xfs_attr_shortform_addname?  Then this function can just go away.

>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args->trans);
> -
> -	error2 = xfs_trans_commit(args->trans);
> -	args->trans = NULL;
> -	return error ? error : error2;
> +	return error;
>  }
>  
>  /*
> @@ -258,7 +252,7 @@ xfs_attr_set_args(
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_buf          *leaf_bp = NULL;
> -	int			error;
> +	int			error, error2 = 0;;
>  
>  	/*
>  	 * If the attribute list is non-existent or a shortform list,
> @@ -278,8 +272,15 @@ xfs_attr_set_args(
>  		 * Try to add the attr to the attribute list in the inode.
>  		 */
>  		error = xfs_attr_try_sf_addname(dp, args);
> -		if (error != -ENOSPC)
> -			return error;
> +		if (error != -ENOSPC) {
> +			if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC)
> +				xfs_trans_set_sync(args->trans);
> +
> +			error2 = xfs_trans_commit(args->trans);
> +			args->trans = NULL;
> +			return error ? error : error2;

Can error be something other than 0 or EEXIST?  If so, does it make
sense to commit even in those cases?  (Have I asked this before...?) It
looks odd to me that we'd commit the transaction even if something
handed back EFSCORRUPTED.

Hm, it's a local attr fork so I guess the only possible error is ENOSPC?
If that's true then please add a comment/ASSERT to that effect.

--D

> +		}
> +
>  
>  		/*
>  		 * It won't fit in the shortform, transform to a leaf block.
> -- 
> 2.7.4
>