Re: [PATCH v16 06/11] xfs: Add helper xfs_attr_node_addname_find_attr

[Allison Henderson <allison.henderson@oracle.com>]

On 4/1/21 9:05 PM, Chandan Babu R wrote:
> On 26 Mar 2021 at 06:03, Allison Henderson wrote:
>> This patch separates the first half of xfs_attr_node_addname into a
>> helper function xfs_attr_node_addname_find_attr.  It also replaces the
>> restart goto with with an EAGAIN return code driven by a loop in the
>> calling function.  This looks odd now, but will clean up nicly once we
>> introduce the state machine.  It will also enable hoisting the last
>> state out of xfs_attr_node_addname with out having to plumb in a "done"
>> parameter to know if we need to move to the next state or not.
>>
> 
> Looks good to me.
> 
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Thanks for the reviews!
Allison

> 
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 86 ++++++++++++++++++++++++++++++------------------
>>   1 file changed, 54 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 531ff56..16159f6 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -52,7 +52,10 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>    * Internal routines when attribute list is more than one block.
>>    */
>>   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_addname(struct xfs_da_args *args,
>> +				 struct xfs_da_state *state);
>> +STATIC int xfs_attr_node_addname_find_attr(struct xfs_da_args *args,
>> +				 struct xfs_da_state **state);
>>   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,
>> @@ -267,6 +270,7 @@ xfs_attr_set_args(
>>   	struct xfs_da_args	*args)
>>   {
>>   	struct xfs_inode	*dp = args->dp;
>> +	struct xfs_da_state     *state;
>>   	int			error;
>>   
>>   	/*
>> @@ -312,7 +316,14 @@ xfs_attr_set_args(
>>   			return error;
>>   	}
>>   
>> -	return xfs_attr_node_addname(args);
>> +	do {
>> +		error = xfs_attr_node_addname_find_attr(args, &state);
>> +		if (error)
>> +			return error;
>> +		error = xfs_attr_node_addname(args, state);
>> +	} while (error == -EAGAIN);
>> +
>> +	return error;
>>   }
>>   
>>   /*
>> @@ -885,47 +896,26 @@ xfs_attr_node_hasname(
>>    * External routines when attribute list size > geo->blksize
>>    *========================================================================*/
>>   
>> -/*
>> - * Add a name to a Btree-format attribute list.
>> - *
>> - * This will involve walking down the Btree, and may involve splitting
>> - * leaf nodes and even splitting intermediate nodes up to and including
>> - * the root node (a special case of an intermediate node).
>> - *
>> - * "Remote" attribute values confuse the issue and atomic rename operations
>> - * add a whole extra layer of confusion on top of that.
>> - */
>>   STATIC int
>> -xfs_attr_node_addname(
>> -	struct xfs_da_args	*args)
>> +xfs_attr_node_addname_find_attr(
>> +	struct xfs_da_args	*args,
>> +	struct xfs_da_state     **state)
>>   {
>> -	struct xfs_da_state	*state;
>> -	struct xfs_da_state_blk	*blk;
>> -	struct xfs_inode	*dp;
>> -	int			retval, error;
>> -
>> -	trace_xfs_attr_node_addname(args);
>> +	int			retval;
>>   
>>   	/*
>> -	 * Fill in bucket of arguments/results/context to carry around.
>> -	 */
>> -	dp = args->dp;
>> -restart:
>> -	/*
>>   	 * Search to see if name already exists, and get back a pointer
>>   	 * to where it should go.
>>   	 */
>> -	retval = xfs_attr_node_hasname(args, &state);
>> +	retval = xfs_attr_node_hasname(args, state);
>>   	if (retval != -ENOATTR && retval != -EEXIST)
>> -		goto out;
>> +		goto error;
>>   
>> -	blk = &state->path.blk[ state->path.active-1 ];
>> -	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>>   	if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
>> -		goto out;
>> +		goto error;
>>   	if (retval == -EEXIST) {
>>   		if (args->attr_flags & XATTR_CREATE)
>> -			goto out;
>> +			goto error;
>>   
>>   		trace_xfs_attr_node_replace(args);
>>   
>> @@ -943,6 +933,38 @@ xfs_attr_node_addname(
>>   		args->rmtvaluelen = 0;
>>   	}
>>   
>> +	return 0;
>> +error:
>> +	if (*state)
>> +		xfs_da_state_free(*state);
>> +	return retval;
>> +}
>> +
>> +/*
>> + * Add a name to a Btree-format attribute list.
>> + *
>> + * This will involve walking down the Btree, and may involve splitting
>> + * leaf nodes and even splitting intermediate nodes up to and including
>> + * the root node (a special case of an intermediate node).
>> + *
>> + * "Remote" attribute values confuse the issue and atomic rename operations
>> + * add a whole extra layer of confusion on top of that.
>> + */
>> +STATIC int
>> +xfs_attr_node_addname(
>> +	struct xfs_da_args	*args,
>> +	struct xfs_da_state	*state)
>> +{
>> +	struct xfs_da_state_blk	*blk;
>> +	struct xfs_inode	*dp;
>> +	int			retval, error;
>> +
>> +	trace_xfs_attr_node_addname(args);
>> +
>> +	dp = args->dp;
>> +	blk = &state->path.blk[state->path.active-1];
>> +	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>> +
>>   	retval = xfs_attr3_leaf_add(blk->bp, state->args);
>>   	if (retval == -ENOSPC) {
>>   		if (state->path.active == 1) {
>> @@ -968,7 +990,7 @@ xfs_attr_node_addname(
>>   			if (error)
>>   				goto out;
>>   
>> -			goto restart;
>> +			return -EAGAIN;
>>   		}
>>   
>>   		/*
> 
>