xfs
[Top] [All Lists]

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

To: "Christoph Hellwig" <hch@xxxxxxxxxxxxx>
Subject: RE: [PATCH 09/14] repair: track logical to physical block mapping moreeffeciently
From: "Alex Elder" <aelder@xxxxxxx>
Date: Wed, 21 Oct 2009 14:06:19 -0500
Cc: "Barry Naujok" <bnaujok@xxxxxxx>, <xfs@xxxxxxxxxxx>
In-reply-to: <20090902175841.284697389@xxxxxxxxxxxxxxxxxxxxxx>
Thread-index: Acor/SdIGFrU3xWBSayVlv//xvOCAAme/71w
Thread-topic: [PATCH 09/14] repair: track logical to physical block mapping moreeffeciently
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@xxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

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

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> 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).

>  }
> 
>  /*

. . .

<Prev in Thread] Current Thread [Next in Thread>
  • RE: [PATCH 09/14] repair: track logical to physical block mapping moreeffeciently, Alex Elder <=