Re: [PATCH v16 03/11] xfs: Hoist xfs_attr_set_shortform

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

On 3/29/21 2:21 AM, Chandan Babu R wrote:
> On 26 Mar 2021 at 06:03, Allison Henderson wrote:
>> This patch hoists xfs_attr_set_shortform into the calling function. This
>> will help keep all state management code in the same scope.
>>
> 
> That looks simple enough.
> 
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Thanks!
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 | 81 ++++++++++++++++--------------------------------
>>   1 file changed, 27 insertions(+), 54 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 32c7447..5216f67 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -217,53 +217,6 @@ xfs_attr_is_shortform(
>>   }
>>   
>>   /*
>> - * Attempts to set an attr in shortform, or converts short form to leaf form if
>> - * there is not enough room.  If the attr is set, the transaction is committed
>> - * and set to NULL.
>> - */
>> -STATIC int
>> -xfs_attr_set_shortform(
>> -	struct xfs_da_args	*args,
>> -	struct xfs_buf		**leaf_bp)
>> -{
>> -	struct xfs_inode	*dp = args->dp;
>> -	int			error, error2 = 0;
>> -
>> -	/*
>> -	 * Try to add the attr to the attribute list in the inode.
>> -	 */
>> -	error = xfs_attr_try_sf_addname(dp, args);
>> -	if (error != -ENOSPC) {
>> -		error2 = xfs_trans_commit(args->trans);
>> -		args->trans = NULL;
>> -		return error ? error : error2;
>> -	}
>> -	/*
>> -	 * It won't fit in the shortform, transform to a leaf block.  GROT:
>> -	 * another possible req'mt for a double-split btree op.
>> -	 */
>> -	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>> -	if (error)
>> -		return error;
>> -
>> -	/*
>> -	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
>> -	 * push cannot grab the half-baked leaf buffer and run into problems
>> -	 * with the write verifier. Once we're done rolling the transaction we
>> -	 * can release the hold and add the attr to the leaf.
>> -	 */
>> -	xfs_trans_bhold(args->trans, *leaf_bp);
>> -	error = xfs_defer_finish(&args->trans);
>> -	xfs_trans_bhold_release(args->trans, *leaf_bp);
>> -	if (error) {
>> -		xfs_trans_brelse(args->trans, *leaf_bp);
>> -		return error;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -/*
>>    * Set the attribute specified in @args.
>>    */
>>   int
>> @@ -272,7 +225,7 @@ xfs_attr_set_args(
>>   {
>>   	struct xfs_inode	*dp = args->dp;
>>   	struct xfs_buf          *leaf_bp = NULL;
>> -	int			error = 0;
>> +	int			error2, error = 0;
>>   
>>   	/*
>>   	 * If the attribute list is already in leaf format, jump straight to
>> @@ -281,16 +234,36 @@ xfs_attr_set_args(
>>   	 * again.
>>   	 */
>>   	if (xfs_attr_is_shortform(dp)) {
>> +		/*
>> +		 * Try to add the attr to the attribute list in the inode.
>> +		 */
>> +		error = xfs_attr_try_sf_addname(dp, args);
>> +		if (error != -ENOSPC) {
>> +			error2 = xfs_trans_commit(args->trans);
>> +			args->trans = NULL;
>> +			return error ? error : error2;
>> +		}
>> +
>> +		/*
>> +		 * It won't fit in the shortform, transform to a leaf block.
>> +		 * GROT: another possible req'mt for a double-split btree op.
>> +		 */
>> +		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
>> +		if (error)
>> +			return error;
>>   
>>   		/*
>> -		 * If the attr was successfully set in shortform, the
>> -		 * transaction is committed and set to NULL.  Otherwise, is it
>> -		 * converted from shortform to leaf, and the transaction is
>> -		 * retained.
>> +		 * Prevent the leaf buffer from being unlocked so that a
>> +		 * concurrent AIL push cannot grab the half-baked leaf buffer
>> +		 * and run into problems with the write verifier.
>>   		 */
>> -		error = xfs_attr_set_shortform(args, &leaf_bp);
>> -		if (error || !args->trans)
>> +		xfs_trans_bhold(args->trans, leaf_bp);
>> +		error = xfs_defer_finish(&args->trans);
>> +		xfs_trans_bhold_release(args->trans, leaf_bp);
>> +		if (error) {
>> +			xfs_trans_brelse(args->trans, leaf_bp);
>>   			return error;
>> +		}
>>   	}
>>   
>>   	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> 
>