Re: [PATCH v9 13/24] xfs: Remove unneeded xfs_trans_roll_inode calls

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

On 5/4/20 6:30 AM, Brian Foster wrote:
> On Thu, Apr 30, 2020 at 03:50:05PM -0700, Allison Collins wrote:
>> Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
>> needed. If they are the last operations executed in these functions, and
>> no further changes are made, then higher level routines will roll or
>> commit the tranactions. The xfs_trans_roll in _removename is also not
>> needed because invalidating blocks is an incore-only change.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 40 ----------------------------------------
>>   1 file changed, 40 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index d83443c..af47566 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
>> @@ -784,9 +770,6 @@ xfs_attr_leaf_removename(
>>   		/* bp is gone due to xfs_da_shrink_inode */
>>   		if (error)
>>   			return error;
>> -		error = xfs_defer_finish(&args->trans);
>> -		if (error)
>> -			return error;
>>   	}
>>   	return 0;
> 
> Looks like this could be simplified to return error at the end of the
> function. Some of the others might have similar simplifications
> available.
Sure, will do
> 
>>   }
>> @@ -1074,13 +1057,6 @@ xfs_attr_node_addname(
>>   				goto out;
>>   		}
>>   
>> -		/*
>> -		 * Commit and start the next trans in the chain.
>> -		 */
>> -		error = xfs_trans_roll_inode(&args->trans, dp);
>> -		if (error)
>> -			goto out;
>> -
> 
> There's an xfs_defer_finish() before this roll. Is that not also
> extraneous? It looks like we return straight up to xfs_attr_set() (trans
> commit) from here..
Ok, I see it.  Yes I think so, I will see if I can take it out

> 
>>   	} else if (args->rmtblkno > 0) {
>>   		/*
>>   		 * Added a "remote" value, just clear the incomplete flag.
> ...
>> @@ -1194,10 +1158,6 @@ xfs_attr_node_removename(
>>   		if (error)
>>   			goto out;
>>   
>> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>> -		if (error)
>> -			goto out;
>> -
> 
> I'm still a bit on the fence about this one. At first glance it looks
> like it's not necessary, but technically we're changing behavior by
> combining setting the inactive flag and the first remote block unmap,
> right? If so, that seems fairly reasonable, but it's hard to say for
> sure with the state of the xattr transaction reservations...
> 
As I recall, I had initially not removed this roll in v5, and I had an 
extra XFS_DAS_RM_INVALIDATE state to roll the transaction here.  And 
then I think we reasoned that because invalidating blocks didnt have in 
core changes, we could just remove it, thus getting rid of the extra 
state.  Later I split it out into this separate patch along with a few 
other transaction rolls that we identified as not being necessary.

link just for reference...
https://patchwork.kernel.org/patch/11287029/


> Looking around some more, I suppose this could be analogous to the
> !remote remove case where an entry is removed and a potential dabtree
> join occurs under the same transaction. If that's the best argument we
> have, however, I might suggest to split this one out into an independent
> patch and let the commit log describe what's going on in more detail.
> That way it's more obvious to reviewers and if it's wrong it's easier to
> revert.
I'm not sure that was the argument at the time, but I think it makes 
sense. :-)  Will split off into separate patch.

Thanks for the reviews!

Allison

> 
> Brian



> 
>>   		error = xfs_attr_rmtval_invalidate(args);
>>   		if (error)
>>   			return error;
>> -- 
>> 2.7.4
>>
>