Re: [PATCH] blk-wbt: fix indefinite background writeback sleep

[Ming Lei <ming.lei@redhat.com>]

On Fri, Jun 22, 2018 at 05:13:31PM -0600, Jens Axboe wrote:
> On 6/22/18 5:11 PM, Ming Lei wrote:
> > On Fri, Jun 22, 2018 at 04:51:26PM -0600, Jens Axboe wrote:
> >> On 6/22/18 4:43 PM, Ming Lei wrote:
> >>> On Fri, Jun 22, 2018 at 01:26:10PM -0600, Jens Axboe wrote:
> >>>> blk-wbt adds waiters to the tail of the waitqueue, and factors in the
> >>>> task placement in its decision making on whether or not the current task
> >>>> can proceed. This can cause issues for the lowest class of writers,
> >>>> since they can get woken up, denied access, and then put back to sleep
> >>>> at the end of the waitqueue.
> >>>>
> >>>> Fix this so that we only utilize the tail add for the initial sleep, and
> >>>> we don't factor in the wait queue placement after we've slept (and are
> >>>> now using the head addition).
> >>>>
> >>>> Fixes: e34cbd307477 ("blk-wbt: add general throttling mechanism")
> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>>>
> >>>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> >>>> index 4f89b28fa652..7beeabd05f4a 100644
> >>>> --- a/block/blk-wbt.c
> >>>> +++ b/block/blk-wbt.c
> >>>> @@ -550,7 +550,7 @@ static inline bool may_queue(struct rq_wb *rwb, struct rq_wait *rqw,
> >>>>  	 * If the waitqueue is already active and we are not the next
> >>>>  	 * in line to be woken up, wait for our turn.
> >>>>  	 */
> >>>> -	if (waitqueue_active(&rqw->wait) &&
> >>>> +	if (wait && waitqueue_active(&rqw->wait) &&
> >>>>  	    rqw->wait.head.next != &wait->entry)
> >>>>  		return false;
> >>>>  
> >>>> @@ -567,16 +567,27 @@ static void __wbt_wait(struct rq_wb *rwb, enum wbt_flags wb_acct,
> >>>>  	__acquires(lock)
> >>>>  {
> >>>>  	struct rq_wait *rqw = get_rq_wait(rwb, wb_acct);
> >>>> +	struct wait_queue_entry *waitptr = NULL;
> >>>>  	DEFINE_WAIT(wait);
> >>>>  
> >>>> -	if (may_queue(rwb, rqw, &wait, rw))
> >>>> +	if (may_queue(rwb, rqw, waitptr, rw))
> >>>>  		return;
> >>>>  
> >>>> +	waitptr = &wait;
> >>>>  	do {
> >>>> -		prepare_to_wait_exclusive(&rqw->wait, &wait,
> >>>> +		/*
> >>>> +		 * Don't add ourselves to the wq tail if we've already
> >>>> +		 * slept. Otherwise we can penalize background writes
> >>>> +		 * indefinitely.
> >>>> +		 */
> >>>
> >>> I saw this indefinite wbt_wait() in systemd-journal when running
> >>> aio-stress read test, but just once, not figured out one approach
> >>> to reproduce it yet, just wondering if you have quick test case for
> >>> reproducing and verifying this issue.
> >>
> >> I've seen it in production, but I'm currently relying on someone else
> >> to reproduce it synthetically. I'm just providing the patches for
> >> testing.
> >>
> >>>> +		if (waitptr)
> >>>> +			prepare_to_wait_exclusive(&rqw->wait, &wait,
> >>>> +							TASK_UNINTERRUPTIBLE);
> >>>> +		else
> >>>> +			prepare_to_wait(&rqw->wait, &wait,
> >>>>  						TASK_UNINTERRUPTIBLE);
> >>>
> >>> Could you explain a bit why the 'wait_entry' order matters wrt. this
> >>> issue? Since other 'wait_entry' still may come at the head meantime to
> >>> the same wq before checking in may_queue().
> >>
> >> Let's say we have 10 tasks queued up. Each one gets added to the tail,
> >> so when it's our turn, we've now reached the head. We fail to get a
> >> queue token, so we go back to sleep. At that point we should add
> >> back to the head, not the tail, for fairness purposes.
> > 
> > OK, it is reasonable to do it for fairness purpose, but seems we still
> > don't know how the wait forever in wbt_wait() is fixed by this way.
> 
> It's not, it's just removing a class of unfairness. You should not go
> to the back of the queue if you fail, you should remain near the top.
> 
> > I guess the reason is in the check of 'rqw->wait.head.next != &wait->entry',
> > which is basically removed when the waiter is waken up.
> 
> Plus at that point it's useless, since we are the head of queue. The
> check only makes sense if we tail add.

Seems not safe to run the check in case of tail add too:

- just during or after checking 'rqw->wait.head.next != &wait->entry', all
  inflight requests in this wq are done

- may_queue() still returns false because 'rqw->wait.head.next != &wait->entry'
  returns true, then __wbt_wait() may wait forever.

Thanks,
Ming