Re: [PATCH take 2] UBIFS - new flash file system

From: Adrian Hunter
Date: Fri May 16 2008 - 09:14:38 EST

Thanks for the review.

Christoph Hellwig wrote:
General comment:

- not supporting a flash page size different from the system page
size is a horrible thing for people trying to use the same storage
on multiple systems. For a block based filesystem that alone would
be enough reason not to merge it. For a flash filesystem I'm not
entirely sure given that flash isn't moved between systems all that

We are working on that. It will be added next week probably.

VFS/VM interaction comments:

- splitting most of the mount code out to build.c is rather odd.
Most filesystems have this in super.c

We will get rid of build.c and move everything to super.c.

- calling convention for mount_ubifs is nasty because it doesn't
clean up it's own errors. You might think it's easier to do
all that in ubifs_umount but beeing in the process of untangling
that mess for xfs I'd recomment against it. Unless there's
a very good reason for it functions should always clean up
the resources they allocated in the error case.


- ubifs_get_sb would benefit from splitting out a ubifs_fill_super
routine that allocates a new sb when it's actually needed.


- why do you do the commit_on_unmount in your own ->shutdown_super
instead of the normal ->put_super? If there's a good reason
this at least needs a big comment explaining why.

It is in ->shutdown_super to be outside BKL. Will add comment.

- ubifs_lookup doesn't really need to use d_splice_alias unless
you want to support nfs exporting

Well we would like to support nfs one day, so maybe we could just
leave it?

- in ubifs_new_inode you inherit the inode flags from the parent.
This probably wants splitting out in a helper that documents
explicitly what flags are inherited and what not. Given that
you store the general indoe flags settable by chattr in there
it seems like a bad idea to inherit them by default.


- the read dir implementation won't ever support nfs exporting
due to having to keep per open file state. Nor would it support
thing like checkpoint and restart.

We could get rid of the per-open-file state if we could come up
with a scheme that would make fpos unique. At the moment it is
not unique because it is the name hash, but we could perhaps
allocate a unique number to each colliding entry. We will
think some more about this.

- just opencode you mmap routine, there's nothing helpful
in generic_file_mmap if you set your own vm_ops.


- ubifs_trunc should never be called on anything but a regular
file, so the check for it seems superflous. Having it after
the S_ISREG is rather odd too even if you want to have
an assertation.


- please implement the unlocked_ioctl file operation instead of
the old ioctl operation that comes with the BKL locked.


Misc comments:

- ubifs_bg_thread shouldn't set a nice level, especially when it's the
default one anyway.


- the mainoop of ubifs_bg_thread looks a bit odd either, when you
first to an interruotible sleep and then possible one while you
still have TASK_RUNNING set. Also the need_bgt flag is not needed
because the thrad is only woken up to perform it's action.
In the end the main loop should look something like:

while (1) {
if (kthread_should_stop())
if (try_to_freeze())




Same comments on naming in the code:

- bgt is not very descriptive for your kernel thread. These are per
defintion in the background, so just call them thread or
<somethinginformative>_thread. In this case it would probably
be commit_thread.


- any chance you could spell out journal instead of jrn? jrn always
sounds like joern with the wovel eaten by a mailer.. :)

It is named after Jœrn not journal ;-)

How about jnl after Janelle? It uses the same number of characters.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at