Re: [PATCH 09/15] fold the call of retain_dentry() into fast_dput()

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Nov 01, 2023 at 07:30:34AM -1000, Linus Torvalds wrote:
> On Tue, 31 Oct 2023 at 22:45, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Wed, Nov 01, 2023 at 06:20:58AM +0000, Al Viro wrote:
> > > Calls of retain_dentry() happen immediately after getting false
> > > from fast_dput() and getting true from retain_dentry() is
> > > treated the same way as non-zero refcount would be treated by
> > > fast_dput() - unlock dentry and bugger off.
> > >
> > > Doing that in fast_dput() itself is simpler.
> >
> > FWIW, I wonder if it would be better to reorganize it a bit -
> 
> Hmm. Yes. Except I don't love how the retaining logic is then duplicated.

Er...  That change would be an equivalent transformation - the same duplication
is there right now...

> Could we perhaps at least try to share the dentry flag tests between
> the "real" retain_dentry() code and the lockless version?

Umm...  There are 3 groups:

DONTCACHE, DISCONNECTED - hard false
!LRU_LIST, !REFERENCED - not an obstacle to true, but need to take locks
OP_DELETE - can't tell without asking filesystem, which would need ->d_lock.

gcc-12 on x86 turns the series of ifs into
        movl    %edi, %eax
	andl    $32832, %eax
	cmpl    $32832, %eax
	jne     .L17
	andl    $168, %edi
	jne     .L17
instead of combining that into
        andl    $33000, %edi
	cmpl    $32832, %edi
	jne     .L17

OTOH, that's not much of pessimization...  Up to you.


> > Another thing: would you mind
> >
> > #if USE_CMPXCHG_LOCKREF
> > extern int lockref_put_return(struct lockref *);
> > #else
> > static inline int lockref_put_return(struct lockref *l)
> > {
> >         return -1;
> > }
> > #endif
> >
> > in include/linux/lockref.h?  Would be useful on DEBUG_SPINLOCK configs...
> 
> The above sounds like a good idea, not only for better code generation
> for the debug case, but because it would have possibly made the erofs
> misuse more obvious to people.

To make it even more obvious, perhaps rename it as well?  I.e.

/*
 * unlike the rest of these primitives, the one below does *not* contain
 * a fallback; caller needs to take care of handling that.
 */
#if USE_CMPXCHG_LOCKREF
extern int __lockref_put_return(struct lockref *);
#else
static inline int __lockref_put_return(struct lockref *l)
{
	return -1;
}
#endif