From: Jeff Moyer <jmoyer@redhat.com> To: linuxram@us.ibm.com (Ram Pai) Cc: autofs@linux.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix Date: Tue, 29 Nov 2005 20:16:44 -0500 [thread overview] Message-ID: <17292.64892.680738.833917@segfault.boston.redhat.com> (raw) In-Reply-To: <20051116101740.GA9551@RAM> ==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; linuxram@us.ibm.com (Ram Pai) adds: linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case. linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate(). linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore. Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held: static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) { ... /* * Note: we're not holding inode->i_sem and so may be racing with * operations that change the directory. We therefore save the * change attribute *before* we do the RPC call. */ Can you try out a patch which does this? -Jeff --- linux-2.6.14/fs/namei.c.orig 2005-11-29 20:14:30.000000000 -0500 +++ linux-2.6.14/fs/namei.c 2005-11-29 20:14:48.000000000 -0500 @@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str dentry = d_lookup(parent, name); if (dentry && dentry->d_op && dentry->d_op->d_revalidate) { + up(&parent->d_inode->i_sem); if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) { dput(dentry); dentry = NULL; } + down(&parent->d_inode->i_sem); } return dentry; }
WARNING: multiple messages have this Message-ID (diff)
From: Jeff Moyer <jmoyer@redhat.com> To: Ram Pai <linuxram@us.ibm.com> Cc: autofs@linux.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix Date: Tue, 29 Nov 2005 20:16:44 -0500 [thread overview] Message-ID: <17292.64892.680738.833917@segfault.boston.redhat.com> (raw) In-Reply-To: <20051116101740.GA9551@RAM> ==> Regarding [autofs] [RFC PATCH]autofs4: hang and proposed fix; linuxram@us.ibm.com (Ram Pai) adds: linuxram> Autofs4 assumes that its ->revalidate() function gets called with linuxram> the parent_dentry's_inode_semaphore released. This is true mostly linuxram> but not in one particular case. linuxram> Process P1 calls autofs4's ->lookup(). The lookup finds that the linuxram> dentry does not exist. It creates a dentry and adds to the linuxram> cache. Releases the parent's inode's semaphore and than calls linuxram> ->revalidate(). linuxram> Process P2 meanwhile comes in and cached_lookup() gets called. It linuxram> finds the dentry in the cache and finds ->revalidate() function linuxram> exists. So it calls ->revalidate() holding the parent's inode's linuxram> semaphore. Can't we simply fix this case? It seems like it should be perfectly safe to drop the parent's i_sem before calling revalidate in cached_lookup. In fact, there are comments in the NFS code that would lead one to believe that revalidate is not supposed to be called with the parent's i_sem held: static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd) { ... /* * Note: we're not holding inode->i_sem and so may be racing with * operations that change the directory. We therefore save the * change attribute *before* we do the RPC call. */ Can you try out a patch which does this? -Jeff --- linux-2.6.14/fs/namei.c.orig 2005-11-29 20:14:30.000000000 -0500 +++ linux-2.6.14/fs/namei.c 2005-11-29 20:14:48.000000000 -0500 @@ -332,10 +332,12 @@ static struct dentry * cached_lookup(str dentry = d_lookup(parent, name); if (dentry && dentry->d_op && dentry->d_op->d_revalidate) { + up(&parent->d_inode->i_sem); if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) { dput(dentry); dentry = NULL; } + down(&parent->d_inode->i_sem); } return dentry; }
next prev parent reply other threads:[~2005-11-30 1:17 UTC|newest] Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top 2005-11-16 10:17 [RFC PATCH]autofs4: hang and proposed fix Ram Pai 2005-11-16 12:41 ` [autofs] " Ian Kent 2005-11-16 16:50 ` Ram Pai 2005-11-16 22:57 ` Ian Kent 2005-11-17 1:52 ` [autofs] " Ram Pai 2005-11-17 18:50 ` Ian Kent 2005-11-17 19:19 ` William H. Taber 2005-11-17 20:39 ` Ram Pai 2005-11-17 22:31 ` William H. Taber 2005-11-18 14:57 ` Ian Kent 2005-11-18 14:54 ` Ian Kent 2005-11-18 14:44 ` Ian Kent 2005-11-18 15:20 ` William H. Taber 2005-11-18 16:30 ` Ian Kent 2005-11-18 17:12 ` William H. Taber 2005-11-18 18:57 ` Ram Pai 2005-11-18 20:08 ` William H. Taber 2005-11-19 2:52 ` Ian Kent 2005-11-21 16:40 ` William H. Taber 2005-11-22 13:13 ` Ian Kent 2005-11-22 17:48 ` [autofs] " William H. Taber 2005-11-23 14:11 ` Ian Kent 2005-11-23 16:42 ` William H. Taber 2005-11-23 17:52 ` Ian Kent 2005-11-23 18:47 ` William H. Taber 2005-11-23 17:52 ` Ian Kent 2005-11-19 1:40 ` [autofs] " Ian Kent 2005-11-16 15:22 ` Jeff Moyer 2005-11-16 15:22 ` Jeff Moyer 2005-11-16 17:00 ` [autofs] " Ram Pai 2005-11-16 18:25 ` Jeff Moyer 2005-11-16 19:24 ` William H. Taber 2005-11-16 19:51 ` Ram Pai 2005-11-27 10:47 ` Ian Kent 2005-11-28 17:19 ` William H. Taber 2005-11-28 23:12 ` Badari Pulavarty 2005-11-29 14:19 ` Ian Kent 2005-11-29 16:34 ` William H. Taber 2005-11-30 14:02 ` Ian Kent 2005-11-30 16:49 ` Badari Pulavarty 2005-11-30 17:04 ` Trond Myklebust 2005-11-30 21:10 ` William H. Taber 2005-11-29 14:20 ` Ian Kent 2005-11-30 1:16 ` Jeff Moyer [this message] 2005-11-30 1:16 ` [autofs] " Jeff Moyer 2005-11-30 1:56 ` Trond Myklebust 2005-11-30 4:15 ` Jeff Moyer 2005-11-30 6:14 ` Trond Myklebust 2005-11-30 15:44 ` Ian Kent 2005-11-30 15:53 ` [autofs] " Trond Myklebust 2005-11-30 16:12 ` Ian Kent 2005-11-30 16:27 ` Ian Kent 2005-11-30 16:45 ` [autofs] " Trond Myklebust 2005-11-30 20:32 ` William H. Taber 2005-11-30 20:53 ` Trond Myklebust 2005-11-30 21:30 ` William H. Taber 2005-11-30 22:32 ` Trond Myklebust 2005-12-01 16:27 ` William H. Taber 2005-12-01 12:09 ` Ian Kent 2005-12-01 16:30 ` William H. Taber 2005-12-02 13:49 ` Ian Kent 2005-12-02 14:07 ` Jeff Moyer 2005-12-02 15:21 ` Ian Kent 2005-12-02 16:35 ` [autofs] " Will Taber 2005-12-02 17:11 ` Ian Kent 2005-12-02 15:34 ` Will Taber 2005-12-02 17:29 ` Ian Kent 2005-12-02 18:12 ` Trond Myklebust 2005-12-04 12:56 ` Christoph Hellwig 2005-12-04 12:57 ` Christoph Hellwig 2005-12-04 14:58 ` Ian Kent 2005-12-04 17:17 ` [autofs] " Christoph Hellwig 2005-12-05 14:02 ` Ian Kent 2005-12-06 21:20 ` Jeff Moyer 2005-12-06 21:40 ` Christoph Hellwig 2005-12-06 22:37 ` Jeff Moyer 2005-12-07 14:52 ` Will Taber 2005-12-07 15:18 ` Christoph Hellwig 2005-12-07 15:22 ` Brian Long 2005-12-07 15:25 ` Christoph Hellwig 2005-12-07 17:46 ` Will Taber 2005-12-08 14:16 ` Ian Kent 2005-12-09 12:12 ` Christoph Hellwig 2005-12-09 13:33 ` John T. Kohl 2005-12-13 18:39 ` Christoph Hellwig 2005-12-04 14:56 ` Ian Kent 2005-12-02 19:04 ` [autofs] " Will Taber 2005-12-04 9:39 ` Ian Kent 2005-12-02 16:04 ` [autofs] " Jeff Moyer 2005-12-02 17:36 ` Ian Kent 2005-12-02 18:33 ` [autofs] " Will Taber 2005-12-04 9:52 ` Ian Kent 2005-12-04 14:54 ` Ian Kent 2005-12-05 15:40 ` Ian Kent 2005-11-30 14:48 ` [autofs] " Ian Kent
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=17292.64892.680738.833917@segfault.boston.redhat.com \ --to=jmoyer@redhat.com \ --cc=autofs@linux.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linuxram@us.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.