Re: [RFC] Tux3 for review

[Dave Chinner]

On Fri, May 16, 2014 at 05:50:59PM -0700, Daniel Phillips wrote:
> We would like to offer Tux3 for review for mainline merge. We have
> prepared a new repository suitable for pulling:
> 
> https://git.kernel.org/cgit/linux/kernel/git/daniel/linux-tux3.git/
> 
> Tux3 kernel module files are here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/daniel/linux-tux3.git/tree/fs/tux3
> 
> Tux3 userspace tools and tests are here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/daniel/linux-tux3.git/tree/fs/tux3/user?h=user

Post patches for review, please.  Go and look at the process used to
merge f2fs for an example of how to filesystem merged....

Ignoring this, I had a quick look at the code.  This is not a code
review - it's a message to tell everyone else not to waste their
time looking at the code right now...

The code is a dog's breakfast of #ifdef hackery, stuff that doesn't
work (lots of code surrounded by "#if 0"), there's "#if __KERNEL__
...  #else .... #endif" all through the code, etc. The "declarations
within code" stuff is just horrible - it's not even used
consistently so it just looks like laziness to me.  IOWs, the code
is an ugly mess and needs a serious amount of cleanup work. Example:

static const struct inode_operations tux_file_iops = {
//      .permission     = ext4_permission,
        .setattr        = tux3_setattr,
        .getattr        = tux3_getattr,
#ifdef CONFIG_EXT4DEV_FS_XATTR
//      .setxattr       = generic_setxattr,
//      .getxattr       = generic_getxattr,
//      .listxattr      = ext4_listxattr,
//      .removexattr    = generic_removexattr,
#endif
//      .fallocate      = ext4_fallocate,
//      .fiemap         = ext4_fiemap,
        .update_time    = tux3_file_update_time,
};

That's code ready for review and merging? Really?

The hacks around VFS and MM functionality need to have demonstrated
methods for being removed. We're not going to merge that page
forking stuff (like you were told at LSF 2013 more than a year ago:
http://lwn.net/Articles/548091/) without rigorous design review and
a demonstration of the solutions to all the hard corner cases it
has. The current code doesn't solve them (e.g. direct IO doesn't
work in tux3), and there's no clear patch set we can review that
demonstrates how it is all supposed to work. i.e. you need to
separate out all the page forking code into a separate patchset for
review, independent of the tux3 code and applies to the core mm/
code.

Then there's all the writeback hacks. You've simply copy-n-pasted
most of fs-writeback.c, including duplicating structures like struct
wb_writeback_work and then hacked in crap (kallsyms lookups!) to be
able to access core structures from kernel module context
(tux3_setup_writeback(), I'm looking at you). This is completely
unacceptible for a merge. Again, you need to separate out all the
writeback changes you need into an independent patchset so that they
can be reviewed independently of the tux3 code that uses it. 

Now, one of the big features tux3 you hyped is built-in snapshotting
capability. All that talk efficient pointer trees (or whatever they
were called) and being so much better than ZFS/btrfs-like COW.
Well, I can't find it anywhere in the code - the only references to
snapshots are 5 comments like this:

	* FIXME: what happen if snapshot was introduced?

IOWs, tux3 is just a prototype of a standard journaling filesystem.
The tux3 code is still missing large parts of it's intended core
functionality and there is nothing to tell us when that might
appear. It really appears to me that tux3 is where btrfs was 5-6
years ago - the core of an idea, but a long, long way from being
feature complete or production ready. btrfs still doesn't handle
ENOSPC well and given that tux3's is following the same development
path (BUG on ENOSPC) it doesn't fill me with any confidence that
tux3 is going to turn out any better than btrfs in 5 years time.

Really, I don't see how you plan to bring tux3 to be feature
complete and production ready in less than 2-3 years. The current
code is barely functional at this point and there's still questions
that haven't been answered about whether core tux3 functionality can
even be made to work properly, let alone integrated effectively.

IMO, it's a waste of time right now asking anyone to review this
code for inclusion until it has been cleaned up, the core
infrastructure problems have been solved and the core filesystem
code is much closer to feature complete.....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/