[PATCH 09/14] repair: track logical to physical block mapping moreeffeciently

[Alex Elder]

Christoph Hellwig wrote:
> Currently we track the logical to physical block mapping by a structure which
> contains an array of physicial blocks.  This is extremly efficient and is

Should this be "extremely inefficient?"

> replaced with the normal starblock storage we use in the kernel and on disk
> in this patch.

While you're at fixing the above comment, maybe just re-word this
sentence because I don't really grok it very well...

. . .

> [hch: added a small fix in blkmap_set_ext to not call memmove unless needed]
> 
> Signed-off-by: Barry Naujok <bnaujok at sgi.com>
> Signed-off-by: Christoph Hellwig <hch at lst.de>

A few minor nits mentioned above and below.  But
overall this looks good.

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

> Index: xfsprogs-dev/repair/bmap.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/bmap.c	2009-08-20 02:32:34.000000000 +0000
> +++ xfsprogs-dev/repair/bmap.c	2009-08-20 02:32:45.000000000 +0000

> @@ -21,106 +21,46 @@
>  #include "bmap.h"
> 

. . .

> +	ASSERT(whichfork == XFS_DATA_FORK || whichfork == XFS_ATTR_FORK);
> +
>  	if (nex < 1)
>  		nex = 1;
> -	if ((blkmap = malloc(BLKMAP_SIZE(nex))) == NULL) {
> -		do_warn(_("malloc failed in blkmap_alloc (%u bytes)\n"),
> -			BLKMAP_SIZE(nex));
> -		return blkmap;
> +
> +	key = whichfork ? ablkmap_key : dblkmap_key;
> +	blkmap = pthread_getspecific(key);
> +	if (!blkmap || blkmap->naexts < nex) {
> +		blkmap = realloc(blkmap, BLKMAP_SIZE(nex));

Does the above really have to be a realloc() call, or can
it simply be a free()/malloc() instead?  Also, could the
existing ts_alloc() function be adjusted to accomodate the
usage here?

> +		if (!blkmap) {
> +			do_warn(_("malloc failed in blkmap_alloc (%u bytes)\n"),
> +				BLKMAP_SIZE(nex));

. . .

> @@ -131,14 +71,7 @@ void
>  blkmap_free(
>  	blkmap_t	*blkmap)
>  {
> -	blkent_t	**entp;
> -	xfs_extnum_t	i;
> -
> -	if (blkmap == NULL)
> -		return;
> -	for (i = 0, entp = blkmap->ents; i < blkmap->nents; i++, entp++)
> -		free(*entp);
> -	free(blkmap);
> +	/* nothing to do! - keep the memory around for the next inode */

Nobody ever frees it though, either.  I guess it gets done at
exit but I like things tidy (could arrange for a destructor
function to be called, at pthread_key_create() time).

>  }
> 
>  /*

. . .