On Tue, Apr 02 2019, 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@xxxxxxxxxx> 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. 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. > > 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... I would find it useful if the documentation said something about why this API exists at all. As you say, it cannot change the dentry - so what is it expected to do. I had a look at the two in-tree users and my guess is that it can be useful if the filesystem caches some other information which would be invalidated by a dentry being removed. I *think* cephfs has a flag which records if "All entries in a directory are currently in the dcache". When a dentry is pruned, that flag needs to be cleared. i.e. ->d_prune allows a filesystem to maintain summary state about what is currently in the dcache. ?? Thanks, NeilBrown > > [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).
Attachment:
signature.asc
Description: PGP signature
