Re: [PATCH] fs: Introducing Lanyard Filesystem

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sun, Aug 19, 2012 at 05:33:52PM +0200, Dan Luedtke wrote:

> Still wondering if anyone bothers to actually look at the code?

Some obvious notes:
	* unlimited recursion is a killer; here its depth is controlled
by the fs image contents and it's trivial to cook one that would overflow
kernel stack.  Seeing that you want to use it for removable media, that's
a gaping security hole right there
	* unlink() does *not* truncate the file contents; file that had been
opened and unlinked should keep its contents until it's closed.  The same
goes for overwriting rename().
	* while we are at it, neither of those should free the on-disk
inode; again, that should happen only when the inode is evicted.
	* I might be missing something, but copying a bunch of files
with something like cp /foo/* /mnt seems to be guaranteed to create
really lousy binary tree in target directory (they will go in lexicographical
order and you don't seem to rebalance the tree at all)
	* you are really abusing iget() there.  Leaving the locking issues
aside, that's going to get you icache filled with irrelevant stuff.  Moreover,
it's far too heavy a club; allocating and filling struct inode when all you
really need is name and a couple of pointers in the disk block?
	* minor point, but endianness-flipping in place is *the* way to get
hard-to-catch endianness bugs.  foo = cpu_to_le64(foo) is a bloody bad idea;
either use object for host-endian all along, or use it only for (in your
case) little-endian.