Some nilfs comments

Hello,

I've been meaning to read through nilfs for a while, and I grabbed the code today to take a look. The code is very clean and it ran well out of the box here.

One problem I hit early on was that nilfs doesn't seem to zero out the block device during mkfs. So after mkfs.nilfs2, mount still found my old btrfs filesystem. It would be a good idea to zero out the first and last 1MB on the device (except for the first sector which might have the partition table).

I haven't dug too deeply in yet, but if there are parts you're most interested in comments on, please let me know.

It looks like nilfs_writepage ignores WB_SYNC_NONE, which is used by do_sync_mapping_range().

nilfs_page_mkwrite doesn't seem to dirty the page? block_page_mkwrite does more, including checks against i_size and others.

readpages and O_DIRECT both set b_size on the map_bh to reflect the total size of the region they are trying to map. It seems like an easy optimization to map more in nilfs_get_block.

Hopefully this helps, I'll try to send a few more ideas after I get to know the code better.

-chris

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- struct the_nilfs:
NILFS allows users to mount snapshots without making additional devices or volumes. This is achieved by sharing a block device among multiple mount instances (i.e. super_block structs). the_nilfs struct is used for this sharing.

This approach seems to be peculiar to nilfs, and I feel it needs attention.

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

Thanks, I've also started to read btrfs.
I'll see it during the Christmas holidays ;)

Quoted 11 lines · Ryusuke Konishi

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- struct the_nilfs:
NILFS allows users to mount snapshots without making additional devices or volumes. This is achieved by sharing a block device among multiple mount instances (i.e. super_block structs). the_nilfs struct is used for this sharing.

This approach seems to be peculiar to nilfs, and I feel it needs attention.

I've dug into this too and decided to use the same mechanism for my implementation. I think its nice to have all the fs's administration centered at one place; i even have all the btree stuff there. One thing indeed is that its not possible to do say `unmount /dev/wd0a' if there are multiple mounts on it... but maybe thats better even ;) or its even a positive thing to just unmount all the mounts in one go.

Quoted 2 lines

Thanks, I've also started to read btrfs.
I'll see it during the Christmas holidays ;)

Whats your first impression? Want to take over things/ideas? Isn't that a quite different FS structurally since it doesn't have segments? (AFAIK!)

Quoted 5 lines

Quoted 2 lines

Thanks, I've also started to read btrfs.
I'll see it during the Christmas holidays ;)

Whats your first impression? Want to take over things/ideas? Isn't that a quite different FS structurally since it doesn't have segments? (AFAIK!)

Yeah, btrfs has flexible and rich volume management layer, so very interesting. The snapshot code and c-tree also look like fun.

Hi Chris,

Quoted 5 lines · Chris Mason

Hello,

I've been meaning to read through nilfs for a while, and I grabbed the code today to take a look. The code is very clean and it ran well out of the box here.

Thank you very much for helpful comments!

Quoted 5 lines

One problem I hit early on was that nilfs doesn't seem to zero out the block device during mkfs. So after mkfs.nilfs2, mount still found my old btrfs filesystem. It would be a good idea to zero out the first and last 1MB on the device (except for the first sector which might have the partition table).

Okay, I'll take this idea in mkfs.nilfs2.

Quoted 2 lines

I haven't dug too deeply in yet, but if there are parts you're most interested in comments on, please let me know.

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- struct the_nilfs:
NILFS allows users to mount snapshots without making additional devices or volumes. This is achieved by sharing a block device among multiple mount instances (i.e. super_block structs). the_nilfs struct is used for this sharing.

This approach seems to be peculiar to nilfs, and I feel it needs attention.

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

Quoted 4 lines

It looks like nilfs_writepage ignores WB_SYNC_NONE, which is used by do_sync_mapping_range().

Thanks! I didn't notice that this function was added. Uum, it seems to require reconsidering the way to initiate writing of data pages.

Quoted 8 lines

- struct the_nilfs:
NILFS allows users to mount snapshots without making additional devices or volumes. This is achieved by sharing a block device among multiple mount instances (i.e. super_block structs). the_nilfs struct is used for this sharing.

This approach seems to be peculiar to nilfs, and I feel it needs attention.

Btrfs also shares super blocks between snapshot mounts, and each snapshot becomes a private inode number space.

So, it sounds like we have some things in common there. I'm looking at using more explicit mounts than I am now (with some hints from Christoph). I'll take a look at the nilfs code in this area and see how similar we really are.

Quoted 5 lines

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

Ok, I'll take a look here as well. The same goes for btrfs ;)

Quoted 6 lines

Quoted 2 lines

It looks like nilfs_writepage ignores WB_SYNC_NONE, which is used by do_sync_mapping_range().

Thanks! I didn't notice that this function was added. Uum, it seems to require reconsidering the way to initiate writing of data pages.

Nick Piggin has been making noise to change do_sync_mapping_range to pass WB_SYNC_ALL instead. You can trick it a bit while things get worked out and use current_is_pdflush() to only treat WB_SYNC_NONE as WB_SYNC_NONE when pdflush is the one calling (sorry, its an ugly suggestion).

Quoted 2 lines

I haven't dug too deeply in yet, but if there are parts you're most interested in comments on, please let me know.

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

Quoted 12 lines · Ryusuke Konishi

Quoted 2 lines

I haven't dug too deeply in yet, but if there are parts you're most interested in comments on, please let me know.

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

It took me far too long to look at this, I'm sorry. But, looking through the ioctl code I think you'll have a few (easy to fix) problems.

struct nilfs_argv {
        void *v_base;
        size_t v_nmembs;        /* number of members */
        size_t v_size;          /* size of members */
        int v_index;
        int v_flags;
};

The structs that get passed from userland to kernel should use fixed sized types.

I get around this by using u64s (and __u64s) everywhere. It looks like you have compat ioctls to fix this too, I find it easier to use the fixed types.

Otherwise the ioctl code looks pretty reasonable.

-chris

Quoted 32 lines · Chris Mason

Quoted 12 lines · Ryusuke Konishi

Quoted 2 lines

I haven't dug too deeply in yet, but if there are parts you're most interested in comments on, please let me know.

Well, I feel that the following two matters are particularlly questionable and need to be checked:

- ioctl:
Ioctl interface (routines and structures) were implemented in an own way. These seems to be checked whether to comply with the rules of ioctl design.

It took me far too long to look at this, I'm sorry. But, looking through the ioctl code I think you'll have a few (easy to fix) problems.

struct nilfs_argv {
        void *v_base;
        size_t v_nmembs;        /* number of members */
        size_t v_size;          /* size of members */
        int v_index;
        int v_flags;
};

The structs that get passed from userland to kernel should use fixed sized types.

I get around this by using u64s (and __u64s) everywhere. It looks like you have compat ioctls to fix this too, I find it easier to use the fixed types.

Quoted 2 lines

Otherwise the ioctl code looks pretty reasonable.

I think it actually should be fixed to comply with the kernel rules. The same problem is found in a few other ioctl structures.

I'd like to modify them and remove compat ioctls some time soon.