Hi,
blk_mq_alloc_request_hctx() is used by NVMe fc/rdma/tcp/loop to connect
io queue. Also the sw ctx is chosen as the 1st online cpu in hctx->cpumask.
However, all cpus in hctx->cpumask may be offline.
This usage model isn't well supported by blk-mq which supposes allocator is
always done on one online CPU in hctx->cpumask. This assumption is
related with managed irq, which also requires blk-mq to drain inflight
request in this hctx when the last cpu in hctx->cpumask is going to
offline.
However, NVMe fc/rdma/tcp/loop don't use managed irq, so we should allow
them to ask for request allocation when the specified hctx is inactive
(all cpus in hctx->cpumask are offline). Fix blk_mq_alloc_request_hctx() by
allowing to allocate request when all CPUs of this hctx are offline.
Wen Xiong has verified V4 in her nvmef test.
V7:
- move blk_mq_hctx_use_managed_irq() into block/blk-mq.c, 3/3
V6:
- move device_has_managed_msi_irq() into kernel/irq/msi.c
V5:
- take John Garry's suggestion to replace device field with
new helper of device_has_managed_msi_irq()
V4:
- remove patches for cleanup queue map helpers
- take Christoph's suggestion to add field into 'struct device' for
describing if managed irq is allocated from one device
V3:
- cleanup map queues helpers, and remove pci/virtio/rdma queue
helpers
- store use managed irq info into qmap
V2:
- use flag of BLK_MQ_F_MANAGED_IRQ
- pass BLK_MQ_F_MANAGED_IRQ from driver explicitly
- kill BLK_MQ_F_STACKING
Ming Lei (3):
genirq: add device_has_managed_msi_irq
blk-mq: mark if one queue map uses managed irq
blk-mq: don't deactivate hctx if managed irq isn't used
block/blk-mq-pci.c | 2 ++
block/blk-mq-rdma.c | 7 ++++++
block/blk-mq-virtio.c | 2 ++
block/blk-mq.c | 35 ++++++++++++++++++--------
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
include/linux/blk-mq.h | 3 ++-
include/linux/msi.h | 5 ++++
kernel/irq/msi.c | 18 +++++++++++++
8 files changed, 62 insertions(+), 11 deletions(-)
--
2.31.1
irq vector allocation with managed affinity may be used by driver, and
blk-mq needs this info for draining queue because genirq core will shutdown
managed irq when all CPUs in the affinity mask are offline.
The info of using managed irq is often produced by drivers, and it is
consumed by blk-mq, so different subsystems are involved in this info flow.
Address this issue by adding one helper of device_has_managed_msi_irq()
which is suggested by John Garry.
Tested-by: Wen Xiong <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Suggested-by: John Garry <redacted>
Signed-off-by: Ming Lei <redacted>
---
include/linux/msi.h | 5 +++++
kernel/irq/msi.c | 18 ++++++++++++++++++
2 files changed, 23 insertions(+)
Retrieve this info via new added helper of device_has_managed_msi_irq,
then we can decide if one hctx needs to be drained before all its CPUs
become offline.
Tested-by: Wen Xiong <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <redacted>
Signed-off-by: Ming Lei <redacted>
---
block/blk-mq-pci.c | 2 ++
block/blk-mq-rdma.c | 7 +++++++
block/blk-mq-virtio.c | 2 ++
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
include/linux/blk-mq.h | 3 ++-
5 files changed, 14 insertions(+), 1 deletion(-)
blk-mq deactivates one hctx when the last CPU in hctx->cpumask become
offline by draining all requests originated from this hctx and moving new
allocation to other active hctx. This way is for avoiding inflight IO in
case of managed irq because managed irq is shutdown when the last CPU in
the irq's affinity becomes offline.
However, lots of drivers(nvme fc, rdma, tcp, loop, ...) don't use managed
irq, so they needn't to deactivate hctx when the last CPU becomes offline.
Also, some of them are the only user of blk_mq_alloc_request_hctx() which
is used for connecting io queue. And their requirement is that the connect
request needs to be submitted successfully via one specified hctx even
though all CPUs in this hctx->cpumask have become offline.
Addressing the requirement for nvme fc/rdma/loop by allowing to
allocate request from one hctx when all CPUs in this hctx are offline,
since these drivers don't use managed irq.
Finally don't deactivate one hctx when it doesn't use managed irq.
Tested-by: Wen Xiong <redacted>
Reviewed-by: John Garry <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <redacted>
---
block/blk-mq.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
@@ -2556,6 +2567,10 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node)structblk_mq_hw_ctx*hctx=hlist_entry_safe(node,structblk_mq_hw_ctx,cpuhp_online);+/* hctx needn't to be deactivated in case managed irq isn't used */+if(!blk_mq_hctx_use_managed_irq(hctx))+return0;+if(!cpumask_test_cpu(cpu,hctx->cpumask)||!blk_mq_last_cpu_in_hctx(cpu,hctx))return0;
On Wed, Aug 18, 2021 at 10:44:25PM +0800, Ming Lei wrote:
Hi,
blk_mq_alloc_request_hctx() is used by NVMe fc/rdma/tcp/loop to connect
io queue. Also the sw ctx is chosen as the 1st online cpu in hctx->cpumask.
However, all cpus in hctx->cpumask may be offline.
This usage model isn't well supported by blk-mq which supposes allocator is
always done on one online CPU in hctx->cpumask. This assumption is
related with managed irq, which also requires blk-mq to drain inflight
request in this hctx when the last cpu in hctx->cpumask is going to
offline.
However, NVMe fc/rdma/tcp/loop don't use managed irq, so we should allow
them to ask for request allocation when the specified hctx is inactive
(all cpus in hctx->cpumask are offline). Fix blk_mq_alloc_request_hctx() by
allowing to allocate request when all CPUs of this hctx are offline.
Wen Xiong has verified V4 in her nvmef test.
V7:
- move blk_mq_hctx_use_managed_irq() into block/blk-mq.c, 3/3
Hello Jens,
NVMe TCP and others have been a bit popular recent days, and the kernel panic
of blk_mq_alloc_request_hctx() has annoyed people for a bit long.
Any chance to pull the three patches in so we can fix them in 5.15?
Thanks,
Ming
From: Daniel Wagner <hidden> Date: 2021-09-15 16:15:02
On Wed, Aug 18, 2021 at 10:44:28PM +0800, Ming Lei wrote:
quoted hunk
struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
unsigned int op, blk_mq_req_flags_t flags, unsigned int hctx_idx)
{
@@ -468,7 +485,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, data.hctx = q->queue_hw_ctx[hctx_idx]; if (!blk_mq_hw_queue_mapped(data.hctx)) goto out_queue_exit;- cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);++ WARN_ON_ONCE(blk_mq_hctx_use_managed_irq(data.hctx));++ cpu = blk_mq_first_mapped_cpu(data.hctx); data.ctx = __blk_mq_get_ctx(q, cpu);
I was pondering how we could address the issue that the qla2xxx driver
is using managed IRQs which makes nvme-fc depending as class on managed
IRQ.
blk_mq_alloc_request_hctx() is the only place where we really need to
distinguish between managed and !managed IRQs. As far I undertand the
situation, if all CPUs for a hctx are going offline, the driver wont use
this context. So there is only the case we end up in this code path is
when the driver tries to reconnect the queues, e.g. after
devloss. Couldn't we in this case not just return an error and go into
error recovery? Something like this:
On Wed, Sep 15, 2021 at 06:14:59PM +0200, Daniel Wagner wrote:
quoted hunk
On Wed, Aug 18, 2021 at 10:44:28PM +0800, Ming Lei wrote:
quoted
struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
unsigned int op, blk_mq_req_flags_t flags, unsigned int hctx_idx)
{
@@ -468,7 +485,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, data.hctx = q->queue_hw_ctx[hctx_idx]; if (!blk_mq_hw_queue_mapped(data.hctx)) goto out_queue_exit;- cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);++ WARN_ON_ONCE(blk_mq_hctx_use_managed_irq(data.hctx));++ cpu = blk_mq_first_mapped_cpu(data.hctx); data.ctx = __blk_mq_get_ctx(q, cpu);
I was pondering how we could address the issue that the qla2xxx driver
is using managed IRQs which makes nvme-fc depending as class on managed
IRQ.
blk_mq_alloc_request_hctx() is the only place where we really need to
distinguish between managed and !managed IRQs. As far I undertand the
situation, if all CPUs for a hctx are going offline, the driver wont use
this context. So there is only the case we end up in this code path is
when the driver tries to reconnect the queues, e.g. after
devloss. Couldn't we in this case not just return an error and go into
error recovery? Something like this:
Firstly, even with patches of 'qla2xxx - add nvme map_queues support',
the knowledge if managed irq is used in nvmef LLD is still missed, so
blk_mq_hctx_use_managed_irq() may always return false, but that
shouldn't be hard to solve.
The problem is that we still should make connect io queue completed
when all CPUs of this hctx is offline in case of managed irq.
One solution might be to use io polling for connecting io queue, but nvme fc
doesn't support polling, all the other nvme hosts do support it.
Thanks,
Ming
From: Daniel Wagner <hidden> Date: 2021-09-16 07:42:34
On Thu, Sep 16, 2021 at 10:17:18AM +0800, Ming Lei wrote:
Firstly, even with patches of 'qla2xxx - add nvme map_queues support',
the knowledge if managed irq is used in nvmef LLD is still missed, so
blk_mq_hctx_use_managed_irq() may always return false, but that
shouldn't be hard to solve.
@@ -7914,6 +7914,9 @@ static int qla2xxx_map_queues(struct Scsi_Host *shost)rc=blk_mq_map_queues(qmap);elserc=blk_mq_pci_map_queues(qmap,vha->hw->pdev,vha->irq_offset);++qmap->use_managed_irq=true;+returnrc;}
The problem is that we still should make connect io queue completed
when all CPUs of this hctx is offline in case of managed irq.
I agree, though if I understand this right, the scenario where all CPUs
are offline in a hctx and we want to use this htcx is only happening
after an initial setup and then reconnect attempt happens. That is
during the first connect attempt only online CPUs are assigned to the
hctx. When the CPUs are taken offline the block layer makes sure not to
use those queues anymore (no problem for the hctx so far). Then for some
reason the nmve-fc layer decides to reconnect and we end up in the
situation where we don't have any online CPU in given hctx.
One solution might be to use io polling for connecting io queue, but nvme fc
doesn't support polling, all the other nvme hosts do support it.
No idea, something to explore for sure :)
My point is that your series is fixing existing bugs and doesn't
introduce a new one. qla2xxx is already depending on managed IRQs. I
would like to see your series accepted with my hack as stop gap solution
until we have a proper fix.
On Thu, Sep 16, 2021 at 09:42:29AM +0200, Daniel Wagner wrote:
quoted hunk
On Thu, Sep 16, 2021 at 10:17:18AM +0800, Ming Lei wrote:
quoted
Firstly, even with patches of 'qla2xxx - add nvme map_queues support',
the knowledge if managed irq is used in nvmef LLD is still missed, so
blk_mq_hctx_use_managed_irq() may always return false, but that
shouldn't be hard to solve.
@@ -7914,6 +7914,9 @@ static int qla2xxx_map_queues(struct Scsi_Host *shost)rc=blk_mq_map_queues(qmap);elserc=blk_mq_pci_map_queues(qmap,vha->hw->pdev,vha->irq_offset);++qmap->use_managed_irq=true;+returnrc;}
blk_mq_alloc_request_hctx() won't be called into qla2xxx queue, what we
need is to mark the nvmef queue as .use_managed_irq if the LLD uses
managed irq.
quoted
The problem is that we still should make connect io queue completed
when all CPUs of this hctx is offline in case of managed irq.
I agree, though if I understand this right, the scenario where all CPUs
are offline in a hctx and we want to use this htcx is only happening
after an initial setup and then reconnect attempt happens. That is
during the first connect attempt only online CPUs are assigned to the
hctx. When the CPUs are taken offline the block layer makes sure not to
use those queues anymore (no problem for the hctx so far). Then for some
reason the nmve-fc layer decides to reconnect and we end up in the
situation where we don't have any online CPU in given hctx.
It is simply that blk_mq_alloc_request_hctx() allocates request from one
specified hctx, and the specified hctx can be offline any time.
quoted
One solution might be to use io polling for connecting io queue, but nvme fc
doesn't support polling, all the other nvme hosts do support it.
No idea, something to explore for sure :)
It is totally a raw idea, something like: start each queue in poll mode,
and run connect IO queue command via polling. Once the connect io queue command
is done, switch the queue into normal mode. Then
blk_mq_alloc_request_hctx() is guaranteed to be successful.
My point is that your series is fixing existing bugs and doesn't
introduce a new one. qla2xxx is already depending on managed IRQs. I
would like to see your series accepted with my hack as stop gap solution
until we have a proper fix.
I am fine to work this way first if no one objects.
Thanks,
Ming
From: Daniel Wagner <hidden> Date: 2021-10-04 12:26:02
nvme-fc is currently the only user of blk_mq_alloc_request_hctx().
With the recent changes to teach the nvme subsystem to honor managed
IRQs, the assumption was the complete nvme-fc doesn't use managed
IRQs. Unfortunately, the qla2xxx driver uses the managed IRQs.
Add an interface the nvme-fc drivers to update the mapping and also to
set the use_managed_irq flag. This is very ugly as we have to pass
down struct blk_mq_tag_set. I haven't found any better way so far.
Relax the requirement in the blk_mq_alloc_request_hctx() that only
!managed IRQs are supported. As long one CPU is online in the
requested hctx all is good. If this is not the case we return an
error which allows the upper layer to start the reconnect loop.
As the current qla2xxx already depends on managed IRQs the main
difference with and without this patch is, that we see
nvme nvme8: Connect command failed, error wo/DNR bit: -16402
nvme nvme8: NVME-FC{8}: reset: Reconnect attempt failed (-18)
instead of just timeouts such as
qla2xxx [0000:81:00.0]-5032:1: ABT_IOCB: Invalid completion handle (1da) -- timed-out.
In both cases the system recovers as soon at least one CPUs is online
in all hctx. Also note, this is only for admin request. As long
no FC reset happens and a reconnect attempt is triggered, user space
is able to issue I/Os do the target.
Signed-off-by: Daniel Wagner <redacted>
---
Hi,
I've played a bit with this patch to figure out what the impact is for
the qla2xxx driver. Basically, the situation doesn't change a lot with
Ming's patches. If we happen to run into the situation that all CPUs
are offline in one hctx and a reconnect attempt is triggered all
traffic to the target cease. But as soon we have at least one CPU
online in all hctx the system recovers.
This patch just makes it a bit more verbose (maybe a warning could be
added to blk_mq_alloc_request_hctx()).
Thanks,
Daniel
block/blk-mq.c | 10 +++++++---
drivers/nvme/host/fc.c | 13 +++++++++++++
drivers/scsi/qla2xxx/qla_nvme.c | 14 ++++++++++++++
drivers/scsi/qla2xxx/qla_os.c | 3 +++
include/linux/nvme-fc-driver.h | 4 ++++
5 files changed, 41 insertions(+), 3 deletions(-)
From: Varad Gautam <hidden> Date: 2021-10-11 18:23:21
Hi Ming,
On 8/18/21 4:44 PM, Ming Lei wrote:
quoted hunk
irq vector allocation with managed affinity may be used by driver, and
blk-mq needs this info for draining queue because genirq core will shutdown
managed irq when all CPUs in the affinity mask are offline.
The info of using managed irq is often produced by drivers, and it is
consumed by blk-mq, so different subsystems are involved in this info flow.
Address this issue by adding one helper of device_has_managed_msi_irq()
which is suggested by John Garry.
Tested-by: Wen Xiong <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Suggested-by: John Garry <redacted>
Signed-off-by: Ming Lei <redacted>
---
include/linux/msi.h | 5 +++++
kernel/irq/msi.c | 18 ++++++++++++++++++
2 files changed, 23 insertions(+)
This led me to the following build warning:
In file included from ../drivers/iommu/irq_remapping.c:6:0:
../include/linux/msi.h:23:40: warning: 'struct device' declared inside parameter list will not be visible outside of this definition or declaration
A forward declaration for struct device before the #ifdef
would fix this.
Regards,
Varad