Re: NFSD dentry manipulation (was Re: d_move())
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 != >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 != >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 =
Re: NFSD dentry manipulation (was Re: d_move())
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);
-
