Re: [PATCH v7 13/19] xfs: Add delay ready attr remove routines

Allison Collins
2020-03-03T05:40:49Z

On 3/2/20 10:03 PM, Chandan Rajendra wrote:
> On Sunday, February 23, 2020 7:36 AM Allison Collins 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 has
>> become xfs_attr_remove_iter, which uses a sort of state machine like switch to keep
>> track of where it was when EAGAIN was returned. xfs_attr_node_removename has also
>> been modified to use the switch, and a new version of xfs_attr_remove_args
>> consists of a simple loop to refresh the transaction until the operation is
>> completed.
>>
>> 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.
>>
>> Below is a state machine diagram for attr remove operations. The XFS_DAS_* states
>> indicate places where the function would return -EAGAIN, and then immediately
>> resume from after being recalled by the calling function. States marked as a
>> "subroutine state" indicate that they belong to a subroutine, and so the calling
>> function needs to pass them back to that subroutine to allow it to finish where
>> it left off. But they otherwise do not have a role in the calling function other
>> than just passing through.
>>
>> xfs_attr_remove_iter()
>> XFS_DAS_RM_SHRINK â”€â”
>> (subroutine state) â”‚
>> â”‚
>> XFS_DAS_RMTVAL_REMOVE â”€â”¤
>> (subroutine state) â”‚
>> â””â”€>xfs_attr_node_removename()
>> â”‚
>> v
>> need to remove
>> â”Œâ”€nâ”€â”€ rmt blocks?
>> â”‚ â”‚
>> â”‚ y
>> â”‚ â”‚
>> â”‚ v
>> â”‚ â”Œâ”€>XFS_DAS_RMTVAL_REMOVE
>> â”‚ â”‚ â”‚
>> â”‚ â”‚ v
>> â”‚ â””â”€â”€yâ”€â”€ more blks
>> â”‚ to remove?
>> â”‚ â”‚
>> â”‚ n
>> â”‚ â”‚
>> â”‚ v
>> â”‚ need to
>> â””â”€â”€â”€â”€â”€> shrink tree? â”€nâ”€â”
>> â”‚ â”‚
>> y â”‚
>> â”‚ â”‚
>> v â”‚
>> XFS_DAS_RM_SHRINK â”‚
>> â”‚ â”‚
>> v â”‚
>> done <â”€â”€â”€â”€â”€â”˜
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 114 +++++++++++++++++++++++++++++++++++++------
>> fs/xfs/libxfs/xfs_attr.h | 1 +
>> fs/xfs/libxfs/xfs_da_btree.h | 30 ++++++++++++
>> fs/xfs/scrub/common.c | 2 +
>> fs/xfs/xfs_acl.c | 2 +
>> fs/xfs/xfs_attr_list.c | 1 +
>> fs/xfs/xfs_ioctl.c | 2 +
>> fs/xfs/xfs_ioctl32.c | 2 +
>> fs/xfs/xfs_iops.c | 2 +
>> fs/xfs/xfs_xattr.c | 1 +
>> 10 files changed, 141 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 5d73bdf..cd3a3f7 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -368,11 +368,60 @@ xfs_has_attr(
>> */
>> int
>> xfs_attr_remove_args(
>> + struct xfs_da_args *args)
>> +{
>> + int error = 0;
>> + int err2 = 0;
>> +
>> + do {
>> + error = xfs_attr_remove_iter(args);
>> + if (error && error != -EAGAIN)
>> + goto out;
>> +
>> + if (args->dac.flags & XFS_DAC_FINISH_TRANS) {
>> + args->dac.flags &= ~XFS_DAC_FINISH_TRANS;
>> +
>> + err2 = xfs_defer_finish(&args->trans);
>> + if (err2) {
>> + error = err2;
>> + goto out;
>> + }
>> + }
>> +
>> + err2 = xfs_trans_roll_inode(&args->trans, args->dp);
>> + if (err2) {
>> + error = err2;
>> + goto out;
>> + }
>> +
>> + } while (error == -EAGAIN);
>> +out:
>> + return error;
>> +}
>> +
>> +/*
>> + * Remove the attribute specified in @args.
>> + *
>> + * This function may return -EAGAIN to signal that the transaction needs to be
>> + * rolled. Callers should continue calling this function until they receive a
>> + * return value other than -EAGAIN.
>> + */
>> +int
>> +xfs_attr_remove_iter(
>> struct xfs_da_args *args)
>> {
>> struct xfs_inode *dp = args->dp;
>> int error;
>>
>> + /* State machine switch */
>> + switch (args->dac.dela_state) {
>> + case XFS_DAS_RM_SHRINK:
>> + case XFS_DAS_RMTVAL_REMOVE:
>> + goto node;
>> + default:
>> + break;
>> + }
>> +
>
> On the very first invocation of xfs_attr_remote_iter() from
> xfs_attr_remove_args() (via a call from xfs_attr_remove()),
> args->dac.dela_state is set to a value of 0. This happens because
> xfs_attr_args_init() invokes memset() on args. A value of 0 for
> args->dac.dela_state maps to XFS_DAS_RM_SHRINK.
>
> If the xattr was stored in say local or leaf format we end up incorrectly
> invoking xfs_attr_node_removename() right?
>
Hi Chandan,

Yes, this came up in one of the other reviews too. The indexing for the
XFS_DAS_* enum should start at 1, not zero. I had pulled it out of the
last version thinking it would be ok, but I should have kept the
indexing starting at 1, allowing the switch to fall through to default.

Allison