[PATCH 23/27] rearrange bmapi and bmalloca structures for best packing

[Alex Elder]

On Sun, 2011-09-18 at 16:41 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-bmalloca-shrink)
> Minimise the stack overhead of the remaining stack variables and
> structures placed on the stack by packing them without holes. pahole
> is used to optimise allocation args structures, stack variables are
> done manually.
> 
> [hch: various updates while forward porting the changes]
> 
> Signed-off-by: Dave Chinner <dchinner at redhat.com>
> Signed-off-by: Christoph Hellwig <hch at lst.de>

I don't object to it, but I do comment on something
below that I think is not an improvement.  I'll
take this as-is anyway, unless you care to re-submit
it.

Reviewed-by: Alex Elder <aelder at sgi.com>

. . .

> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c	2011-09-11 08:47:11.743121736 -0400
> +++ xfs/fs/xfs/xfs_bmap.c	2011-09-11 09:05:37.706448434 -0400

. . .

> @@ -4899,19 +4886,21 @@ xfs_bmapi_write(
>  	bma.userdata = 0;
>  	bma.flist = flist;
>  	bma.firstblock = firstblock;
> +	bma.eof = n;
> +	bma.conv = !!(flags & XFS_BMAPI_CONVERT);
>  
> +	n = 0;
> +	end = bno + len;
> +	obno = bno;
>  	while (bno < end && n < *nmap) {
> -		inhole = eof || bma.got.br_startoff > bno;
> -		wasdelay = !inhole && isnullstartblock(bma.got.br_startblock);
> -
>  		/*
> -		 * First, deal with the hole before the allocated space
> -		 * that we found, if any.
> +		 * If we are past EOF, in a hole in a delayed allocation call
> +		 * the allocator to get us a real allocation first.
>  		 */
> -		if (inhole || wasdelay) {
> -			bma.eof = eof;
> -			bma.conv = !!(flags & XFS_BMAPI_CONVERT);
> -			bma.wasdel = wasdelay;
> +		if (bma.eof || bma.got.br_startoff > bno ||
> +		    isnullstartblock(bma.got.br_startblock)) {
> +			bma.wasdel = isnullstartblock(bma.got.br_startblock) &&
> +					!bma.eof && bma.got.br_startoff <= bno;

I think this detracts from readability and simplicity
rather than adding to it.  I understand the desire to
get rid of the local variables, and I do follow what
each piece of this conditional means, but "inhole"
and "wasdelay" were actually quite nice shorthands
for what that logic represents.

>  			bma.length = len;
>  			bma.offset = bno;
>  

. . .