On Wed, Nov 01, 2023 at 07:30:34AM -1000, Linus Torvalds wrote:
> On Tue, 31 Oct 2023 at 22:45, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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
