Re: [patch] blk-flush: fix flush policy calculation

[Tejun Heo <tj@kernel.org>]

Hello,

On Mon, Aug 01, 2011 at 04:32:17PM -0400, Jeff Moyer wrote:
>         /*
>          * If there's data but flush is not necessary, the request can be
>          * processed directly without going through flush machinery.  Queue
>          * for normal execution.
>          */
>         if ((policy & REQ_FSEQ_DATA) &&
>             !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>                 list_add_tail(&rq->queuelist, &q->queue_head);
>                 return;
>         }
> 
> Given the code as it stands, I don't think the body of this if statement
> will ever be executed.

It does if the device has a WB cache and filesystem sends down a FUA
(but no FLUSH) request.

> I've attached a fix for this below.  It seems
> like this could be both a performance and a correctness issue, though
> I've not run into any problems I can directly attribute to this (perhaps
> due to file systems not issuing flushes when support is not advertised?).
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Hmmm... yes, this can become a correctness issue if (and only if)
blk_queue_flush() is called to change q->flush_flags while requests
are in-flight; otherwise, requests wouldn't reach the function at all.
Also, I think it would be a generally good idea to always set
FSEQ_DATA if the request has data.

 Acked-by: Tejun Heo <tj@kernel.org>

but can you please update the description?

Thank you.

-- 
tejun