From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Kent Subject: Re: [PATCH 1/2] vfs: make real_lookup do dentry revalidation with i_mutex held Date: Thu, 26 Mar 2009 17:00:10 +0900 Message-ID: <49CB360A.1010601@themaw.net> References: <1237493790-5665-1-git-send-email-sage@newdream.net> <49C85E3E.7030505@themaw.net> <49C85F0A.9030409@themaw.net> <49C9CA5A.5080809@themaw.net> <49CA579B.5080307@themaw.net> <49CAE3DB.3030909@themaw.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, hch@infradead.org, akpm@linux-foundation.org, Al Viro , Andreas Dilger , Yehuda Sadeh To: Sage Weil Return-path: Received: from outbound.icp-qv1-irony-out3.iinet.net.au ([203.59.1.148]:39726 "EHLO outbound.icp-qv1-irony-out3.iinet.net.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755725AbZCZIAV (ORCPT ); Thu, 26 Mar 2009 04:00:21 -0400 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Sage Weil wrote: > On Thu, 26 Mar 2009, Ian Kent wrote: >>> Would it be possible to avoid the upcall in revalidate, and instead fail >>> and let the subsequent lookup path do it? (I'm guessing the upcall >>> doesn't happen for _every_ revalidate?) >> Yes, that's right, just every revalidate from processes that aren't the >> automount process itself. The normal case is the mount succeeds and >> further walks follow the mount from then on until it expires. >> >> It was more than three years ago when I tried to make everything go >> through lookup so my memory is pretty unclear now. In the end I think >> there was one case in real_lookup() where the lookup was skipped, >> revalidate was called and failed but lookup wasn't then called again and >> I got an incorrect failure. > > That is _exactly_ the bug this patch is fixing. :) A (racing) process > ends up in real_lookup(), takes the mutex and finds the dentry has already > been added to the cache by someone else. The mutex is dropped, revalidate > is called, and if it fails, real_lookup() returns ENOENT (!!) without ever > trying lookup. The basic problem is that the fs revalidate might fail, > expecting lookup to get called, but real_lookup() returns ENOENT > instead... which is just wrong. > >> AFAICR all other code paths that hold the >> mutex over lookup and revalidate perform the revalidate first and then >> the lookup if that fails, which avoids this case. > > If you mean the paths autofs manages to avoid (unlinkat, rmdir, etc.), > yeah: the mutex is taken, then cached_lookup() (and revalidate), then > lookup if necessary. Holding the mutex over revalidate avoids dealing > with various races. > > So, it sounds like this fix would need to go in along with an autofs patch > that moves the upcall back into lookup exclusively, now that a revalidate > failure does the right thing (always calls lookup). Hopefully that would > mean a net simplification on the autofs side as well? Maybe ... doesn't look like it so far ... but it's to soon to tell. autofs4 - always use lookup for mount From: Ian Kent --- fs/autofs4/autofs_i.h | 15 ------ fs/autofs4/root.c | 117 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 81 insertions(+), 51 deletions(-) Ian