Don't use d_alloc_anon for open_by_handle

Don't use d_alloc_anon for open_by_handle

[xaiki]

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.

[PATCH] Don't use hashed dentries when doing open_by_handle

[xaiki]

From: Niv Sardi <xaiki@debian.org>

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.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 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(-)

diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index 4c82a05..66ed268 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -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);
diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index cc4abd3..cf5e83f 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -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

[PATCH] Use xfs_d_alloc_anon for DM rdwr using handle code.

[xaiki]

From: Niv Sardi <xaiki@debian.org>

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.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/dmapi/xfs_dm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/dmapi/xfs_dm.c b/fs/xfs/dmapi/xfs_dm.c
index c4f57a9..5aa00a9 100644
--- a/fs/xfs/dmapi/xfs_dm.c
+++ b/fs/xfs/dmapi/xfs_dm.c
@@ -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

Re: Don't use d_alloc_anon for open_by_handle

[Christoph Hellwig]

On Fri, May 02, 2008 at 11:55:37AM +1000, xaiki@sgi.com wrote:
> 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.

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.

Re: Don't use d_alloc_anon for open_by_handle

[Niv Sardi]

Christoph Hellwig <hch@infradead.org> writes:
> On Fri, May 02, 2008 at 11:55:37AM +1000, xaiki@sgi.com wrote:
>> 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.
>
> No, this is even more buggy than using d_alloc_anon.

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.

> What really needs to be done in the handle code is to do the full
> reconnect logic nfsd would be doing. 

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.

> 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.

Cheers,
-- 
Niv Sardi

Re: Don't use d_alloc_anon for open_by_handle

[Christoph Hellwig]

On Mon, May 05, 2008 at 04:33:34PM +1000, Niv Sardi wrote:
> >
> > No, this is even more buggy than using d_alloc_anon.
> 
> 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.
> 
> > What really needs to be done in the handle code is to do the full
> > reconnect logic nfsd would be doing. 
> 
> 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.

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..

Re: Don't use d_alloc_anon for open_by_handle

[Christoph Hellwig]

On Mon, May 05, 2008 at 05:53:16AM -0400, Christoph Hellwig wrote:
> 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.

Re: Don't use d_alloc_anon for open_by_handle

[Greg Banks]

Christoph Hellwig wrote:
> On Mon, May 05, 2008 at 05:53:16AM -0400, Christoph Hellwig wrote:
>   
>> 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 
Agreed.
> and
> I think even the default these days.
I believe so.  We also use that as default on our shipping NAS servers
anyway.
>   So we'd better fix the lacking
> expiry in the core code. 
Mmm, sounds like fun.
>  Cc'ing Greg as he's been fighting this code
> quite a bit in the past.
>
>   
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.

Re: Don't use d_alloc_anon for open_by_handle

[Niv Sardi]

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, May 05, 2008 at 05:53:16AM -0400, Christoph Hellwig wrote:
>> 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.

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