Re: 2.6.8.1-mm2

[Rik van Riel]

On Thu, 19 Aug 2004, Andrew Morton wrote:

> - Added the reiser4 filesystem.

Time for a quick scan through the code, comments in order in which
I ran into stuff, not in order of importance.

> reiser4-include-reiser4.patch

Might be an idea to trim some of the excess text from the help
entry and make things a bit more professional.

> reiser4-perthread-pages.patch

If a task exits unexpectedly, it will leak the reserved pages.
This memory leak wants fixing...

Also, why the !in_interrupt() test in perthread_pages_alloc() ?
Surely this function shouldn't be called from interrupts, since
it is a general purpose pool of pages.

> reiser4-radix-tree-tag.patch

Just a nitpick here, could we rename PAGECACHE_TAG_FS_SPECIFIC
to PAGECACHE_TAG_FS_PRIVATE, since we're using the name "private"
in half a number of other places for the exact same purpose ?

> reiser4-radix_tree_lookup_slot.patch

Having reiserfs dig into the radix tree looks like a layering
violation to me.  If there is a real need to replace pagecache
pages with other pages in the radix tree, maybe we should have
a function to do that in the pagecache code, leaving reiserfs
to call things at the right abstraction level ?

I see a potential for race conditions when reiserfs changes a
page which write has just looked up, and what about mmap?
Even if the code is safe now, this is bound to result in a
maintenance nightmare down the road.

> reiser4-truncate_inode_pages_range.patch

This has the same race issue as any of the "hole punch"
patches that have been floating around in the past.  The
truncate path has some (subtle!) race prevention that
depends on the nopage functions not searching past i_size,
but this hole punch code doesn't.

I am not convinced this is SMP safe.

cheers,

Rik
-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

-
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/