Messages in this thread | | | Date | Fri, 16 May 2008 16:10:13 +0300 | From | Adrian Hunter <> | Subject | Re: [PATCH take 2] UBIFS - new flash file system |
| |
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 > often.
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.
Ok
> - ubifs_get_sb would benefit from splitting out a ubifs_fill_super > routine that allocates a new sb when it's actually needed.
Ok
> - 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.
Ok
> - 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.
Ok
> - 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.
Ok
> - please implement the unlocked_ioctl file operation instead of > the old ioctl operation that comes with the BKL locked.
Ok
> > Misc comments: > > - ubifs_bg_thread shouldn't set a nice level, especially when it's the > default one anyway.
Ok
> - 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()) > break; > if (try_to_freeze()) > continue; > > run_bg_commit(c); > > set_current_state(TASK_INTERRUPTIBLE); > schedule(); > __set_current_state(TASK_RUNNING); > }
Ok
> > > > > 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.
Ok
> - 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |