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@xxxxxxxxxx>
> ---
> 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?
--
chandan
