Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes <jaharkes@cs.cmu.edu>]

On Thu, Oct 17, 2002 at 11:48:27PM +0200, Trond Myklebust wrote:
> >>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> 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