lkml.org 
[lkml]   [2022]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 6.0 517/862] sbitmap: Avoid leaving waitqueue in invalid state in __sbq_wake_up()
    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
    >
    >
    >
    >

    \
     
     \ /
      Last update: 2022-10-19 17:18    [W:4.012 / U:0.660 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site