| Message ID | 20191217160024.GA23066@redhat (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sbitmap: only queue kyber's wait callback if not already active | expand |
On 12/17/19 9:00 AM, David Jeffery wrote: > Under heavy loads where the kyber I/O scheduler hits the token limits for > its scheduling domains, kyber can become stuck. When active requests > complete, kyber may not be woken up leaving the I/O requests in kyber > stuck. > > This stuck state is due to a race condition with kyber and the sbitmap > functions it uses to run a callback when enough requests have completed. > The running of a sbt_wait callback can race with the attempt to insert the > sbt_wait. Since sbitmap_del_wait_queue removes the sbt_wait from the list > first then sets the sbq field to NULL, kyber can see the item as not on a > list but the call to sbitmap_add_wait_queue will see sbq as non-NULL. This > results in the sbt_wait being inserted onto the wait list but ws_active > doesn't get incremented. So the sbitmap queue does not know there is a > waiter on a wait list. > > Since sbitmap doesn't think there is a waiter, kyber may never be > informed that there are domain tokens available and the I/O never advances. > With the sbt_wait on a wait list, kyber believes it has an active waiter > so cannot insert a new waiter when reaching the domain's full state. > > This race can be fixed by only adding the sbt_wait to the queue if the > sbq field is NULL. If sbq is not NULL, there is already an action active > which will trigger the re-running of kyber. Let it run and add the > sbt_wait to the wait list if still needing to wait. > > Signed-off-by: David Jeffery <djeffery@redhat.com> > Reported-by: John Pittman <jpittman@redhat.com> > Tested-by: John Pittman <jpittman@redhat.com> > --- > > This bug was reliably being triggered on several test systems. With the > fix, the tests no longer fail. > > sbitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/sbitmap.c b/lib/sbitmap.c > index 33feec8989f1..af88d1346dd7 100644 > --- a/lib/sbitmap.c > +++ b/lib/sbitmap.c > @@ -650,8 +650,8 @@ void sbitmap_add_wait_queue(struct sbitmap_queue *sbq, > if (!sbq_wait->sbq) { > sbq_wait->sbq = sbq; > atomic_inc(&sbq->ws_active); > + add_wait_queue(&ws->wait, &sbq_wait->wait); > } > - add_wait_queue(&ws->wait, &sbq_wait->wait); > } > EXPORT_SYMBOL_GPL(sbitmap_add_wait_queue); This looks good to me, waiting for Omar to take a look (CC'ed).
On Tue, Dec 17, 2019 at 11:00:24AM -0500, David Jeffery wrote: > Under heavy loads where the kyber I/O scheduler hits the token limits for > its scheduling domains, kyber can become stuck. When active requests > complete, kyber may not be woken up leaving the I/O requests in kyber > stuck. > > This stuck state is due to a race condition with kyber and the sbitmap > functions it uses to run a callback when enough requests have completed. > The running of a sbt_wait callback can race with the attempt to insert the > sbt_wait. Since sbitmap_del_wait_queue removes the sbt_wait from the list > first then sets the sbq field to NULL, kyber can see the item as not on a > list but the call to sbitmap_add_wait_queue will see sbq as non-NULL. This > results in the sbt_wait being inserted onto the wait list but ws_active > doesn't get incremented. So the sbitmap queue does not know there is a > waiter on a wait list. > > Since sbitmap doesn't think there is a waiter, kyber may never be > informed that there are domain tokens available and the I/O never advances. > With the sbt_wait on a wait list, kyber believes it has an active waiter > so cannot insert a new waiter when reaching the domain's full state. > > This race can be fixed by only adding the sbt_wait to the queue if the > sbq field is NULL. If sbq is not NULL, there is already an action active > which will trigger the re-running of kyber. Let it run and add the > sbt_wait to the wait list if still needing to wait. So the race here is: Thread 1 Thread 2 kyber_domain_wake sbitmap_del_wait_queue list_del_init atomic_dec sbq->ws_active kyber_get_domain_token list_empty_careful sbitmap_add_wait_queue if (!sqb_wait->sb) // false add_wait_queue sbq_wait->sbq = NULL Now sbq_wait->sbq == NULL, sbq->ws_active = 0, and !list_empty(domain_wait), so sbq_wake_ptr returns NULL and sbitmap_queue_wake_up does nothing. I get the feeling that sbitmap_{add,del}_wait_queue need some memory barriers... But ignoring that, this fix seems right. Reviewed-by: Omar Sandoval <osandov@fb.com> P.S. s/sbt_wait/sbq_wait/g in the commit message.
diff --git a/lib/sbitmap.c b/lib/sbitmap.c index 33feec8989f1..af88d1346dd7 100644 --- a/lib/sbitmap.c +++ b/lib/sbitmap.c @@ -650,8 +650,8 @@ void sbitmap_add_wait_queue(struct sbitmap_queue *sbq, if (!sbq_wait->sbq) { sbq_wait->sbq = sbq; atomic_inc(&sbq->ws_active); + add_wait_queue(&ws->wait, &sbq_wait->wait); } - add_wait_queue(&ws->wait, &sbq_wait->wait); } EXPORT_SYMBOL_GPL(sbitmap_add_wait_queue);