* Don't use d_alloc_anon for open_by_handle [not found] <20080501070244.GH108924158@sgi.com> @ 2008-05-02 1:55 ` xaiki 2008-05-02 1:55 ` [PATCH] Don't use hashed dentries when doing open_by_handle xaiki 2008-05-02 6:06 ` Don't use d_alloc_anon for open_by_handle Christoph Hellwig 0 siblings, 2 replies; 9+ messages in thread From: xaiki @ 2008-05-02 1:55 UTC (permalink / raw) To: xfs-dev; +Cc: xfs 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Don't use hashed dentries when doing open_by_handle 2008-05-02 1:55 ` Don't use d_alloc_anon for open_by_handle xaiki @ 2008-05-02 1:55 ` xaiki 2008-05-02 1:55 ` [PATCH] Use xfs_d_alloc_anon for DM rdwr using handle code xaiki 2008-05-02 6:06 ` Don't use d_alloc_anon for open_by_handle Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: xaiki @ 2008-05-02 1:55 UTC (permalink / raw) To: xfs-dev; +Cc: xfs, Niv Sardi, Niv Sardi 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] Use xfs_d_alloc_anon for DM rdwr using handle code. 2008-05-02 1:55 ` [PATCH] Don't use hashed dentries when doing open_by_handle xaiki @ 2008-05-02 1:55 ` xaiki 0 siblings, 0 replies; 9+ messages in thread From: xaiki @ 2008-05-02 1:55 UTC (permalink / raw) To: xfs-dev; +Cc: xfs, Niv Sardi, Niv Sardi 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-02 1:55 ` Don't use d_alloc_anon for open_by_handle xaiki 2008-05-02 1:55 ` [PATCH] Don't use hashed dentries when doing open_by_handle xaiki @ 2008-05-02 6:06 ` Christoph Hellwig 2008-05-05 6:33 ` Niv Sardi 1 sibling, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2008-05-02 6:06 UTC (permalink / raw) To: xaiki; +Cc: xfs-dev, xfs 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-02 6:06 ` Don't use d_alloc_anon for open_by_handle Christoph Hellwig @ 2008-05-05 6:33 ` Niv Sardi 2008-05-05 9:53 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Niv Sardi @ 2008-05-05 6:33 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs-dev, xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-05 6:33 ` Niv Sardi @ 2008-05-05 9:53 ` Christoph Hellwig 2008-05-05 18:44 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2008-05-05 9:53 UTC (permalink / raw) To: Niv Sardi; +Cc: Christoph Hellwig, xfs-dev, xfs 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.. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-05 9:53 ` Christoph Hellwig @ 2008-05-05 18:44 ` Christoph Hellwig 2008-05-05 20:51 ` Greg Banks 2008-05-06 1:38 ` Niv Sardi 0 siblings, 2 replies; 9+ messages in thread From: Christoph Hellwig @ 2008-05-05 18:44 UTC (permalink / raw) To: Niv Sardi; +Cc: xfs-dev, xfs, gnb 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-05 18:44 ` Christoph Hellwig @ 2008-05-05 20:51 ` Greg Banks 2008-05-06 1:38 ` Niv Sardi 1 sibling, 0 replies; 9+ messages in thread From: Greg Banks @ 2008-05-05 20:51 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Niv Sardi, xfs-dev, xfs, gnb 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Don't use d_alloc_anon for open_by_handle 2008-05-05 18:44 ` Christoph Hellwig 2008-05-05 20:51 ` Greg Banks @ 2008-05-06 1:38 ` Niv Sardi 1 sibling, 0 replies; 9+ messages in thread From: Niv Sardi @ 2008-05-06 1:38 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs-dev, xfs, gnb 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-05-06 1:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20080501070244.GH108924158@sgi.com>
2008-05-02 1:55 ` Don't use d_alloc_anon for open_by_handle xaiki
2008-05-02 1:55 ` [PATCH] Don't use hashed dentries when doing open_by_handle xaiki
2008-05-02 1:55 ` [PATCH] Use xfs_d_alloc_anon for DM rdwr using handle code xaiki
2008-05-02 6:06 ` Don't use d_alloc_anon for open_by_handle Christoph Hellwig
2008-05-05 6:33 ` Niv Sardi
2008-05-05 9:53 ` Christoph Hellwig
2008-05-05 18:44 ` Christoph Hellwig
2008-05-05 20:51 ` Greg Banks
2008-05-06 1:38 ` Niv Sardi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox