Re: [patch,v2] block: fix flush machinery for stacking drivers with differring flush flags

[Jeff Moyer <jmoyer@redhat.com>]

Tejun Heo <tj@kernel.org> 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