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

From: Al Viro <viro@zeniv.linux.org.uk>
To: NeilBrown <neilb@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	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, 3 Sep 2022 03:12:26 +0100	[thread overview]
Message-ID: <YxK4CiVNaQ6egobJ@ZenIV> (raw)
In-Reply-To: <166216924401.28768.5809376269835339554@noble.neil.brown.name>

On Sat, Sep 03, 2022 at 11:40:44AM +1000, NeilBrown wrote:

> I don't think that is a good idea.  Once you call d_lookup_done()
> (without having first called d_add() or similar) the dentry becomes
> invisible to normal path lookup, so another might be created.  But the
> dentry will still be used for the 'create' or 'rename' and may then be
> added to the dcache - at which point you could have two dentries with the
> same name.
> 
> When ->lookup() returns success without d_add()ing the dentry, that
> means that something else will complete the d_add() if/when necessary.
> For NFS, it specifically means that the lookup is effectively being
> combined with the following CREATE or RENAME.  In this case there is no
> d_lookup_done() until the full operation is complete.
>
> For autofs (thanks for pointing me to that) the operation is completed
> when d_automount() signals the daemon to create the directory or
> symlink.  In that case there IS a d_lookup_done() call and autofs needs
> some extra magic (the internal 'active' list) to make sure subsequent
> ->lookup requests can see that dentry which is still in the process of
> being set up.
> 
> It might be nice if the dentry passed to autofs_lookup() could remain
> "d_inlookup()" until after d_automount has completed.  Then autofs
> wouldn't need that active list.  However I haven't yet looked at how
> disruptive such a change might be.

Very much so.  You are starting to invent new rules for ->lookup() that
just never had been there, basing on nothing better than a couple of
examples.  They are nowhere near everything there is.

And you can't rely upon d_add() done by a method, for very obvious
reasons.  They are out of your control, they might very well decide
that object creation has failed and drop the damn thing.  Which is
not allowed for in-lookup dentries without d_lookup_done().

Neil, *IF* you are introducing new rules like that, the absolutely minimal
requirement is having them in Documentation/filesystems/porting.rst.
And that includes "such-and-such method might be called with parent
locked only shared; in that case it's guaranteed such-and-such things
about its arguments (bitlocks held, etc.)".

One thing we really need to avoid is that thing coming undocumented, with
"NFS copes, nobody else has it enabled, whoever does it for other
filesystems will just have to RTFS".  I hope it's obvious that this
is not an option.  Because I can bloody guarantee that it will be
cargo-culted over to other filesystems, with nobody (you and me included)
understanding the resulting code.