[Cluster-devel] [PATCH 4/8] libgfs2: Improve and simplify blk_alloc_in_rg

[Andrew Price <anprice@redhat.com>]

Hi Bob,

On 27/01/14 15:46, Bob Peterson wrote:
> ----- Original Message -----
> | This function included naive implementations of gfs2_setbit and
> | gfs2_bitfit so these have been replaced with calls to those functions.
> | The 'type' parameter has been replaced with 'state' and the type
> | defines removed in favour of the GFS2_BLKST_* values. As these were the
> | same for both meta and data block allocations, data_alloc() has been
> | removed and its callers updated to use meta_alloc().
> |
> | Signed-off-by: Andrew Price <anprice@redhat.com>
>
> Hi,
>
> I've been staring at this patch for a long time now.
> The patch looks correct. However, my concern is this:
>
> The fsck.gfs2 tool is designed to repair either GFS2 or GFS1 file systems,
> and GFS1 uses bitmaps differently from GFS2. An indirect block, for example,
> will be marked as a data block in a GFS2 file system, but "meta" in GFS1,
> (which translates to unlinked dinode in a GFS2 bitmap).
>
> Of course, since this is upstream code, it's tempting to say that there
> won't be any GFS1 file systems hanging around. However, in theory, users
> might take a GFS1 file system from a legacy system and try migrate it to
> a newer system, upstream, RHEL7, Fedora, whatever, in which case they
> want to use gfs2_convert. But gfs2_convert doesn't do any error checking,
> so we recommend running fsck before the convert. However, on newer OSes,
> there isn't a gfs_fsck, there's only fsck.gfs2, which should handle both.

I'm certainly not suggesting dropping gfs1 support in fsck.gfs2 with 
this patch, and it really shouldn't be semantically different to before 
these changes anyway since it's just a code reorganisation.

> So we need to ask some hard questions:
>
> (1) Does it matter? If the fsck is only being run for the sake of sanity
>      for gfs2_convert, allocating those "metadata" blocks as "data" blocks
>      will likely be all right, since gfs2_convert will need to convert them
>      anyway.

I'm not sure where the problem you're describing might be. Before this 
patch both meta_alloc() and data_alloc() allocated blocks using the 
GFS2_BLKST_USED state:

     -       case DATA:
     -       case META:
     -               state = GFS2_BLKST_USED;
     -               break;

so their effects were identical and that's why I felt it ok to combine 
those functions. I've not got my hands very dirty with gfs1 code but I 
understand that, whereas gfs2 has _UNLINKED and _DINODE, gfs1 has 
_FREEMETA and _USEDMETA, so perhaps we just need to look at whether the 
gfs1 code is calling the correct allocation function for the block state 
it requires.

> (2) From the patch, it looks like "data" and "meta" are being treated the
>      same anyway, so is there a bug in today's fsck.gfs2? This might
>      show up if there's a GFS1 file system that has enough damage as to
>      push a pile of directory entries into lost+found, thus increasing its
>      size enough to add indirect blocks. Due to that same bug, fsck.gfs2
>      might not catch the discrepancy, so we probably should check it by hand
>      using gfs2_edit.
> (3) If there is a bug with how fsck.gfs2 handles GFS1 bitmaps, do we need
>      to fix it? Or is it too much work for too little gain?

If we do find a bug here I think we should fix it if at all possible. If 
we ever do drop support for gfs1 in fsck.gfs2 it should be done in a 
more considered way than letting the gfs1 bits go rotten :)

> (4) I've got a test case I run called fsck.gfs2.nightmare2.sh, which tests
>      fsck against my entire collection of metadata sets, both GFS and GFS2.
>      It can run for days, depending on the hardware. Do the GFS1 metadata
>      sets still pass with this patch?

Well I would like to subject these patches to those tests in any case 
(and add some of them to the in-tree test suite where possible), but the 
intention here was to better organise the code rather than change any 
functionality so I would be surprised if this patch introduced the bug 
that you're theorizing rather than simply highlighting it.

Andy