Re: Block: Fix handling of stopped queues and a plugging issue

[Jens Axboe]

On Thu, Oct 02 2008, Elias Oltmanns wrote:
> Make sure that ->request_fn() really isn't called on queues that are
> supposed to be stopped. While at it, don't remove the plug in
> __blk_run_queue() unconditionally since this might lead to system hangs.

What system hangs, can you detail it?

> 
> Signed-off-by: Elias Oltmanns <eo@xxxxxxxxxxxxxx>
> ---
> Applies to your devel tree.
> 
>  block/blk-core.c |    6 +++---
>  block/elevator.c |   12 +++++++-----
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2d053b5..ecc5443 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -404,14 +404,14 @@ EXPORT_SYMBOL(blk_sync_queue);
>   */
>  void __blk_run_queue(struct request_queue *q)
>  {
> -	blk_remove_plug(q);
> -
>  	/*
>  	 * Only recurse once to avoid overrunning the stack, let the unplug
>  	 * handling reinvoke the handler shortly if we already got there.
>  	 */
> -	if (!elv_queue_empty(q))
> +	if (!elv_queue_empty(q) && likely(!blk_queue_stopped(q))) {
> +		blk_remove_plug(q);
>  		blk_invoke_request_fn(q);
> +	}

Doing

        if (foo && likely(bar))

doesn't make a lot of sense, you need to move the entire block or just
don't do it. Just don't add it :-)

>  }
>  EXPORT_SYMBOL(__blk_run_queue);
>  
> diff --git a/block/elevator.c b/block/elevator.c
> index 0451892..43a4257 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -611,8 +611,10 @@ void elv_insert(struct request_queue *q, struct request *rq, int where)
>  		 *   with anything.  There's no point in delaying queue
>  		 *   processing.
>  		 */
> -		blk_remove_plug(q);
> -		q->request_fn(q);
> +		if (likely(!blk_queue_stopped(q))) {
> +			blk_remove_plug(q);
> +			q->request_fn(q);
> +		}
>  		break;
>  
>  	case ELEVATOR_INSERT_SORT:
> @@ -950,7 +952,8 @@ void elv_completed_request(struct request_queue *q, struct request *rq)
>  		    blk_ordered_cur_seq(q) == QUEUE_ORDSEQ_DRAIN &&
>  		    blk_ordered_req_seq(first_rq) > QUEUE_ORDSEQ_DRAIN) {
>  			blk_ordered_complete_seq(q, QUEUE_ORDSEQ_DRAIN, 0);
> -			q->request_fn(q);
> +			if (likely(!blk_queue_stopped(q)))
> +				q->request_fn(q);
>  		}
>  	}
>  }

I wonder if most of these should not just be blk_start_queueing(), would
clean things up.

> @@ -1109,8 +1112,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
>  	elv_drain_elevator(q);
>  
>  	while (q->rq.elvpriv) {
> -		blk_remove_plug(q);
> -		q->request_fn(q);
> +		blk_start_queueing(q);
>  		spin_unlock_irq(q->queue_lock);
>  		msleep(10);
>  		spin_lock_irq(q->queue_lock);

Like so.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/