Re: [PATCH v17 05/11] xfs: Separate xfs_attr_node_addname and xfs_attr_node_addname_clear_incomplete

[Chandan Babu R <chandanrlinux@gmail.com>]

On 16 Apr 2021 at 14:50, Allison Henderson wrote:
> This patch separate xfs_attr_node_addname into two functions.  This will
> help to make it easier to hoist parts of xfs_attr_node_addname that need
> state management
>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index fff1c6f..d9dfc8d2 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>  STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> +STATIC int xfs_attr_node_addname_clear_incomplete(struct xfs_da_args *args);
>  STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>  				 struct xfs_da_state **state);
>  STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> @@ -1062,6 +1063,25 @@ xfs_attr_node_addname(
>  			return error;
>  	}
>
> +	error = xfs_attr_node_addname_clear_incomplete(args);
> +out:
> +	if (state)
> +		xfs_da_state_free(state);
> +	if (error)
> +		return error;
> +	return retval;

I think the above code incorrectly returns -ENOSPC when the user is performing
an xattr rename operation and the call to xfs_attr3_leaf_add() resulted in
returning -ENOSPC,
1. xfs_attr3_leaf_add() returns -ENOSPC.
2. xfs_da3_split() allocates a new leaf and inserts the new xattr into it.
3. If the user was performing a rename operation (i.e. XFS_DA_OP_RENAME is
   set), we flip the "incomplete" flag.
4. Remove the old xattr's remote blocks (if any).
5. Remove old xattr's name.
6. If "error" has zero as its value, we return the value of "retval". At this
   point in execution, "retval" would have -ENOSPC as its value.

--
chandan