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

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

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.

> +		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().

Can we remove 'rqw->wait.head.next != &wait->entry' from may_queue()?
I guess that should be one optimization, but seems quite dangerous since
'rqw->wait.head.next' may point to one freed stack variable.


Thanks,
Ming