[PATCH] 2.2.18: d_move() with self-root dentries

[PATCH] 2.2.18: d_move() with self-root dentries

[Chip Salzenberg]

The d_move() function doesn't correctly handle dentries that have no
parents (i.e. 'x->d_parent==x').  This patch lets it do so.

Normally, the only parentless dentries are filesystem roots.  However,
the NFS server creates (temporary) parentless dentries whenever dentry
pruning has deleted the dentries referring to clients' open files.

Making nfsd's d_splice() compensate for d_move's limitations is not
only a kludge, but also it harder to keep nfsd correct.  Besides,
someday, nfsd may not be the only creator of this kind of dentry.

Thus, this patch.  If you apply this, you'll also want to patch
fs/nfsd/nfsfh.c to stop compensating for d_move's old limitations.
(Alan, this is 2.2.19 material [if then].)

Index: fs/dcache.c
--- fs/dcache.c.prev
+++ fs/dcache.c	Mon Nov 20 22:31:09 2000
@@ -749,16 +749,28 @@ void d_move(struct dentry * dentry, stru
 	INIT_LIST_HEAD(&target->d_hash);
 
+	/* Switch the names */
+	switch_names(dentry, target);
+	do_switch(dentry->d_name.len, target->d_name.len);
+	do_switch(dentry->d_name.hash, target->d_name.hash);
+
+	/* Switch parentage, allowing for self-parents */
+
 	list_del(&dentry->d_child);
 	list_del(&target->d_child);
 
-	/* Switch the parents and the names.. */
-	switch_names(dentry, target);
 	do_switch(dentry->d_parent, target->d_parent);
-	do_switch(dentry->d_name.len, target->d_name.len);
-	do_switch(dentry->d_name.hash, target->d_name.hash);
 
-	/* And add them back to the (new) parent lists */
-	list_add(&target->d_child, &target->d_parent->d_subdirs);
-	list_add(&dentry->d_child, &dentry->d_parent->d_subdirs);
+	if (dentry->d_parent != target)
+		list_add(&dentry->d_child, &dentry->d_parent->d_subdirs);
+	else {
+		INIT_LIST_HEAD(&dentry->d_child);
+		dentry->d_parent = dentry;
+	}
+	if (target->d_parent != dentry)
+		list_add(&target->d_child, &target->d_parent->d_subdirs);
+	else {
+		INIT_LIST_HEAD(&target->d_child);
+		target->d_parent = target;
+	}
 }
 

-- 
Chip Salzenberg            - a.k.a. -            <chip@valinux.com>
   "Give me immortality, or give me death!"  // Firesign Theatre
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

[PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)

[Chip Salzenberg]

This may be 2.2.18 material after all....  I wrote last night:
> Making nfsd's d_splice() compensate for d_move's limitations is not
> only a kludge, but also it harder to keep nfsd correct.
> someday, nfsd may not be the only creator of this kind of dentry.

Sure enough, there is just such a bug *already* in nfsd.  Nfsd's
cleanup after d_move is incomplete: It handles one of the dentries
being parentless, but not the other one.  This bug *will* cause dentry
corruption.[1]  It may well be what's been causing the hangs that my
recent patches seem to have fixed.

Therefore, in the mainline kernel, we need either the below patch to
d_move (along with a trivial simplifcation of nfsd's use of it), or an
expansion of the kludge in nfsd.  You can guess which one I favor....

[1] The bug can only show up when reconstructing pruned dentries, and
    only under a specific pattern of client requests, so it's not
    surprising that it is rarely observed in the wild.

Index: fs/dcache.c
--- fs/dcache.c.prev
+++ fs/dcache.c	Mon Nov 20 22:31:09 2000
@@ -749,16 +749,28 @@ void d_move(struct dentry * dentry, stru
 	INIT_LIST_HEAD(&target->d_hash);
 
+	/* Switch the names */
+	switch_names(dentry, target);
+	do_switch(dentry->d_name.len, target->d_name.len);
+	do_switch(dentry->d_name.hash, target->d_name.hash);
+
+	/* Switch parentage, allowing for self-parents */
+
 	list_del(&dentry->d_child);
 	list_del(&target->d_child);
 
-	/* Switch the parents and the names.. */
-	switch_names(dentry, target);
 	do_switch(dentry->d_parent, target->d_parent);
-	do_switch(dentry->d_name.len, target->d_name.len);
-	do_switch(dentry->d_name.hash, target->d_name.hash);
 
-	/* And add them back to the (new) parent lists */
-	list_add(&target->d_child, &target->d_parent->d_subdirs);
-	list_add(&dentry->d_child, &dentry->d_parent->d_subdirs);
+	if (dentry->d_parent != target)
+		list_add(&dentry->d_child, &dentry->d_parent->d_subdirs);
+	else {
+		INIT_LIST_HEAD(&dentry->d_child);
+		dentry->d_parent = dentry;
+	}
+	if (target->d_parent != dentry)
+		list_add(&target->d_child, &target->d_parent->d_subdirs);
+	else {
+		INIT_LIST_HEAD(&target->d_child);
+		target->d_parent = target;
+	}
 }
 
-- 
Chip Salzenberg            - a.k.a. -            <chip@valinux.com>
   "Give me immortality, or give me death!"  // Firesign Theatre
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)

[Neil Brown]

On Tuesday November 21, chip@valinux.com wrote:
> This may be 2.2.18 material after all....  I wrote last night:
> > Making nfsd's d_splice() compensate for d_move's limitations is not
> > only a kludge, but also it harder to keep nfsd correct.
> > someday, nfsd may not be the only creator of this kind of dentry.
> 
> Sure enough, there is just such a bug *already* in nfsd.  Nfsd's
> cleanup after d_move is incomplete: It handles one of the dentries
> being parentless, but not the other one.  This bug *will* cause dentry
> corruption.[1]  It may well be what's been causing the hangs that my
> recent patches seem to have fixed.
> 
> Therefore, in the mainline kernel, we need either the below patch to
> d_move (along with a trivial simplifcation of nfsd's use of it), or an
> expansion of the kludge in nfsd.  You can guess which one I favor....
> 
> [1] The bug can only show up when reconstructing pruned dentries, and
>     only under a specific pattern of client requests, so it's not
>     surprising that it is rarely observed in the wild.

Hi Chip,
 I am trying to understand what might be going on and why this fix
 might be needed, and I'm not making much progress.

 You suggest that d_move (or it's caller) must be able to deal with
 the target being an "root" dentry (x->d_parent == x). However I
 cannot see that this could possibly happen.

 (looking at 2.2.18pre21 which should be much the same as any othe
 2.2 knfsd..)

 The only place that knfsd calls d_move is in d_splice.
 Here the "dentry" argument is "target" (just to confuse the innocent)
 and this is almost certainly an "root" entry passed in from "splice".
 However the "target" argument is "tdentry" which was just created
 with d_alloc which has given a parent which is certainly non-NULL,
 otherwise we would have an oops much earlier.
 So the parent of the "target" is "parent", which is certainly
 different from "tdentry", so d_move is *not* being asked to
 move something onto a "root" dentry, so the change is not needed.
 

 Did I miss something in the above, or can you in some other way
 convince me that d_move needs to handle is_root targets.

Thanks,
NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: NFSD dentry manipulation (was Re: d_move())

[Chip Salzenberg]

According to Neil Brown:
>  You suggest that d_move (or it's caller) must be able to deal with
>  the [second parameter] being an "root" dentry (x->d_parent == x).
>  However, I cannot see that this could possibly happen.

Hmph.  It seems you're right; my d_move changes [1][2] were apparently
not required after all.  However...

The sum of my recent NFS patches incontrovertably turned a totally
unstable server into the very model of stability.  Why?

Let's assume that d_move was a red herring.  What else in the old code
could have cause dentry bugs?  My first thought on that is that the
old code (1) created multiple disconnected dentries for a given inode,
and yet (2) assumed that multiple disconnected dentries would never be
found.  I invite others' opinions.  And I've included a copy of my
recent patches (less d_move) below.

[1] I still think it would be reasonable for d_move() to handle root
    dentries, or at least oops on them for debugging.
[2] One bit of the d_splice patches is unrelated to d_move, namely,
    the move of 'dput(tdentry)' to the bottom of d_splice, allowing
    the 'parent' flag manipulation to finish before calling dput(),
    which can sleep.  But knfsd uses the big kernel lock, so I guess
    that's probably no issue either.

Index: fs/nfsd/nfsfh.c
--- fs/nfsd/nfsfh.c.prev
+++ fs/nfsd/nfsfh.c	Mon Nov 20 22:31:52 2000
@@ -108,4 +108,15 @@ static int get_ino_name(struct dentry *d
 	}
 
+	if (!error) {
+	    /*
+	     * Check for a fs-specific hash function. Note that we must
+	     * calculate the standard hash first, as the d_op->d_hash()
+	     * routine may choose to leave the hash value unchanged.
+	     */
+	    name->hash = full_name_hash(name->name, name->len);
+	    if (dentry->d_op && dentry->d_op->d_hash)
+		error = dentry->d_op->d_hash(dentry, name);
+	}
+
 out_close:
 	if (file.f_op->release)
@@ -115,4 +126,35 @@ out:
 }
 
+/* Arrange a dentry for the given inode:
+ *  1. Prefer an existing connected dentry.
+ *  2. Settle for an existing disconnected dentry.
+ *  3. If necessary, create a (disconnected) dentry.
+ */
+static struct dentry *nfsd_arrange_dentry(struct inode *inode)
+{
+	struct list_head *lp;
+	struct dentry *result;
+
+	result = NULL;
+	for (lp = inode->i_dentry.next; lp != &inode->i_dentry ; lp=lp->next) {
+		result = list_entry(lp,struct dentry, d_alias);
+		if (! (result->d_flags & DCACHE_NFSD_DISCONNECTED))
+			break;
+	}
+	if (result) {
+		dget(result);
+		iput(inode);
+	} else {
+		result = d_alloc_root(inode, NULL);
+		if (!result) {
+			iput(inode);
+			return ERR_PTR(-ENOMEM);
+		}
+		result->d_flags |= DCACHE_NFSD_DISCONNECTED;
+		d_rehash(result); /* so a dput won't loose it */
+	}
+	return result;
+}
+
 /* this should be provided by each filesystem in an nfsd_operations interface;
  * iget isn't really the right interface
@@ -121,6 +163,4 @@ static struct dentry *nfsd_iget(struct s
 {
 	struct inode *inode;
-	struct list_head *lp;
-	struct dentry *result;
 
 	inode = iget(sb, ino);
@@ -149,23 +189,6 @@ static struct dentry *nfsd_iget(struct s
 		return ERR_PTR(-ESTALE);
 	}
-	/* now to find a dentry.
-	 * If possible, get a well-connected one
-	 */
-	for (lp = inode->i_dentry.next; lp != &inode->i_dentry ; lp=lp->next) {
-		result = list_entry(lp,struct dentry, d_alias);
-		if (! (result->d_flags & DCACHE_NFSD_DISCONNECTED)) {
-			dget(result);
-			iput(inode);
-			return result;
-		}
-	}
-	result = d_alloc_root(inode, NULL);
-	if (result == NULL) {
-		iput(inode);
-		return ERR_PTR(-ENOMEM);
-	}
-	result->d_flags |= DCACHE_NFSD_DISCONNECTED;
-	d_rehash(result); /* so a dput won't loose it */
-	return result;
+
+	return nfsd_arrange_dentry(inode);
 }
 
@@ -228,43 +245,21 @@ int d_splice(struct dentry *target, stru
 struct dentry *nfsd_findparent(struct dentry *child)
 {
-	struct dentry *tdentry, *pdentry;
-	tdentry = d_alloc(child, &(const struct qstr) {"..", 2, 0});
-	if (!tdentry)
+	struct dentry *dotdot, *parent;
+
+	dotdot = d_alloc(child, &(const struct qstr) {"..", 2, 0});
+	if (!dotdot)
 		return ERR_PTR(-ENOMEM);
+	parent = child->d_inode->i_op->lookup(child->d_inode, dotdot);
+	d_drop(dotdot); /* we never want ".." hashed */
 
-	/* I'm going to assume that if the returned dentry is different, then
-	 * it is well connected.  But nobody returns different dentrys do they?
-	 */
-	pdentry = child->d_inode->i_op->lookup(child->d_inode, tdentry);
-	d_drop(tdentry); /* we never want ".." hashed */
-	if (!pdentry) {
-		/* I don't want to return a ".." dentry.
-		 * I would prefer to return an unconnected "IS_ROOT" dentry,
-		 * though a properly connected dentry is even better
-		 */
-		/* if first or last of alias list is not tdentry, use that
-		 * else make a root dentry
-		 */
-		struct list_head *aliases = &tdentry->d_inode->i_dentry;
-		if (aliases->next != aliases) {
-			pdentry = list_entry(aliases->next, struct dentry, d_alias);
-			if (pdentry == tdentry)
-				pdentry = list_entry(aliases->prev, struct dentry, d_alias);
-			if (pdentry == tdentry)
-				pdentry = NULL;
-			if (pdentry) dget(pdentry);
-		}
-		if (pdentry == NULL) {
-			pdentry = d_alloc_root(igrab(tdentry->d_inode), NULL);
-			if (pdentry) {
-				pdentry->d_flags |= DCACHE_NFSD_DISCONNECTED;
-				d_rehash(pdentry);
-			}
-		}
-		if (pdentry == NULL)
-			pdentry = ERR_PTR(-ENOMEM);
+	if (parent)
+		dput(dotdot);	/* not hashed, thus discarded */
+	else {
+		/* Discard the ".." dentry, then arrange for a better one. */
+		struct inode *inode = igrab(dotdot->d_inode);
+		dput(dotdot);	/* not hashed, thus discarded */
+		parent = nfsd_arrange_dentry(inode);
 	}
-	dput(tdentry); /* it is not hashed, it will be discarded */
-	return pdentry;
+	return parent;
 }
 

-- 
Chip Salzenberg            - a.k.a. -            <chip@valinux.com>
   "Give me immortality, or give me death!"  // Firesign Theatre
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/