Page MenuHomeFreeBSD

Fix LK_TRYUPGRADE
ClosedPublic

Authored by asomers on Jan 4 2021, 4:05 AM.

Details

Summary

It was broken by r363393

Test Plan

fusefs test suite

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/kern/kern_lock.c
987

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

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).)

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.

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.

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.

Sounds good. Are you going to commit, @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.

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

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2021, 12:22 PM
Closed by commit R10:38baca17e01e: lockmgr: fix upgrade (authored by mjg). · Explain Why
This revision was automatically updated to reflect the committed changes.

Take over as my patch went in per above agreement.