xfs
[Top] [All Lists]

Re: [PATCH 23/27] rearrange bmapi and bmalloca structures for best packi

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 23/27] rearrange bmapi and bmalloca structures for best packing
From: Alex Elder <aelder@xxxxxxx>
Date: Mon, 19 Sep 2011 10:52:27 -0500
Cc: <xfs@xxxxxxxxxxx>, Dave Chinner <dchinner@xxxxxxxxxx>
In-reply-to: <20110918204147.015140236@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110918204040.266805129@xxxxxxxxxxxxxxxxxxxxxx> <20110918204147.015140236@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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@xxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

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@xxxxxxx>

. . .

> 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;
>  

. . .

<Prev in Thread] Current Thread [Next in Thread>