vn_lock_pair(): allow to request shared locking
ClosedPublic
Actions

Authored by kib on Apr 6 2023, 4:13 AM.

Details

Reviewers

markj
mjg
rmacklem

Commits

rGbb24eaea4982: vn_lock_pair(): allow to request shared locking

Summary

If either of vnodes is shared locked, lock must not be recursed.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Apr 6 2023, 4:13 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 6 2023, 4:13 AM

kib requested review of this revision.Apr 6 2023, 4:13 AM

just a note this is going to need a __freebsd_version bump due to zfs

sys/kern/vfs_vnops.c
3792	does this matter? will not this transparently upgrade to exclusive internally? point is while lockmgr is the standard thing today, it adds knowledge about it when it perhaps can be avoided

LGTM. mjg@ can decide if he needs this for his
patch against zfs_copy_file_range().

If/when it goes into main, I'll take a look at changing
nfs_copy_file_range() to use it as well.

sys/kern/vfs_vnops.c
3792	I think this is needed here for the check on lkflags1 done at line#3791 just below.

This revision is now accepted and ready to land.Apr 6 2023, 3:23 PM

markj added inline comments.Apr 6 2023, 3:30 PM

sys/kern/vfs_vnops.c
3803	How do you know that the vnode isn't locked by another thread?

kib marked 3 inline comments as done.Apr 6 2023, 6:17 PM

kib added inline comments.

sys/kern/vfs_vnops.c
3792	Yes the code needs to differentiate between vnode states for real, to properly unlock. If this causes a negative feeling, we can add a predicate function testing for LK_NOSHARE, to hide the implementation.
3803	This is wrong, of course. Hopefully updated version corrects the block.

Do not check sharers to detect recursion. Just handle the case where the current lock type does not match the request (i.e. LK_SHARED lock, but need LK_EXCLUSIVE).

This revision now requires review to proceed.Apr 6 2023, 6:18 PM

markj added inline comments.Apr 7 2023, 1:17 PM

sys/kern/vfs_vnops.c
3798	I still don't understand how this can work. ASSERT_VOP_UNLOCKED() just looks at the lock state, it does not consult witness.
3816
3817
3833	Can the three lines above be replaced with while (!vp1_locked \|\| !vp2_locked) ?

kib marked 4 inline comments as done.Apr 7 2023, 2:05 PM

kib added inline comments.

sys/kern/vfs_vnops.c
3798	Exactly what are you asking about? VOP_UNLOCK() unlock the lock once, if it is recursed, the lock is still owned. ASSERT_VOP_UNLOCKED() is false-positive-safe, in fact it only checks that the lock is not exclusively owned by the current thread.