2.2.18: d_move() with self-root dentries

PATCH In review Chip Salzenberg 1 participant 3 messages Nov 21, 2000 – Nov 24, 2000

Share & tools

Chip Salzenberg

author

2000-11-21 09:54 UTC #0 lore raw

PATCH 1/1 2.2.18: d_move() with self-root dentries

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

dcache.c renamed from dcache.c.prev +19 -7

@@ -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;
+	}
 }

Signature · 6 lines

Chip Salzenberg            - a.k.a. -            <[email protected]>
   "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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Chip Salzenberg

author

2000-11-21 18:54 UTC #1 lore raw

PATCH 1/1 2.2.18: d_move() with self-root dentries (Dentry Corruption!)

This may be 2.2.18 material after all.... I wrote last night:

Quoted 3 lines

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

dcache.c renamed from dcache.c.prev +19 -7

@@ -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;
+	}
 }

Signature · 6 lines

Chip Salzenberg            - a.k.a. -            <[email protected]>
   "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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Chip Salzenberg

author

2000-11-24 05:29 UTC #2 lore raw

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

In reply to Chip Salzenberg

According to Neil Brown:

Quoted 3 lines

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

nfsd/nfsfh.c renamed from nfsd/nfsfh.c.prev +58 ~~-57~~

@@ -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;
 }

Signature · 6 lines

Chip Salzenberg            - a.k.a. -            <[email protected]>
   "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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/