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

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

On Tuesday, February 25, 2020 9:57 PM Brian Foster wrote: 
> 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.
> 

You are right. The caller can derive total blocks from the other components. I
will fix this up in the next iteration of the patchset.

-- 
chandan