Re: [PATCH v17 10/11] xfs: Add delay ready attr remove routines

Allison Henderson
2021-04-26T16:53:06Z

On 4/26/21 4:49 AM, Brian Foster wrote:
> On Sat, Apr 24, 2021 at 08:56:45AM -0700, Darrick J. Wong wrote:
>> On Fri, Apr 23, 2021 at 08:27:28PM -0700, Allison Henderson wrote:
>>>
>>>
>>> On 4/23/21 10:06 AM, Brian Foster wrote:
>>>> On Fri, Apr 16, 2021 at 02:20:44AM -0700, Allison Henderson wrote:
>>>>> This patch modifies the attr remove 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_remove_args is merged with
>>>>> xfs_attr_node_removename become a new function, xfs_attr_remove_iter.
>>>>> This new version uses a sort of state machine like switch to keep track
>>>>> of where it was when EAGAIN was returned. A new version of
>>>>> xfs_attr_remove_args consists of a simple loop to refresh the
>>>>> transaction until the operation is completed. A new XFS_DAC_DEFER_FINISH
>>>>> flag is used to finish the transaction where ever the existing code used
>>>>> to.
>>>>>
>>>>> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
>>>>> version __xfs_attr_rmtval_remove. We will rename
>>>>> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
>>>>> done.
>>>>>
>>>>> xfs_attr_rmtval_remove itself is still in use by the set routines (used
>>>>> during a rename). For reasons of preserving existing function, we
>>>>> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
>>>>> set. Similar to how xfs_attr_remove_args does here. Once we transition
>>>>> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
>>>>> used and will be removed.
>>>>>
>>>>> This patch also adds a new struct xfs_delattr_context, which we will use
>>>>> to keep track of the current state of an attribute operation. The new
>>>>> xfs_delattr_state enum is used to track various operations that are in
>>>>> progress so that we know not to repeat them, and resume where we left
>>>>> off before EAGAIN was returned to cycle out the transaction. Other
>>>>> members take the place of local variables that need to retain their
>>>>> values across multiple function recalls. See xfs_attr.h for a more
>>>>> detailed diagram of the states.
>>>>>
>>>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>>>>> ---
>>>>> fs/xfs/libxfs/xfs_attr.c | 208 +++++++++++++++++++++++++++-------------
>>>>> fs/xfs/libxfs/xfs_attr.h | 131 +++++++++++++++++++++++++
>>>>> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
>>>>> fs/xfs/libxfs/xfs_attr_remote.c | 48 ++++++----
>>>>> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
>>>>> fs/xfs/xfs_attr_inactive.c | 2 +-
>>>>> 6 files changed, 305 insertions(+), 88 deletions(-)
>>>>>
>>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>>> index ed06b60..0bea8dd 100644
>>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>> ...
>>>>> @@ -1231,70 +1262,117 @@ xfs_attr_node_remove_cleanup(
>>>>> }
>>>>> /*
>>>>> - * Remove a name from a B-tree attribute list.
>>>>> + * Remove the attribute specified in @args.
>>>>> *
>>>>> * This will involve walking down the Btree, and may involve joining
>>>>> * leaf nodes and even joining intermediate nodes up to and including
>>>>> * the root node (a special case of an intermediate node).
>>>>> + *
>>>>> + * This routine is meant to function as either an in-line or delayed operation,
>>>>> + * and may return -EAGAIN when the transaction needs to be rolled. Calling
>>>>> + * functions will need to handle this, and recall the function until a
>>>>> + * successful error code is returned.
>>>>> */
>>>>> -STATIC int
>>>>> -xfs_attr_node_removename(
>>>>> - struct xfs_da_args *args)
>>>>> +int
>>>>> +xfs_attr_remove_iter(
>>>>> + struct xfs_delattr_context *dac)
>>>>> {
>>>>> - struct xfs_da_state *state;
>>>>> - int retval, error;
>>>>> - struct xfs_inode *dp = args->dp;
>>>>> + struct xfs_da_args *args = dac->da_args;
>>>>> + struct xfs_da_state *state = dac->da_state;
>>>>> + int retval, error;
>>>>> + struct xfs_inode *dp = args->dp;
>>>>> trace_xfs_attr_node_removename(args);
>>>> ...
>>>>> + case XFS_DAS_CLNUP:
>>>>> + retval = xfs_attr_node_remove_cleanup(args, state);
>>>>
>>>> This is a nit, but when reading the code the "cleanup" name gives the
>>>> impression that this is a resource cleanup or something along those
>>>> lines, when this is actually a primary component of the operation where
>>>> we remove the attr name. That took me a second to find. Could we tweak
>>>> the state and rename the helper to something like DAS_RMNAME /
>>>> _node_remove_name() so the naming is a bit more explicit?
>>> Sure, this helper is actually added in patch 2 of this set. I can rename it
>>> there? People have already added their rvb's, but I'm assuming people are
>>> not bothered by small tweeks like that? That way this patch just sort of
>>> moves it and XFS_DAS_CLNUP can turn into XFS_DAS_RMNAME here.
>>
>> <bikeshed> "RMNAME" looks too similar to "RENAME" for my old eyes, can
>> we please pick something else? Like "RM_NAME", or "REMOVE_NAME" ?
>>
>
> Either of those seem fine to me, FWIW. I think anything that expresses
> removal of the name/entry over the more generic "cleanup" name is an
> improvement..
>
> Brian
Alrighty, lets go with RM_NAME then. Will update. Thanks for the feedback.

Allison

>
>> --D
>>
>>>
>>>>
>>>>> - /*
>>>>> - * Check to see if the tree needs to be collapsed.
>>>>> - */
>>>>> - if (retval && (state->path.active > 1)) {
>>>>> - error = xfs_da3_join(state);
>>>>> - if (error)
>>>>> - goto out;
>>>>> - error = xfs_defer_finish(&args->trans);
>>>>> - if (error)
>>>>> - goto out;
>>>>> /*
>>>>> - * Commit the Btree join operation and start a new trans.
>>>>> + * Check to see if the tree needs to be collapsed. Set the flag
>>>>> + * to indicate that the calling function needs to move the
>>>>> + * shrink operation
>>>>> */
>>>>> - error = xfs_trans_roll_inode(&args->trans, dp);
>>>>> - if (error)
>>>>> - goto out;
>>>>> - }
>>>>> + if (retval && (state->path.active > 1)) {
>>>>> + error = xfs_da3_join(state);
>>>>> + if (error)
>>>>> + goto out;
>>>>> - /*
>>>>> - * If the result is small enough, push it all into the inode.
>>>>> - */
>>>>> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>>>>> - error = xfs_attr_node_shrink(args, state);
>>>>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>>>>> + dac->dela_state = XFS_DAS_RM_SHRINK;
>>>>> + return -EAGAIN;
>>>>> + }
>>>>> +
>>>>> + /* fallthrough */
>>>>> + case XFS_DAS_RM_SHRINK:
>>>>> + /*
>>>>> + * If the result is small enough, push it all into the inode.
>>>>> + */
>>>>> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>>>>> + error = xfs_attr_node_shrink(args, state);
>>>>> +
>>>>> + break;
>>>>> + default:
>>>>> + ASSERT(0);
>>>>> + error = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> + if (error == -EAGAIN)
>>>>> + return error;
>>>>> out:
>>>>> if (state)
>>>>> xfs_da_state_free(state);
>>>> ...
>>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>>>>> index 48d8e9c..908521e7 100644
>>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> ...
>>>>> @@ -685,31 +687,29 @@ xfs_attr_rmtval_remove(
>>>>> * Keep de-allocating extents until the remote-value region is gone.
>>>>> */
>>>>> do {
>>>>> - retval = __xfs_attr_rmtval_remove(args);
>>>>> - if (retval && retval != -EAGAIN)
>>>>> - return retval;
>>>>> + error = __xfs_attr_rmtval_remove(&dac);
>>>>> + if (error != -EAGAIN)
>>>>> + break;
>>>>
>>>> Shouldn't this retain the (error && error != -EAGAIN) logic to roll the
>>>> transaction after the final unmap? Even if this is transient, it's
>>>> probably best to preserve behavior if this is unintentional.
>>> Sure, I dont think it's intentional, I think back in v10 we had a different
>>> arangement here with a helper inside the while() expression that had
>>> equivelent error handling logic. But that got nak'd in the next review and
>>> I think I likley forgot to put back this handling. Will fix.
>>>
>>>>
>>>> Otherwise my only remaining feedback was to add/tweak some comments that
>>>> I think make the iteration function easier to follow. I've appended a
>>>> diff for that. If you agree with the changes feel free to just fold them
>>>> in and/or tweak as necessary. With those various nits and Chandan's
>>>> feedback addressed, I think this patch looks pretty good.
>>> Sure, those all look reasonable. Will add. Thanks for the reviews!
>>> Allison
>>>
>>>>
>>>> Brian
>>>>
>>>> --- 8< ---
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>> index 0bea8dd34902..ee885c649c26 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>> @@ -1289,14 +1289,21 @@ xfs_attr_remove_iter(
>>>> if (!xfs_inode_hasattr(dp))
>>>> return -ENOATTR;
>>>> + /*
>>>> + * Shortform or leaf formats don't require transaction rolls and
>>>> + * thus state transitions. Call the right helper and return.
>>>> + */
>>>> if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>>>> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>>>> return xfs_attr_shortform_remove(args);
>>>> }
>>>> -
>>>> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>>>> return xfs_attr_leaf_removename(args);
>>>> + /*
>>>> + * Node format may require transaction rolls. Set up the
>>>> + * state context and fall into the state machine.
>>>> + */
>>>> if (!dac->da_state) {
>>>> error = xfs_attr_node_removename_setup(dac);
>>>> if (error)
>>>> @@ -1304,7 +1311,7 @@ xfs_attr_remove_iter(
>>>> state = dac->da_state;
>>>> }
>>>> - /* fallthrough */
>>>> + /* fallthrough */
>>>> case XFS_DAS_RMTBLK:
>>>> dac->dela_state = XFS_DAS_RMTBLK;
>>>> @@ -1316,7 +1323,8 @@ xfs_attr_remove_iter(
>>>> */
>>>> if (args->rmtblkno > 0) {
>>>> /*
>>>> - * May return -EAGAIN. Remove blocks until 0 is returned
>>>> + * May return -EAGAIN. Roll and repeat until all remote
>>>> + * blocks are removed.
>>>> */
>>>> error = __xfs_attr_rmtval_remove(dac);
>>>> if (error == -EAGAIN)
>>>> @@ -1325,26 +1333,26 @@ xfs_attr_remove_iter(
>>>> goto out;
>>>> /*
>>>> - * Refill the state structure with buffers, the prior
>>>> - * calls released our buffers.
>>>> + * Refill the state structure with buffers (the prior
>>>> + * calls released our buffers) and close out this
>>>> + * transaction before proceeding.
>>>> */
>>>> ASSERT(args->rmtblkno == 0);
>>>> error = xfs_attr_refillstate(state);
>>>> if (error)
>>>> goto out;
>>>> -
>>>> dac->dela_state = XFS_DAS_CLNUP;
>>>> dac->flags |= XFS_DAC_DEFER_FINISH;
>>>> return -EAGAIN;
>>>> }
>>>> + /* fallthrough */
>>>> case XFS_DAS_CLNUP:
>>>> retval = xfs_attr_node_remove_cleanup(args, state);
>>>> /*
>>>> - * Check to see if the tree needs to be collapsed. Set the flag
>>>> - * to indicate that the calling function needs to move the
>>>> - * shrink operation
>>>> + * Check to see if the tree needs to be collapsed. If so, roll
>>>> + * the transacton and fall into the shrink state.
>>>> */
>>>> if (retval && (state->path.active > 1)) {
>>>> error = xfs_da3_join(state);
>>>> @@ -1360,10 +1368,12 @@ xfs_attr_remove_iter(
>>>> case XFS_DAS_RM_SHRINK:
>>>> /*
>>>> * If the result is small enough, push it all into the inode.
>>>> + * This is our final state so it's safe to return a dirty
>>>> + * transaction.
>>>> */
>>>> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>>>> error = xfs_attr_node_shrink(args, state);
>>>> -
>>>> + ASSERT(error != -EAGAIN);
>>>> break;
>>>> default:
>>>> ASSERT(0);
>>>>
>>
>