Re: [PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)
On Tuesday November 21, [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)
On Tuesday November 21, [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)
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(>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(>d_child);
list_del(>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(>d_child, >d_parent->d_subdirs);
- list_add(>d_child, >d_parent->d_subdirs);
+ if (dentry->d_parent != target)
+ list_add(>d_child, >d_parent->d_subdirs);
+ else {
+ INIT_LIST_HEAD(>d_child);
+ dentry->d_parent = dentry;
+ }
+ if (target->d_parent != dentry)
+ list_add(>d_child, >d_parent->d_subdirs);
+ else {
+ INIT_LIST_HEAD(>d_child);
+ target->d_parent = target;
+ }
}
--
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/
[PATCH] 2.2.18: d_move() with self-root dentries (Dentry Corruption!)
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. -[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/
