Re: [PATCH V4 RESEND 1/7] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[Christoph Hellwig <hch@infradead.org>]

> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fae322105457a..65a3bf40c4f9d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1403,6 +1404,7 @@ xfs_attr3_leaf_add_work(
>  	struct xfs_attr_leaf_name_local *name_loc;
>  	struct xfs_attr_leaf_name_remote *name_rmt;
>  	struct xfs_mount	*mp;
> +	int			entsize;
>  	int			tmp;
>  	int			i;
>  
> @@ -1432,11 +1434,14 @@ xfs_attr3_leaf_add_work(
>  	ASSERT(ichdr->freemap[mapindex].base < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].base & 0x3) == 0);
>  	ASSERT(ichdr->freemap[mapindex].size >=
> -		xfs_attr_leaf_newentsize(args, NULL));
> +		xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL));
>  	ASSERT(ichdr->freemap[mapindex].size < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].size & 0x3) == 0);
>  
> -	ichdr->freemap[mapindex].size -= xfs_attr_leaf_newentsize(args, &tmp);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, &tmp);
> +	ichdr->freemap[mapindex].size -= entsize;

As-is this entsize variable is a little pointless.  Please move the
assignment to it up and reuse it in the assert.

> @@ -1824,6 +1829,8 @@ xfs_attr3_leaf_figure_balance(
>  	struct xfs_attr_leafblock	*leaf1 = blk1->bp->b_addr;
>  	struct xfs_attr_leafblock	*leaf2 = blk2->bp->b_addr;
>  	struct xfs_attr_leaf_entry	*entry;
> +	struct xfs_da_args		*args;
> +	int				entsize;
>  	int				count;
>  	int				max;
>  	int				index;
> @@ -1833,14 +1840,16 @@ xfs_attr3_leaf_figure_balance(
>  	int				foundit = 0;
>  	int				tmp;
>  
> +	args = state->args;

Please assign the value to the variable at the time of declaration.

>  	/*
>  	 * Examine entries until we reduce the absolute difference in
>  	 * byte usage between the two blocks to a minimum.
>  	 */
>  	max = ichdr1->count + ichdr2->count;
>  	half = (max + 1) * sizeof(*entry);
> -	half += ichdr1->usedbytes + ichdr2->usedbytes +
> -			xfs_attr_leaf_newentsize(state->args, NULL);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, NULL);
> +	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
>  	half /= 2;
>  	lastdelta = state->args->geo->blksize;
>  	entry = xfs_attr3_leaf_entryp(leaf1);
> @@ -1851,8 +1860,9 @@ xfs_attr3_leaf_figure_balance(
>  		 * The new entry is in the first block, account for it.
>  		 */
>  		if (count == blk1->index) {
> -			tmp = totallen + sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +			entsize = xfs_attr_leaf_newentsize(args->geo,
> +					args->namelen, args->valuelen, NULL);
> +			tmp = totallen + sizeof(*entry) + entsize;
>  			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
>  				break;
>  			lastdelta = XFS_ATTR_ABS(half - tmp);
> @@ -1887,8 +1897,9 @@ xfs_attr3_leaf_figure_balance(
>  	 */
>  	totallen -= count * sizeof(*entry);
>  	if (foundit) {
> -		totallen -= sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +		entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL);
> +		totallen -= sizeof(*entry) + entsize;

AFAICS there is no need to assign the same value to entsize again and
again in this function.  It should be enough to assign to it once and
then reuse the value.