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

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

On Wednesday, February 26, 2020 10:28 PM Christoph Hellwig wrote: 
> > 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.
> 

Sorry about those redundant function calls. I will include the changes
suggested in the next version of the patchset.

-- 
chandan