Re: [PATCH v7 12/19] xfs: Add helper function xfs_attr_rmtval_unmap

Allison Collins
2020-02-25T23:27:31Z

On 2/25/20 12:21 AM, Dave Chinner wrote:
> On Sat, Feb 22, 2020 at 07:06:04PM -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
>
> 68-72 columns...
>
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
>> fs/xfs/libxfs/xfs_attr_remote.h | 1 +
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index 3de2eec..da40f85 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
>> }
>> 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_unmap(
>> + struct xfs_da_args *args)
>> +{
>> + int error, done;
>
> Best to initialise done to zero here.
>
>> +
>> + /*
>> + * Unmap value blocks for this attr. This is similar to
>> + * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>> + * for new transactions
>> + */
>> + error = xfs_bunmapi(args->trans, args->dp,
>> + args->rmtblkno, args->rmtblkcnt,
>> + XFS_BMAPI_ATTRFORK, 1, &done);
>
> Got 80 columns for code, please use them all :)
>
>> + if (error)
>> + return error;
>> +
>> + if (!done)
>> + return -EAGAIN;
>
> Hmmm, I guess I'm missing the context at this point: why not just pass the done
> variable all the way back to the caller that will be looping on this function
> call?
>
> That turns this function into:
>
> int
> xfs_attr_rmtval_unmap(
> struct xfs_da_args *args,
> bool *donec
> {
> return xfs_bunmapi(args->trans, args->dp, args->rmtblkno,
> args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, done);
> }
Well, this is a helper we talked about adding in the v5 review ([PATCH
v5 13/14] xfs: Add delay ready attr remove routines) In v5, I did not
have this helper function, and just open coded xfs_attr_rmtval_remove in
the middle of the calling function (xfs_attr_remove_iter), except with
out the transaction. Then later we identified it as an area we could
modularize. It's kind of hard to see the bigger picture with out the
states being here yet. But really what we're trying to do is modularize
everything that shows up between the states later in the set.

Hope that helps a bit!
Allison

>
> Cheers,
>
> Dave.
>