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

[Tejun Heo]

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?

> @@ -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.

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.

Thank you.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/