[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]