On Mon, 2008-12-22 at 18:07 +0900, Ryusuke Konishi wrote: > Hi Chris, > > On Wed, 17 Dec 2008 21:52:55 -0500, Chris Mason wrote: > > 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! >
;) I'll be slow to answer since I'm traveling a bit. > > 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. > > > 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. 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. > > - 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 ;) > > > > 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). -chris _______________________________________________ users mailing list [email protected] https://www.nilfs.org/mailman/listinfo/users
