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

[Chandan Rajendra <chandan@linux.ibm.com>]

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?

-- 
chandan