On Mon, Sep 08, 2025 at 02:50:10PM +1000, NeilBrown wrote: > On Mon, 08 Sep 2025, Al Viro wrote: > > That way xfs hits will be down to that claim_stability() and the obscenity in > > trace.h - until the users of the latter get wrapped into something that would > > take snapshots and pass those instead of messing with ->d_name. Considering > > the fun quoted above, not having to repeat that digging is something I'd > > count as a win... > > > > What would you think of providing an accessor function and insisting > everyone use it - and have some sort of lockdep_assert_held() to that > function so that developers who test their code will see these problem? > > Then a simple grep can find any unapproved uses. Really? Consider ->link(). Both arguments *are* stable, but the reasons are not just different - they don't even intersect. Old: known to be a regular file, held locked. The former guarantees that it can't be moved by d_splice_alias(), the latter prevents that being done by vfs_rename(), which also locks the objects being moved. New: has been looked up after its parent had been locked. Note that _after_ is important here - you can't just blindly fetch ->d_parent and check if its inode is locked (not to mention anything else, you'd need to check it being non-NULL, do it under rcu_read_lock(), et sodding cetera - ->d_parent stability rules are not that different from ->d_name ones). And this "everyone use it" is not going to fly - you still have places where it's done simply under ->d_lock. Or ->d_lock on known parent - either would suffice. Besides, there's "which primitive do I use here?" - with the annotation approach it's as simple as "if I have it as stable_dentry, just use stable_dentry_name(), otherwise think hard - it may be tricky". I don't believe that lockdep is an answer here - annotations (and these *are* annotations - no code generation changes at all) give better coverage, and bitrot tends to happen in rarely taken failure exits.
