[patch 0/3] vfs: d_path cleanups and fixes - Kernel

This is a discussion on [patch 0/3] vfs: d_path cleanups and fixes - Kernel ; Al, Could you please take care of these dcache patches for 2.6.27? Thanks, Miklos -- -- 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 ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: [patch 0/3] vfs: d_path cleanups and fixes

  1. [patch 0/3] vfs: d_path cleanups and fixes

    Al,

    Could you please take care of these dcache patches for 2.6.27?

    Thanks,
    Miklos
    --
    --
    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/

  2. [patch 3/3] vfs: make d_path() consistent across mount operations

    From: Andreas Gruenbacher

    The path that __d_path() computes can become slightly inconsistent when it
    races with mount operations: it grabs the vfsmount_lock when traversing mount
    points but immediately drops it again, only to re-grab it when it reaches the
    next mount point. The result is that the filename computed is not always
    consisent, and the file may never have had that name. (This is unlikely, but
    still possible.)

    Fix this by grabbing the vfsmount_lock for the whole duration of
    __d_path().

    Signed-off-by: Andreas Gruenbacher
    Signed-off-by: John Johansen
    Signed-off-by: Miklos Szeredi
    Acked-by: Christoph Hellwig
    ---
    fs/dcache.c | 12 +++++++-----
    1 file changed, 7 insertions(+), 5 deletions(-)

    Index: linux-2.6/fs/dcache.c
    ================================================== =================
    --- linux-2.6.orig/fs/dcache.c 2008-06-13 13:13:01.000000000 +0200
    +++ linux-2.6/fs/dcache.c 2008-06-13 13:13:04.000000000 +0200
    @@ -1784,6 +1784,7 @@ char *__d_path(const struct path *path,
    char *retval;
    struct qstr *name;

    + spin_lock(&vfsmount_lock);
    prepend(&end, &buflen, "\0", 1);
    if (!IS_ROOT(dentry) && d_unhashed(dentry) &&
    (prepend(&end, &buflen, " (deleted)", 10) != 0))
    @@ -1802,14 +1803,11 @@ char *__d_path(const struct path *path,
    break;
    if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
    /* Global root? */
    - spin_lock(&vfsmount_lock);
    if (vfsmnt->mnt_parent == vfsmnt) {
    - spin_unlock(&vfsmount_lock);
    goto global_root;
    }
    dentry = vfsmnt->mnt_mountpoint;
    vfsmnt = vfsmnt->mnt_parent;
    - spin_unlock(&vfsmount_lock);
    continue;
    }
    parent = dentry->d_parent;
    @@ -1822,6 +1820,8 @@ char *__d_path(const struct path *path,
    dentry = parent;
    }

    +out:
    + spin_unlock(&vfsmount_lock);
    return retval;

    global_root:
    @@ -1841,9 +1841,11 @@ global_root:
    }
    root->mnt = vfsmnt;
    root->dentry = dentry;
    - return retval;
    + goto out;
    +
    Elong:
    - return ERR_PTR(-ENAMETOOLONG);
    + retval = ERR_PTR(-ENAMETOOLONG);
    + goto out;
    }

    /**

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

  3. [patch 2/3] vfs: fix sys_getcwd for detached mounts

    From: Miklos Szeredi

    Currently getcwd(2) on a detached mount will give a garbled result:

    > mkdir /mnt/foo
    > mount --bind /etc /mnt/foo
    > cd /mnt/foo/skel
    > /bin/pwd

    /mnt/foo/skel
    > umount -l /mnt/foo
    > /bin/pwd

    etcskel

    After the patch it will give a much saner "/skel" result.

    Thanks to John Johansen for pointing out this bug.

    Reported-by: John Johansen
    Signed-off-by: Miklos Szeredi
    Acked-by: Christoph Hellwig
    ---
    fs/dcache.c | 18 ++++++++++++++----
    1 file changed, 14 insertions(+), 4 deletions(-)

    Index: linux-2.6/fs/dcache.c
    ================================================== =================
    --- linux-2.6.orig/fs/dcache.c 2008-06-13 13:12:57.000000000 +0200
    +++ linux-2.6/fs/dcache.c 2008-06-13 13:13:01.000000000 +0200
    @@ -1825,10 +1825,20 @@ char *__d_path(const struct path *path,
    return retval;

    global_root:
    - retval += 1; /* hit the slash */
    - name = &dentry->d_name;
    - if (prepend(&retval, &buflen, name->name, name->len) != 0)
    - goto Elong;
    + /*
    + * If this is a root dentry, then overwrite the slash. This
    + * will also DTRT with pseudo filesystems which have root
    + * dentries named "foo:".
    + *
    + * Otherwise this is the root of a detached mount, so don't do
    + * anything.
    + */
    + if (IS_ROOT(dentry)) {
    + retval += 1;
    + name = &dentry->d_name;
    + if (prepend(&retval, &buflen, name->name, name->len) != 0)
    + goto Elong;
    + }
    root->mnt = vfsmnt;
    root->dentry = dentry;
    return retval;

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

  4. Re: [patch 2/3] vfs: fix sys_getcwd for detached mounts

    On Mon, Jun 16, 2008 at 01:28:06PM +0200, Miklos Szeredi wrote:

    > After the patch it will give a much saner "/skel" result.


    I'm not sure that /skel is much saner, to be honest. Existing
    behaviour is BS, no arguments about that, but I suspect that
    we want that to be recognizable. How about doing something
    like detached: instead (c.f. "pipe:[6969]" and
    its ilk)?

    Lack of leading / currently points to pathname *not* resolving
    to object; it might be worth doing the same here. Comments?

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

  5. Re: [patch 3/3] vfs: make d_path() consistent across mount operations

    On Mon, Jun 16, 2008 at 01:28:07PM +0200, Miklos Szeredi wrote:
    > From: Andreas Gruenbacher
    >
    > The path that __d_path() computes can become slightly inconsistent when it
    > races with mount operations: it grabs the vfsmount_lock when traversing mount
    > points but immediately drops it again, only to re-grab it when it reaches the
    > next mount point. The result is that the filename computed is not always
    > consisent, and the file may never have had that name. (This is unlikely, but
    > still possible.)
    >
    > Fix this by grabbing the vfsmount_lock for the whole duration of
    > __d_path().


    Umm... I don't see problems with doing that, but I hope you realize that
    the notion of "ever having that name" is not the same as "pathnam resolution
    on that name ever leading to that file" - path_walk() is *NOT* atomic
    wrt rename() (or mount --move, indeed) and it's quite possible to walk
    into subdirectory, have it moved under you, then see .. as the next pathname
    component and step out into new parent.

    Said that, it makes sense to avoid dropping/regaining the lock in that
    case - it's less work and I don't believe that it would increase contention
    on vfsmount_lock. So I'm applying that one, just be careful with assumptions
    about consistency, etc. in the general area.
    --
    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/

  6. Re: [patch 2/3] vfs: fix sys_getcwd for detached mounts

    > > After the patch it will give a much saner "/skel" result.
    >
    > I'm not sure that /skel is much saner, to be honest.


    Just for argument's sake: before the patch getcwd() on detached object
    wasn't consistent with any definition of "absolute path". After the
    patch it's at least consistent with defining it as the path from the
    ultimate reachable ancestor (which does have at least some historical
    relevance).

    > Existing
    > behaviour is BS, no arguments about that, but I suspect that
    > we want that to be recognizable. How about doing something
    > like detached: instead (c.f. "pipe:[6969]" and
    > its ilk)?


    OK, obviously current users don't care, otherwise somebody would have
    complained about this issue. So if we agree on "detached:...", I'm
    fine with that.

    Thanks,
    Miklos
    --
    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/

  7. Re: [patch 2/3] vfs: fix sys_getcwd for detached mounts

    On Mon, Jun 23, 2008 at 04:34:00PM +0200, Miklos Szeredi wrote:
    > > > After the patch it will give a much saner "/skel" result.

    > >
    > > I'm not sure that /skel is much saner, to be honest.

    >
    > Just for argument's sake: before the patch getcwd() on detached object
    > wasn't consistent with any definition of "absolute path". After the
    > patch it's at least consistent with defining it as the path from the
    > ultimate reachable ancestor (which does have at least some historical
    > relevance).


    Before the patch getcwd() on detached object generated junk, period.
    As for the path from ultimate reachable ancestor... I'm not sure that
    this definition actually matches any real-world problem.

    > > Existing
    > > behaviour is BS, no arguments about that, but I suspect that
    > > we want that to be recognizable. How about doing something
    > > like detached: instead (c.f. "pipe:[6969]" and
    > > its ilk)?

    >
    > OK, obviously current users don't care, otherwise somebody would have
    > complained about this issue. So if we agree on "detached:...", I'm
    > fine with that.


    Care to resend? BTW, one more thing - in the 1/3 I'd probably add a
    wrapper around prepend() that would take struct qstr * instead of
    name/length and used it instead of your locals. As in
    prepend_name(&end, &buflen, &dentry->d_name)
    etc.
    --
    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/

  8. Re: [patch 3/3] vfs: make d_path() consistent across mount operations

    On Monday 23 June 2008 16:01:44 Al Viro wrote:
    > Umm... I don't see problems with doing that, but I hope you realize that
    > the notion of "ever having that name" is not the same as "pathnam
    > resolution on that name ever leading to that file" - path_walk() is *NOT*
    > atomic wrt rename() (or mount --move, indeed) and it's quite possible to
    > walk into subdirectory, have it moved under you, then see .. as the next
    > pathname component and step out into new parent.


    Yes, that's understood. Relative lookups don't visit all
    directories up to the root (unless via ".."), so there can be
    infinite time between walking down a directory and computing
    a pathname for it.

    > Said that, it makes sense to avoid dropping/regaining the lock in that
    > case - it's less work and I don't believe that it would increase contention
    > on vfsmount_lock. So I'm applying that one, just be careful with
    > assumptions about consistency, etc. in the general area.


    Thanks.

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

+ Reply to Thread