Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Jens Axboe]

On 09/14/2017 10:33 AM, Ming Lei wrote:
> On Fri, Sep 15, 2017 at 12:30 AM, Jens Axboe  wrote:
>> On 09/14/2017 09:57 AM, Ming Lei wrote:
>>> On Tue, Sep 12, 2017 at 5:27 AM, Jens Axboe  wrote:
 On 09/11/2017 03:13 PM, Mike Snitzer wrote:
> On Mon, Sep 11 2017 at  4:51pm -0400,
> Jens Axboe  wrote:
>
>> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
>>> Here is v2 that should obviate the need to rename blk_mq_insert_request
>>> (by using bools to control run_queue and async).
>>>
>>> As for inserting directly into dispatch, if that can be done that is
>>> great but I'd prefer to have that be a follow-up optimization.  This
>>> fixes the regression in question, and does so in well-known terms.
>>>
>>> What do you think?
>>
>> I think it looks reasonable. My only concern is the use of the software
>> queues. Depending on the scheduler, they may or may not be used. I'd
>> need to review the code, but my first thought is that this would break
>> if you use blk_mq_insert_request() on a device that is managed by
>> mq-deadline or bfq, for instance. Schedulers are free to use the
>> software queues, but they are also free to ignore them and use internal
>> queuing.
>>
>> Looking at the code, looks like this was changed slightly at some point,
>> we always flush the software queues, if any of them contain requests. So
>> it's probably fine.
>
> OK good, but is that too brittle to rely on? Something that might change
> in the future?

 I'm actually surprised we do flush software queues for that case, since
 we don't always have to. So it is a bit of a wart. If we don't have a
 scheduler, software queues is where IO goes. If we have a scheduler, the
 scheduler has complete control of where to queue IO. Generally, the
 scheduler will either utilize the software queues or it won't, there's
 nothing in between.

 I know realize I'm an idiot and didn't read it right. So here's the code
 in question:

 const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;

 [...]

 } else if (!has_sched_dispatch) {
 blk_mq_flush_busy_ctxs(hctx, _list);
 blk_mq_dispatch_rq_list(q, _list);
 }

 so we do only enter sw queue flushing, if we don't have a scheduler with
 a dispatch_request hook. So now I am really wondering how your patch
 could work if the bottom device has bfq or mq-deadline attached?

>> My earlier suggestion to use just hctx->dispatch for the IO and bypass
>> the software queues completely. The use case for the dispatch list is
>> the same, regardless of whether the device has a scheduler attached or
>> not.
>
> I'm missing how these details relate to the goal of bypassing any
> scheduler that might be attached.  Are you saying the attached elevator
> would still get in the way?

 See above.

> Looking at blk_mq_sched_insert_request(), submission when an elevator
> isn't attached is exactly what I made blk_mq_insert_request() do
> (which is exactly what it did in the past).

 Right, but that path is only used if we don't have a scheduler attached.
 So while the code will use that path IFF a scheduler isn't attached to
 that device, your use case will use it for both cases.

> In the case of DM multipath, nothing else should be submitting IO to
> the device so elevator shouldn't be used -- only interface for
> submitting IO would be blk_mq_insert_request().  So even if a
> scheduler is attached it should be bypassed right?

 The problem is the usage of the sw queue.

 Does the below work for you?


 diff --git a/block/blk-core.c b/block/blk-core.c
 index d709c0e3a2ac..aebe676225e6 100644
 --- a/block/blk-core.c
 +++ b/block/blk-core.c
 @@ -2342,7 +2342,12 @@ blk_status_t blk_insert_cloned_request(struct 
 request_queue *q, struct request *
 if (q->mq_ops) {
 if (blk_queue_io_stat(q))
 blk_account_io_start(rq, true);
 -   blk_mq_sched_insert_request(rq, false, true, false, false);
 +   /*
 +* Since we have a scheduler attached on the top device,
 +* bypass a potential scheduler on the bottom device for
 +* insert.
 +*/
 +   blk_mq_request_bypass_insert(rq);
 return BLK_STS_OK;
 }

 diff --git a/block/blk-mq.c b/block/blk-mq.c
 index 3f18cff80050..98a18609755e 100644
 --- a/block/blk-mq.c
 +++ b/block/blk-mq.c
 @@ -1401,6 +1401,22 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx 
 *hctx, struct request *rq,
 blk_mq_hctx_mark_pending(hctx, ctx);

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Ming Lei]

On Fri, Sep 15, 2017 at 12:30 AM, Jens Axboe  wrote:
> On 09/14/2017 09:57 AM, Ming Lei wrote:
>> On Tue, Sep 12, 2017 at 5:27 AM, Jens Axboe  wrote:
>>> On 09/11/2017 03:13 PM, Mike Snitzer wrote:
 On Mon, Sep 11 2017 at  4:51pm -0400,
 Jens Axboe  wrote:

> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
>> Here is v2 that should obviate the need to rename blk_mq_insert_request
>> (by using bools to control run_queue and async).
>>
>> As for inserting directly into dispatch, if that can be done that is
>> great but I'd prefer to have that be a follow-up optimization.  This
>> fixes the regression in question, and does so in well-known terms.
>>
>> What do you think?
>
> I think it looks reasonable. My only concern is the use of the software
> queues. Depending on the scheduler, they may or may not be used. I'd
> need to review the code, but my first thought is that this would break
> if you use blk_mq_insert_request() on a device that is managed by
> mq-deadline or bfq, for instance. Schedulers are free to use the
> software queues, but they are also free to ignore them and use internal
> queuing.
>
> Looking at the code, looks like this was changed slightly at some point,
> we always flush the software queues, if any of them contain requests. So
> it's probably fine.

 OK good, but is that too brittle to rely on? Something that might change
 in the future?
>>>
>>> I'm actually surprised we do flush software queues for that case, since
>>> we don't always have to. So it is a bit of a wart. If we don't have a
>>> scheduler, software queues is where IO goes. If we have a scheduler, the
>>> scheduler has complete control of where to queue IO. Generally, the
>>> scheduler will either utilize the software queues or it won't, there's
>>> nothing in between.
>>>
>>> I know realize I'm an idiot and didn't read it right. So here's the code
>>> in question:
>>>
>>> const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;
>>>
>>> [...]
>>>
>>> } else if (!has_sched_dispatch) {
>>> blk_mq_flush_busy_ctxs(hctx, _list);
>>> blk_mq_dispatch_rq_list(q, _list);
>>> }
>>>
>>> so we do only enter sw queue flushing, if we don't have a scheduler with
>>> a dispatch_request hook. So now I am really wondering how your patch
>>> could work if the bottom device has bfq or mq-deadline attached?
>>>
> My earlier suggestion to use just hctx->dispatch for the IO and bypass
> the software queues completely. The use case for the dispatch list is
> the same, regardless of whether the device has a scheduler attached or
> not.

 I'm missing how these details relate to the goal of bypassing any
 scheduler that might be attached.  Are you saying the attached elevator
 would still get in the way?
>>>
>>> See above.
>>>
 Looking at blk_mq_sched_insert_request(), submission when an elevator
 isn't attached is exactly what I made blk_mq_insert_request() do
 (which is exactly what it did in the past).
>>>
>>> Right, but that path is only used if we don't have a scheduler attached.
>>> So while the code will use that path IFF a scheduler isn't attached to
>>> that device, your use case will use it for both cases.
>>>
 In the case of DM multipath, nothing else should be submitting IO to
 the device so elevator shouldn't be used -- only interface for
 submitting IO would be blk_mq_insert_request().  So even if a
 scheduler is attached it should be bypassed right?
>>>
>>> The problem is the usage of the sw queue.
>>>
>>> Does the below work for you?
>>>
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index d709c0e3a2ac..aebe676225e6 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -2342,7 +2342,12 @@ blk_status_t blk_insert_cloned_request(struct 
>>> request_queue *q, struct request *
>>> if (q->mq_ops) {
>>> if (blk_queue_io_stat(q))
>>> blk_account_io_start(rq, true);
>>> -   blk_mq_sched_insert_request(rq, false, true, false, false);
>>> +   /*
>>> +* Since we have a scheduler attached on the top device,
>>> +* bypass a potential scheduler on the bottom device for
>>> +* insert.
>>> +*/
>>> +   blk_mq_request_bypass_insert(rq);
>>> return BLK_STS_OK;
>>> }
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 3f18cff80050..98a18609755e 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -1401,6 +1401,22 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx 
>>> *hctx, struct request *rq,
>>> blk_mq_hctx_mark_pending(hctx, ctx);
>>>  }
>>>
>>> +/*
>>> + * Should only be used carefully, when the caller knows we want to
>>> + * bypass a potential IO scheduler on the target 

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Jens Axboe]

On 09/14/2017 09:57 AM, Ming Lei wrote:
> On Tue, Sep 12, 2017 at 5:27 AM, Jens Axboe  wrote:
>> On 09/11/2017 03:13 PM, Mike Snitzer wrote:
>>> On Mon, Sep 11 2017 at  4:51pm -0400,
>>> Jens Axboe  wrote:
>>>
 On 09/11/2017 10:16 AM, Mike Snitzer wrote:
> Here is v2 that should obviate the need to rename blk_mq_insert_request
> (by using bools to control run_queue and async).
>
> As for inserting directly into dispatch, if that can be done that is
> great but I'd prefer to have that be a follow-up optimization.  This
> fixes the regression in question, and does so in well-known terms.
>
> What do you think?

 I think it looks reasonable. My only concern is the use of the software
 queues. Depending on the scheduler, they may or may not be used. I'd
 need to review the code, but my first thought is that this would break
 if you use blk_mq_insert_request() on a device that is managed by
 mq-deadline or bfq, for instance. Schedulers are free to use the
 software queues, but they are also free to ignore them and use internal
 queuing.

 Looking at the code, looks like this was changed slightly at some point,
 we always flush the software queues, if any of them contain requests. So
 it's probably fine.
>>>
>>> OK good, but is that too brittle to rely on? Something that might change
>>> in the future?
>>
>> I'm actually surprised we do flush software queues for that case, since
>> we don't always have to. So it is a bit of a wart. If we don't have a
>> scheduler, software queues is where IO goes. If we have a scheduler, the
>> scheduler has complete control of where to queue IO. Generally, the
>> scheduler will either utilize the software queues or it won't, there's
>> nothing in between.
>>
>> I know realize I'm an idiot and didn't read it right. So here's the code
>> in question:
>>
>> const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;
>>
>> [...]
>>
>> } else if (!has_sched_dispatch) {
>> blk_mq_flush_busy_ctxs(hctx, _list);
>> blk_mq_dispatch_rq_list(q, _list);
>> }
>>
>> so we do only enter sw queue flushing, if we don't have a scheduler with
>> a dispatch_request hook. So now I am really wondering how your patch
>> could work if the bottom device has bfq or mq-deadline attached?
>>
 My earlier suggestion to use just hctx->dispatch for the IO and bypass
 the software queues completely. The use case for the dispatch list is
 the same, regardless of whether the device has a scheduler attached or
 not.
>>>
>>> I'm missing how these details relate to the goal of bypassing any
>>> scheduler that might be attached.  Are you saying the attached elevator
>>> would still get in the way?
>>
>> See above.
>>
>>> Looking at blk_mq_sched_insert_request(), submission when an elevator
>>> isn't attached is exactly what I made blk_mq_insert_request() do
>>> (which is exactly what it did in the past).
>>
>> Right, but that path is only used if we don't have a scheduler attached.
>> So while the code will use that path IFF a scheduler isn't attached to
>> that device, your use case will use it for both cases.
>>
>>> In the case of DM multipath, nothing else should be submitting IO to
>>> the device so elevator shouldn't be used -- only interface for
>>> submitting IO would be blk_mq_insert_request().  So even if a
>>> scheduler is attached it should be bypassed right?
>>
>> The problem is the usage of the sw queue.
>>
>> Does the below work for you?
>>
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index d709c0e3a2ac..aebe676225e6 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2342,7 +2342,12 @@ blk_status_t blk_insert_cloned_request(struct 
>> request_queue *q, struct request *
>> if (q->mq_ops) {
>> if (blk_queue_io_stat(q))
>> blk_account_io_start(rq, true);
>> -   blk_mq_sched_insert_request(rq, false, true, false, false);
>> +   /*
>> +* Since we have a scheduler attached on the top device,
>> +* bypass a potential scheduler on the bottom device for
>> +* insert.
>> +*/
>> +   blk_mq_request_bypass_insert(rq);
>> return BLK_STS_OK;
>> }
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 3f18cff80050..98a18609755e 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1401,6 +1401,22 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx 
>> *hctx, struct request *rq,
>> blk_mq_hctx_mark_pending(hctx, ctx);
>>  }
>>
>> +/*
>> + * 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)
>> +{
>> +   struct blk_mq_ctx *ctx = rq->mq_ctx;
>> +   struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, 

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Ming Lei]

On Tue, Sep 12, 2017 at 5:27 AM, Jens Axboe  wrote:
> On 09/11/2017 03:13 PM, Mike Snitzer wrote:
>> On Mon, Sep 11 2017 at  4:51pm -0400,
>> Jens Axboe  wrote:
>>
>>> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
 Here is v2 that should obviate the need to rename blk_mq_insert_request
 (by using bools to control run_queue and async).

 As for inserting directly into dispatch, if that can be done that is
 great but I'd prefer to have that be a follow-up optimization.  This
 fixes the regression in question, and does so in well-known terms.

 What do you think?
>>>
>>> I think it looks reasonable. My only concern is the use of the software
>>> queues. Depending on the scheduler, they may or may not be used. I'd
>>> need to review the code, but my first thought is that this would break
>>> if you use blk_mq_insert_request() on a device that is managed by
>>> mq-deadline or bfq, for instance. Schedulers are free to use the
>>> software queues, but they are also free to ignore them and use internal
>>> queuing.
>>>
>>> Looking at the code, looks like this was changed slightly at some point,
>>> we always flush the software queues, if any of them contain requests. So
>>> it's probably fine.
>>
>> OK good, but is that too brittle to rely on? Something that might change
>> in the future?
>
> I'm actually surprised we do flush software queues for that case, since
> we don't always have to. So it is a bit of a wart. If we don't have a
> scheduler, software queues is where IO goes. If we have a scheduler, the
> scheduler has complete control of where to queue IO. Generally, the
> scheduler will either utilize the software queues or it won't, there's
> nothing in between.
>
> I know realize I'm an idiot and didn't read it right. So here's the code
> in question:
>
> const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;
>
> [...]
>
> } else if (!has_sched_dispatch) {
> blk_mq_flush_busy_ctxs(hctx, _list);
> blk_mq_dispatch_rq_list(q, _list);
> }
>
> so we do only enter sw queue flushing, if we don't have a scheduler with
> a dispatch_request hook. So now I am really wondering how your patch
> could work if the bottom device has bfq or mq-deadline attached?
>
>>> My earlier suggestion to use just hctx->dispatch for the IO and bypass
>>> the software queues completely. The use case for the dispatch list is
>>> the same, regardless of whether the device has a scheduler attached or
>>> not.
>>
>> I'm missing how these details relate to the goal of bypassing any
>> scheduler that might be attached.  Are you saying the attached elevator
>> would still get in the way?
>
> See above.
>
>> Looking at blk_mq_sched_insert_request(), submission when an elevator
>> isn't attached is exactly what I made blk_mq_insert_request() do
>> (which is exactly what it did in the past).
>
> Right, but that path is only used if we don't have a scheduler attached.
> So while the code will use that path IFF a scheduler isn't attached to
> that device, your use case will use it for both cases.
>
>> In the case of DM multipath, nothing else should be submitting IO to
>> the device so elevator shouldn't be used -- only interface for
>> submitting IO would be blk_mq_insert_request().  So even if a
>> scheduler is attached it should be bypassed right?
>
> The problem is the usage of the sw queue.
>
> Does the below work for you?
>
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d709c0e3a2ac..aebe676225e6 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2342,7 +2342,12 @@ blk_status_t blk_insert_cloned_request(struct 
> request_queue *q, struct request *
> if (q->mq_ops) {
> if (blk_queue_io_stat(q))
> blk_account_io_start(rq, true);
> -   blk_mq_sched_insert_request(rq, false, true, false, false);
> +   /*
> +* Since we have a scheduler attached on the top device,
> +* bypass a potential scheduler on the bottom device for
> +* insert.
> +*/
> +   blk_mq_request_bypass_insert(rq);
> return BLK_STS_OK;
> }
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3f18cff80050..98a18609755e 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1401,6 +1401,22 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx 
> *hctx, struct request *rq,
> blk_mq_hctx_mark_pending(hctx, ctx);
>  }
>
> +/*
> + * 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)
> +{
> +   struct blk_mq_ctx *ctx = rq->mq_ctx;
> +   struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu);
> +
> +   spin_lock(>lock);
> +   list_add_tail(>queuelist, >dispatch);
> +   spin_unlock(>lock);
> +
> +   

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Jens Axboe]

On 09/11/2017 04:30 PM, Mike Snitzer wrote:
> On Mon, Sep 11 2017 at  5:51pm -0400,
> Mike Snitzer  wrote:
> 
>> On Mon, Sep 11 2017 at  5:27pm -0400,
>> Jens Axboe  wrote:
>>>
>>> Does the below work for you?
>>
>> I _will_ test your patch and let you know!
> 
> Tested with bfq on underlying paths and both none and bfq on upper-level
> DM multipath request_queue.
> 
> Works perfectly, feel free to add my:
> 
> Tested-by: Mike Snitzer 
> 
> Thanks again!

Great, thanks for testing! I'll commit this, stealing your change
log almost verbatim.

-- 
Jens Axboe

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Mike Snitzer]

On Mon, Sep 11 2017 at  5:51pm -0400,
Mike Snitzer  wrote:

> On Mon, Sep 11 2017 at  5:27pm -0400,
> Jens Axboe  wrote:
> > 
> > Does the below work for you?
> 
> I _will_ test your patch and let you know!

Tested with bfq on underlying paths and both none and bfq on upper-level
DM multipath request_queue.

Works perfectly, feel free to add my:

Tested-by: Mike Snitzer 

Thanks again!

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Mike Snitzer]

On Mon, Sep 11 2017 at  5:27pm -0400,
Jens Axboe  wrote:

> On 09/11/2017 03:13 PM, Mike Snitzer wrote:
> > On Mon, Sep 11 2017 at  4:51pm -0400,
> > Jens Axboe  wrote:
> > 
> >> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
> >>> Here is v2 that should obviate the need to rename blk_mq_insert_request
> >>> (by using bools to control run_queue and async).
> >>>
> >>> As for inserting directly into dispatch, if that can be done that is
> >>> great but I'd prefer to have that be a follow-up optimization.  This
> >>> fixes the regression in question, and does so in well-known terms.
> >>>
> >>> What do you think?
> >>
> >> I think it looks reasonable. My only concern is the use of the software
> >> queues. Depending on the scheduler, they may or may not be used. I'd
> >> need to review the code, but my first thought is that this would break
> >> if you use blk_mq_insert_request() on a device that is managed by
> >> mq-deadline or bfq, for instance. Schedulers are free to use the
> >> software queues, but they are also free to ignore them and use internal
> >> queuing.
> >>
> >> Looking at the code, looks like this was changed slightly at some point,
> >> we always flush the software queues, if any of them contain requests. So
> >> it's probably fine.
> > 
> > OK good, but is that too brittle to rely on? Something that might change
> > in the future?
> 
> I'm actually surprised we do flush software queues for that case, since
> we don't always have to. So it is a bit of a wart. If we don't have a
> scheduler, software queues is where IO goes. If we have a scheduler, the
> scheduler has complete control of where to queue IO. Generally, the
> scheduler will either utilize the software queues or it won't, there's
> nothing in between.
> 
> I know realize I'm an idiot and didn't read it right. So here's the code
> in question:
> 
> const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;  
> 
> [...]
> 
> } else if (!has_sched_dispatch) {   
> blk_mq_flush_busy_ctxs(hctx, _list); 
> blk_mq_dispatch_rq_list(q, _list);   
> }
> 
> so we do only enter sw queue flushing, if we don't have a scheduler with
> a dispatch_request hook. So now I am really wondering how your patch
> could work if the bottom device has bfq or mq-deadline attached?

I didn't test it.. I was an even bigger idiot and assumed blk-mq core
wouldn't alter its IO processing based on scheduler or no.  Nevermind
that I tagged my patch for stable@ without testing.. /me knows better

> >> My earlier suggestion to use just hctx->dispatch for the IO and bypass
> >> the software queues completely. The use case for the dispatch list is
> >> the same, regardless of whether the device has a scheduler attached or
> >> not.
> > 
> > I'm missing how these details relate to the goal of bypassing any
> > scheduler that might be attached.  Are you saying the attached elevator
> > would still get in the way?
> 
> See above.

Yeap, got it.

> > Looking at blk_mq_sched_insert_request(), submission when an elevator
> > isn't attached is exactly what I made blk_mq_insert_request() do
> > (which is exactly what it did in the past).
> 
> Right, but that path is only used if we don't have a scheduler attached.
> So while the code will use that path IFF a scheduler isn't attached to
> that device, your use case will use it for both cases.
> 
> > In the case of DM multipath, nothing else should be submitting IO to
> > the device so elevator shouldn't be used -- only interface for
> > submitting IO would be blk_mq_insert_request().  So even if a
> > scheduler is attached it should be bypassed right?
> 
> The problem is the usage of the sw queue.
> 
> Does the below work for you?

I _will_ test your patch and let you know!

Thanks, much appreciated.
Mike

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Jens Axboe]

On 09/11/2017 03:13 PM, Mike Snitzer wrote:
> On Mon, Sep 11 2017 at  4:51pm -0400,
> Jens Axboe  wrote:
> 
>> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
>>> Here is v2 that should obviate the need to rename blk_mq_insert_request
>>> (by using bools to control run_queue and async).
>>>
>>> As for inserting directly into dispatch, if that can be done that is
>>> great but I'd prefer to have that be a follow-up optimization.  This
>>> fixes the regression in question, and does so in well-known terms.
>>>
>>> What do you think?
>>
>> I think it looks reasonable. My only concern is the use of the software
>> queues. Depending on the scheduler, they may or may not be used. I'd
>> need to review the code, but my first thought is that this would break
>> if you use blk_mq_insert_request() on a device that is managed by
>> mq-deadline or bfq, for instance. Schedulers are free to use the
>> software queues, but they are also free to ignore them and use internal
>> queuing.
>>
>> Looking at the code, looks like this was changed slightly at some point,
>> we always flush the software queues, if any of them contain requests. So
>> it's probably fine.
> 
> OK good, but is that too brittle to rely on? Something that might change
> in the future?

I'm actually surprised we do flush software queues for that case, since
we don't always have to. So it is a bit of a wart. If we don't have a
scheduler, software queues is where IO goes. If we have a scheduler, the
scheduler has complete control of where to queue IO. Generally, the
scheduler will either utilize the software queues or it won't, there's
nothing in between.

I know realize I'm an idiot and didn't read it right. So here's the code
in question:

const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;  

[...]

} else if (!has_sched_dispatch) {   
blk_mq_flush_busy_ctxs(hctx, _list); 
blk_mq_dispatch_rq_list(q, _list);   
}

so we do only enter sw queue flushing, if we don't have a scheduler with
a dispatch_request hook. So now I am really wondering how your patch
could work if the bottom device has bfq or mq-deadline attached?

>> My earlier suggestion to use just hctx->dispatch for the IO and bypass
>> the software queues completely. The use case for the dispatch list is
>> the same, regardless of whether the device has a scheduler attached or
>> not.
> 
> I'm missing how these details relate to the goal of bypassing any
> scheduler that might be attached.  Are you saying the attached elevator
> would still get in the way?

See above.

> Looking at blk_mq_sched_insert_request(), submission when an elevator
> isn't attached is exactly what I made blk_mq_insert_request() do
> (which is exactly what it did in the past).

Right, but that path is only used if we don't have a scheduler attached.
So while the code will use that path IFF a scheduler isn't attached to
that device, your use case will use it for both cases.

> In the case of DM multipath, nothing else should be submitting IO to
> the device so elevator shouldn't be used -- only interface for
> submitting IO would be blk_mq_insert_request().  So even if a
> scheduler is attached it should be bypassed right?

The problem is the usage of the sw queue.

Does the below work for you?


diff --git a/block/blk-core.c b/block/blk-core.c
index d709c0e3a2ac..aebe676225e6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2342,7 +2342,12 @@ blk_status_t blk_insert_cloned_request(struct 
request_queue *q, struct request *
if (q->mq_ops) {
if (blk_queue_io_stat(q))
blk_account_io_start(rq, true);
-   blk_mq_sched_insert_request(rq, false, true, false, false);
+   /*
+* Since we have a scheduler attached on the top device,
+* bypass a potential scheduler on the bottom device for
+* insert.
+*/
+   blk_mq_request_bypass_insert(rq);
return BLK_STS_OK;
}
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3f18cff80050..98a18609755e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1401,6 +1401,22 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx *hctx, 
struct request *rq,
blk_mq_hctx_mark_pending(hctx, ctx);
 }
 
+/*
+ * 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)
+{
+   struct blk_mq_ctx *ctx = rq->mq_ctx;
+   struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu);
+
+   spin_lock(>lock);
+   list_add_tail(>queuelist, >dispatch);
+   spin_unlock(>lock);
+
+   blk_mq_run_hw_queue(hctx, false);
+}
+
 void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
struct list_head *list)
 
diff 

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Mike Snitzer]

On Mon, Sep 11 2017 at  4:51pm -0400,
Jens Axboe  wrote:

> On 09/11/2017 10:16 AM, Mike Snitzer wrote:
> > Here is v2 that should obviate the need to rename blk_mq_insert_request
> > (by using bools to control run_queue and async).
> > 
> > As for inserting directly into dispatch, if that can be done that is
> > great but I'd prefer to have that be a follow-up optimization.  This
> > fixes the regression in question, and does so in well-known terms.
> > 
> > What do you think?
> 
> I think it looks reasonable. My only concern is the use of the software
> queues. Depending on the scheduler, they may or may not be used. I'd
> need to review the code, but my first thought is that this would break
> if you use blk_mq_insert_request() on a device that is managed by
> mq-deadline or bfq, for instance. Schedulers are free to use the
> software queues, but they are also free to ignore them and use internal
> queuing.
> 
> Looking at the code, looks like this was changed slightly at some point,
> we always flush the software queues, if any of them contain requests. So
> it's probably fine.

OK good, but is that too brittle to rely on? Something that might change
in the future?

> My earlier suggestion to use just hctx->dispatch for the IO and bypass
> the software queues completely. The use case for the dispatch list is
> the same, regardless of whether the device has a scheduler attached or
> not.

I'm missing how these details relate to the goal of bypassing any
scheduler that might be attached.  Are you saying the attached elevator
would still get in the way?

Looking at blk_mq_sched_insert_request(), submission when an elevator
isn't attached is exactly what I made blk_mq_insert_request() do (which
is exactly what it did in the past).

In the case of DM multipath, nothing else should be submitting IO to the
device so elevator shouldn't be used -- only interface for submitting IO
would be blk_mq_insert_request().  So even if a scheduler is attached it
should be bypassed right?

Mike

Re: [PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Jens Axboe]

On 09/11/2017 10:16 AM, Mike Snitzer wrote:
> Here is v2 that should obviate the need to rename blk_mq_insert_request
> (by using bools to control run_queue and async).
> 
> As for inserting directly into dispatch, if that can be done that is
> great but I'd prefer to have that be a follow-up optimization.  This
> fixes the regression in question, and does so in well-known terms.
> 
> What do you think?

I think it looks reasonable. My only concern is the use of the software
queues. Depending on the scheduler, they may or may not be used. I'd
need to review the code, but my first thought is that this would break
if you use blk_mq_insert_request() on a device that is managed by
mq-deadline or bfq, for instance. Schedulers are free to use the
software queues, but they are also free to ignore them and use internal
queuing.

Looking at the code, looks like this was changed slightly at some point,
we always flush the software queues, if any of them contain requests. So
it's probably fine.

My earlier suggestion to use just hctx->dispatch for the IO and bypass
the software queues completely. The use case for the dispatch list is
the same, regardless of whether the device has a scheduler attached or
not.

-- 
Jens Axboe

[PATCH v2] block: directly insert blk-mq request from blk_insert_cloned_request()

[Mike Snitzer]

Here is v2 that should obviate the need to rename blk_mq_insert_request
(by using bools to control run_queue and async).

As for inserting directly into dispatch, if that can be done that is
great but I'd prefer to have that be a follow-up optimization.  This
fixes the regression in question, and does so in well-known terms.

What do you think?

Thanks,
Mike

From: Mike Snitzer <snit...@redhat.com>
Date: Fri, 8 Sep 2017 11:45:13 -0400
Subject: [PATCH v2] block: directly insert blk-mq request from 
blk_insert_cloned_request()

A NULL pointer crash was reported for the case of having the BFQ IO
scheduler attached to the underlying blk-mq paths of a DM multipath
device.  The crash occured in blk_mq_sched_insert_request()'s call to
e->type->ops.mq.insert_requests().

Paolo Valente correctly summarized why the crash occured with:
"the call chain (dm_mq_queue_rq -> map_request -> setup_clone ->
blk_rq_prep_clone) creates a cloned request without invoking
e->type->ops.mq.prepare_request for the target elevator e.  The cloned
request is therefore not initialized for the scheduler, but it is
however inserted into the scheduler by blk_mq_sched_insert_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.

To fix this re-introduce a blk-mq private blk_mq_insert_request() that
blk_insert_cloned_request() calls to insert the request without
involving any elevator that may be attached to the cloned request's
request_queue.

Fixes: bd166ef18 ("blk-mq-sched: add framework for MQ capable IO schedulers")
Cc: sta...@vger.kernel.org
Reported-by: Bart Van Assche <bart.vanass...@wdc.com>
Signed-off-by: Mike Snitzer <snit...@redhat.com>
---
 block/blk-core.c |  2 +-
 block/blk-mq.c   | 28 +++-
 block/blk-mq.h   |  1 +
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index dbecbf4..9085013 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2330,7 +2330,7 @@ blk_status_t blk_insert_cloned_request(struct 
request_queue *q, struct request *
if (q->mq_ops) {
if (blk_queue_io_stat(q))
blk_account_io_start(rq, true);
-   blk_mq_sched_insert_request(rq, false, true, false, false);
+   blk_mq_insert_request(rq, true, false);
return BLK_STS_OK;
}
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4603b11..05d9f7c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1357,6 +1357,25 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx *hctx, 
struct request *rq,
blk_mq_hctx_mark_pending(hctx, ctx);
 }
 
+static inline void blk_mq_queue_io(struct blk_mq_hw_ctx *hctx,
+  struct blk_mq_ctx *ctx,
+  struct request *rq)
+{
+   spin_lock(>lock);
+   __blk_mq_insert_request(hctx, rq, false);
+   spin_unlock(>lock);
+}
+
+void blk_mq_insert_request(struct request *rq, bool run_queue, bool async)
+{
+   struct blk_mq_ctx *ctx = rq->mq_ctx;
+   struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu);
+
+   blk_mq_queue_io(hctx, ctx, rq);
+   if (run_queue)
+   blk_mq_run_hw_queue(hctx, async);
+}
+
 void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
struct list_head *list)
 
@@ -1450,15 +1469,6 @@ static inline bool hctx_allow_merges(struct 
blk_mq_hw_ctx *hctx)
!blk_queue_nomerges(hctx->queue);
 }
 
-static inline void blk_mq_queue_io(struct blk_mq_hw_ctx *hctx,
-  struct blk_mq_ctx *ctx,
-  struct request *rq)
-{
-   spin_lock(>lock);
-   __blk_mq_insert_request(hctx, rq, false);
-   spin_unlock(>lock);
-}
-
 static blk_qc_t request_to_qc_t(struct blk_mq_hw_ctx *hctx, struct request *rq)
 {
if (rq->tag != -1)
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 60b01c0..01067b2 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -54,6 +54,7 @@ int blk_mq_alloc_rqs(struct blk_mq_tag_set *set, struct 
blk_mq_tags *tags,
  */
 void __blk_mq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
bool at_head);
+void blk_mq_insert_request(struct request *rq, bool run_queue, bool async);
 void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
struct list_head *list);
 
-- 
2.10.1