Re: [PATCH 2/4] blk-mq: merge mq and sq make_request instances

2017-03-20 Thread Bart Van Assche 
On 03/20/2017 04:39 PM, Christoph Hellwig wrote:
> @@ -1534,7 +1529,36 @@ static blk_qc_t blk_mq_make_request(struct 
> request_queue *q, struct bio *bio)
>   }
>  
>   plug = current->plug;
> - if (((plug && !blk_queue_nomerges(q)) || is_sync)) {
> + if (plug && q->nr_hw_queues == 1) {
> + [ ... ]
> + } else if (((plug && !blk_queue_nomerges(q)) || is_sync)) {
>   struct request *old_rq = NULL;
>  
>   blk_mq_bio_to_request(rq, bio);

I think this patch will change the behavior for the plug == NULL &&
q->nr_hw_queues == 1 && is_sync case: with this patch applied the code
under "else if" will be executed for that case but that wasn't the case
before this patch.

Bart.

Re: [PATCH 2/4] blk-mq: merge mq and sq make_request instances

2017-03-13 Thread h...@lst.de 
On Mon, Mar 13, 2017 at 09:01:08PM +, Bart Van Assche wrote:
> > +   /*
> > +* @request_count may become stale because of schedule
> > +* out, so check the list again.
> > +*/
> 
> The above comment was relevant as long as there was a request_count assignment
> above blk_mq_sched_get_request(). This patch moves that assignment inside if
> (plug && q->nr_hw_queues == 1). Does that mean that the above comment should 
> be
> removed entirely?

I don't think so - for the !blk_queue_nomerges cases we still rely
on blk_attempt_plug_merge calculatіng request_count, so the comment
still applies.

Re: [PATCH 2/4] blk-mq: merge mq and sq make_request instances

2017-03-13 Thread Bart Van Assche 
On Mon, 2017-03-13 at 09:48 -0600, Christoph Hellwig wrote:
> @@ -1534,7 +1529,36 @@ static blk_qc_t blk_mq_make_request(struct 
> request_queue *q, struct bio *bio)
>   }
>  
>   plug = current->plug;
> - if (((plug && !blk_queue_nomerges(q)) || is_sync)) {
> + if (plug && q->nr_hw_queues == 1) {
> + struct request *last = NULL;
> +
> + blk_mq_bio_to_request(rq, bio);
> +
> + /*
> +  * @request_count may become stale because of schedule
> +  * out, so check the list again.
> +  */

The above comment was relevant as long as there was a request_count assignment
above blk_mq_sched_get_request(). This patch moves that assignment inside if
(plug && q->nr_hw_queues == 1). Does that mean that the above comment should be
removed entirely?

> + if (list_empty(>mq_list))
> + request_count = 0;
> + else if (blk_queue_nomerges(q))
> + request_count = blk_plug_queued_count(q);
> +
> + if (!request_count)
> + trace_block_plug(q);
> + else
> + last = list_entry_rq(plug->mq_list.prev);
> +
> + blk_mq_put_ctx(data.ctx);
> +
> + if (request_count >= BLK_MAX_REQUEST_COUNT || (last &&
> + blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
> + blk_flush_plug_list(plug, false);
> + trace_block_plug(q);
> + }
> +
> + list_add_tail(>queuelist, >mq_list);
> + goto done;
> + } else if (((plug && !blk_queue_nomerges(q)) || is_sync)) {
>   struct request *old_rq = NULL;
>  
>   blk_mq_bio_to_request(rq, bio);

Bart.