Commit Message
Greg KH
Oct. 19, 2022, 8:30 a.m. UTC
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> --- lib/sbitmap.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
Comments
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. > + */ > if (wait_cnt > 0) > - 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 > > > >
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. + */ if (wait_cnt > 0) - 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)