Re: [PATCH v9 10/24] xfs: Add helper function __xfs_attr_rmtval_remove

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

On 5/5/20 5:03 AM, Brian Foster wrote:
> On Mon, May 04, 2020 at 02:36:39PM -0700, Allison Collins wrote:
>>
>>
>> On 5/4/20 6:27 AM, Brian Foster wrote:
>>> On Thu, Apr 30, 2020 at 03:50:02PM -0700, Allison Collins wrote:
>>>> This function is similar to xfs_attr_rmtval_remove, but adapted to
>>>> return EAGAIN for new transactions. We will use this later when we
>>>> introduce delayed attributes.  This function will eventually replace
>>>> xfs_attr_rmtval_remove
>>>>
>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
>>>> ---
>>>
>>> Looks like the commit log needs some rewording now that this is a
>>> refactor patch. With that fixed:
>> Ok, maybe just an extra line like "Refactor xfs_attr_rmtval_remove to add
>> helper function __xfs_attr_rmtval_remove" ?
>>
> 
> I'd update the first sentence to say something like that instead of how
> the function is similar to xfs_attr_rmtval_remove().
> 
> Brian

Ok then, will update.

Allison

> 
>>>
>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Alrighty, thank you!
>>
>> Allison
>>
>>>
>>>>    fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++---------
>>>>    fs/xfs/libxfs/xfs_attr_remote.h |  1 +
>>>>    2 files changed, 37 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> index 4d51969..02d1a44 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> @@ -681,7 +681,7 @@ xfs_attr_rmtval_remove(
>>>>    	xfs_dablk_t		lblkno;
>>>>    	int			blkcnt;
>>>>    	int			error = 0;
>>>> -	int			done = 0;
>>>> +	int			retval = 0;
>>>>    	trace_xfs_attr_rmtval_remove(args);
>>>> @@ -693,14 +693,10 @@ xfs_attr_rmtval_remove(
>>>>    	 */
>>>>    	lblkno = args->rmtblkno;
>>>>    	blkcnt = args->rmtblkcnt;
>>>> -	while (!done) {
>>>> -		error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt,
>>>> -				    XFS_BMAPI_ATTRFORK, 1, &done);
>>>> -		if (error)
>>>> -			return error;
>>>> -		error = xfs_defer_finish(&args->trans);
>>>> -		if (error)
>>>> -			return error;
>>>> +	do {
>>>> +		retval = __xfs_attr_rmtval_remove(args);
>>>> +		if (retval && retval != EAGAIN)
>>>> +			return retval;
>>>>    		/*
>>>>    		 * Close out trans and start the next one in the chain.
>>>> @@ -708,6 +704,36 @@ xfs_attr_rmtval_remove(
>>>>    		error = xfs_trans_roll_inode(&args->trans, args->dp);
>>>>    		if (error)
>>>>    			return error;
>>>> -	}
>>>> +	} while (retval == -EAGAIN);
>>>> +
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +/*
>>>> + * Remove the value associated with an attribute by deleting the out-of-line
>>>> + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the
>>>> + * transaction and recall the function
>>>> + */
>>>> +int
>>>> +__xfs_attr_rmtval_remove(
>>>> +	struct xfs_da_args	*args)
>>>> +{
>>>> +	int			error, done;
>>>> +
>>>> +	/*
>>>> +	 * Unmap value blocks for this attr.
>>>> +	 */
>>>> +	error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno,
>>>> +			    args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done);
>>>> +	if (error)
>>>> +		return error;
>>>> +
>>>> +	error = xfs_defer_finish(&args->trans);
>>>> +	if (error)
>>>> +		return error;
>>>> +
>>>> +	if (!done)
>>>> +		return -EAGAIN;
>>>> +
>>>> +	return error;
>>>> +}
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>>>> index eff5f95..ee3337b 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.h
>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
>>>> @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args);
>>>>    int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map,
>>>>    		xfs_buf_flags_t incore_flags);
>>>>    int xfs_attr_rmtval_invalidate(struct xfs_da_args *args);
>>>> +int __xfs_attr_rmtval_remove(struct xfs_da_args *args);
>>>>    #endif /* __XFS_ATTR_REMOTE_H__ */
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>
>