From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Harkes Subject: Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12) Date: Thu, 17 Oct 2002 18:16:52 -0400 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <20021017221652.GA26692@ravel.coda.cs.cmu.edu> References: <200205162142.AWF00051@netmail.netcologne.de> <20020517034357.GA18449@ravel.coda.cs.cmu.edu> <20021017203804.GA24523@ravel.coda.cs.cmu.edu> <15791.12331.663212.692225@charged.uio.no> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Harkes , linux-fsdevel@vger.kernel.org, Marcelo Tosatti , alan@lxorguk.ukuu.org.uk, Alexander Viro Return-path: To: Trond Myklebust Content-Disposition: inline In-Reply-To: <15791.12331.663212.692225@charged.uio.no> List-Id: linux-fsdevel.vger.kernel.org On Thu, Oct 17, 2002 at 11:48:27PM +0200, Trond Myklebust wrote: > >>>>> " " == Jan Harkes writes: > > Originally, returning a error from d_revalidate in cached_lookup would > > force a subsequent real_lookup. After the patch, returning an error from > > d_revalidate propagates (in some cases) ESTALE back up to > > userspace. > > Which is the whole point of the patch. If you are trying to read or > modify a directory that is invalid, you need to be notified of that. Yes, by failing in real_lookup, not randomly crapping out with ESTALE. > > Now either every filesystem will have to follow NFS's lead and > ^^^^^ > The vast majority of filesystems are coping fine as they are. Right, well only network filesystems could possibly be affected as on-disk filesystems don't have any reason why the cached path would suddenly become invalid, so let's look at network filesytems, - Coda definitely doesn't cope just fine. - smbfs, calls into revalidate_inode instead of retrying the lookup. Broken. - ncpfs, dentry->d_parent->d_inode. Hmm, probably breaks when revalidating the root of the mounted fs? - intermezzo, revalidates data and attributes of the object, same broken behaviour as smbfs. - NFS works! Were there any other network filesystems in the kernel? So we've already got 4 out of 5 broken. Now let's look at out of kernel network and virtual filesystems, - OpenAFS, guess they're broken. - Arla, guess what :) - Supermount, well that is the guy who traced where the strange ESTALE problems came from, he worked on a patch that probably ended up working because I guess nothing can suddenly get renamed from underneath him. - CDFS, same deal. > > implement some form of real_lookup inside of the > > dentry_revalidate function which is not the prettiest > > solution. Or the VFS patch should be reverted/fixed to actually > > walk the tree for '.' and '..' lookups. > > The whole problem was that we did *not* walk the tree for '.' and > '.. 'lookups. Reverting the VFS patch simply results in us stupidly > reusing current->fs->pwd, dentry->d_parent and friends without > checking if it is safe to do so. Sorry it should have been 'reverted, or fixed to actually revalidate all entries on the cached tree leading up to '.' and '..'. > > btw. the patch also leaks dentries when a 'stale dentry' > > happens to have children because it doesn't properly check and > > handle the d_invalidate returncode. > > Why? d_invalidate() doesn't dget() anything, and if you are doing > 'open(".")' then it will return -EBUSY anyway. What are you going to > do about it? /** * d_invalidate - invalidate a dentry * @dentry: dentry to invalidate * * Try to invalidate the dentry if it turns out to be * possible. If there are other dentries that can be * reached through this one we can't delete it and we * return -EBUSY. On success we return 0. * * no dcache lock. */ It's the missing 'd_put' that is the problem. The code should probably look like all other places where d_invalidate is called, i.e. if (!dentry->d_op->d_revalidate(dentry, flags)) { - d_invalidate(dentry); + if (!d_invalidate(dentry)) { + dput(dentry); + dentry = NULL; + } break; } But why would I submit a patch to fix something that I believe is fundamentally broken in the first place. I agree about the problem, I just disagree about the fix that was applied. And I actually thought it had already been reverted last May. Jan