On Fri, Aug 26, 2022 at 05:13:38PM -0700, Linus Torvalds wrote: > On Fri, Aug 26, 2022 at 4:07 PM NeilBrown <neilb@xxxxxxx> 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...
