Re: [PATCH v1 5/6] blk-mq: fix for trace_block_plug()

[Ming Lei]

On Tue, Oct 20, 2015 at 1:59 AM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Ming Lei <ming.lei@xxxxxxxxxxxxx> writes:
>
>> On Mon, Oct 19, 2015 at 11:38 PM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
>>> Ming Lei <ming.lei@xxxxxxxxxxxxx> writes:
>>>
>>>> The trace point is for tracing plug event of each request
>>>> queue instead of each task, so we should check the request
>>>> count in the plug list from current queue instead of
>>>> current task.
>>>
>>> If blk_queue_nomerges returns true, request_count won't be updated, so
>>> you've essentially broken plugging for those queues.
>>
>> Yes, you are right, and blk_queue_bio() has the same problem too,
>> and looks automatic flush plug can't work for nomerges queue at all.
>
> Hi Ming,
>
> Something like this would fix it.  Feel free to add it into your patch
> set if you're going to do another spin.  Otherwise I'll just send it off
> separately.

Looks fine for me, will add it in V2, thanks!


>
> -Jeff
>
> From: Jeff Moyer <jmoyer@xxxxxxxxxx>
> Subject: block: fix plug list flushing for nomerge queues
>
> Request queues with merging disabled will not flush the plug list after
> BLK_MAX_REQUEST_COUNT requests have been queued, since the code relies
> on blk_attempt_plug_merge to compute the request_count.  Fix this by
> computing the number of queued requests even for nomerge queues.
>
> Signed-off-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2eb722d..f0ae087 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1594,6 +1594,30 @@ out:
>         return ret;
>  }
>
> +unsigned int blk_plug_queued_count(struct request_queue *q)
> +{
> +       struct blk_plug *plug;
> +       struct request *rq;
> +       struct list_head *plug_list;
> +       unsigned int ret = 0;
> +
> +       plug = current->plug;
> +       if (!plug)
> +               goto out;
> +
> +       if (q->mq_ops)
> +               plug_list = &plug->mq_list;
> +       else
> +               plug_list = &plug->list;
> +
> +       list_for_each_entry(rq, plug_list, queuelist) {
> +               if (rq->q == q)
> +                       ret++;
> +       }
> +out:
> +       return ret;
> +}
> +
>  void init_request_from_bio(struct request *req, struct bio *bio)
>  {
>         req->cmd_type = REQ_TYPE_FS;
> @@ -1641,9 +1665,11 @@ static void blk_queue_bio(struct request_queue *q, struct bio *bio)
>          * Check if we can merge with the plugged list before grabbing
>          * any locks.
>          */
> -       if (!blk_queue_nomerges(q) &&
> -           blk_attempt_plug_merge(q, bio, &request_count, NULL))
> -               return;
> +       if (!blk_queue_nomerges(q)) {
> +               if (blk_attempt_plug_merge(q, bio, &request_count, NULL))
> +                       return;
> +       } else
> +               request_count = blk_plug_queued_count(q);
>
>         spin_lock_irq(q->queue_lock);
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 7785ae9..604a1f3 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1267,9 +1267,12 @@ static void blk_mq_make_request(struct request_queue *q, struct bio *bio)
>
>         blk_queue_split(q, &bio, q->bio_split);
>
> -       if (!is_flush_fua && !blk_queue_nomerges(q) &&
> -           blk_attempt_plug_merge(q, bio, &request_count, &same_queue_rq))
> -               return;
> +       if (!is_flush_fua && !blk_queue_nomerges(q)) {
> +               if (blk_attempt_plug_merge(q, bio, &request_count,
> +                                          &same_queue_rq))
> +                       return;
> +       } else
> +               request_count = blk_plug_queued_count(q);
>
>         rq = blk_mq_map_request(q, bio, &data);
>         if (unlikely(!rq))
> diff --git a/block/blk.h b/block/blk.h
> index 98614ad..aa27d02 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -86,6 +86,7 @@ bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
>  bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
>                             unsigned int *request_count,
>                             struct request **same_queue_rq);
> +unsigned int blk_plug_queued_count(struct request_queue *q);
>
>  void blk_account_io_start(struct request *req, bool new_io);
>  void blk_account_io_completion(struct request *req, unsigned int bytes);
>
--
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/