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

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

[Miklos Szeredi]

Al,

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

Thanks,
Miklos
--

[patch 1/3] vfs: dcache cleanups

[Miklos Szeredi]

[-- Attachment #1: vfs-dcache-cleanups.patch --]
[-- Type: text/plain, Size: 6278 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

d_path() cleanups and fix the following sparse warnings:

fs/dcache.c:2183:19: warning: symbol 'filp_cachep' was not declared. Should it be static?
fs/dcache.c:115:3: warning: context imbalance in 'dentry_iput' - unexpected unlock
fs/dcache.c:188:2: warning: context imbalance in 'dput' - different lock contexts for basic block
fs/dcache.c:400:2: warning: context imbalance in 'prune_one_dentry' - different lock contexts for basic block
fs/dcache.c:431:22: warning: context imbalance in 'prune_dcache' - different lock contexts for basic block
fs/dcache.c:563:2: warning: context imbalance in 'shrink_dcache_sb' - different lock contexts for basic block
fs/dcache.c:1385:6: warning: context imbalance in 'd_delete' - wrong count at exit
fs/dcache.c:1636:2: warning: context imbalance in '__d_unalias' - unexpected unlock
fs/dcache.c:1735:2: warning: context imbalance in 'd_materialise_unique' - different lock contexts for basic block

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Reviewed-by: Matthew Wilcox <willy@linux.intel.com>
Acked-by: Christoph Hellwig <hch@infradead.org>
---
 fs/dcache.c |   51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2008-06-11 10:16:10.000000000 +0200
+++ linux-2.6/fs/dcache.c	2008-06-13 13:12:57.000000000 +0200
@@ -17,6 +17,7 @@
 #include <linux/syscalls.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/fdtable.h>
 #include <linux/fs.h>
 #include <linux/fsnotify.h>
 #include <linux/slab.h>
@@ -106,9 +107,10 @@ static void dentry_lru_remove(struct den
 /*
  * Release the dentry's inode, using the filesystem
  * d_iput() operation if defined.
- * Called with dcache_lock and per dentry lock held, drops both.
  */
 static void dentry_iput(struct dentry * dentry)
+	__releases(dentry->d_lock)
+	__releases(dcache_lock)
 {
 	struct inode *inode = dentry->d_inode;
 	if (inode) {
@@ -132,12 +134,13 @@ static void dentry_iput(struct dentry * 
  * d_kill - kill dentry and return parent
  * @dentry: dentry to kill
  *
- * Called with dcache_lock and d_lock, releases both.  The dentry must
- * already be unhashed and removed from the LRU.
+ * The dentry must already be unhashed and removed from the LRU.
  *
  * If this is the root of the dentry tree, return NULL.
  */
 static struct dentry *d_kill(struct dentry *dentry)
+	__releases(dentry->d_lock)
+	__releases(dcache_lock)
 {
 	struct dentry *parent;
 
@@ -383,11 +386,11 @@ restart:
  * Try to prune ancestors as well.  This is necessary to prevent
  * quadratic behavior of shrink_dcache_parent(), but is also expected
  * to be beneficial in reducing dentry cache fragmentation.
- *
- * Called with dcache_lock, drops it and then regains.
- * Called with dentry->d_lock held, drops it.
  */
 static void prune_one_dentry(struct dentry * dentry)
+	__releases(dentry->d_lock)
+	__releases(dcache_lock)
+	__acquires(dcache_lock)
 {
 	__d_drop(dentry);
 	dentry = d_kill(dentry);
@@ -1604,10 +1607,9 @@ static int d_isparent(struct dentry *p1,
  *
  * Note: If ever the locking in lock_rename() changes, then please
  * remember to update this too...
- *
- * On return, dcache_lock will have been unlocked.
  */
 static struct dentry *__d_unalias(struct dentry *dentry, struct dentry *alias)
+	__releases(dcache_lock)
 {
 	struct mutex *m1 = NULL, *m2 = NULL;
 	struct dentry *ret;
@@ -1743,7 +1745,6 @@ out_nolock:
 shouldnt_be_hashed:
 	spin_unlock(&dcache_lock);
 	BUG();
-	goto shouldnt_be_hashed;
 }
 
 static int prepend(char **buffer, int *buflen, const char *str,
@@ -1758,7 +1759,7 @@ static int prepend(char **buffer, int *b
 }
 
 /**
- * d_path - return the path of a dentry
+ * __d_path - return the path of a dentry
  * @path: the dentry/vfsmount to report
  * @root: root vfsmnt/dentry (may be modified by this function)
  * @buffer: buffer to return value in
@@ -1779,8 +1780,9 @@ char *__d_path(const struct path *path, 
 {
 	struct dentry *dentry = path->dentry;
 	struct vfsmount *vfsmnt = path->mnt;
-	char * end = buffer+buflen;
-	char * retval;
+	char *end = buffer + buflen;
+	char *retval;
+	struct qstr *name;
 
 	prepend(&end, &buflen, "\0", 1);
 	if (!IS_ROOT(dentry) && d_unhashed(dentry) &&
@@ -1812,8 +1814,8 @@ char *__d_path(const struct path *path, 
 		}
 		parent = dentry->d_parent;
 		prefetch(parent);
-		if ((prepend(&end, &buflen, dentry->d_name.name,
-				dentry->d_name.len) != 0) ||
+		name = &dentry->d_name;
+		if ((prepend(&end, &buflen, name->name, name->len) != 0) ||
 		    (prepend(&end, &buflen, "/", 1) != 0))
 			goto Elong;
 		retval = end;
@@ -1824,8 +1826,8 @@ char *__d_path(const struct path *path, 
 
 global_root:
 	retval += 1;	/* hit the slash */
-	if (prepend(&retval, &buflen, dentry->d_name.name,
-		    dentry->d_name.len) != 0)
+	name = &dentry->d_name;
+	if (prepend(&retval, &buflen, name->name, name->len) != 0)
 		goto Elong;
 	root->mnt = vfsmnt;
 	root->dentry = dentry;
@@ -1845,7 +1847,7 @@ Elong:
  *
  * Returns the buffer or an error code if the path was too long.
  *
- * "buflen" should be positive. Caller holds the dcache_lock.
+ * "buflen" should be positive.
  */
 char *d_path(struct path *path, char *buf, int buflen)
 {
@@ -1915,16 +1917,13 @@ char *dentry_path(struct dentry *dentry,
 	retval = end-1;
 	*retval = '/';
 
-	for (;;) {
-		struct dentry *parent;
-		if (IS_ROOT(dentry))
-			break;
+	while (!IS_ROOT(dentry)) {
+		struct dentry *parent = dentry->d_parent;
+		struct qstr *name;
 
-		parent = dentry->d_parent;
 		prefetch(parent);
-
-		if ((prepend(&end, &buflen, dentry->d_name.name,
-				dentry->d_name.len) != 0) ||
+		name = &dentry->d_name;
+		if ((prepend(&end, &buflen, name->name, name->len) != 0) ||
 		    (prepend(&end, &buflen, "/", 1) != 0))
 			goto Elong;
 
@@ -1975,7 +1974,7 @@ asmlinkage long sys_getcwd(char __user *
 	error = -ENOENT;
 	/* Has the current directory has been unlinked? */
 	spin_lock(&dcache_lock);
-	if (pwd.dentry->d_parent == pwd.dentry || !d_unhashed(pwd.dentry)) {
+	if (IS_ROOT(pwd.dentry) || !d_unhashed(pwd.dentry)) {
 		unsigned long len;
 		struct path tmp = root;
 		char * cwd;

--

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

[Miklos Szeredi]

[-- Attachment #1: vfs-fix-sys_getcwd-for-detached-mounts.patch --]
[-- Type: text/plain, Size: 1540 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

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 <jjohansen@suse.de>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: Christoph Hellwig <hch@infradead.org>
---
 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;

--

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

[Al Viro]

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:<rest of path> 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?

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

[Miklos Szeredi]

> > 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:<rest of path> 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

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

[Al Viro]

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:<rest of path> 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.

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

[Miklos Szeredi]

[-- Attachment #1: vfs-make-d_path-consistent-across-mount-operations.patch --]
[-- Type: text/plain, Size: 2119 bytes --]

From: Andreas Gruenbacher <agruen@suse.de>

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 <agruen@suse.de>
Signed-off-by: John Johansen <jjohansen@suse.de>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: Christoph Hellwig <hch@infradead.org>
---
 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;
 }
 
 /**

--

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

[Al Viro]

On Mon, Jun 16, 2008 at 01:28:07PM +0200, Miklos Szeredi wrote:
> From: Andreas Gruenbacher <agruen@suse.de>
> 
> 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.

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

[Andreas Gruenbacher]

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