Message-ID: <20080516104002.GA6962@infradead.org>
Date:	Fri, 16 May 2008 06:40:02 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Artem Bityutskiy <Artem.Bityutskiy@...ia.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Adrian Hunter <ext-adrian.hunter@...ia.com>
Subject: Re: [PATCH take 2] UBIFS - new flash file system

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.

VFS/VM interaction comments:

 - splitting most of the mount code out to build.c is rather odd.
   Most filesystems have this in 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.
 - ubifs_lookup doesn't really need to use d_splice_alias unless
   you want to support nfs exporting
 - 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.
 - 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())
			break;
		if (try_to_freeze())
			continue;

		run_bg_commit(c);

		set_current_state(TASK_INTERRUPTIBLE);
		schedule();
		__set_current_state(TASK_RUNNING);
	}

			


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.. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists