Re: [PATCH V4 RESEND 4/7] xfs: Introduce struct xfs_attr_set_resv

[Brian Foster <bfoster@redhat.com>]

On Mon, Feb 24, 2020 at 09:30:41AM +0530, Chandan Rajendra wrote:
> The intermediate numbers calculated by xfs_attr_calc_size() will be needed by
> a future commit to correctly calculate log reservation for xattr set
> operation. Towards this goal, this commit introduces 'struct
> xfs_attr_set_resv' to collect,
> 1. Number of dabtree blocks.
> 2. Number of remote blocks.
> 3. Number of Bmbt blocks.
> 4. Total number of blocks we need to reserve.
> 
> This will be returned as an out argument by xfs_attr_calc_size().
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 50 ++++++++++++++++++++++------------------
>  fs/xfs/libxfs/xfs_attr.h | 13 +++++++++++
>  2 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index a708b142f69b6..921acac71e5d9 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -136,16 +136,14 @@ xfs_attr_get(
>  /*
>   * Calculate how many blocks we need for the new attribute,
>   */
> -STATIC int
> +STATIC void
>  xfs_attr_calc_size(
> -	struct xfs_da_args	*args,
> -	int			*local)
> +	struct xfs_da_args		*args,
> +	struct xfs_attr_set_resv	*resv,
> +	int				*local)
>  {
> -	struct xfs_mount	*mp = args->dp->i_mount;
> -	unsigned int		total_dablks;
> -	unsigned int		bmbt_blks;
> -	unsigned int		rmt_blks;
> -	int			size;
> +	struct xfs_mount		*mp = args->dp->i_mount;
> +	int				size;
>  
>  	/*
>  	 * Determine space new attribute will use, and if it would be
> @@ -153,25 +151,27 @@ xfs_attr_calc_size(
>  	 */
>  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
>  			args->valuelen, local);
> -	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
>  	if (*local) {
>  		if (size > (args->geo->blksize / 2)) {
>  			/* Double split possible */
> -			total_dablks *= 2;
> +			resv->total_dablks *= 2;
>  		}
> -		rmt_blks = 0;
> +		resv->rmt_blks = 0;
>  	} else {
>  		/*
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
>  	}
>  
> -	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> -			XFS_ATTR_FORK);
> +	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> +				resv->total_dablks + resv->rmt_blks,
> +				XFS_ATTR_FORK);
>  
> -	return total_dablks + rmt_blks + bmbt_blks;
> +	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
> +		resv->bmbt_blks;

Do we really need a field to track the total of three other fields in
the same structure? I'd rather just let the caller add them up for
args.total if that's the only usage.

Brian

>  }
>  
>  STATIC int
> @@ -295,14 +295,17 @@ xfs_attr_remove_args(
>   */
>  int
>  xfs_attr_set(
> -	struct xfs_da_args	*args)
> +	struct xfs_da_args		*args)
>  {
> -	struct xfs_inode	*dp = args->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> -	struct xfs_trans_res	tres;
> -	bool			rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
> -	int			error, local;
> -	unsigned int		total;
> +	struct xfs_inode		*dp = args->dp;
> +	struct xfs_mount		*mp = dp->i_mount;
> +	struct xfs_attr_set_resv	resv;
> +	struct xfs_trans_res		tres;
> +	bool				rsvd;
> +	int				error, local;
> +	unsigned int			total;
> +
> +	rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
>  
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>  		return -EIO;
> @@ -326,7 +329,8 @@ xfs_attr_set(
>  		XFS_STATS_INC(mp, xs_attr_set);
>  
>  		args->op_flags |= XFS_DA_OP_ADDNAME;
> -		args->total = xfs_attr_calc_size(args, &local);
> +		xfs_attr_calc_size(args, &resv, &local);
> +		args->total = resv.alloc_blks;
>  
>  		/*
>  		 * If the inode doesn't have an attribute fork, add one.
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 861c81f9bb918..dc08bdfbc9615 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -73,6 +73,19 @@ struct xfs_attr_list_context {
>  	int			index;		/* index into output buffer */
>  };
>  
> +struct xfs_attr_set_resv {
> +	/* Number of unlogged blocks needed to store the remote attr value. */
> +	unsigned int		rmt_blks;
> +
> +	/* Number of filesystem blocks to allocate for the da btree. */
> +	unsigned int		total_dablks;
> +
> +	/* Blocks we might need to create all the new attr fork mappings. */
> +	unsigned int		bmbt_blks;
> +
> +	/* Total number of blocks we might have to allocate. */
> +	unsigned int		alloc_blks;
> +};
>  
>  /*========================================================================
>   * Function prototypes for the kernel.
> -- 
> 2.19.1
>