Re: [PATCH v9 16/24] xfs: Add remote block helper functions

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

On 5/4/20 11:55 AM, Darrick J. Wong wrote:
> On Thu, Apr 30, 2020 at 03:50:08PM -0700, Allison Collins wrote:
>> This patch adds two new helper functions xfs_attr_store_rmt_blk and
>> xfs_attr_restore_rmt_blk. These two helpers assist to remove redundant
>> code associated with storing and retrieving remote blocks during the
>> attr set operations.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
>> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> Hmm.  This is an ok-enough refactoring of the same idiom repeated over
> and over, but ...ugh, there has got to be a less weird way of pushing
> and popping state like this.
> 
> Unfortunately, the only thing I can think of would be to pass a "which
> attr state?" argument to all the attr functions, and that might just
> make things worse, particularly since we already have da state that gets
> passed around everywhere, and afaict there's on a few users of this.
> 
Hmm, ok, I'll give it some thought.  I just spotted it as a sort of 
clean up while trying to organize things.

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Alrighty, thank you!

Allison
> 
> --D
> 
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index df77a3c..feae122 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -563,6 +563,30 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
>>    * External routines when attribute list is one block
>>    *========================================================================*/
>>   
>> +/* Store info about a remote block */
>> +STATIC void
>> +xfs_attr_save_rmt_blk(
>> +	struct xfs_da_args	*args)
>> +{
>> +	args->blkno2 = args->blkno;
>> +	args->index2 = args->index;
>> +	args->rmtblkno2 = args->rmtblkno;
>> +	args->rmtblkcnt2 = args->rmtblkcnt;
>> +	args->rmtvaluelen2 = args->rmtvaluelen;
>> +}
>> +
>> +/* Set stored info about a remote block */
>> +STATIC void
>> +xfs_attr_restore_rmt_blk(
>> +	struct xfs_da_args	*args)
>> +{
>> +	args->blkno = args->blkno2;
>> +	args->index = args->index2;
>> +	args->rmtblkno = args->rmtblkno2;
>> +	args->rmtblkcnt = args->rmtblkcnt2;
>> +	args->rmtvaluelen = args->rmtvaluelen2;
>> +}
>> +
>>   /*
>>    * Tries to add an attribute to an inode in leaf form
>>    *
>> @@ -597,11 +621,7 @@ xfs_attr_leaf_try_add(
>>   
>>   		/* save the attribute state for later removal*/
>>   		args->op_flags |= XFS_DA_OP_RENAME;	/* an atomic rename */
>> -		args->blkno2 = args->blkno;		/* set 2nd entry info*/
>> -		args->index2 = args->index;
>> -		args->rmtblkno2 = args->rmtblkno;
>> -		args->rmtblkcnt2 = args->rmtblkcnt;
>> -		args->rmtvaluelen2 = args->rmtvaluelen;
>> +		xfs_attr_save_rmt_blk(args);
>>   
>>   		/*
>>   		 * clear the remote attr state now that it is saved so that the
>> @@ -700,11 +720,8 @@ xfs_attr_leaf_addname(
>>   		 * Dismantle the "old" attribute/value pair by removing
>>   		 * a "remote" value (if it exists).
>>   		 */
>> -		args->index = args->index2;
>> -		args->blkno = args->blkno2;
>> -		args->rmtblkno = args->rmtblkno2;
>> -		args->rmtblkcnt = args->rmtblkcnt2;
>> -		args->rmtvaluelen = args->rmtvaluelen2;
>> +		xfs_attr_restore_rmt_blk(args);
>> +
>>   		if (args->rmtblkno) {
>>   			error = xfs_attr_rmtval_invalidate(args);
>>   			if (error)
>> @@ -929,11 +946,7 @@ xfs_attr_node_addname(
>>   
>>   		/* save the attribute state for later removal*/
>>   		args->op_flags |= XFS_DA_OP_RENAME;	/* atomic rename op */
>> -		args->blkno2 = args->blkno;		/* set 2nd entry info*/
>> -		args->index2 = args->index;
>> -		args->rmtblkno2 = args->rmtblkno;
>> -		args->rmtblkcnt2 = args->rmtblkcnt;
>> -		args->rmtvaluelen2 = args->rmtvaluelen;
>> +		xfs_attr_save_rmt_blk(args);
>>   
>>   		/*
>>   		 * clear the remote attr state now that it is saved so that the
>> @@ -1045,11 +1058,8 @@ xfs_attr_node_addname(
>>   		 * Dismantle the "old" attribute/value pair by removing
>>   		 * a "remote" value (if it exists).
>>   		 */
>> -		args->index = args->index2;
>> -		args->blkno = args->blkno2;
>> -		args->rmtblkno = args->rmtblkno2;
>> -		args->rmtblkcnt = args->rmtblkcnt2;
>> -		args->rmtvaluelen = args->rmtvaluelen2;
>> +		xfs_attr_restore_rmt_blk(args);
>> +
>>   		if (args->rmtblkno) {
>>   			error = xfs_attr_rmtval_invalidate(args);
>>   			if (error)
>> -- 
>> 2.7.4
>>