[PATCH] blk-flush: Queue through IO scheduler when flush not required

[PATCH] blk-flush: Queue through IO scheduler when flush not required

[Jan Kara]

Currently blk_insert_flush() just adds flush request to q->queue_head
when flush is not required. That completely bypasses IO scheduler so
e.g. CFQ can be idling waiting for new request to arrive and will idle
through the whole window unnecessarily. Luckily this only happens in
rare cases as usually checks in generic_make_request_checks() clear
FLUSH and FUA flags early if they are not needed.

When no flushing is actually required, we can easily fix the problem by
properly queueing the request through the IO scheduler. Ideally IO
scheduler should be also made aware of requests queued via
blk_flush_queue_rq(). However inserting flush request through IO
scheduler can have unwanted side-effects since due to flush batching
delaying the flush request in IO scheduler will delay all flush requests
possibly coming from other processes. So we keep adding the request
directly to q->queue_head.

Signed-off-by: Jan Kara <jack@suse.com>
---
 block/blk-flush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 9c423e53324a..c81d56ec308f 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -422,7 +422,7 @@ void blk_insert_flush(struct request *rq)
 		if (q->mq_ops) {
 			blk_mq_insert_request(rq, false, false, true);
 		} else
-			list_add_tail(&rq->queuelist, &q->queue_head);
+			q->elevator->type->ops.elevator_add_req_fn(q, rq);
 		return;
 	}
 
-- 
2.1.4

Re: [PATCH] blk-flush: Queue through IO scheduler when flush not required

[Jeff Moyer]

Jan Kara <jack@suse.com> writes:

> Currently blk_insert_flush() just adds flush request to q->queue_head
> when flush is not required. That completely bypasses IO scheduler so
> e.g. CFQ can be idling waiting for new request to arrive and will idle
> through the whole window unnecessarily. Luckily this only happens in
> rare cases as usually checks in generic_make_request_checks() clear
> FLUSH and FUA flags early if they are not needed.

Right.  I think the only way we'd even enter that 'if' block was if the
drive state changed (from WB cache to WT cache) between
generic_make_request_checks and blk_insert_flush.

> When no flushing is actually required, we can easily fix the problem by
> properly queueing the request through the IO scheduler. Ideally IO
> scheduler should be also made aware of requests queued via
> blk_flush_queue_rq(). However inserting flush request through IO
> scheduler can have unwanted side-effects since due to flush batching
> delaying the flush request in IO scheduler will delay all flush requests
> possibly coming from other processes. So we keep adding the request
> directly to q->queue_head.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> Signed-off-by: Jan Kara <jack@suse.com>
> ---
>  block/blk-flush.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index 9c423e53324a..c81d56ec308f 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -422,7 +422,7 @@ void blk_insert_flush(struct request *rq)
>  		if (q->mq_ops) {
>  			blk_mq_insert_request(rq, false, false, true);
>  		} else
> -			list_add_tail(&rq->queuelist, &q->queue_head);
> +			q->elevator->type->ops.elevator_add_req_fn(q, rq);
>  		return;
>  	}

Re: [PATCH] blk-flush: Queue through IO scheduler when flush not required

[Jens Axboe]

On 11/12/2015 06:25 AM, Jan Kara wrote:
> Currently blk_insert_flush() just adds flush request to q->queue_head
> when flush is not required. That completely bypasses IO scheduler so
> e.g. CFQ can be idling waiting for new request to arrive and will idle
> through the whole window unnecessarily. Luckily this only happens in
> rare cases as usually checks in generic_make_request_checks() clear
> FLUSH and FUA flags early if they are not needed.
>
> When no flushing is actually required, we can easily fix the problem by
> properly queueing the request through the IO scheduler. Ideally IO
> scheduler should be also made aware of requests queued via
> blk_flush_queue_rq(). However inserting flush request through IO
> scheduler can have unwanted side-effects since due to flush batching
> delaying the flush request in IO scheduler will delay all flush requests
> possibly coming from other processes. So we keep adding the request
> directly to q->queue_head.

Applied, thanks Jan.

-- 
Jens Axboe