All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Ming Lei <ming.lei@redhat.com>
Cc: Jens Axboe <axboe@fb.com>,
	linux-block@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Mike Snitzer <snitzer@redhat.com>,
	dm-devel@redhat.com, Bart Van Assche <bart.vanassche@sandisk.com>,
	Laurence Oberman <loberman@redhat.com>,
	Paolo Valente <paolo.valente@linaro.org>,
	Oleksandr Natalenko <oleksandr@natalenko.name>,
	Tom Nguyen <tom81094@gmail.com>,
	linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
	Omar Sandoval <osandov@fb.com>
Subject: Re: [PATCH V5 1/7] blk-mq: issue rq directly in blk_mq_request_bypass_insert()
Date: Tue, 3 Oct 2017 01:58:50 -0700	[thread overview]
Message-ID: <20171003085850.GA21184@infradead.org> (raw)
In-Reply-To: <20170930102720.30219-2-ming.lei@redhat.com>

This patch does two many things at once and needs a split. I also
don't really understand why it's in this series and not your dm-mpath
performance one.

> +static void blk_mq_request_direct_insert(struct blk_mq_hw_ctx *hctx,
> +					 struct request *rq)
> +{
> +	spin_lock(&hctx->lock);
> +	list_add_tail(&rq->queuelist, &hctx->dispatch);
> +	spin_unlock(&hctx->lock);
> +
> +	blk_mq_run_hw_queue(hctx, false);
> +}

Why doesn't this share code with blk_mq_sched_bypass_insert?

>  /*
>   * Should only be used carefully, when the caller knows we want to
>   * bypass a potential IO scheduler on the target device.
>   */
> -void blk_mq_request_bypass_insert(struct request *rq)
> +blk_status_t blk_mq_request_bypass_insert(struct request *rq)
>  {
>  	struct blk_mq_ctx *ctx = rq->mq_ctx;
>  	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu);
> +	blk_qc_t cookie;
> +	blk_status_t ret;
>  
> -	spin_lock(&hctx->lock);
> -	list_add_tail(&rq->queuelist, &hctx->dispatch);
> -	spin_unlock(&hctx->lock);
> -
> -	blk_mq_run_hw_queue(hctx, false);
> +	ret = blk_mq_try_issue_directly(hctx, rq, &cookie, true);
> +	if (ret == BLK_STS_RESOURCE)
> +		blk_mq_request_direct_insert(hctx, rq);
> +	return ret;

If you actually insert the request on BLK_STS_RESOURCE why do you
pass the error on?  In general BLK_STS_RESOURCE indicates a failure
to issue.

> +/*
> + * 'dispatch_only' means we only try to dispatch it out, and
> + * don't deal with dispatch failure if BLK_STS_RESOURCE or
> + * BLK_STS_IOERR happens.
> + */
> +static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> +		struct request *rq, blk_qc_t *cookie, bool may_sleep,
> +		bool dispatch_only)

This dispatch_only argument that completely changes behavior is a
nightmare.  Try to find a way to have a low-level helper that
always behaves as if dispatch_only is set, and then build another
helper that actually issues/completes around it.

  reply	other threads:[~2017-10-03  8:58 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-30 10:27 [PATCH V5 00/14] blk-mq-sched: improve sequential I/O performance(part 1) Ming Lei
2017-09-30 10:27 ` [PATCH V5 1/7] blk-mq: issue rq directly in blk_mq_request_bypass_insert() Ming Lei
2017-10-03  8:58   ` Christoph Hellwig [this message]
2017-10-03 13:39     ` Ming Lei
2017-09-30 10:27 ` [PATCH V5 2/7] blk-mq-sched: fix scheduler bad performance Ming Lei
2017-10-02 14:19   ` Christoph Hellwig
2017-09-30 10:27 ` [PATCH V5 3/7] sbitmap: introduce __sbitmap_for_each_set() Ming Lei
2017-09-30 10:27 ` [PATCH V5 4/7] blk-mq: introduce blk_mq_dequeue_from_ctx() Ming Lei
2017-10-03  9:01   ` Christoph Hellwig
2017-10-09  4:36     ` Ming Lei
2017-09-30 10:27 ` [PATCH V5 5/7] blk-mq-sched: move actual dispatching into one helper Ming Lei
2017-10-02 14:19   ` Christoph Hellwig
2017-10-09  9:07     ` Ming Lei
2017-09-30 10:27 ` [PATCH V5 6/7] blk-mq-sched: improve dispatching from sw queue Ming Lei
2017-10-03  9:05   ` Christoph Hellwig
2017-10-09 10:15     ` Ming Lei
2017-09-30 10:27 ` [PATCH V5 7/7] blk-mq-sched: don't dequeue request until all in ->dispatch are flushed Ming Lei
2017-10-03  9:11   ` Christoph Hellwig
2017-10-09 10:40     ` Ming Lei
2017-09-30 10:32 ` [PATCH V5 00/14] blk-mq-sched: improve sequential I/O performance(part 1) Ming Lei
2017-10-09 12:09 ` John Garry
2017-10-09 12:09   ` John Garry
2017-10-09 15:04   ` Ming Lei
2017-10-10  1:46     ` Ming Lei
2017-10-10 12:24       ` John Garry
2017-10-10 12:24         ` John Garry
2017-10-10 12:34         ` Johannes Thumshirn
2017-10-10 12:34           ` Johannes Thumshirn
2017-10-10 12:37           ` Paolo Valente
2017-10-10 12:37             ` Paolo Valente
2017-10-10 13:45         ` Ming Lei
2017-10-10 15:10           ` John Garry
2017-10-10 15:10             ` John Garry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171003085850.GA21184@infradead.org \
    --to=hch@infradead.org \
    --cc=axboe@fb.com \
    --cc=bart.vanassche@sandisk.com \
    --cc=dm-devel@redhat.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=loberman@redhat.com \
    --cc=ming.lei@redhat.com \
    --cc=oleksandr@natalenko.name \
    --cc=osandov@fb.com \
    --cc=paolo.valente@linaro.org \
    --cc=snitzer@redhat.com \
    --cc=tom81094@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.