Re: reiserfs support

[Andrew Clausen]

Hi Yura,

On Fri, Jan 04, 2002 at 09:16:53PM +0200, Yura Umanets wrote:
> Hi all!
> 
> I have done reiserfs support for parted. Sorry,  but reiserfs_create not 
> stable yet and I don't include it in my patch.
> I will send a patch tomorrow.

No problems :)  Anyway, stability is less important than cleanliness
and maintainability at this stage.  (It is much easier to get stability
from the later two, than the other direction, right?)

> Implemented: reiserfs_open, reiserfs_close, reiserfs_resize, 
> reiserfs_get_resize_constraint.

Excellent :)

Some questions:
* you haven't introduced any dependency on libreiser or whatever
it's called.  Does that mean you intend to maintain both pieces
of code in parrallel?  Is this code a hacked-up version of
something else?  (it looks like it!)
        In general: maintaining headers this way is ok, but
anything else is much more work.

* the copyright information looks incorrect.  What is the status
of this anyway?  (Do you intend to assign to the FSF?  IANAL, but
you still get a non-exclusive licence, etc.)

* an online resizer for reiser is available, right?  I guess
it could be nice to include support for this.  Do you think
a get_online_resize_constraint() and resize_online() interface
in PedFileSystemOps is sufficient?  For ext2, it appears more
complicated than this (see ext2prepare).  This is the main reason
I've shied away from it so far.

Also, some comments:
* the (possibly undocumented, sorry) convention wrt exceptions
in libparted is that the callee is responsible for throwing
exceptions.  Therefore things like:

        if (!ped_geometry_write (...))
                ped_exception_throw (..

Are stupid.  (See _reiserfs_block_read and friends)

* in general, I find the code difficult to understand, with
bad choices for variable names, etc.  I remember having the same
opinion when I looked at reiserfs extensively about 1 year ago.
For example, look at _reiserfs_block_move_generic():

> /* Moving of the generic file system block */
> static unsigned long _reiserfs_block_move_generic(reiserfs_fs_t *fs,
>    unsigned long block, unsigned long bnd, int h)

What are bnd and h?  What is being returned?  I think it would be
wise to typedef unsigned long to reiser_blk or similar.

Anyway, it seems the "right" interface is:

+ /* moves a generic block, returning the new block */
+ static reiserfs_blk_t
+ _reiserfs_block_move_generic (reiserfs_fs_t *fs, reiserfs_blk_t blk)

If you need extra state (like what the maximum block is you can
allocate... i.e. bnd), then you should probably have a separate
struct to contain all this state, rather than pass it around
everywhere.  The result is it's clear what information is central
to the operation.


-    struct buffer_head *bh, *bh2;
-    unsigned long unused_block = 1;

Bad variable names.  It's obvious that bh and bh2 are buffer
heads.  What isn't obvious is what they are used for... which
is what the variable names should be describing.  bh is
blk's buffer, and bh2 and unused_block's buffer.

unused_block is also a bad name, because it describes how it
is computed, not what it is.  (mixing levels of abstractions!
yuck!)

A better name is new_blk.  So, replacement code is:

+       reiserfs_blk_t new_blk;
+       struct buffer_head blk_bh, new_blk_bh;


-     /* primitive fsck */
-     if (block > get_sb_block_count(fs->fs_ondisk_sb)) {
-         ped_exception_throw(PED_EXCEPTION_ERROR, PED_EXCEPTION_CANCEL,
-             _("Invalid block number (%lu) found"), block);
-         return 0;
-     }

This is checking for a bug, not a on-disk inconsistency.  Much
easier (and efficient) to use PED_ASSERT:

+        PED_ASSERT (block > get_sb_block_count(fs->fs_ondisk_sb),
+                    return 0);


-     total_node_cnt ++;
- 
-     /* infinite loop check */
-     if(total_node_cnt > blocks_used && !block_count_mismatch) {
-         ped_exception_throw(PED_EXCEPTION_WARNING, PED_EXCEPTION_IGNORE,
-             _("Block count exeeded"));
-         block_count_mismatch = 1;
-     }
- 
-     if (block < bnd) return 0;
...

This is code is operating on a different levels of abstraction.  It
is defining HOW to allocate a block when all it should be doing
is defining how to MOVE a block.  This is my biggest problem with
reiser code in general.
        Why can't you call some _reiserfs_alloc_block() function?

Also, why is the decision being made to move the block in a function
called "move"?  (another mixing of levels of abstraction)


Here is my final version, with more coments:

/* even nicer is to have old_fs and new_fs, and be able to manipulate
 * both simultaneously, like get_sb_block_count (ctx->new_fs).
 * See parted's FAT code for an example...
 */
typedef struct {
        reiserfs_fs_t*  fs;
        reiserfs_blk_t  old_size;
        reiserfs_blk_t  new_size;
} reiserfs_resize_ctx_t;

/* moves a generic block, returning the new block */
static reiserfs_blk_t
_reiserfs_block_move_generic (reiserfs_resize_ctx_t* ctx, reiserfs_blk_t blk)
{
        reiserfs_blk_t new_blk;
        struct buffer_head blk_bh, new_blk_bh;

        PED_ASSERT (block > get_sb_block_count(ctx->fs->fs_ondisk_sb),
                    goto error);

        new_blk = _reiserfs_block_alloc_bounded (ctx->fs, ctx->new_size);
        if (!new_blk)
                goto error;

        /* fix the _reiserfs_block_{read,get} interfaces to look like
         * this!
         */
        blk_bh = _reiserfs_block_read (ctx->fs, block);
        if (!blk_bh)
                goto error;
        new_blk_bh = _reiserfs_block_get (ctx->fs, new_blk);
        if (!new_blk_bh)
                goto error_free_blk_bh;

        memcpy (new_blk_bh->data, blk_bh->data, blk_bh->b_size);

        /* should probably have front-ends, like _reiserfs_mark_free,
         * etc.  At a minimum, a better name for fs_bitmap2.
         */
        _reiserfs_bitmap_clear_bit(fs->fs_bitmap2, blk);
        _reiserfs_bitmap_set_bit(fs->fs_bitmap2, new_blk);

        if (!_reiserfs_block_write(ctx->fs, new_blk_bh))
                goto error_free_new_blk_bh;

        _reiserfs_block_release(blk_bh);
        _reiserfs_block_release(new_blk_bh);
        return new_blk;

error_free_new_blk_bh:
        _reiserfs_block_release(new_blk_bh);
error_free_blk_bh: 
        _reiserfs_block_release(blk_bh);
error:
        return 0;
}


If you expect me to maintain any of this code, then I expect
big improvements in this direction.  OTOH, if you are going
to maintain it, it's your problem, and I won't be overly
authoritarian ;)  Either way, I think my suggestions will
lead to a better reiserfs.

If you want me to comment more, I'm happy to.

Thanks,
Andrew