Tejun Heo <tj@xxxxxxxxxx> writes:
> Hello,
>
> On Fri, Aug 12, 2011 at 03:07:51PM -0400, Jeff Moyer wrote:
>> Changes from v1->v2:
>> - Moved the detection of empty flush requests into blk_insert_flush.
>> - Got rid of REQ_FLUSH_SEQ in the CLONE_FLAGS.
>
> Heh yeah, this looks pretty good to me. :)
>
>> @@ -312,6 +309,19 @@ void blk_insert_flush(struct request *rq)
>> rq->cmd_flags &= ~REQ_FUA;
>>
>> /*
>> + * An empty flush handed down from a stacking driver may
>> + * translate into nothing if the underlying device does not
>> + * advertise a write-back cache. In this case, simply
>> + * complete the request.
>> + */
>> + if (!policy && !blk_rq_bytes(rq)) {
>> + __blk_end_bidi_request(rq, 0, 0, 0);
>> + return;
>> + }
>
> Hmmm... doesn't !policy imply !blk_rq_bytes() with your change just
> merged to Jens' tree?
Yes, I'll fix that up.
>> @@ -319,6 +329,7 @@ void blk_insert_flush(struct request *rq)
>> if ((policy & REQ_FSEQ_DATA) &&
>> !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>> list_add_tail(&rq->queuelist, &q->queue_head);
>> + blk_run_queue_async(q);
>> return;
>> }
>
> In the other message, you said,
>
>> Well, the only time we need to run the queue is when the request has
>> data, has REQ_FUA set, and the underlying queue's flush flags contain
>> only REQ_FUA. In code:
>>
>> if (rq->cmd_flags & REQ_FUA && q->flush_flags == REQ_FUA)
>> blk_run_queue_async(q);
>
> But this can't happen because a queue can't have REQ_FUA without
> REQ_FLUSH (it doesn't make any sense). blk_queue_flush() will trigger
> WARN_ON_ONCE() and turn off REQ_FUA in such cases.
I confused REQ_FSEQ_DATA with REQ_FUA. The bottom line is that the
logic in blk_insert_flush would have to be duplicated in
blk_insert_cloned_request in order to move the queue kick up the call
stack. I don't think that's a clean way to do things.
> That said, it's kinda unclear who should be responsible for kicking
> the queue. __elv_add_request() does it for some but not all.
> __make_request() always activates the queue which sometimes ends up
> doing it again after __elv_add_request(). I think kicking the queue
> after short circuit insert probably is the right thing to do.
OK. I'll post a follow-up with that one fix above. Hopefully that will
do it. ;-)
Cheers,
Jeff
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
