On 6/3/20 9:16 AM, John Garry wrote:
> On 03/06/2020 14:36, Ming Lei wrote:
>> On Wed, Jun 03, 2020 at 01:53:47PM +0200, Christoph Hellwig wrote:
>>> On Wed, Jun 03, 2020 at 06:51:27PM +0800, Ming Lei wrote:
>>>> Commit bf0beec0607d ("blk-mq: drain I/O when all CPUs in a hctx are offline")
>>>> prevents new request from being allocated on hctx which is going to be
>>>> inactive,
>>>> meantime drains all in-queue requests.
>>>>
>>>> We needn't to prevent driver tag from being allocated during cpu hotplug, more
>>>> importantly we have to provide driver tag for requests, so that the cpu hotplug
>>>> handling can move on. blk_mq_get_tag() is shared for allocating both internal
>>>> tag and drive tag, so driver tag allocation may fail because the hctx is
>>>> marked as inactive.
>>>>
>>>> Fix the issue by moving BLK_MQ_S_INACTIVE check to __blk_mq_alloc_request().
>>>
>>> This looks correct, but a little ugly. How about we resurrect my
>>> patch to split off blk_mq_get_driver_tag entirely? Quick untested rebase
>>
>> OK, I am fine.
>>
>>> below, still needs a better changelog and fixes tg:
>>>
>>> ---
>>> From e432011e2eb5ac7bd1046bbf936645e9f7b74e64 Mon Sep 17 00:00:00 2001
>>> From: Christoph Hellwig <hch@xxxxxx>
>>> Date: Sat, 16 May 2020 08:03:48 +0200
>>> Subject: blk-mq: split out a __blk_mq_get_driver_tag helper
>>>
>>> Allocation of the driver tag in the case of using a scheduler shares very
>>> little code with the "normal" tag allocation. Split out a new helper to
>>> streamline this path, and untangle it from the complex normal tag
>>> allocation.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>>> ---
>>> block/blk-mq-tag.c | 27 +++++++++++++++++++++++++++
>>> block/blk-mq-tag.h | 8 ++++++++
>>> block/blk-mq.c | 29 -----------------------------
>>> block/blk-mq.h | 1 -
>>> 4 files changed, 35 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>>> index 96a39d0724a29..cded7fdcad8ef 100644
>>> --- a/block/blk-mq-tag.c
>>> +++ b/block/blk-mq-tag.c
>>> @@ -191,6 +191,33 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>> return tag + tag_offset;
>>> }
>>> +bool __blk_mq_get_driver_tag(struct request *rq)
>>> +{
>>> + struct sbitmap_queue *bt = &rq->mq_hctx->tags->bitmap_tags;
>>> + unsigned int tag_offset = rq->mq_hctx->tags->nr_reserved_tags;
>>> + bool shared = blk_mq_tag_busy(rq->mq_hctx);
>>
>> Not necessary to add 'shared' which is just used once.
>>
>>> + int tag;
>>> +
>>> + if (blk_mq_tag_is_reserved(rq->mq_hctx->sched_tags, rq->internal_tag)) {
>>> + bt = &rq->mq_hctx->tags->breserved_tags;
>>
>> Too many rq->mq_hctx->tags, you can add one local variable to store it.
>>
>> Otherwise, looks fine:
>>
>> Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
>
> So I did see this for mq-deadline scheduler:
>
> ] Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000040
> [ 518.688402] Mem abort info:
> [ 518.691183]ESR = 0x96000004
> [ 518.694233]EC = 0x25: DABT (current EL), IL = 32 bits
> [ 518.699568]SET = 0, FnV = 0
> [ 518.702667] irq_shutdown irq143
> [ 518.705937] CPU43: shutdown
> [ 518.706093]EA = 0, S1PTW = 0
> [ 518.708733] psci: CPU43 killed (polled 0 ms)
> [ 518.716141] Data abort info:
> [ 518.719050]ISV = 0, ISS = 0x00000004
> [ 518.722892]CM = 0, WnR = 0
> [ 518.725867] user pgtable: 4k pages, 48-bit VAs, pgdp=0000041fa4eae000
> [ 518.732318] [0000000000000040] pgd=0000000000000000
> [ 518.737208] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [ 518.742768] Modules linked in:
> [ 518.745812] CPU: 0 PID: 1309 Comm: kworker/43:1H Not tainted
> 5.7.0-rc2-00125-gf498b5de8a1c #390
> [ 518.754496] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21
> Nemo 2.0 RC0 04/18/2018
> [ 518.763624] Workqueue: kblockd blk_mq_run_work_fn
> [ 518.768315] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [ 518.773094] pc : blk_mq_put_tag+0x2c/0x68
> [ 518.777091] lr : blk_mq_get_tag+0x2ec/0x338
> [ 518.781260] sp : ffff800021a53ab0
> [ 518.784562] x29: ffff800021a53ab0 x28: ffff041f9e81d248
> [ 518.789861] x27: 00000000000002aa x26: ffff041fa3e49300
> [ 518.795159] x25: ffff001fb0178fc0 x24: ffff800021a53c68
> [ 518.800458] x23: 0000000000000000 x22: 0000000000000000
> [ 518.805756] x21: ffff800011a69000 x20: ffff041fa3e49310
> [ 518.811055] x19: ffff800021a53b90 x18: 0000000000000004
> [ 518.816354] x17: 0000000000000000 x16: 0000000000000000
> [ 518.821652] x15: 0000000000000000 x14: 00000000000000ef
> [ 518.826951] x13: 00000000000000ef x12: 0000000000000001
> [ 518.832249] x11: 0000000000000000 x10: 00000000000009c0
> [ 518.837548] x9 : ffff800021a53d50 x8 : ffff041f9e39a620
> [ 518.842847] x7 : fefefefefefefeff x6 : 0000000000000000
> [ 518.848146] x5 : 400007ffffffffff x4 : 0000000000001000
> [ 518.853445] x3 : 00000000000002aa x2 : 0000000000000000
> [ 518.858743] x1 : 0000000000000000 x0 : ffff041fa3e49300
> [ 518.864042] Call trace:
> [ 518.866477] blk_mq_put_tag+0x2c/0x68
> [ 518.870126] blk_mq_get_tag+0x2ec/0x338
> [ 518.873948] blk_mq_get_driver_tag+0xf8/0x168
> [ 518.878292] blk_mq_dispatch_rq_list+0x118/0x670
> [ 518.882896] blk_mq_do_dispatch_sched+0xb0/0x148
> [ 518.887500] __blk_mq_sched_dispatch_requests+0xec/0x1d8
> [ 518.892799] blk_mq_sched_dispatch_requests+0x3c/0x78
> [ 518.897837] __blk_mq_run_hw_queue+0xb4/0x130
> [ 518.902181] blk_mq_run_work_fn+0x20/0x30
> [ 518.906179] process_one_work+0x1e8/0x360
> [ 518.910176] worker_thread+0x44/0x488
> [ 518.913825] kthread+0x150/0x158
> [ 518.917041] ret_from_fork+0x10/0x1c
> [ 518.920606] Code: b9400004 4b020063 6b04007f 540001e2 (b9404022)
>
> With Christoph's patch and "blk-mq: get ctx in order to handle BLK_MQ_S_INACTIVE
> in blk_mq_get_tag()" [I didn't test without that patch], no issue detected so far.
>
I think my patch is no longer required with Christoph's patch.
Dongli Zhang
Dongli Zhang
