On Wed, 19 Oct 2022, Greg Kroah-Hartman wrote:
From: Jan Kara jack@suse.cz
[ Upstream commit 48c033314f372478548203c583529f53080fd078 ]
When __sbq_wake_up() decrements wait_cnt to 0 but races with someone else waking the waiter on the waitqueue (so the waitqueue becomes empty), it exits without reseting wait_cnt to wake_batch number. Once wait_cnt is 0, nobody will ever reset the wait_cnt or wake the new waiters resulting in possible deadlocks or busyloops. Fix the problem by making sure we reset wait_cnt even if we didn't wake up anybody in the end.
Fixes: 040b83fcecfb ("sbitmap: fix possible io hung due to lost wakeup") Reported-by: Keith Busch kbusch@kernel.org Signed-off-by: Jan Kara jack@suse.cz Link: https://lore.kernel.org/r/20220908130937.2795-1-jack@suse.cz Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Sasha Levin sashal@kernel.org
I have no authority on linux-block, but I'll say NAK to this one (and 479/862), and let Jens and Jan overrule me if they disagree.
This was another of several 6.1-rc1 commits which had given me lost wakeups never suffered before; was not tagged Cc stable; and (unless I've missed it on lore) never had AUTOSEL posted to linux-block or linux-kernel.
Hugh
lib/sbitmap.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/lib/sbitmap.c b/lib/sbitmap.c index 1f31147872e6..bb1970ad4875 100644 --- a/lib/sbitmap.c +++ b/lib/sbitmap.c @@ -605,6 +605,7 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq) struct sbq_wait_state *ws; unsigned int wake_batch; int wait_cnt;
- bool ret;
ws = sbq_wake_ptr(sbq); if (!ws) @@ -615,12 +616,23 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq) * For concurrent callers of this, callers should call this function * again to wakeup a new batch on a different 'ws'. */
- if (wait_cnt < 0 || !waitqueue_active(&ws->wait))
- if (wait_cnt < 0) return true;
- /*
* If we decremented queue without waiters, retry to avoid lost* wakeups.*/
return false;
return !waitqueue_active(&ws->wait);
- /*
* When wait_cnt == 0, we have to be particularly careful as we are* responsible to reset wait_cnt regardless whether we've actually* woken up anybody. But in case we didn't wakeup anybody, we still* need to retry.*/- ret = !waitqueue_active(&ws->wait); wake_batch = READ_ONCE(sbq->wake_batch);
/* @@ -649,7 +661,7 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq) sbq_index_atomic_inc(&sbq->wake_index); atomic_set(&ws->wait_cnt, wake_batch);
- return false;
- return ret;
} void sbitmap_queue_wake_up(struct sbitmap_queue *sbq) -- 2.35.1