Re: [PATCH 2/3] Improve fairness when locking the per-superblock s_anon list

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: NeilBrown <neilb@suse.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] Improve fairness when locking the per-superblock s_anon list
Date: Thu, 9 Nov 2017 16:02:21 -0800	[thread overview]
Message-ID: <CA+55aFxW6vt8bt6DeWC6JLcWj+PYaptO2LLH4rqv5QRNMAONPQ@mail.gmail.com> (raw)
In-Reply-To: <20171109205029.GD21978@ZenIV.linux.org.uk>

On Thu, Nov 9, 2017 at 12:50 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That looks like a bloody painful approach, IMO.  I'm not saying I like
> Neil's patch, but I doubt that "let's just walk the entire dcache on
> umount" is a good idea.

There may be other approaches, but how often do you umount a
filesystem that you have NFS-exported? Not often.

So making that slower doesn't really sound horrible.

> I wonder if separating the d_obtain_alias() and d_obtain_root() would be
> a good idea; the former outnumber the latter by many orders of magnitude.
> The tricky part is that we could have a disconnected directory from
> d_obtain_alias() with children already connected to it (and thus normally
> hashed by d_splice_alias()) and fail to connect the whole thing to parent.

Well, the real issue (I think) is that we abuse that hash list for the
s_anon thing. I'd like to get rid of that part.

Now, the reason we do that is because we want to have some way to find
dentries that come from that filesystem, but aren't connected.

So I really would like to get rid of s_anon entirely.

We could easily use _another_ list entirely for holding the
"unconnected dentries". Because we do have such a list: the dentry
sibling list.

So what if we instead of using s_anon, we create a "disconnected
root", and make all the disconnected dentries be children of that
disconnected root instead?

Wouldn't that be nicer? The "reconnect" would be basically a "move
from disconnected parent to right place".

Maybe I'm missing something, but that sounds much more logical than
the current s_anon list.

> That leaves us with an orphaned tree that might stick around past the
> time when we drop all references to dentries in it.  And we want to
> get those hunted down and shot on umount.  Could we
>         * make s_anon hold d_obtain_root ones + orphans from such
> failed reconnects
>         * make final dput() treat hashed IS_ROOT as "don't retain it"
>         * have d_obtain_alias() put into normal hash, leaving the
> "move to s_anon" part to reconnect failures.
>         * keep umount side of things unchanged.

I guess that would work too, but I'm not seeing why s_anon is so wonderful.

If we want to make those entries look hashed, let's just hash them,
and use a special parent for it (that "disconnected root"). Why
wouldn't that work?

That would make the umount side _simpler_, because the disconnected
root would be handled exactly like we currently handle the real root.

So we'd just do that same "do_one_tree()" on the disconnected root,
the same way we do on the real one.

That sounds _so_ straightforward that I'm probably missing something
important. And if the "move from disconnected state" is very common,
then maybe the disconnected root lock ends up being a horrible
choke-point instead, simply because we'd take that parent lock all the
time for the add/move operations..

So maybe it's a bad idea. But it sounds clean.

             Linus