Re: [PATCH v13 03/10] xfs: Add delay ready attr set routines

Allison Henderson
2020-11-13T17:12:17Z

On 11/13/20 2:16 AM, Chandan Babu R wrote:
> On Friday 13 November 2020 7:03:13 AM IST Allison Henderson wrote:
>>
>> On 11/10/20 2:57 PM, Darrick J. Wong wrote:
>>> On Tue, Oct 27, 2020 at 07:02:55PM +0530, Chandan Babu R wrote:
>>>> On Friday 23 October 2020 12:04:28 PM IST Allison Henderson wrote:
>>>>> This patch modifies the attr set routines to be delay ready. This means
>>>>> they no longer roll or commit transactions, but instead return -EAGAIN
>>>>> to have the calling routine roll and refresh the transaction. In this
>>>>> series, xfs_attr_set_args has become xfs_attr_set_iter, which uses a
>>>>> state machine like switch to keep track of where it was when EAGAIN was
>>>>> returned. See xfs_attr.h for a more detailed diagram of the states.
>>>>>
>>>>> Two new helper functions have been added: xfs_attr_rmtval_set_init and
>>>>> xfs_attr_rmtval_set_blk. They provide a subset of logic similar to
>>>>> xfs_attr_rmtval_set, but they store the current block in the delay attr
>>>>> context to allow the caller to roll the transaction between allocations.
>>>>> This helps to simplify and consolidate code used by
>>>>> xfs_attr_leaf_addname and xfs_attr_node_addname. xfs_attr_set_args has
>>>>> now become a simple loop to refresh the transaction until the operation
>>>>> is completed. Lastly, xfs_attr_rmtval_remove is no longer used, and is
>>>>> removed.
>>>>
>>>> One nit. xfs_attr_rmtval_remove()'s prototype declaration needs to be removed
>>>> from xfs_attr_remote.h.
>> Alrighty, will pull out
>>
>>>>
>>>>>
>>>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>>>>> ---
>>>>> fs/xfs/libxfs/xfs_attr.c | 370 ++++++++++++++++++++++++++--------------
>>>>> fs/xfs/libxfs/xfs_attr.h | 126 +++++++++++++-
>>>>> fs/xfs/libxfs/xfs_attr_remote.c | 99 +++++++----
>>>>> fs/xfs/libxfs/xfs_attr_remote.h | 4 +
>>>>> fs/xfs/xfs_trace.h | 1 -
>>>>> 5 files changed, 439 insertions(+), 161 deletions(-)
>>>>>
>>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>>> index 6ca94cb..95c98d7 100644
>>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>>> @@ -44,7 +44,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
>>>>> * Internal routines when attribute list is one block.
>>>>> */
>>>>> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
>>>>> -STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
>>>>> +STATIC int xfs_attr_leaf_addname(struct xfs_delattr_context *dac);
>>>>> STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
>>>>> STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>>>>
>>>>> @@ -52,12 +52,15 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>>>> * Internal routines when attribute list is more than one block.
>>>>> */
>>>>> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>>>> -STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>>>> +STATIC int xfs_attr_node_addname(struct xfs_delattr_context *dac);
>>>>> STATIC int xfs_attr_node_removename_iter(struct xfs_delattr_context *dac);
>>>>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>>>> struct xfs_da_state **state);
>>>>> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>>>>> STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
>>>>> +STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
>>>>> +STATIC int xfs_attr_set_iter(struct xfs_delattr_context *dac,
>>>>> + struct xfs_buf **leaf_bp);
>>>>>
>>>>> int
>>>>> xfs_inode_hasattr(
>>>>> @@ -218,8 +221,11 @@ xfs_attr_is_shortform(
>>>>>
>>>>> /*
>>>>> * Attempts to set an attr in shortform, or converts short form to leaf form if
>>>>> - * there is not enough room. If the attr is set, the transaction is committed
>>>>> - * and set to NULL.
>>>>> + * there is not enough room. This function is meant to operate as a helper
>>>>> + * routine to the delayed attribute functions. It returns -EAGAIN to indicate
>>>>> + * that the calling function should roll the transaction, and then proceed to
>>>>> + * add the attr in leaf form. This subroutine does not expect to be recalled
>>>>> + * again like the other delayed attr routines do.
>>>>> */
>>>>> STATIC int
>>>>> xfs_attr_set_shortform(
>>>>> @@ -227,16 +233,16 @@ xfs_attr_set_shortform(
>>>>> struct xfs_buf **leaf_bp)
>>>>> {
>>>>> struct xfs_inode *dp = args->dp;
>>>>> - int error, error2 = 0;
>>>>> + int error = 0;
>>>>>
>>>>> /*
>>>>> * Try to add the attr to the attribute list in the inode.
>>>>> */
>>>>> error = xfs_attr_try_sf_addname(dp, args);
>>>>> +
>>>>> + /* Should only be 0, -EEXIST or ENOSPC */
>>>>> if (error != -ENOSPC) {
>>>>> - error2 = xfs_trans_commit(args->trans);
>>>>> - args->trans = NULL;
>>>>> - return error ? error : error2;
>>>>> + return error;
>>>>> }
>>>>> /*
>>>>> * It won't fit in the shortform, transform to a leaf block. GROT:
>>>>> @@ -249,18 +255,10 @@ xfs_attr_set_shortform(
>>>>> /*
>>>>> * Prevent the leaf buffer from being unlocked so that a concurrent AIL
>>>>> * push cannot grab the half-baked leaf buffer and run into problems
>>>>> - * with the write verifier. Once we're done rolling the transaction we
>>>>> - * can release the hold and add the attr to the leaf.
>>>>> + * with the write verifier.
>>>>> */
>>>>> xfs_trans_bhold(args->trans, *leaf_bp);
>>>>> - error = xfs_defer_finish(&args->trans);
>>>>> - xfs_trans_bhold_release(args->trans, *leaf_bp);
>>>>> - if (error) {
>>>>> - xfs_trans_brelse(args->trans, *leaf_bp);
>>>>> - return error;
>>>>> - }
>>>>> -
>>>>> - return 0;
>>>>> + return -EAGAIN;
>>>>> }
>>>>>
>>>>> /*
>>>>> @@ -268,7 +266,7 @@ xfs_attr_set_shortform(
>>>>> * also checks for a defer finish. Transaction is finished and rolled as
>>>>> * needed, and returns true of false if the delayed operation should continue.
>>>>> */
>>>>> -int
>>>>> +STATIC int
>>>>> xfs_attr_trans_roll(
>>>>> struct xfs_delattr_context *dac)
>>>>> {
>>>>> @@ -297,61 +295,130 @@ int
>>>>> xfs_attr_set_args(
>>>>> struct xfs_da_args *args)
>>>>> {
>>>>> - struct xfs_inode *dp = args->dp;
>>>>> - struct xfs_buf *leaf_bp = NULL;
>>>>> - int error = 0;
>>>>> + struct xfs_buf *leaf_bp = NULL;
>>>>> + int error = 0;
>>>>> + struct xfs_delattr_context dac = {
>>>>> + .da_args = args,
>>>>> + };
>>>>> +
>>>>> + do {
>>>>> + error = xfs_attr_set_iter(&dac, &leaf_bp);
>>>>> + if (error != -EAGAIN)
>>>>> + break;
>>>>> +
>>>>> + error = xfs_attr_trans_roll(&dac);
>>>>> + if (error)
>>>>> + return error;
>>>>> +
>>>>> + if (leaf_bp) {
>>>>> + xfs_trans_bjoin(args->trans, leaf_bp);
>>>>> + xfs_trans_bhold(args->trans, leaf_bp);
>>>>> + }
>>>>
>>>> When xfs_attr_set_iter() causes a "short form" attribute list to be converted
>>>> to "leaf form", leaf_bp would point to an xfs_buf which has been added to the
>>>> transaction and also XFS_BLI_HOLD flag is set on the buffer (last statement in
>>>> xfs_attr_set_shortform()). XFS_BLI_HOLD flag makes sure that the new
>>>> transaction allocated by xfs_attr_trans_roll() would continue to have leaf_bp
>>>> in the transaction's item list. Hence I think the above calls to
>>>> xfs_trans_bjoin() and xfs_trans_bhold() are not required.
>> Sorry, I just noticed Chandans commentary for this patch. Apologies. I
>> think we can get away with out this now, but yes this routine disappears
>> at the end of the set now. Will clean out anyway for bisecting reasons
>> though. :-)
>
> No problem. As an aside, I stopped reviewing the patchset after I noticed
> Brian's review comments for "[PATCH v13 02/10] xfs: Add delay ready attr
> remove routines" suggesting some more code refactoring work.
>
No worries, thats reasonable. It's why I only send this out in subsets
to try and keep people sort of focused on a smaller area because stuff
at the end of the set changes more often as a result of things moving
around at the bottom of the set. It doesn't make sense to channel too
much effort into something that's still moving around so much :-)

Allison
>>
>>>
>>> I /think/ the defer ops will rejoin the buffer each time it rolls, which
>>> means that xfs_attr_trans_roll returns with the buffer already joined to
>>> the transaction? And I think you're right that the bhold isn't needed,
>>> because holding is dictated by the lower levels (i.e. _set_iter).
>>>
>>>> Please let me know if I am missing something obvious here.
>>>
>>> The entire function goes away by the end of the series. :)
>>>
>>> --D
>>>
>>>>
>>>>
>>>>
>>>>
>>
>
>