Re: [PATCH v3 00/24] Convert vfs.txt to vfs.rst

From: "Tobin C. Harding" <me@tobin.cc>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jonathan Corbet <corbet@lwn.net>,
	"Tobin C. Harding" <tobin@kernel.org>,
	Mauro Carvalho Chehab <mchehab@s-opensource.com>,
	Neil Brown <neilb@suse.com>, Randy Dunlap <rdunlap@infradead.org>,
	linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 00/24] Convert vfs.txt to vfs.rst
Date: Wed, 3 Apr 2019 06:25:20 +1100	[thread overview]
Message-ID: <20190402192520.GA6285@eros.localdomain> (raw)
In-Reply-To: <20190402164824.GK2217@ZenIV.linux.org.uk>

On Tue, Apr 02, 2019 at 05:48:24PM +0100, Al Viro wrote:
> On Tue, Apr 02, 2019 at 09:49:34AM -0600, Jonathan Corbet wrote:
> > On Wed, 27 Mar 2019 16:16:53 +1100
> > "Tobin C. Harding" <tobin@kernel.org> wrote:
> > 
> > > Hi Al,
> > > 
> > > This series converts the VFS file Documentation/filesystems/vfs.txt to
> > > reStructuredText format.  Please consider taking this series through
> > > your tree as apposed to Jon's tree because this set makes a fair amount
> > > of changes to VFS files (and also the VFS tree and docs tree are out of
> > > sync right now with the recent work by Mauro and Neil).
> > 
> > Al, do you have any thoughts on how you want to handle this?  I was about
> > to apply Jeff Layton's vfs.txt update, but would rather not create
> > conflicts unnecessarily.  Let me know if you'd like me to pick this work
> > up.
> 
> Frankly, I would rather see that file be eventually replaced by something
> saner, and I'm not talking about the format.

Are you able to extrapolate on this comment please?  Is this something
someone new to the VFS (me) can do with a little nudge in the right
direction or is this something that needs thorough knowledge of the VFS? 

> Re Jeff's patch... 
> 
> +  d_prune: called prior to pruning (i.e. unhashing and killing) a hashed
> +       dentry from the dcache.
> 
> is flat-out misguiding.  First of all, it *is* called for unhashed dentries,
> TYVM.  Furthermore, "prior to" is far too vague.

This patch includes documentation for d_prune() very similar to Jeff's
patch (taken from a comment somewhere in filesystems code IIRC).  I can
have a go at working all the comments below into better documentation if
that is useful (dependant on answer to question above).

thanks,
Tobin.

(leaving text below for reference)

> What really happens: there's a point in state diagram for dentries where
> we commit to destroying a dentry and start taking it apart.  That transition
> happens with ->d_lock of dentry, ->i_lock of its inode (if any) and
> ->d_lock of the parent (again, if any) held; ->d_prune() is the last
> chance for filesystem to see the (now doomed) dentry still intact.
> 
> It doesn't matter whether it's hashed or not, etc.  The locks held
> are sufficient to stabilize pretty much everything[1] in dentry and
> nothing is destroyed yet.  The only apparent exception is ->d_count,
> but that's not real - we are guaranteed that there had been no other
> counted references to dentry at the decision point and that none
> could've been added.  So this "oh, it's not 0 now, it's gone negative
> after lockref_mark_dead() the caller has just done" is a red herring.
> 
> ->d_prune() must not drop/regain any of the locks held by caller.
> It must _not_ free anything attached to dentry - that belongs
> later in the shutdown sequence.  If anything, I'm tempted to
> make it take const struct dentry * as argument, just to make
> that clear.
> 
> No new (counted) references can be acquired by that point;
> lockless dcache lookup might find our dentry a match, but
> result of such lookup is not going to be legitimized - it's
> doomed to be thrown out as stale.
> 
> It really makes more sense as part of struct dentry lifecycle
> description...  
> 
> [1] in theory, ->d_time might be changed by overlapping lockless
> call of ->d_revalidate().  Up to filesystem - VFS doesn't touch
> that field (and AFAICS only NFS uses it these days).