Re: [rfc][possible solution] RCU vfsmounts

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sun, Sep 29, 2013 at 10:19:59AM -0700, Linus Torvalds wrote:
> I have to say, that when I was working with the dcache lockref code, I
> absolutely _detested_ the magical shrink_dcache_for_umount() code that
> violated all the locking rules.

... and duplicated random-half-of-an-arseload of stuff done in other
shrinking paths.  You are not alone at that - it's been a serious
source of annoyances all along.

> So I actually wouldn't mind at all if that was all forced to follow
> all the same rules that the live filesystem code is forced to follow.
> Yes, yes, it's going to slow things down, but it's not like umount()
> is _that_ performance critical. And I think the whole "let's ignore
> locking rules" actually comes from back when we had one global
> dcache_lock: we used to have batching in order to not hold the
> dcache_lock over long periods, and then it got converted to the
> per-dentry locking, and then that got removed entirely with the whole
> RCU lookup etc.
> 
> So I would be *entirely* ok with having
> shrink_dcache_for_umount_subtree() take the d_lock on the dentry as it
> shrinks it etc etc.

I'm not even sure it will slow the things down that much these days; needs
to be tested, obviously...

FWIW, right now I'm reviewing the subset of fs code that can be hit in
RCU mode.  Not a pretty sight, that... ;-/  First catch: in
fuse_dentry_revalidate() we have a case (reachable with LOOKUP_RCU) where
we do this:
        } else if (inode) {
                fc = get_fuse_conn(inode);
                if (fc->readdirplus_auto) {
                        parent = dget_parent(entry);
                        fuse_advise_use_readdirplus(parent->d_inode);
                        dput(parent);
                }
        }
First of all, that'll lead to obvious nastiness if we get here when
->s_fs_info has already been freed in process of fs shutdown; fc will
be pointing to kfreed object and no, freeing it isn't RCU-delayed.
That's not a problem with the current tree, of course, but this
dput(parent) very much is - doing that under rcu_read_lock() is
a Bloody Bad Idea(tm).

If my reading of that code is right, the proper fix would be to
turn that else if (inode) into else if (inode && !(flags & LOOKUP_RCU))

Miklos, could you confirm that?  Or would you prefer to deal with that
in some other way?