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?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
