Re: [PATCH 1/2]block: optimize non-queueable flush request drive

[Tejun Heo <htejun@gmail.com>]

Hello, Shaohua.

> +	list_splice_init(&q->flush_queue[q->flush_running_idx], &proceed_list);
> +	/*
> +	 * If queue doesn't support queueable flush request, we can push the
> +	 * pending requests to the next stage too. For such queue, there are no
> +	 * normal requests running when flush request is running, so this still
> +	 * guarantees the correctness.
> +	 */
> +	if (!blk_queue_flush_queueable(q))
> +		list_splice_tail_init(&q->flush_queue[q->flush_pending_idx],
> +			&proceed_list);

I can't see how this is safe.  Request completion is decoupled from
issue.  What prevents low level driver from take in other requests
before control hits here?  And even if that holds for the current
implementation, that's hardly something which can be guaranteed from
!flush_queueable.  Am I missing something?

This kind of micro optimization is gonna bring very painful bugs which
are extremely difficult to reproduce and track down.  It scares the
hell out of me.  It's gonna silently skip flushes where it shouldn't.

If you wanna optimize this case, a much better way would be
implementing back-to-back flush optimization properly such that when
block layer detects two flushes back-to-back and _KNOWS_ that no
request has been issued inbetween, the second one is handled as noop.
Mark the queue clean on flush, dirty on any other request and if the
queue is clean all flushes can be completed immediately on issue which
would also allow us to avoid the whole queue at the front or back
issue without bothering low level drivers at all.

Thanks.

-- 
tejun