Re: [PATCH v3 07/19] xfs: Factor out xfs_attr_leaf_addname helper

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

On 9/20/19 6:49 AM, Brian Foster wrote:
> On Thu, Sep 05, 2019 at 03:18:25PM -0700, Allison Collins wrote:
>> Factor out new helper function xfs_attr_leaf_try_add.
>> Because new delayed attribute routines cannot roll
>> transactions, we carve off the parts of
>> xfs_attr_leaf_addname that we can use.  This will help
>> to reduce repetitive code later when we introduce
>> delayed attributes.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 43 +++++++++++++++++++++++++++++--------------
>>   1 file changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 7a6dd37..f27e2c6 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -593,19 +593,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
>>    * External routines when attribute list is one block
>>    *========================================================================*/
>>   
>> -/*
>> - * Add a name to the leaf attribute list structure
>> - *
>> - * This leaf block cannot have a "remote" value, we only call this routine
>> - * if bmap_one_block() says there is only one block (ie: no remote blks).
>> - */
>>   STATIC int
>> -xfs_attr_leaf_addname(
>> -	struct xfs_da_args	*args)
>> +xfs_attr_leaf_try_add(
>> +	struct xfs_da_args	*args,
>> +	struct xfs_buf		*bp)
>>   {
>> -	struct xfs_buf		*bp;
>> -	int			retval, error, forkoff;
>> -	struct xfs_inode	*dp = args->dp;
>> +	int			retval, error;
> 
> It looks like we could pick either retval or error and use it
> consistently throughout the new function.
> 
>>   
>>   	trace_xfs_attr_leaf_addname(args);
>>   
> 
> I also wonder if this tracepoint should remain in the caller.

Alrihty, will clean out retval and move trace point

> 
>> @@ -650,13 +643,35 @@ xfs_attr_leaf_addname(
>>   	retval = xfs_attr3_leaf_add(bp, args);
>>   	if (retval == -ENOSPC) {
>>   		/*
>> -		 * Promote the attribute list to the Btree format, then
>> -		 * Commit that transaction so that the node_addname() call
>> -		 * can manage its own transactions.
>> +		 * Promote the attribute list to the Btree format.
>>   		 */
>>   		error = xfs_attr3_leaf_to_node(args);
>>   		if (error)
>>   			return error;
>> +	}
>> +	return retval;
>> +}
>> +
>> +
>> +/*
>> + * Add a name to the leaf attribute list structure
>> + *
>> + * This leaf block cannot have a "remote" value, we only call this routine
>> + * if bmap_one_block() says there is only one block (ie: no remote blks).
>> + */
>> +STATIC int
>> +xfs_attr_leaf_addname(struct xfs_da_args	*args)
>> +{
>> +	int			retval, error, forkoff;
>> +	struct xfs_buf		*bp = NULL;
>> +	struct xfs_inode	*dp = args->dp;
>> +
>> +	retval = xfs_attr_leaf_try_add(args, bp);
>> +	if (retval == -ENOSPC) {
>> +		/*
>> +		 * Commit that transaction so that the node_addname() call
>> +		 * can manage its own transactions.
>> +		 */
>>   		error = xfs_defer_finish(&args->trans);
>>   		if (error)
>>   			return error;
> 
> Hmm.. I find this bit of factoring a little strange. We do part of the
> -ENOSPC handling (leaf to node) in one place and another part
> (xfs_defer_finish()) in the caller. I'm assuming we intentionally don't
> finish dfops in the new helper because the delayed attr bits shouldn't
> do that, but I'm wondering whether the helper should just return -ENOSPC
> and the caller should be responsible for whatever needs to happen based
> on that in the associated context. Hm?
> 
> Brian

Yes, the idea is that the new helper function avoids anything with 
transactions.  We could factor up the ENOSPC handling into the caller, 
that may look cleaner.  I think there's really only one place it's called.

Thanks!
Allison

> 
>> -- 
>> 2.7.4
>>