Don't use d_alloc_anon for open_by_handle

One of our DMAPI user found that in some cases inodes weren't getting through xfs_inactive() in any reasonable amount of time. Investigation tracked it down to the use of d_alloc_anon() combined with another thread accessing the same inode via an open().

So we introduce a stripped down version of d_alloc_anon, that won't try to find an existing dentry, nor will hash it.

From: Niv Sardi <[email protected]>

Open by handle only require a handle, but that doesn't give us a file descriptor that we require for any IO, before we used to call d_alloc_annon() that created an annonymous disconnected but hashed dentry, from which we extracted the file descriptor, if no one else uses the dentry, the dput on fclose should unhash the dentry and (if not used) tear it appart,

A repported issue is the DMAPI code was, that in some cases (as the dentry was hashed) another thread could use the same dentry inibiting the unhash on the first dput, and hence leaving that dentry on the unused list for ever (or untill caches are dropped).

This cuts down d_alloc_annon to a subset that will only give us an anon dentry, and NOT hash it.

---
 fs/xfs/linux-2.6/xfs_ioctl.c |    2 +-
 fs/xfs/linux-2.6/xfs_iops.c  |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

index 4c82a05..66ed268 100644
@@ -311,7 +311,7 @@ xfs_open_by_handle(
 		return new_fd;
 	}
 
-	dentry = d_alloc_anon(inode);
+	dentry = xfs_d_alloc_anon(inode);
 	if (dentry == NULL) {
 		iput(inode);
 		put_unused_fd(new_fd);

index cc4abd3..cf5e83f 100644
@@ -190,6 +190,23 @@ xfs_ichgtime_fast(
 		mark_inode_dirty_sync(inode);
 }
 
+struct dentry *
+xfs_d_alloc_anon(struct inode *inode)
+{
+	static const struct qstr anonstring = { .name = ""};
+	struct dentry	*res;
+
+	res = d_alloc(NULL, &anonstring);
+	if (!res)
+		return NULL;
+
+	/* attach a disconnected dentry */
+	res->d_sb = inode->i_sb;
+	res->d_parent = res;
+	res->d_inode = inode;
+
+	return res;
+}
 
 /*
  * Pull the link count and size up from the xfs inode to the linux inode

1.5.5.1

From: Niv Sardi <[email protected]>

When doing invisible IO through the DMAPI we only require a handle, but that doesn't give us a file descriptor that we require, before we used to call d_alloc_annon() that created an annonymous disconnected but hashed dentry, from which we extracted the file descriptor, if no one else uses the dentry, the dput on fclose should unhash the dentry and (if not used) tear appart the dentry, and call into xfs_inactive where we will send the DESTROY EVENT. The issue was, that in some cases (as the dentry was hashed) another thread could use the same dentry inibiting the unhash on the first dput, and hence leaving that dentry on the unused list for ever (or untill caches are dropped).

This uses the cuted down d_alloc_annon xfs implementation so that the dentry is never hashed.

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

index c4f57a9..5aa00a9 100644
@@ -1126,7 +1126,7 @@ xfs_dm_rdwr(
 
 	igrab(inode);
 
-	dentry = d_alloc_anon(inode);
+	dentry = xfs_d_alloc_anon(inode);
 	if (dentry == NULL) {
 		iput(inode);
 		return ENOMEM;

1.5.5.1

No, this is even more buggy than using d_alloc_anon. What really needs to be done in the handle code is to do the full reconnect logic nfsd would be doing. Aka you should be using exportfs_decode_fh and update xfs's fh_to_dentry method to accept the file handle type used by the handle interface.

I must be missing a point here, can you please further explain why is it buggy ? We are indeed abusing the normal dentry life, but that's what we want libhandle to be. i.e. We really don't need it connected, we don't need to know anything about that file, we just want to access it.

That would be slow, and we don't really need it. What is wrong with having a temp hanging dentry if it gets thorn appart properly ? The dentry we're using never makes it to any kind of cache. We don't even have security concerns as the 2 pieces of code can only be triggered by root.

Cheers,

Niv Sardi

It shouldn't be slow. You'd do the equivalent no_subtree check export without parent fh, so what we do is call the fh_to_dentry method and then call find_acceptable_alias to check if there's already an dentry around and if yes use that one. That latter part is what should fix your problem. If you want to be lazy you could just copy find_acceptable_alias into the xfs code and call it directly and let me clean up the mess later..

Sorry, this was written before my cup of tea in the morning. find_acceptable_alias is of course a no-op in the no_subtree_check case, and thus it's identical to what we're currently doing in the handle code. So any problem you see here will also be seen in an nfs environment with no_subtree_check, which is the sensible choise and I think even the default these days. So we'd better fix the lacking expiry in the core code. Cc'ing Greg as he's been fighting this code quite a bit in the past.

Agreed.

I believe so. We also use that as default on our shipping NAS servers anyway.

Mmm, sounds like fun.

Thanks. I'm on xfs-dev now so I've been lurking while you guys discussed this :-)

Greg Banks, P.Engineer, SGI Australian Software Group.
The cake is *not* a lie.
I don't speak for SGI.

So I've looked at find_acceptable_alias, and all it does is going through the dentries associated with the inode associated with the dentry we give it, testing if it's 'acceptable' with a function we provide. In this very case *any* dentry is acceptable, so we'd just provide a NO-OP there. So if I do as you suggest and call xfs_fh_to_dentry, which would give me a dentry using d_alloc_anon, which already tries to do the same thing with d_find_alias (just lacking the 'acceptable' check)… So we're back to the exact same situation as now.

The argument is that we should then fix the core code… which should also fix the issue in the way it is NOW… may I ask: ¿ what's the point in moving things around ?

Further more, the only critic you gave to the proposed solution is 'even more buggy than d_alloc_anon', from anyone else than you that would have been a plain useless troll, but I belive you have your reasons to have this position, can you please explain ?

The situation being now, that users of the xfs libhandle interface will have dentries and inodes hanging in space and time for ever until OOM.

Cheers,

Niv Sardi