Re: [PATCH v7 19/19] xfs: Remove xfs_attr_rmtval_remove

Allison Collins
2020-02-23T18:52:15Z

On 2/23/20 6:54 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <allison.henderson@oracle.com> wrote:
>>
>> xfs_attr_rmtval_remove is no longer used. Clear it out now
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>
> Patch 12/19 add a new function similar to this one called
> xfs_attr_rmtval_unmap() and now this function is removed.
> I wonder if it wouldn't have been simpler to keep the original function
> name and change its behavior to that of xfs_attr_rmtval_unmap().
I thought about this, but then decided against it. Really what's
happening is that the original function disappears across patches 13 and
14, and I wanted to keep those patches focused on the state machine
rather than lumping the helpers in with it.

>
> Unless the function name change makes the logic change more clear
> for the future users???

I think they are about equivalent myself. But the name difference helps
the old function to stick around until it is completely phased out. If
folks feel strongly though, please chime in :-)

Thanks for the reviews! I know it's a lot!

Allison

>
>> fs/xfs/libxfs/xfs_attr_remote.c | 42 -----------------------------------------
>> fs/xfs/xfs_trace.h | 1 -
>> 2 files changed, 43 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index a0e79db..0cc0ec1 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -734,48 +734,6 @@ xfs_attr_rmtval_invalidate(
>> }
>>
>> /*
>> - * Remove the value associated with an attribute by deleting the
>> - * out-of-line buffer that it is stored on.
>> - */
>> -int
>> -xfs_attr_rmtval_remove(
>> - struct xfs_da_args *args)
>> -{
>> - xfs_dablk_t lblkno;
>> - int blkcnt;
>> - int error = 0;
>> - int done = 0;
>> -
>> - trace_xfs_attr_rmtval_remove(args);
>> -
>> - error = xfs_attr_rmtval_invalidate(args);
>> - if (error)
>> - return error;
>> - /*
>> - * Keep de-allocating extents until the remote-value region is gone.
>> - */
>> - 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;
>> -
>> - /*
>> - * Close out trans and start the next one in the chain.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> - if (error)
>> - return error;
>> - }
>> - 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
>> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
>> index 159b8af..bf9a683 100644
>> --- a/fs/xfs/xfs_trace.h
>> +++ b/fs/xfs/xfs_trace.h
>> @@ -1775,7 +1775,6 @@ DEFINE_ATTR_EVENT(xfs_attr_refillstate);
>>
>> DEFINE_ATTR_EVENT(xfs_attr_rmtval_get);
>> DEFINE_ATTR_EVENT(xfs_attr_rmtval_set);
>> -DEFINE_ATTR_EVENT(xfs_attr_rmtval_remove);
>>
>> #define DEFINE_DA_EVENT(name) \
>> DEFINE_EVENT(xfs_da_class, name, \
>> --
>> 2.7.4
>>