From: Cool F. <fin...@ya...> - 2005-09-28 17:37:02
|
Hi, I apologise in advance for the highly-technical nature of this email. :-) I am trying to implement proper NFS support for FUSE by implementing my own FUSE user-space library that can support nodeid-based lookups. To get this support, I have implemented two export_operations: get_dentry() and get_parent() which look something like this: struct inode* fuse_ilookup(struct super_block *sb, unsigned long nodeid) { struct fuse_conn *fc = get_fuse_conn_super(sb); struct fuse_req *req = fuse_get_request(fc); struct fuse_entry_out outarg; struct inode* inode = NULL; int err; if (!req) return 0; req->in.h.opcode = FUSE_ILOOKUP; req->in.h.nodeid = nodeid; req->inode = NULL; req->in.numargs = 0; req->out.numargs = 1; req->out.args[0].size = sizeof(struct fuse_entry_out); req->out.args[0].value = &outarg; request_send(fc, req); err = req->out.h.error; if (!err) { inode = fuse_iget(sb, nodeid, outarg.generation, &outarg.attr); if (!inode) { fuse_send_forget(fc, req, outarg.nodeid, 1); return ERR_PTR(-ENOMEM); } } fuse_put_request(fc, req); return inode; } static struct dentry *fuse_get_dentry(struct super_block *sb, void *vobjp) { __u32 *objp = vobjp; unsigned long nodeid = objp[0]; __u32 generation = objp[1]; struct inode *inode; struct dentry *entry; if (nodeid == 0) return ERR_PTR(-ESTALE); inode = ilookup5(sb, nodeid, fuse_inode_eq, &nodeid); if (!inode) { inode = fuse_ilookup(sb, nodeid); if (IS_ERR(inode)) return ERR_PTR(PTR_ERR(inode)); } if (!inode) return ERR_PTR(-ESTALE); if (inode->i_generation != generation) { iput(inode); return ERR_PTR(-ESTALE); } entry = d_alloc_anon(inode); if (!entry) { iput(inode); return ERR_PTR(-ENOMEM); } return entry; } extern int fuse_lookup_iget(struct inode* dir, struct dentry *entry, struct inode **inodep); static struct dentry* fuse_get_parent(struct dentry* child) { struct inode *inode = NULL; struct dentry *parent; struct dentry dotdot; dotdot.d_name.name = ".."; dotdot.d_name.len = 2; int err = fuse_lookup_iget(child->d_inode, &dotdot, &inode); if (err) return ERR_PTR(err); if (!inode) return ERR_PTR(-EACCES); parent = d_alloc_anon(inode); if (!parent) { iput(inode); parent = ERR_PTR(-ENOMEM); } return parent; } Essentially, I modified get_dentry to do an "ilookup" if the inode is not in the cache. Also, I added get_parent. I based these on implementation in other filesystems. The problem I have is with this set of operations for NFS mount on /mnt: mkdir -p /mnt/a/b/c/ echo foo > /mnt/a/b/c/foo cd /mnt/a/b/c cat foo #stop NFS server #unmount FUSE #mount FUSE #start NFS server cat foo This causes get_dentry() and get_parent() to be exercised. I don't get ESTALE as with original FUSE implementation. However, on unmount, I get busy inodes for all the parent directories of "foo". Any idea what I might be doing wrong for the refcounts in these functions? All other filesystems implement them in similar ways. Thanks, Akshat __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Miklos S. <mi...@sz...> - 2005-09-29 08:46:01
|
> Any idea what I might be doing wrong for the refcounts > in these functions? All other filesystems implement > them in similar ways. I don't see that you increase ->nlookup after a successful ILOOKUP operation. Maybe that's causing the problem. Miklos |
From: Cool F. <fin...@ya...> - 2005-09-29 13:45:47
|
--- Miklos Szeredi <mi...@sz...> wrote: > > Any idea what I might be doing wrong for the > refcounts > > in these functions? All other filesystems > implement > > them in similar ways. > > I don't see that you increase ->nlookup after a > successful ILOOKUP > operation. Maybe that's causing the problem. > the ILOOKUP operation uses fuse_iget after we get the attributes from userspace. fuse_iget will increase nlookup. One difference between other filesystems and FUSE is that FUSE is not device-backed. Could that be an issue? > Miklos > -Akshat __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Miklos S. <mi...@sz...> - 2005-09-29 14:06:01
|
> > I don't see that you increase ->nlookup after a > > successful ILOOKUP > > operation. Maybe that's causing the problem. > > > > the ILOOKUP operation uses fuse_iget after we get the > attributes from userspace. fuse_iget will increase > nlookup. Right, I missed that. > One difference between other filesystems and FUSE is > that FUSE is not device-backed. Could that be an > issue? Lots of other filesystems are not device-backed: all network filesystems, ramfs, tmpfs, proc, etc. But some of them surely support NFS exporting. I don't know what is causing leaked inodes, but the key is probably in those export methods. Miklos |
From: Cool F. <fin...@ya...> - 2005-09-29 15:01:04
|
--- Miklos Szeredi <mi...@sz...> wrote: > > > One difference between other filesystems and FUSE > is > > that FUSE is not device-backed. Could that be an > > issue? > > Lots of other filesystems are not device-backed: all > network > filesystems, ramfs, tmpfs, proc, etc. But some of > them surely support > NFS exporting. > There's a difference between exporting and "exporting properly", meaning that you provide proper implementations for export operations that would let you create an inode based on the inode number offered by the client and connect it back into the dentry tree by walking up. ramfs, tmpfs, and proc aren't even persistent, so an inode-number based lookup is a moot point. I don't know if any of the network filesystems can be re-exported though. I looked through the code, and nfs client-side does not seem to provide export operations. > I don't know what is causing leaked inodes, but the > key is probably in > those export methods. > > Miklos > -Akshat __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Cool F. <fin...@ya...> - 2005-09-29 20:56:25
|
Miklos, I believe I have tracked down the problem that I'm getting. fuse_lookup calls d_find_alias for a directory to check if an alias is being created. There is a check: if (alias && !(alias->d_flags & DCACHE_DISCONNECTED)) which, if true, dputs the alias and iputs the inode and returns. However, in my case, with NFS, alias is present but the other condition is false, so this alias is never put back and there is an extra reference for the dentry. If I change the code to: if (alias && !(alias->d_flags & DCACHE_DISCONNECTED)) { dput(alias); iput(inode); return ERR_PTR(-EIO); } else if (alias) { dput(alias); } this seems to fix the problem. Is this fix correct? -Akshat --- Miklos Szeredi <mi...@sz...> wrote: > > > I don't see that you increase ->nlookup after a > > > successful ILOOKUP > > > operation. Maybe that's causing the problem. > > > > > > > the ILOOKUP operation uses fuse_iget after we get > the > > attributes from userspace. fuse_iget will > increase > > nlookup. > > Right, I missed that. > > > One difference between other filesystems and FUSE > is > > that FUSE is not device-backed. Could that be an > > issue? > > Lots of other filesystems are not device-backed: all > network > filesystems, ramfs, tmpfs, proc, etc. But some of > them surely support > NFS exporting. > > I don't know what is causing leaked inodes, but the > key is probably in > those export methods. > > Miklos > > > ------------------------------------------------------- > This SF.Net email is sponsored by: > Power Architecture Resource Center: Free content, > downloads, discussions, > and more. > http://solutions.newsforge.com/ibmarch.tmpl > _______________________________________________ > fuse-devel mailing list > fus...@li... > https://lists.sourceforge.net/lists/listinfo/fuse-devel > __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Cool F. <fin...@ya...> - 2005-09-29 21:17:00
|
Here's a patch against the latest cvs: Signed-off-by: Akshat Aranya <fin...@ya...> Index: kernel/dir.c =================================================================== RCS file: /cvsroot/fuse/fuse/kernel/dir.c,v retrieving revision 1.87 diff -u -r1.87 dir.c --- kernel/dir.c 23 Sep 2005 13:34:44 -0000 1.87 +++ kernel/dir.c 29 Sep 2005 21:15:18 -0000 @@ -821,10 +821,14 @@ if (inode && S_ISDIR(inode->i_mode)) { /* Don't allow creating an alias to a directory */ struct dentry *alias = d_find_alias(inode); - if (alias && !(alias->d_flags & DCACHE_DISCONNECTED)) { - dput(alias); - iput(inode); - return ERR_PTR(-EIO); + if (alias) { + if (alias->d_flags & DCACHE_DISCONNECTED) { + dput(alias); + } else { + dput(alias); + iput(inode); + return ERR_PTR(-EIO); + } } } return d_splice_alias(inode, entry); -Akshat --- Cool Frood <fin...@ya...> wrote: > Miklos, > > I believe I have tracked down the problem that I'm > getting. fuse_lookup calls d_find_alias for a > directory to check if an alias is being created. > There is a check: > > if (alias && !(alias->d_flags & > DCACHE_DISCONNECTED)) > > which, if true, dputs the alias and iputs the inode > and returns. However, in my case, with NFS, alias > is > present but the other condition is false, so this > alias is never put back and there is an extra > reference for the dentry. If I change the code to: > > if (alias && !(alias->d_flags & > DCACHE_DISCONNECTED)) > { > dput(alias); > iput(inode); > return ERR_PTR(-EIO); > } else if (alias) { > dput(alias); > } > > this seems to fix the problem. Is this fix correct? > > -Akshat > > > > --- Miklos Szeredi <mi...@sz...> wrote: > > > > > I don't see that you increase ->nlookup after > a > > > > successful ILOOKUP > > > > operation. Maybe that's causing the problem. > > > > > > > > > > the ILOOKUP operation uses fuse_iget after we > get > > the > > > attributes from userspace. fuse_iget will > > increase > > > nlookup. > > > > Right, I missed that. > > > > > One difference between other filesystems and > FUSE > > is > > > that FUSE is not device-backed. Could that be > an > > > issue? > > > > Lots of other filesystems are not device-backed: > all > > network > > filesystems, ramfs, tmpfs, proc, etc. But some of > > them surely support > > NFS exporting. > > > > I don't know what is causing leaked inodes, but > the > > key is probably in > > those export methods. > > > > Miklos > > > > > > > ------------------------------------------------------- > > This SF.Net email is sponsored by: > > Power Architecture Resource Center: Free content, > > downloads, discussions, > > and more. > > http://solutions.newsforge.com/ibmarch.tmpl > > _______________________________________________ > > fuse-devel mailing list > > fus...@li... > > > https://lists.sourceforge.net/lists/listinfo/fuse-devel > > > > > > > __________________________________ > Yahoo! Mail - PC Magazine Editors' Choice 2005 > http://mail.yahoo.com > > > ------------------------------------------------------- > This SF.Net email is sponsored by: > Power Architecture Resource Center: Free content, > downloads, discussions, > and more. > http://solutions.newsforge.com/ibmarch.tmpl > _______________________________________________ > fuse-devel mailing list > fus...@li... > https://lists.sourceforge.net/lists/listinfo/fuse-devel > __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Miklos S. <mi...@sz...> - 2005-09-30 08:48:50
|
> I believe I have tracked down the problem that I'm > getting. fuse_lookup calls d_find_alias for a > directory to check if an alias is being created. > There is a check: > > if (alias && !(alias->d_flags & DCACHE_DISCONNECTED)) > > which, if true, dputs the alias and iputs the inode > and returns. However, in my case, with NFS, alias is > present but the other condition is false, so this > alias is never put back and there is an extra > reference for the dentry. If I change the code to: > > if (alias && !(alias->d_flags & DCACHE_DISCONNECTED)) > { > dput(alias); > iput(inode); > return ERR_PTR(-EIO); > } else if (alias) { > dput(alias); > } > > this seems to fix the problem. Is this fix correct? Yes, great find! Committed to CVS. Just a minor nit: the 'if (alias)' is not needed, because dput checks for NULL argument. Could this be the cause of the lockups too? Thanks, Miklos |
From: Cool F. <fin...@ya...> - 2005-09-30 13:15:43
|
--- Miklos Szeredi <mi...@sz...> wrote: > > I believe I have tracked down the problem that I'm > > getting. fuse_lookup calls d_find_alias for a > > directory to check if an alias is being created. > > There is a check: > > > > if (alias && !(alias->d_flags & > DCACHE_DISCONNECTED)) > > > > which, if true, dputs the alias and iputs the > inode > > and returns. However, in my case, with NFS, alias > is > > present but the other condition is false, so this > > alias is never put back and there is an extra > > reference for the dentry. If I change the code > to: > > > > if (alias && !(alias->d_flags & > DCACHE_DISCONNECTED)) > > { > > dput(alias); > > iput(inode); > > return ERR_PTR(-EIO); > > } else if (alias) { > > dput(alias); > > } > > > > this seems to fix the problem. Is this fix > correct? > > Yes, great find! Committed to CVS. > > Just a minor nit: the 'if (alias)' is not needed, > because dput checks > for NULL argument. > > Could this be the cause of the lockups too? > I doubt if this is related to the lockup problem, because the lockup scenario is simple WRITE calls which would not use the lookup code, but if Franco can test it out, that would be great. I will get to investigating that eventually. > Thanks, > Miklos > -Akshat __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com |
From: Miklos S. <mi...@sz...> - 2005-09-30 13:25:22
|
> > Could this be the cause of the lockups too? > > > > I doubt if this is related to the lockup problem, > because the lockup scenario is simple WRITE calls > which would not use the lookup code, Traces from Franco indicated that it's in locking up in readdir/getattr. But it was not OOM, so the dentry leak doesn't fit the picture too well, I admit. Miklos |