lifetime of DCACHE_DISCONECTED dentries

DCACHE_DISCONECTED dentries aren't always getting destroyed as soon as I'd have expected in the NFSv4 case.

I'm not sure what the right fix is; any ideas?  The below at least demonstrates the problem.

--b.

commit bb125ffbe5fc3f80ac7a5b20f51cc542c175cd49
Author: J. Bruce Fields <[email protected]>
Date:   Thu Nov 11 19:22:00 2010 -0500

   dput: free DCACHE_DISCONNECTED dentries sooner

   DCACHE_DISCONECTED dentries are normally left around for the benefit of future nfsd operations.  But there's no point keeping them around once the inode has been deleted.

   Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

   the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

   This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 38 lines · J. Bruce Fields

DCACHE_DISCONECTED dentries aren't always getting destroyed as soon as I'd have expected in the NFSv4 case.

I'm not sure what the right fix is; any ideas?  The below at least demonstrates the problem.

--b.

commit bb125ffbe5fc3f80ac7a5b20f51cc542c175cd49
Author: J. Bruce Fields <[email protected]>
Date:   Thu Nov 11 19:22:00 2010 -0500

   dput: free DCACHE_DISCONNECTED dentries sooner

   DCACHE_DISCONECTED dentries are normally left around for the benefit of future nfsd operations.  But there's no point keeping them around once the inode has been deleted.

   Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

   the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

   This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

Quoted 5 lines

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

(quoted diff) +1 -1

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

Quoted 8 lines · Nick Piggin

Quoted 5 lines · J. Bruce Fields

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 2 lines

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

Sounds sensible.  (But can you think of any actual bugs that will result from trying to add a new hashed dentry in this case?)

Quoted 6 lines

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

Quoted 5 lines

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

But nfsd already holds an open file in this case, and you could argue that it should be using that from the start.

So, we could modify nfsd to add a hash mapping filehandles to the filp's that it knows about, and have nfsd consult that hash before calling dentry_to_fh.

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Quoted 5 lines · J. Bruce Fields

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 5 lines

Quoted 2 lines

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

Sounds sensible.  (But can you think of any actual bugs that will result from trying to add a new hashed dentry in this case?)

Well, this one? :)

Quoted 10 lines

Quoted 10 lines

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Quoted 4 lines

Quoted 1 line

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 34 lines · Nick Piggin

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Quoted 5 lines · J. Bruce Fields

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 5 lines

Quoted 2 lines

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

Sounds sensible.  (But can you think of any actual bugs that will result from trying to add a new hashed dentry in this case?)

Well, this one? :)

Quoted 14 lines

Quoted 11 lines

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Quoted 3 lines

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Quoted 39 lines · Nick Piggin

Quoted 37 lines · Nick Piggin

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Quoted 5 lines · J. Bruce Fields

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 5 lines

Quoted 2 lines

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

Sounds sensible.  (But can you think of any actual bugs that will result from trying to add a new hashed dentry in this case?)

Well, this one? :)

Quoted 6 lines

Quoted 6 lines

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

Quoted 8 lines

Quoted 5 lines

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 2 lines

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Bah, sorry, I read too quickly, got the sense of the test backwards, and thought you were suggesting __d_find_alias() shouldn't return an alias in the i_nlink == 0 case!

Yes, agreed, that should solve my problem.

But what's the reason for the d_unhashed() check now?  Could we get rid of it entirely?

Quoted 40 lines · J. Bruce Fields

Quoted 39 lines · Nick Piggin

Quoted 37 lines · Nick Piggin

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Quoted 5 lines · J. Bruce Fields

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Quoted 5 lines

Quoted 2 lines

This seems to be where the thing goes wrong. It isn't a hashed dentry at this point here, so d_obtain_alias should not be making one.

Sounds sensible.  (But can you think of any actual bugs that will result from trying to add a new hashed dentry in this case?)

Well, this one? :)

Quoted 6 lines

Quoted 6 lines

I think the inode i_nlink games are much more appropriate on this side of the equation, rather than the dput side (after all, d_obtain_alias is setting up an alias for the inode).

Can you even put the link check into __d_find_alias?

Quoted 8 lines

Quoted 5 lines

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 8 lines

Quoted 2 lines

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Bah, sorry, I read too quickly, got the sense of the test backwards, and thought you were suggesting __d_find_alias() shouldn't return an alias in the i_nlink == 0 case!

Yes, agreed, that should solve my problem.

OK, good.

Quoted 2 lines

But what's the reason for the d_unhashed() check now?  Could we get rid of it entirely?

Well when the inode still has links I think we actually do want any new references to go to hashed dentries. Definitely for d_splice_alias.

Quoted 19 lines · J. Bruce Fields

Quoted 18 lines · Nick Piggin

Quoted 16 lines · Nick Piggin

Quoted 11 lines · J. Bruce Fields

Quoted 7 lines · Nick Piggin

Can you even put the link check into __d_find_alias?

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 8 lines

Quoted 2 lines

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Bah, sorry, I read too quickly, got the sense of the test backwards, and thought you were suggesting __d_find_alias() shouldn't return an alias in the i_nlink == 0 case!

Yes, agreed, that should solve my problem.

OK, good.

Quoted 2 lines

But what's the reason for the d_unhashed() check now?  Could we get rid of it entirely?

Well when the inode still has links I think we actually do want any new references to go to hashed dentries. Definitely for d_splice_alias.

From: J. Bruce Fields <[email protected]>

Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Cc Nick Piggin <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Quoted 31 lines · Nick Piggin

Quoted 19 lines · J. Bruce Fields

Quoted 18 lines · Nick Piggin

Quoted 16 lines · Nick Piggin

Quoted 11 lines · J. Bruce Fields

Quoted 7 lines · Nick Piggin

Can you even put the link check into __d_find_alias?

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 8 lines

Quoted 2 lines

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Bah, sorry, I read too quickly, got the sense of the test backwards, and thought you were suggesting __d_find_alias() shouldn't return an alias in the i_nlink == 0 case!

Yes, agreed, that should solve my problem.

OK, good.

Quoted 5 lines

Quoted 2 lines

But what's the reason for the d_unhashed() check now?  Could we get rid of it entirely?

Well when the inode still has links I think we actually do want any new references to go to hashed dentries. Definitely for d_splice_alias.

So here's a version with a changelog; objections?

--b.

fs/dcache.c +0 -0

index 23702a9..afa8a0d 100644

Quoted 70 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Cc Nick Piggin <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Quoted 31 lines · Nick Piggin

Quoted 19 lines · J. Bruce Fields

Quoted 18 lines · Nick Piggin

Quoted 16 lines · Nick Piggin

Quoted 11 lines · J. Bruce Fields

Quoted 7 lines · Nick Piggin

Can you even put the link check into __d_find_alias?

-               if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+               if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||

!d_unhashed(alias)) {

Something like that?

The immediate result of that would be for the close rpc (or any rpc's sent after the file was unlinked) to fail with ESTALE.

Why is that? Seems like it would be a bug, because a hashed dentry may be unhashed at any time concurrently to nfsd operation, so it should be able to tolerate that so long as it has a ref on the inode?

Quoted 8 lines

Quoted 2 lines

Ping? Did you work out why nfs fails with ESTALE in that case? It seems to work in my testing (and do the right thing with freeing the inode).

Bah, sorry, I read too quickly, got the sense of the test backwards, and thought you were suggesting __d_find_alias() shouldn't return an alias in the i_nlink == 0 case!

Yes, agreed, that should solve my problem.

OK, good.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

But what's the reason for the d_unhashed() check now?  Could we get rid of it entirely?

Well when the inode still has links I think we actually do want any new references to go to hashed dentries. Definitely for d_splice_alias.

So here's a version with a changelog; objections?

Not sure where Al's hiding...

But I would like to update the comments, and perhaps even a new add a new function here (or new flag to __d_find_alias).

AFAIKS, the callers are OK, however I suppose d_splice_alias and d_materialise_unique should not have unlinked inodes at this point, so at least a BUG_ON for them might be a good idea?

Quoted 4 lines

--b.

fs/dcache.c static struct dentry * __d_find_alias(struct inode *inode, int want_discon) · line 374

Quoted 13 lines

fs/dcache.c +1 -1

index 23702a9..afa8a0d 100644
@@ -368,7 +368,7 @@ static struct dentry * __d_find_alias(struct inode *inode, int want_discon)
 		next = tmp->next;
 		prefetch(next);
 		alias = list_entry(tmp, struct dentry, d_alias);
- 		if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
+		if (S_ISDIR(inode->i_mode) || !inode->i_nlink || !d_unhashed(alias)) {
 			if (IS_ROOT(alias) &&
 			    (alias->d_flags & DCACHE_DISCONNECTED))
 				discon_alias = alias;

Quoted 5 lines · Nick Piggin

Not sure where Al's hiding...

But I would like to update the comments, and perhaps even a new add a new function here (or new flag to __d_find_alias).

Quoted 3 lines

AFAIKS, the callers are OK, however I suppose d_splice_alias and d_materialise_unique should not have unlinked inodes at this point, so at least a BUG_ON for them might be a good idea?

That does sound safer. I'm pretty confused by the various __di_splice_alias callers. I'll go search through them and see if I can understand better....

From: J. Bruce Fields <[email protected]>

Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Leave __d_find_alias() alone to avoid changing behavior of other callers.

Also nfsd doesn't need all the checks of __d_find_alias(); any dentry, hashed or unhashed, disconnected or not, should work.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

Quoted 10 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Not sure where Al's hiding...

But I would like to update the comments, and perhaps even a new add a new function here (or new flag to __d_find_alias).

AFAIKS, the callers are OK, however I suppose d_splice_alias and d_materialise_unique should not have unlinked inodes at this point, so at least a BUG_ON for them might be a good idea?

Quoted 3 lines

That does sound safer. I'm pretty confused by the various __di_splice_alias callers. I'll go search through them and see if I can understand better....

I think a new __d_find_alias flag would just make it more confusing. And it looks like all d_obtain_alias needs is something much simpler. This works for me.

--b.

fs/dcache.c +24 -2

index 5ed93cd..d5f9da4 100644
@@ -1135,6 +1135,28 @@ static inline struct hlist_head *d_hash(struct dentry *parent,
 	return dentry_hashtable + (hash & D_HASHMASK);
 }
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+	struct dentry *alias;
+
+	if (list_empty(&inode->i_dentry))
+		return NULL;
+	alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	__dget_locked(alias);
+	return alias;
+}
+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+	struct dentry *de;
+
+	spin_lock(&dcache_lock);
+	de = __d_find_alias(inode, 0);
+	spin_unlock(&dcache_lock);
+	return de;
+}
+
+
 /**
  * d_obtain_alias - find or allocate a dentry for a given inode
  * @inode: inode to allocate the dentry for

@@ -1164,7 +1186,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
-	res = d_find_alias(inode);
+	res = d_find_any_alias(inode);
 	if (res)
 		goto out_iput;
 

@@ -1176,7 +1198,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	tmp->d_parent = tmp; /* make sure dput doesn't croak */
 
 	spin_lock(&dcache_lock);
-	res = __d_find_alias(inode, 0);
+	res = __d_find_any_alias(inode);
 	if (res) {
 		spin_unlock(&dcache_lock);
 		dput(tmp);

Signature · 1 line

1.7.1

Quoted 58 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

Without this patch

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

       - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Leave __d_find_alias() alone to avoid changing behavior of other callers.

Also nfsd doesn't need all the checks of __d_find_alias(); any dentry, hashed or unhashed, disconnected or not, should work.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

Quoted 13 lines · J. Bruce Fields

Quoted 8 lines · Nick Piggin

Not sure where Al's hiding...

But I would like to update the comments, and perhaps even a new add a new function here (or new flag to __d_find_alias).

AFAIKS, the callers are OK, however I suppose d_splice_alias and d_materialise_unique should not have unlinked inodes at this point, so at least a BUG_ON for them might be a good idea?

That does sound safer. I'm pretty confused by the various __di_splice_alias callers. I'll go search through them and see if I can understand better....

Quoted 3 lines

I think a new __d_find_alias flag would just make it more confusing. And it looks like all d_obtain_alias needs is something much simpler. This works for me.

Well I don't really know if this is clearer. By convention, __ prefix version should be same as non prefix version but just take fewer locks. Why do you have the new d_find_any_alias()?

Quoted 8 lines · Nick Piggin

Quoted 6 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

On Tue, Dec 14, 2010 at 05:01:02PM -0500, J. Bruce Fields wrote:
I think a new __d_find_alias flag would just make it more confusing. And it looks like all d_obtain_alias needs is something much simpler. This works for me.

Quoted 3 lines

Well I don't really know if this is clearer. By convention, __ prefix version should be same as non prefix version but just take fewer locks. Why do you have the new d_find_any_alias()?

Argh, apologies; with typo fixed.

--b.

commit 5a911af645aae9992d465d57c397237fb35d1f93
Author: J. Bruce Fields <[email protected]>
Date:   Fri Dec 17 09:05:04 2010 -0500

   fs/dcache: allow __d_obtain_alias() to return unhashed dentries

   Without this patch

          client$ mount -tnfs4 server:/export/ /mnt/ client$ tail -f /mnt/FOO
          ...
          server$ df -i /export
          server$ rm /export/FOO
          (^C the tail -f)
          server$ df -i /export
          server$ echo 2 >/proc/sys/vm/drop_caches server$ df -i /export

   the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

   This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

          - putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp associated with the nfsd open.  d_obtain_alias() doesn't like this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.

   Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

   Leave __d_find_alias() alone to avoid changing behavior of other callers.

   Also nfsd doesn't need all the checks of __d_find_alias(); any dentry, hashed or unhashed, disconnected or not, should work.

Signed-off-by J. Bruce Fields <[email protected]>

fs/dcache.c +23 -1

index 5ed93cd..5c014a5 100644
@@ -1135,6 +1135,28 @@ static inline struct hlist_head *d_hash(struct dentry *parent,
        return dentry_hashtable + (hash & D_HASHMASK);
  }
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+       struct dentry *alias;
+
+       if (list_empty(&inode->i_dentry))
+               return NULL;
+       alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+       __dget_locked(alias);
+       return alias;
+}
+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+       struct dentry *de;
+
+       spin_lock(&dcache_lock);
+       de = __d_find_any_alias(inode);
+       spin_unlock(&dcache_lock);
+       return de;
+}
+
+
  /**
  * d_obtain_alias - find or allocate a dentry for a given inode
  * @inode: inode to allocate the dentry for

@@ -1164,7 +1186,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
        if (IS_ERR(inode))
                return ERR_CAST(inode);
 
-       res = d_find_alias(inode);
+       res = d_find_any_alias(inode);
        if (res)
                goto out_iput;
 

@@ -1176,7 +1198,7 @@ struct dentry *d_obtain_alias(struct inode *inode)

       tmp->d_parent = tmp; /* make sure dput doesn't croak */

       spin_lock(&dcache_lock);
-       res = __d_find_alias(inode, 0);
+       res = __d_find_any_alias(inode);
       if (res) {
               spin_unlock(&dcache_lock);
               dput(tmp);

Quoted 6 lines · J. Bruce Fields

Quoted 3 lines · Nick Piggin

Well I don't really know if this is clearer. By convention, __ prefix version should be same as non prefix version but just take fewer locks. Why do you have the new d_find_any_alias()?

Argh, apologies; with typo fixed.

OK that makes a lot more sense :)

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Leave __d_find_alias() alone to avoid changing behavior of other callers.

Also nfsd doesn't need all the checks of __d_find_alias(); any dentry, hashed or unhashed, disconnected or not, should work.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |   26 ++++++++++++++++++++++++--

 1 files changed, 24 insertions(+), 2 deletions(-)

fs/dcache.c +6 -0

index 9f493ee..2849258 100644
@@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
  }
  EXPORT_SYMBOL(d_alloc_root);
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+       struct dentry *alias;
+
+       if (list_empty(&inode->i_dentry))
+               return NULL;

Quoted 14 lines · J. Bruce Fields

(quoted diff) +10 -0

@@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
  }
  EXPORT_SYMBOL(d_alloc_root);
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+       struct dentry *alias;
+
+       if (list_empty(&inode->i_dentry))
+               return NULL;
+       alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+       __dget(alias);
+       return alias;
+}

Review struct dentry * d_alloc_root(struct inode * root_inode) · line 1539

Quoted 6 lines

(quoted diff) +6 -0

+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+       struct dentry *de;
+
+       spin_lock(&inode->i_lock);

Yes, i_dentry/d_alias is protected by i_lock, so it looks fine.

Quoted 15 lines · Nick Piggin

Quoted 14 lines · J. Bruce Fields

fs/dcache.c +10 -0

@@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
  }
  EXPORT_SYMBOL(d_alloc_root);
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+       struct dentry *alias;
+
+       if (list_empty(&inode->i_dentry))
+               return NULL;
+       alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+       __dget(alias);
+       return alias;
+}

Quoted 8 lines

fs/dcache.c struct dentry * d_alloc_root(struct inode * root_inode) · line 1539

Quoted 6 lines

fs/dcache.c +6 -0

+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+       struct dentry *de;
+
+       spin_lock(&inode->i_lock);

Yes, i_dentry/d_alias is protected by i_lock, so it looks fine.

OK, thanks; I'm assuming it's OK to re-add your ack in that case. Al, could you apply? (Or if this should go in through an nfsd tree, or some other way, let me know.)

--b.

From: J. Bruce Fields <[email protected]>
Date: Fri, 17 Dec 2010 09:05:04 -0500
Subject: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries

Without this patch, inodes are not promptly freed on last close of an unlinked file by an nfs client:

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until the last step, when it could have been freed after killing the client's tail -f.  On-disk data won't be deallocated either, leading to possible spurious ENOSPC.

This occurs because when the client does the close, it arrives in a compound with a putfh and a close, processed like:

- putfh: look up the filehandle.  The only alias found for the inode will be DCACHE_UNHASHED alias referenced by the filp this, so it creates a new DCACHE_DISCONECTED dentry and returns that instead.
- close: closes the existing filp, which is destroyed immediately by dput() since it's DCACHE_UNHASHED.
- end of the compound: release the reference
to the current filehandle, and dput() the new DCACHE_DISCONECTED dentry, which gets put on the unused list instead of being destroyed immediately.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return the unhashed dentry that is referenced by the filp, instead of making it create a new dentry.

Leave __d_find_alias() alone to avoid changing behavior of other callers.

Also nfsd doesn't need all the checks of __d_find_alias(); any dentry, hashed or unhashed, disconnected or not, should work.

Acked-by Nick Piggin <npiggin-tSWWG44O7X1aa/[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

fs/dcache.c +24 -2

index 9f493ee..2849258 100644
@@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
 }
 EXPORT_SYMBOL(d_alloc_root);
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+	struct dentry *alias;
+
+	if (list_empty(&inode->i_dentry))
+		return NULL;
+	alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	__dget(alias);
+	return alias;
+}
+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+	struct dentry *de;
+
+	spin_lock(&inode->i_lock);
+	de = __d_find_any_alias(inode);
+	spin_unlock(&inode->i_lock);
+	return de;
+}
+
+
 /**
  * d_obtain_alias - find or allocate a dentry for a given inode
  * @inode: inode to allocate the dentry for

@@ -1550,7 +1572,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
-	res = d_find_alias(inode);
+	res = d_find_any_alias(inode);
 	if (res)
 		goto out_iput;
 

@@ -1563,7 +1585,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 
 
 	spin_lock(&inode->i_lock);
-	res = __d_find_alias(inode, 0);
+	res = __d_find_any_alias(inode);
 	if (res) {
 		spin_unlock(&inode->i_lock);
 		dput(tmp);

Signature · 1 line

1.7.1

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

Quoted 2 lines · J. Bruce Fields

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on with __d_find_alias() elsewhere. Namely, in d_splice_alias(). The thing is, unless I'm missing something we ought to use __d_find_any_alias() there as well. Directories really, _really_ should not have more than one alias. And what we get is really weird:
* find (the only) alias
* if it doesn't exist, create one (OK, no problem)
* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange. I'd been staring at the history of this function and I _think_ it's a rudiment of period when we used that stuff for non-directories as well. It used to be directory-only; then Neil had switched it to non-directories as well (in "Fix disconnected dentries on NFS exports" back in 2004), adding want_discon argument to __d_find_alias() in process and using it instead of open-coded equivalent of __d_find_any_alias(). Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias" he'd made that code directory-only again, at which point we arrived to the current situation.

Neil, is there some reason I'm missing that makes __d_find_alias() right in there? And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that mess cleaned up as well or at least explained.

Quoted 27 lines · On Thu, 10 Mar 2011 10:58:21 +0000 Al Viro…

Quoted 2 lines · J. Bruce Fields

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on with __d_find_alias() elsewhere. Namely, in d_splice_alias(). The thing is, unless I'm missing something we ought to use __d_find_any_alias() there as well. Directories really, _really_ should not have more than one alias. And what we get is really weird:
* find (the only) alias
* if it doesn't exist, create one (OK, no problem)
* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange. I'd been staring at the history of this function and I _think_ it's a rudiment of period when we used that stuff for non-directories as well. It used to be directory-only; then Neil had switched it to non-directories as well (in "Fix disconnected dentries on NFS exports" back in 2004), adding want_discon argument to __d_find_alias() in process and using it instead of open-coded equivalent of __d_find_any_alias(). Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias" he'd made that code directory-only again, at which point we arrived to the current situation.

Quoted 5 lines

Neil, is there some reason I'm missing that makes __d_find_alias() right in there? And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that mess cleaned up as well or at least explained.

I'm a bit uncomfortable that d_splice_alias drops all locks before calling d_move. Normally when d_move is called, both directories are locked in some way. In the case the source is not a directory bit is the state of being anonymous. Theoretically two calls to d_splice_alias on the same inode could try to d_move the same anonymous dentry to two different places, which would be bad. So I think some locking is needed there.

Quoted 29 lines · NeilBrown

Quoted 27 lines · On Thu, 10 Mar 2011 10:58:21 +0000 Al Viro…

Quoted 2 lines · J. Bruce Fields

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on with __d_find_alias() elsewhere. Namely, in d_splice_alias(). The thing is, unless I'm missing something we ought to use __d_find_any_alias() there as well. Directories really, _really_ should not have more than one alias. And what we get is really weird:
* find (the only) alias
* if it doesn't exist, create one (OK, no problem)
* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange. I'd been staring at the history of this function and I _think_ it's a rudiment of period when we used that stuff for non-directories as well. It used to be directory-only; then Neil had switched it to non-directories as well (in "Fix disconnected dentries on NFS exports" back in 2004), adding want_discon argument to __d_find_alias() in process and using it instead of open-coded equivalent of __d_find_any_alias(). Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias" he'd made that code directory-only again, at which point we arrived to the current situation.

Quoted 5 lines

Quoted 5 lines

Neil, is there some reason I'm missing that makes __d_find_alias() right in there? And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that mess cleaned up as well or at least explained.

Nothing like seeing somebody actually run across a bug here to motivate getting back to this....

So, like Al I'm wondering whether the __d_find_alias in d_splice_alias should be a __d_find_any_alias.

Disclaimer: the bug manifested in rhel5, and I haven't looked closely at upstream yet, though it seems like it would have the same problem.

In the particular case I'm seeing, the directory already has an alias that is UNHASHED (but not DISCONNECTED). So d_splice_alias adds a second alias.

(And we notice because a rename happens to pick up one of each, and the rename locking then tries to take the parent's i_mutex twice (since the rename code think it's dealing with a cross-directory rename)).

Using __d_find_any_alias seems to fix the problem.

Would rehashing an UNHASHED dentry here violate any important assumptions?

Quoted 7 lines

I'm a bit uncomfortable that d_splice_alias drops all locks before calling d_move. Normally when d_move is called, both directories are locked in some way. In the case the source is not a directory bit is the state of being anonymous. Theoretically two calls to d_splice_alias on the same inode could try to d_move the same anonymous dentry to two different places, which would be bad. So I think some locking is needed there.

I think d_splice_alias is only ever called from lookup, under the parent's i_mutex?

So if we have two tasks in the d_move here, then the two callers must be doing lookups in two different directories, and holding the i_mutex on both of them.

The directory can't have been renamed while either caller was holding the i_mutex, so this is really a directory that's returned on lookup from two different parents.

That sounds like a bug already. Could it happen on a corrupted filesystem maybe?

--b.

Quoted 42 lines · J. Bruce Fields

Quoted 29 lines · NeilBrown

Quoted 27 lines · On Thu, 10 Mar 2011 10:58:21 +0000 Al Viro…

Quoted 2 lines · J. Bruce Fields

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on with __d_find_alias() elsewhere. Namely, in d_splice_alias(). The thing is, unless I'm missing something we ought to use __d_find_any_alias() there as well. Directories really, _really_ should not have more than one alias. And what we get is really weird:
* find (the only) alias
* if it doesn't exist, create one (OK, no problem)
* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange. I'd been staring at the history of this function and I _think_ it's a rudiment of period when we used that stuff for non-directories as well. It used to be directory-only; then Neil had switched it to non-directories as well (in "Fix disconnected dentries on NFS exports" back in 2004), adding want_discon argument to __d_find_alias() in process and using it instead of open-coded equivalent of __d_find_any_alias(). Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias" he'd made that code directory-only again, at which point we arrived to the current situation.

Quoted 5 lines

Quoted 5 lines

Neil, is there some reason I'm missing that makes __d_find_alias() right in there? And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that mess cleaned up as well or at least explained.

Nothing like seeing somebody actually run across a bug here to motivate getting back to this....

So, like Al I'm wondering whether the __d_find_alias in d_splice_alias should be a __d_find_any_alias.

Quoted 6 lines

Disclaimer: the bug manifested in rhel5, and I haven't looked closely at upstream yet, though it seems like it would have the same problem.

In the particular case I'm seeing, the directory already has an alias that is UNHASHED (but not DISCONNECTED). So d_splice_alias adds a second alias.

The reproducer I was using against rhel5 isn't going to work against upstream, because it depended on the dentry_unhash() call in vfs_rmdir() to create that UNHASHED dentry.

Though I think there's still code that can leave UNHASHED dentries around still on the alias list. I'll look some more....

Quoted 50 lines · On Wed, 15 Feb 2012 11:56:33 -0500 "J. Bruce Fields"…

Quoted 42 lines · J. Bruce Fields

Quoted 29 lines · NeilBrown

Quoted 27 lines · On Thu, 10 Mar 2011 10:58:21 +0000 Al Viro…

Quoted 2 lines · J. Bruce Fields

Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on with __d_find_alias() elsewhere. Namely, in d_splice_alias(). The thing is, unless I'm missing something we ought to use __d_find_any_alias() there as well. Directories really, _really_ should not have more than one alias. And what we get is really weird:
* find (the only) alias
* if it doesn't exist, create one (OK, no problem)
* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange. I'd been staring at the history of this function and I _think_ it's a rudiment of period when we used that stuff for non-directories as well. It used to be directory-only; then Neil had switched it to non-directories as well (in "Fix disconnected dentries on NFS exports" back in 2004), adding want_discon argument to __d_find_alias() in process and using it instead of open-coded equivalent of __d_find_any_alias(). Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias" he'd made that code directory-only again, at which point we arrived to the current situation.

Quoted 5 lines

Quoted 5 lines

Neil, is there some reason I'm missing that makes __d_find_alias() right in there? And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that mess cleaned up as well or at least explained.

Nothing like seeing somebody actually run across a bug here to motivate getting back to this....

So, like Al I'm wondering whether the __d_find_alias in d_splice_alias should be a __d_find_any_alias.

Quoted 6 lines

Disclaimer: the bug manifested in rhel5, and I haven't looked closely at upstream yet, though it seems like it would have the same problem.

In the particular case I'm seeing, the directory already has an alias that is UNHASHED (but not DISCONNECTED). So d_splice_alias adds a second alias.

Quoted 6 lines

The reproducer I was using against rhel5 isn't going to work against upstream, because it depended on the dentry_unhash() call in vfs_rmdir() to create that UNHASHED dentry.

Though I think there's still code that can leave UNHASHED dentries around still on the alias list. I'll look some more....

I was going ask how you managed to get an 'unhashed' dentry which was not DISCONNECTED, and belonged to a directory that could be the subject of d_splice_alias (that implies it has a name).

The bug sounds like a race between lookup and rmdir, which should be prevented by i_mutex.

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

I'm seeing something similar happening on an upstream kernel, so I'll try to figure out where that's coming from now....

Quoted 3 lines

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

Right, I throw away that BUG_ON as well. Tested on rhel5 only.

--b.

I was going ask how you managed to get an 'unhashed' dentry which was not DISCONNECTED, and belonged to a directory that could be the subject of d_splice_alias (that implies it has a name).

The bug sounds like a race between lookup and rmdir, which should be prevented by i_mutex.

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

Quoted 9 lines · NeilBrown

I was going ask how you managed to get an 'unhashed' dentry which was not DISCONNECTED, and belonged to a directory that could be the subject of d_splice_alias (that implies it has a name).

The bug sounds like a race between lookup and rmdir, which should be prevented by i_mutex.

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

Looking through the latest upstream code, I can't come up with another obvious reproducer.

But I also can't see the code making any particular effort to ensure that dentries are removed from inode's alias lists at the same time they're unhashed.

E.g., trace up through the callers of d_drop/__d_drop and try to convince yourselves that they all end up removing the dentry from the alias list.

Can you see any reason why the following would actually create a problem?

--b.

commit fcfef6b7319c5d19ea5064317528ff994343b011
Author: J. Bruce Fields <[email protected]>
Date:   Mon Feb 13 13:38:33 2012 -0500

   exports: stop d_splice_alias creating directory aliases

   A directory should never have more than one dentry pointing to it.

   But d_splice_alias() does so in the case of a directory with an already-existing non-DISCONNECTED dentry.

   Prior to the removal of dentry_unhash() from vfs_rmdir(), around v3.0, this could cause an nfsd deadlock like this:

       - Somebody attempts to remove a non-empty directory.
       - The dentry_unhash() in vfs_rmdir() unhashes the dentry pointing to the non-empty directory.
       - ->rmdir() then fails with -ENOTEMPTY
       - Before the vfs_rmdir() caller reaches dput(), an nfsd process in rename looks up the directory by filehandle; at the end of that lookup, this dentry is found by d_alloc_anon(), and a reference is taken on it, preventing dput() from removing it.
       - A regular lookup of the directory calls d_splice_alias(), finds only an unhashed (not a DISCONNECTED) dentry, and insteads adds a new one, so the directory now has two dentries.
       - The nfsd process in rename, which was previously looking up the source directory of the rename, now looks up the target directory (which is the same), and gets the dentry newly created by the previous lookup.
       - The rename, seeing two different dentries, assumes this is a cross-directory rename and attempts to take the i_mutex on the directory twice.

   I don't see as obvious a reproducer now, but I also don't see the existing code taking care to remove dentries from the alias list whenever they're unhashed.

   It therefore seems safest to allow d_splice_alias to use any dentry it finds.

Signed-off-by J. Bruce Fields <[email protected]>

fs/dcache.c +0 -0

index f68e193..1fd2256 100644

fs/dcache.c +1 -2

+               new = __d_find_any_alias(inode);
                if (new) {
-                       BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
                        spin_unlock(&inode->i_lock);
                        security_d_instantiate(new, inode);
                        d_move(new, dentry);
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/dcache.c struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry) · line 1605

Quoted 9 lines

fs/dcache.c +0 -1

index f68e193..1fd2256 100644
@@ -1602,9 +1602,8 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 
        if (inode && S_ISDIR(inode->i_mode)) {
                spin_lock(&inode->i_lock);
-               new = __d_find_alias(inode, 1);

by replacing this, the want_discon argument is no longer in use. care to remove it as well?

Quoted 9 lines · NeilBrown

I was going ask how you managed to get an 'unhashed' dentry which was not DISCONNECTED, and belonged to a directory that could be the subject of d_splice_alias (that implies it has a name).

The bug sounds like a race between lookup and rmdir, which should be prevented by i_mutex.

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

Looking through the latest upstream code, I can't come up with another obvious reproducer.

But I also can't see the code making any particular effort to ensure that dentries are removed from inode's alias lists at the same time they're unhashed.

E.g., trace up through the callers of d_drop/__d_drop and try to convince yourselves that they all end up removing the dentry from the alias list.

Can you see any reason why the following would actually create a problem?

--b.

commit fcfef6b7319c5d19ea5064317528ff994343b011

Author: J. Bruce Fields <[email protected]>
Date:   Mon Feb 13 13:38:33 2012 -0500

exports: stop d_splice_alias creating directory aliases

A directory should never have more than one dentry pointing to it.

But d_splice_alias() does so in the case of a directory with an already-existing non-DISCONNECTED dentry.

Prior to the removal of dentry_unhash() from vfs_rmdir(), around v3.0, this could cause an nfsd deadlock like this:

- Somebody attempts to remove a non-empty directory.
- The dentry_unhash() in vfs_rmdir() unhashes the dentry pointing to the non-empty directory.
- ->rmdir() then fails with -ENOTEMPTY
- Before the vfs_rmdir() caller reaches dput(), an nfsd process in rename looks up the directory by filehandle; at the end of that lookup, this dentry is found by d_alloc_anon(), and a reference is taken on it, preventing dput() from removing it.
- A regular lookup of the directory calls d_splice_alias(), finds only an unhashed (not a DISCONNECTED) dentry, and insteads adds a new one, so the directory now has two dentries.
- The nfsd process in rename, which was previously looking up the source directory of the rename, now looks up the target directory (which is the same), and gets the dentry newly created by the previous lookup.
- The rename, seeing two different dentries, assumes this is a cross-directory rename and attempts to take the i_mutex on the directory twice.

I don't see as obvious a reproducer now, but I also don't see the existing code taking care to remove dentries from the alias list whenever they're unhashed.

It therefore seems safest to allow d_splice_alias to use any dentry it finds.

Signed-off-by J. Bruce Fields <[email protected]>

fs/dcache.c +1 -2

index f68e193..1fd2256 100644
@@ -1602,9 +1602,8 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 
 	if (inode && S_ISDIR(inode->i_mode)) {
 		spin_lock(&inode->i_lock);
-		new = __d_find_alias(inode, 1);
+		new = __d_find_any_alias(inode);
 		if (new) {
-			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
 			spin_unlock(&inode->i_lock);
 			security_d_instantiate(new, inode);
 			d_move(new, dentry);

Quoted 18 lines · On Thu, 16 Feb 2012 17:30:11 -0500 "J. Bruce Fields"…

Quoted 9 lines · NeilBrown

I was going ask how you managed to get an 'unhashed' dentry which was not DISCONNECTED, and belonged to a directory that could be the subject of d_splice_alias (that implies it has a name).

The bug sounds like a race between lookup and rmdir, which should be prevented by i_mutex.

I think that using __d_find_any_alias would just be papering over the problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.

Looking through the latest upstream code, I can't come up with another obvious reproducer.

But I also can't see the code making any particular effort to ensure that dentries are removed from inode's alias lists at the same time they're unhashed.

Quoted 6 lines

E.g., trace up through the callers of d_drop/__d_drop and try to convince yourselves that they all end up removing the dentry from the alias list.

Can you see any reason why the following would actually create a problem?

No, I don't think that would cause problems, so it is probably a good clean up and as Peng says it means we can remove the want_discon arg as well.

However I cannot help thinking that something else must be going wrong before we get to this point.
When you rmdir a directory it must be empty and it will never be linked again. So how does 'lookup' find the inode and want to attach a dentry to it?

So I still think this is just papering over some other problem.

I think that looking at when aliases are removed is missing the point.

A directory can only have one name so it can only have one dentry. If that dentry gets unhashed, that is because the directory was deleted.

So
now it must have zero names. So there is no way that lookup can possibly find it, so there is no way that d_splice_alias can be asked to attach an alias to it.

1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
was only a hint, its absence was a strong statement.
If the flag is set, the dentry might not be linked to the root. If it is clear, it definitely is link through to the root. However I think it was used with stronger intent than that.

Now it seems to mean a little bit more: If it is set and the dentry is hashed, then it must be on the sb->s_anon list.

This is a significant
which I never noticed (I haven't been watching). Originally a disconnected dentry would be attached (and hashed) to its parent. Then that parent would get its own parent and so on until it was attached all the way to the root. Only then would be start clearing DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if that is correct.

Coming back to this now, just trying to review the filehandle-lookup/dcache interactions:

Quoted 8 lines · NeilBrown

1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
was only a hint, its absence was a strong statement.
If the flag is set, the dentry might not be linked to the root. If it is clear, it definitely is link through to the root. However I think it was used with stronger intent than that.

Now it seems to mean a little bit more: If it is set and the dentry is hashed, then it must be on the sb->s_anon list.

The code that makes that assumption is __d_shrink (which does the work of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to lock.

I can't find any basis for that assumption. The only code that clears DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as hashing. Am I missing something?

Quoted 7 lines

This is a significant
which I never noticed (I haven't been watching). Originally a disconnected dentry would be attached (and hashed) to its parent. Then that parent would get its own parent and so on until it was attached all the way to the root. Only then would be start clearing DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if that is correct.

It looks wrong to me:

If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle lookup thinking the dentry is OK to use. That could mean for example trying to rename across directories that don't have any ancestor relationship to each other in the dcache yet.

So we need to wait to clear DCACHE_DISCONNECTED until we *know* the dentry's parents go all the way back to the root. As you say, that's what the current code does.

But that means DCACHE_DISCONNECTED dentries can be hashed to their parents, and __d_shrink can be handed such dentries and then get the locking wrong.

It looks like this bug might originate with Nick Piggin's ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking"? There's no discussion in the changelog, so probably it was just based on an unexamined assumption about DCACHE_DISCONNECTED.

I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test in __d_shrink(), or if we need another flag, or ?

Quoted 44 lines · J. Bruce Fields

Coming back to this now, just trying to review the filehandle-lookup/dcache interactions:

Quoted 8 lines · NeilBrown

1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
was only a hint, its absence was a strong statement.
If the flag is set, the dentry might not be linked to the root. If it is clear, it definitely is link through to the root. However I think it was used with stronger intent than that.

Now it seems to mean a little bit more: If it is set and the dentry is hashed, then it must be on the sb->s_anon list.

The code that makes that assumption is __d_shrink (which does the work of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to lock.

I can't find any basis for that assumption. The only code that clears DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as hashing. Am I missing something?

Quoted 7 lines

This is a significant
which I never noticed (I haven't been watching). Originally a disconnected dentry would be attached (and hashed) to its parent. Then that parent would get its own parent and so on until it was attached all the way to the root. Only then would be start clearing DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if that is correct.

It looks wrong to me:

If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle lookup thinking the dentry is OK to use. That could mean for example trying to rename across directories that don't have any ancestor relationship to each other in the dcache yet.

So we need to wait to clear DCACHE_DISCONNECTED until we *know* the dentry's parents go all the way back to the root. As you say, that's what the current code does.

But that means DCACHE_DISCONNECTED dentries can be hashed to their parents, and __d_shrink can be handed such dentries and then get the locking wrong.

Quoted 7 lines

It looks like this bug might originate with Nick Piggin's ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking"? There's no discussion in the changelog, so probably it was just based on an unexamined assumption about DCACHE_DISCONNECTED.

I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test in __d_shrink(), or if we need another flag, or ?

Bah, sorry, and I only just noticed that you already said as much later and did the IS_ROOT() thing in your patch.

Anyway, here's just that one change with a slightly more painstaking changelog.

--b.

commit b1fa644355122627424fe2240a9fc60cbef4c349

Author: J. Bruce Fields <[email protected]>
Date:   Thu Jun 28 12:10:55 2012 -0400

dcache: use IS_ROOT to decide where dentry is hashed

Every hashed dentry is either hashed in the dentry_hashtable, or a superblock's s_anon list.

__d_shrink assumes it can determine which is the case by checking DCACHE_DISCONNECTED; this is not true.

It is true that when DCACHE_DISCONNECTED is cleared, the dentry is not only hashed on dentry_hashtable, but is fully connected to its parents back to the root.

But the converse is *not* true: fs/exportfs/expfs.c:reconnect_path() attempts to connect a directory (found by filehandle lookup) back to root by ascending to parents and performing lookups one at a time. It does not clear DCACHE_DISCONNECTED until its done, and that is not at all an atomic process.

In particular, it is possible for DCACHE_DISCONNECTED to be set on a dentry which is hashed on the dentry_hashtable.

Instead, use IS_ROOT() to check which hash chain a dentry is on. This *does* work:

Dentries are hashed only by:

- d_obtain_alias, which adds an IS_ROOT() dentry to sb_anon.

- __d_rehash, called by _d_rehash: hashes to the dentry's parent, and all callers of _d_rehash appear to have d_parent set to a "real" parent.
- __d_rehash, called by __d_move: rehashes the moved dentry to hash chain determined by target, and assigns target's d_parent to its d_parent, before dropping the dentry's d_lock.

Therefore I believe it's safe for a holder of a dentry's d_lock to assume that it is hashed on sb_anon if and only if IS_ROOT(dentry) is true.

I believe the incorrect assumption about DCACHE_DISCONNECTED was originally introduced by ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking".

Cc Neil Brown <[email protected]>

Cc Nick Piggin <npiggin-tSWWG44O7X1aa/[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/dcache.c +0 -0

index 87c2da7..b2b382c 100644

Quoted 112 lines · J. Bruce Fields

Quoted 44 lines · J. Bruce Fields

Coming back to this now, just trying to review the filehandle-lookup/dcache interactions:

Quoted 8 lines · NeilBrown

1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
was only a hint, its absence was a strong statement.
If the flag is set, the dentry might not be linked to the root. If it is clear, it definitely is link through to the root. However I think it was used with stronger intent than that.

Now it seems to mean a little bit more: If it is set and the dentry is hashed, then it must be on the sb->s_anon list.

The code that makes that assumption is __d_shrink (which does the work of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to lock.

I can't find any basis for that assumption. The only code that clears DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as hashing. Am I missing something?

Quoted 7 lines

This is a significant
which I never noticed (I haven't been watching). Originally a disconnected dentry would be attached (and hashed) to its parent. Then that parent would get its own parent and so on until it was attached all the way to the root. Only then would be start clearing DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if that is correct.

It looks wrong to me:

If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle lookup thinking the dentry is OK to use. That could mean for example trying to rename across directories that don't have any ancestor relationship to each other in the dcache yet.

So we need to wait to clear DCACHE_DISCONNECTED until we *know* the dentry's parents go all the way back to the root. As you say, that's what the current code does.

But that means DCACHE_DISCONNECTED dentries can be hashed to their parents, and __d_shrink can be handed such dentries and then get the locking wrong.

Quoted 7 lines

It looks like this bug might originate with Nick Piggin's ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking"? There's no discussion in the changelog, so probably it was just based on an unexamined assumption about DCACHE_DISCONNECTED.

I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test in __d_shrink(), or if we need another flag, or ?

Bah, sorry, and I only just noticed that you already said as much later and did the IS_ROOT() thing in your patch.

Anyway, here's just that one change with a slightly more painstaking changelog.

--b.

commit b1fa644355122627424fe2240a9fc60cbef4c349

Author: J. Bruce Fields <[email protected]>
Date:   Thu Jun 28 12:10:55 2012 -0400

dcache: use IS_ROOT to decide where dentry is hashed

Every hashed dentry is either hashed in the dentry_hashtable, or a superblock's s_anon list.

__d_shrink assumes it can determine which is the case by checking DCACHE_DISCONNECTED; this is not true.

It is true that when DCACHE_DISCONNECTED is cleared, the dentry is not only hashed on dentry_hashtable, but is fully connected to its parents back to the root.

But the converse is *not* true: fs/exportfs/expfs.c:reconnect_path() attempts to connect a directory (found by filehandle lookup) back to root by ascending to parents and performing lookups one at a time. It does not clear DCACHE_DISCONNECTED until its done, and that is not at all an atomic process.

In particular, it is possible for DCACHE_DISCONNECTED to be set on a dentry which is hashed on the dentry_hashtable.

Instead, use IS_ROOT() to check which hash chain a dentry is on. This *does* work:

Dentries are hashed only by:

- d_obtain_alias, which adds an IS_ROOT() dentry to sb_anon.

- __d_rehash, called by _d_rehash: hashes to the dentry's parent, and all callers of _d_rehash appear to have d_parent set to a "real" parent.
- __d_rehash, called by __d_move: rehashes the moved dentry to hash chain determined by target, and assigns target's d_parent to its d_parent, before dropping the dentry's d_lock.

Therefore I believe it's safe for a holder of a dentry's d_lock to assume that it is hashed on sb_anon if and only if IS_ROOT(dentry) is true.

I believe the incorrect assumption about DCACHE_DISCONNECTED was originally introduced by ceb5bdc2d246 "fs: dcache per-bucket dcache hash locking".

Cc Neil Brown <[email protected]>

Cc Nick Piggin <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/dcache.c static void __d_shrink(struct dentry *dentry) · line 413

Quoted 10 lines

fs/dcache.c +1 -1

index 87c2da7..b2b382c 100644
@@ -410,7 +410,7 @@ static void __d_shrink(struct dentry *dentry)
 {
 	if (!d_unhashed(dentry)) {
 		struct hlist_bl_head *b;
-		if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
+		if (unlikely(IS_ROOT(dentry->d_flags)))

Um, right--I'll send an actual tested version along with some other stuff later.

--b.

fs/dcache.c static void __d_shrink(struct dentry *dentry) · line 416

Quoted 3 lines

fs/dcache.c +0 -0

 			b = &dentry->d_sb->s_anon;
 		else
 			b = d_hash(dentry->d_parent, dentry->d_name.hash);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html