Re: [PATCH 01/10] VFS: support parallel updates in the one directory.

From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: NeilBrown <neilb@suse.de>, Daire Byrne <daire@dneg.com>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Chuck Lever <chuck.lever@oracle.com>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/10] VFS: support parallel updates in the one directory.
Date: Sat, 27 Aug 2022 22:14:06 +0100	[thread overview]
Message-ID: <YwqJHnomJ+4xCy1n@ZenIV> (raw)
In-Reply-To: <CAHk-=whz69y=98udgGB5ujH6bapYuapwfHS2esWaFrKEoi9-Ow@mail.gmail.com>

On Fri, Aug 26, 2022 at 05:13:38PM -0700, Linus Torvalds wrote:
> On Fri, Aug 26, 2022 at 4:07 PM NeilBrown <neilb@suse.de> wrote:
> >
> > As you note, by the end of the series "create" is not more different
> > from "rename" than it already is.  I only broke up the patches to make
> > review more manageable.
> 
> Yes, I understand. But I'm saying that maybe a filesystem actually
> might want to treat them differently.
> 
> That said, the really nasty part was that 'wq' thing that meant that
> different paths had different directory locking not because of
> low-level filesystem issues, but because of caller issues.
> 
> So that's the one I _really_ disliked, and that I don't think should
> exist even as a partial first step.
> 
> The "tie every operation together with one flag" I can live with, in
> case it turns out that yes, that one flag is all anybody ever really
> wants.

FWIW, what's really missing is the set of rules describing what the
methods can expect from their arguments.

Things like "oh, we can safely use ->d_parent here - we know that
foo_rmdir(dir, child) is called only with dir held exclusive and
child that had been observed to be a child of dentry alias of
dir after dir had been locked, while all places that might change
child->d_parent will be doing that only with child->d_parent->d_inode
held at least shared" rely upon the current locking scheme.

Change that 'held exclusive' to 'held shared' and we need something
different, presumably 'this new bitlock on the child is held by the caller'.
That's nice, but...  What's to guarantee that we won't be hit by
__d_unalias()?  It won't care about the bitlock on existing alias,
would it?  And it only holds the old parent shared, so...

My comments had been along the lines of "doing that would make the
series easier to reason about"; I don't hate the approach, but
	* in the current form it's hard to read; there might be
problems I hadn't even noticed yet
	* it's much easier to verify that stated assertions are
guaranteed by the callers and sufficient for safety of callees
if they *ARE* stated.  Spelling them out is on the patch series
authors, and IME doing that helps a lot when writing a series
like that.  At least on the level of internal notes...  Especially
since NFS is... special (or, as they say in New York, "sophisticated" -
sorry).  There's a plenty of things that are true for it, but do
not hold for filesystems in general.  And without an explicitly
spelled out warranties it's very easy to end up with a mess that
would be hell to apply to other filesystems.  I really don't want
to see an explosion of cargo-culted logics that might or might
not remain valid for NFS by the time it gets copied around...