On Thu, Nov 09, 2023 at 06:20:43AM +0000, Al Viro wrote: > If refcount is less than 1, we should just warn, unlock dentry and > return true, so that the caller doesn't try to do anything else. That's effectively to guard against bugs in filesystems, not in dcache itself, right? Have we observed this frequently? > > Taking care of that leaves the rest of "lockref_put_return() has > failed" case equivalent to "decrement refcount and rejoin the > normal slow path after the point where we grab ->d_lock". > > NOTE: lockref_put_return() is strictly a fastpath thing - unlike > the rest of lockref primitives, it does not contain a fallback. > Caller (and it looks like fast_dput() is the only legitimate one > in the entire kernel) has to do that itself. Reasons for > lockref_put_return() failures: > * ->d_lock held by somebody > * refcount <= 0 > * ... or an architecture not supporting lockref use of > cmpxchg - sparc, anything non-SMP, config with spinlock debugging... > > We could add a fallback, but it would be a clumsy API - we'd have > to distinguish between: > (1) refcount > 1 - decremented, lock not held on return > (2) refcount < 1 - left alone, probably no sense to hold the lock > (3) refcount is 1, no cmphxcg - decremented, lock held on return > (4) refcount is 1, cmphxcg supported - decremented, lock *NOT* held > on return. > We want to return with no lock held in case (4); that's the whole point of that > thing. We very much do not want to have the fallback in case (3) return without > a lock, since the caller might have to retake it in that case. > So it wouldn't be more convenient than doing the fallback in the caller and > it would be very easy to screw up, especially since the test coverage would > suck - no way to test (3) and (4) on the same kernel build. > > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > --- Looks like a good idea, Reviewed-by: Christian Brauner <brauner@xxxxxxxxxx>
