| Message ID | 1542185131-15029-3-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: refactor and fix on issue request directly | expand |
On Wed, Nov 14, 2018 at 04:45:29PM +0800, Jianchao Wang wrote: > When try to issue request directly, if the queue is stopped or > quiesced, 'bypass' will be ignored and return BLK_STS_OK to caller > to avoid it dispatch request again. Then the request will be > inserted with blk_mq_sched_insert_request. This is not correct > for dm-rq case where we should avoid to pass through the underlying > path's io scheduler. > > To fix it, use blk_mq_request_bypass_insert to insert the request > to hctx->dispatch when we cannot pass through io scheduler but have > to insert. Not sure if the current behaviour is wrong, or worth of a fix. Bypassing io scheduler for dm-rq is only for sake of performance because there has been io scheduler for dm device already, and we just don't want to schedule these requests twice. Given it can be thought as error handling, there shouldn't be big difference to put request in scheduler queue or ->dispatch list when queue is quiesced or stopped. What matters is that all these requests can be dispatched again after queue switches back. thanks, Ming
Hi Ming On 11/14/18 5:20 PM, Ming Lei wrote: > On Wed, Nov 14, 2018 at 04:45:29PM +0800, Jianchao Wang wrote: >> When try to issue request directly, if the queue is stopped or >> quiesced, 'bypass' will be ignored and return BLK_STS_OK to caller >> to avoid it dispatch request again. Then the request will be >> inserted with blk_mq_sched_insert_request. This is not correct >> for dm-rq case where we should avoid to pass through the underlying >> path's io scheduler. >> >> To fix it, use blk_mq_request_bypass_insert to insert the request >> to hctx->dispatch when we cannot pass through io scheduler but have >> to insert. > > Not sure if the current behaviour is wrong, or worth of a fix. > > Bypassing io scheduler for dm-rq is only for sake of performance > because there has been io scheduler for dm device already, and we > just don't want to schedule these requests twice. As comment of commit 157f377beb710e84bd8bc7a3c4475c0674ebebd7 (block: directly insert blk-mq request from blk_insert_cloned_request()) All said, a request-based DM multipath device's IO scheduler should be the only one used -- when the original requests are issued to the underlying paths as cloned requests they are inserted directly in the underlying dispatch queue(s) rather than through an additional elevator. But commit bd166ef18 ("blk-mq-sched: add framework for MQ capable IO schedulers") switched blk_insert_cloned_request() from using blk_mq_insert_request() to blk_mq_sched_insert_request(). Which incorrectly added elevator machinery into a call chain that isn't supposed to have any. It sounds like a wrong action. Thanks Jianchao > > thanks, > Ming >
On Wed, Nov 14, 2018 at 05:29:54PM +0800, jianchao.wang wrote: > Hi Ming > > On 11/14/18 5:20 PM, Ming Lei wrote: > > On Wed, Nov 14, 2018 at 04:45:29PM +0800, Jianchao Wang wrote: > >> When try to issue request directly, if the queue is stopped or > >> quiesced, 'bypass' will be ignored and return BLK_STS_OK to caller > >> to avoid it dispatch request again. Then the request will be > >> inserted with blk_mq_sched_insert_request. This is not correct > >> for dm-rq case where we should avoid to pass through the underlying > >> path's io scheduler. > >> > >> To fix it, use blk_mq_request_bypass_insert to insert the request > >> to hctx->dispatch when we cannot pass through io scheduler but have > >> to insert. > > > > Not sure if the current behaviour is wrong, or worth of a fix. > > > > Bypassing io scheduler for dm-rq is only for sake of performance > > because there has been io scheduler for dm device already, and we > > just don't want to schedule these requests twice. > > As comment of commit 157f377beb710e84bd8bc7a3c4475c0674ebebd7 > (block: directly insert blk-mq request from blk_insert_cloned_request()) > > All said, a request-based DM multipath device's IO scheduler should be > the only one used -- when the original requests are issued to the > underlying paths as cloned requests they are inserted directly in the > underlying dispatch queue(s) rather than through an additional elevator. > > But commit bd166ef18 ("blk-mq-sched: add framework for MQ capable IO > schedulers") switched blk_insert_cloned_request() from using > blk_mq_insert_request() to blk_mq_sched_insert_request(). Which > incorrectly added elevator machinery into a call chain that isn't > supposed to have any. > > It sounds like a wrong action. As I mentioned, it is only for the sake of performance, and IO scheduler has to be supported on these devices too, for example, one partition may be under dm-rq, and another partition can be accessed directly. However, you are fixing the handling when queue is quiesced or stopped. Under this situation, it is fine to put requests into scheduler queue, given no performance need to be worried. Thanks, Ming
On 11/14/18 5:35 PM, Ming Lei wrote: > On Wed, Nov 14, 2018 at 05:29:54PM +0800, jianchao.wang wrote: >> Hi Ming >> >> On 11/14/18 5:20 PM, Ming Lei wrote: >>> On Wed, Nov 14, 2018 at 04:45:29PM +0800, Jianchao Wang wrote: >>>> When try to issue request directly, if the queue is stopped or >>>> quiesced, 'bypass' will be ignored and return BLK_STS_OK to caller >>>> to avoid it dispatch request again. Then the request will be >>>> inserted with blk_mq_sched_insert_request. This is not correct >>>> for dm-rq case where we should avoid to pass through the underlying >>>> path's io scheduler. >>>> >>>> To fix it, use blk_mq_request_bypass_insert to insert the request >>>> to hctx->dispatch when we cannot pass through io scheduler but have >>>> to insert. >>> >>> Not sure if the current behaviour is wrong, or worth of a fix. >>> >>> Bypassing io scheduler for dm-rq is only for sake of performance >>> because there has been io scheduler for dm device already, and we >>> just don't want to schedule these requests twice. >> >> As comment of commit 157f377beb710e84bd8bc7a3c4475c0674ebebd7 >> (block: directly insert blk-mq request from blk_insert_cloned_request()) >> >> All said, a request-based DM multipath device's IO scheduler should be >> the only one used -- when the original requests are issued to the >> underlying paths as cloned requests they are inserted directly in the >> underlying dispatch queue(s) rather than through an additional elevator. >> >> But commit bd166ef18 ("blk-mq-sched: add framework for MQ capable IO >> schedulers") switched blk_insert_cloned_request() from using >> blk_mq_insert_request() to blk_mq_sched_insert_request(). Which >> incorrectly added elevator machinery into a call chain that isn't >> supposed to have any. >> >> It sounds like a wrong action. > > As I mentioned, it is only for the sake of performance, and IO scheduler > has to be supported on these devices too, for example, one partition may > be under dm-rq, and another partition can be accessed directly. > > However, you are fixing the handling when queue is quiesced or stopped. > Under this situation, it is fine to put requests into scheduler queue, > given no performance need to be worried. > OK, I drop this one. Thanks Jianchao
diff --git a/block/blk-mq.c b/block/blk-mq.c index 14b4d06..11c52bb 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1772,7 +1772,7 @@ static blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, bool bypass) { struct request_queue *q = rq->q; - bool run_queue = true; + bool run_queue = true, force = false; blk_status_t ret = BLK_STS_RESOURCE; int srcu_idx; @@ -1786,7 +1786,7 @@ static blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, */ if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))) { run_queue = false; - bypass = false; + force = true; goto out_unlock; } @@ -1817,6 +1817,9 @@ static blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, if (!bypass) { blk_mq_sched_insert_request(rq, false, run_queue, false); ret = BLK_STS_OK; + } else if (force) { + blk_mq_request_bypass_insert(rq, run_queue); + ret = BLK_STS_OK; } break; default:
When try to issue request directly, if the queue is stopped or quiesced, 'bypass' will be ignored and return BLK_STS_OK to caller to avoid it dispatch request again. Then the request will be inserted with blk_mq_sched_insert_request. This is not correct for dm-rq case where we should avoid to pass through the underlying path's io scheduler. To fix it, use blk_mq_request_bypass_insert to insert the request to hctx->dispatch when we cannot pass through io scheduler but have to insert. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- block/blk-mq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)