[lkml]   [2010]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 01/46] Revert "fs: use RCU read side protection in d_validate"
    On Wed, Dec 08, 2010 at 08:38:24PM +1100, Nick Piggin wrote:
    > On Wed, Dec 08, 2010 at 12:16:56PM +1100, Dave Chinner wrote:
    > > On Sat, Nov 27, 2010 at 08:56:03PM +1100, Nick Piggin wrote:
    > > > This reverts commit 3825bdb7ed920845961f32f364454bee5f469abb.
    > > >
    > > > Patch is broken, you can't dget() without holding any locks!
    > >
    > > I believe you can - for the same reasons we can take a reference to
    > > an inode without holding the inode_lock. That is, as long as the
    > > caller already holds an active reference to the dentry,
    > > dget() can be used to take another reference without needing the
    > > dcache_lock.
    > >
    > > Such usage appears to be described in the comment above dget() and
    > > there's a BUG_ON() in dget() to catch callers that don't already
    > > have an active reference. An example of a valid unlocked dget():
    > > d_alloc() does an unlocked dget() to take a reference to the parent
    > > dentry whichn we already are guaranteed to have a reference to.
    > Of course you can dget if you already have a reference :)

    Right, so the commit message is wrong. Can you update it to tell us why
    dget() can't be used there - the commit message from the second
    patch explained it far better....

    > > As to d_validate() - it depends on the caller behaviour as to
    > > whether the unlocked dget() is valid or not. From a cursory check
    > > of the NCP and SMB readdir caches, both appear to hold an active
    > > reference to the dentry it is passing to d_validate().
    > I don't see where? Can you point to where the refcount is taken?
    > AFAIKS it drops the reference 3 lines after it puts the pointer
    > into cache.

    Yeah, you're right, I missed that one - I spent more tiem checking
    the validation part of the code than the initial insertion. Hence
    my request:

    > > If that is
    > > the case then there is nothing wrong with the way d_validate uses
    > > dget(). Can someone with more SMB/NCP expertise than me validate the
    > > use of cached dentries?
    > Then why would it have to use d_validate if it has a reference?
    > That is supposed to be for an "untrusted" pointer (which is why
    > it had all the crazy checks that it's in kmem and in the right
    > slab etc).

    Code changes. It may not be doing what it was originally
    needed/intended to be doing - I don't need to waste time on code
    archeology and second guessing when there are others around that can
    tell me this off the top oftheir head. ;)


    Dave Chinner

     \ /
      Last update: 2010-12-09 01:47    [W:0.026 / U:68.472 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site