Re: 2.6.8.1-mm2

From: Rik van Riel (riel_at_redhat.com)
Date: 08/21/04

  • Next message: David S. Miller: "[PATCH] fix bug in sparc64 user copy patch" 
    Date:	Fri, 20 Aug 2004 18:43:28 -0400 (EDT)
To: Andrew Morton <akpm@osdl.org>

    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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
  • Next message: David S. Miller: "[PATCH] fix bug in sparc64 user copy patch"

    Relevant Pages

    • Re: 2.6.8.1-mm2
      ... > Having reiserfs dig into the radix tree looks like a layering ... > pages with other pages in the radix tree, ... > I see a potential for race conditions when reiserfs changes a ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: 2.6.8.1-mm2
      ... it will leak the reserved pages. ... >Having reiserfs dig into the radix tree looks like a layering ... >I see a potential for race conditions when reiserfs changes a ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: question about /proc/<PID>/mem in 2.4 (fwd)
      ... Kind regards ... > the whole design of the dump/suid race fixing work rather than me. ... I followed the advice of Alan Cox and looked up "Solar Designer" on ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: [PATCH] ReiserFS v3 I/O error handling
      ... > One of the most common complaints I've heard about ReiserFS is how ... > graceless it is in handling critical I/O errors. ... > ReiserFS doesn't offer the admin any such choice, ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • 128G Limit in Reiserfs? Or the Kernel? Or something else?
      ... I seem to have hit an odd limit, ... filesystem on it I decided to put a ReiserFS ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)