Re: [PATCH v8 19/20] xfs: Add delay ready attr set routines

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

On 4/20/20 9:20 AM, Brian Foster wrote:
> On Mon, Apr 20, 2020 at 05:15:08PM +0530, Chandan Rajendra wrote:
>> On Saturday, April 4, 2020 3:42 AM Allison Collins wrote:
> ...
>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>> ---
>>>   fs/xfs/libxfs/xfs_attr.c        | 384 +++++++++++++++++++++++++++-------------
>>>   fs/xfs/libxfs/xfs_attr.h        |  16 ++
>>>   fs/xfs/libxfs/xfs_attr_leaf.c   |   1 +
>>>   fs/xfs/libxfs/xfs_attr_remote.c | 111 +++++++-----
>>>   fs/xfs/libxfs/xfs_attr_remote.h |   4 +
>>>   fs/xfs/xfs_attr_inactive.c      |   1 +
>>>   fs/xfs/xfs_trace.h              |   1 -
>>>   7 files changed, 351 insertions(+), 167 deletions(-)
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>> index f700976..c160b7a 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>> +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
>>> @@ -765,22 +873,25 @@ xfs_attr_leaf_addname(
>>>   		error = xfs_attr3_leaf_flipflags(args);
>>>   		if (error)
>>>   			return error;
>>> -		/*
>>> -		 * Commit the flag value change and start the next trans in
>>> -		 * series.
>>> -		 */
>>> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>>> -		if (error)
>>> -			return error;
>>> -
>>> +		dac->dela_state = XFS_DAS_FLIP_LFLAG;
>>> +		return -EAGAIN;
>>> +das_flip_flag:
>>>   		/*
>>>   		 * Dismantle the "old" attribute/value pair by removing
>>>   		 * a "remote" value (if it exists).
>>>   		 */
>>>   		xfs_attr_restore_rmt_blk(args);
>>>   
>>> +		xfs_attr_rmtval_invalidate(args);
>>> +das_rm_lblk:
>>>   		if (args->rmtblkno) {
>>> -			error = xfs_attr_rmtval_remove(args);
>>> +			error = __xfs_attr_rmtval_remove(args);
>>> +
>>> +			if (error == -EAGAIN) {
>>> +				dac->dela_state = XFS_DAS_RM_LBLK;
>>
>> Similar to what I had observed in the patch "Add delay ready attr remove
>> routines",
>>
>> Shouldn't XFS_DAC_DEFER_FINISH be set in dac->flags?
>> __xfs_attr_rmtval_remove() calls __xfs_bunmapi() which would
>> have added items to the deferred list.
>>
> 
> Just note that transaction rolls don't currently finish deferred ops. So
> from the perspective of preserving current behavior it might make sense
> to set the flag here if there was an explicit xfs_defer_finish() that's
> been factored out, but not so if it was just a transaction roll.
> 
> Brian
Yep, I think Chandan is right, xfs_attr_rmtval_remove used to have a 
xfs_defer_finish and __xfs_attr_rmtval_remove does not.  I think 
probably I'll keep the xfs_defer_finish in the __xfs_attr_rmtval_remove 
helper, and then when we get to this patch, turn it into the flag set. 
I believe that should be correct.

Allison
> 
>> -- 
>> chandan
>>
>>
>>
>