Fix LK_TRYUPGRADE

[asomers]

Summary

It was broken by r363393

Test Plan

fusefs test suite

[cem]

sys/kern/kern_lock.c

987

The cute spelling would be v > LK_SHARERS_LOCK(1), which may have been the intent.

[cem]

sys/kern/kern_lock.c

987

(Fail the tryupgrade if: (1) there are 2+ sharers, or (2) if any flag except LK_SHARE is set (i.e., there are waiters).)

[mjg]

Huh. The patch looks good but I'm afraid it will need some testing as it may uncover bugs in tryupgrade users.

I'm going to ask pho to give it a spin along with some other changes, probably tomorrow.

[asomers]

The effect of the bug is that the upgrade always fails. And all consumers are required to deal with both failure and success of a lock upgrade. So we might not see any obvious effects. Performance could be affect, though. But I'm not failure enough with the consuming code to know what kind of benchmark might be expected to show a difference.

[mjg]

So a complete fix would be something in the lines of:

diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c
index 98c6cafde702..091abcda2a1e 100644
--- a/sys/kern/kern_lock.c
+++ b/sys/kern/kern_lock.c
@@ -984,17 +984,19 @@ lockmgr_upgrade(struct lock *lk, u_int flags, struct lock_object *ilk,
        op = flags & LK_TYPE_MASK;
        v = lockmgr_read_value(lk);
        for (;;) {
-               if (LK_SHARERS_LOCK(v) > 1) {
+               if (LK_SHARERS(v) > 1) {
                        if (op == LK_TRYUPGRADE) {
                                LOCK_LOG2(lk, "%s: %p failed the nowait upgrade",
                                    __func__, lk);
                                error = EBUSY;
                                goto out;
                        }
-                       if (lockmgr_sunlock_try(lk, &v)) {
+                       if (atomic_fcmpset_rel_ptr(&lk->lk_lock, &v,
+                           v - LK_ONE_SHARER)) {
                                lockmgr_note_shared_release(lk, file, line);
                                goto out_xlock;
                        }
+                       continue;
                }
                MPASS((v & ~LK_ALL_WAITERS) == LK_SHARERS_LOCK(1));

There is a clear win single-threaded where this avoids one atomic op. For multithreaded, so happens the vfs layer performs a lock upgrade towards the end of path lookup if necessary. I'm going to test the impact later.

[asomers]

Sounds good. Are you going to commit, @mjg ?

[mjg]

Your pick I guess.

Regardless of who lands this in, as noted earlier I'm first going to ask pho to test this (along with some other stuff). Total time is about 2-3 days.

Nice find. I can't recall now how the original commit ended up in the current form.

[mjg]

Sorry for the delay. I'll be sending this (with some other stuff) to pho shortly.

[mjg]

Take over as my patch went in per above agreement.