[PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

The NVMe drivers support a mode where no tagset is allocated for the I/O
queues and only the admin queue is usable.  In that case ctrl->tagset is
NULL and we must not call the block per-tagset quiesce helpers that
dereference it.

Fixes: 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset")
Reported-by: Gerd Bayer <gbayer@linux.ibm.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---

It turns out we have a bunch of other quiesce calls that can happen when
only the admin queue is live.  So bring back the original quick fix to
check for the tagset pointer in the quiesce helpers.

 drivers/nvme/host/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3195ae17df309..fd2e26cb7a688 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5215,6 +5215,8 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze);
 
 void nvme_quiesce_io_queues(struct nvme_ctrl *ctrl)
 {
+	if (!ctrl->tagset)
+		return;
 	if (!test_and_set_bit(NVME_CTRL_STOPPED, &ctrl->flags))
 		blk_mq_quiesce_tagset(ctrl->tagset);
 	else
@@ -5224,6 +5226,8 @@ EXPORT_SYMBOL_GPL(nvme_quiesce_io_queues);
 
 void nvme_unquiesce_io_queues(struct nvme_ctrl *ctrl)
 {
+	if (!ctrl->tagset)
+		return;
 	if (test_and_clear_bit(NVME_CTRL_STOPPED, &ctrl->flags))
 		blk_mq_unquiesce_tagset(ctrl->tagset);
 }
-- 
2.30.2

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Chao Leng]

Reviewed-by: Chao Leng <lengchao@huawei.com>

On 2022/11/16 15:27, Christoph Hellwig wrote:
> The NVMe drivers support a mode where no tagset is allocated for the I/O
> queues and only the admin queue is usable.  In that case ctrl->tagset is
> NULL and we must not call the block per-tagset quiesce helpers that
> dereference it.
> 
> Fixes: 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset")
> Reported-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> It turns out we have a bunch of other quiesce calls that can happen when
> only the admin queue is live.  So bring back the original quick fix to
> check for the tagset pointer in the quiesce helpers.
> 
>   drivers/nvme/host/core.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3195ae17df309..fd2e26cb7a688 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -5215,6 +5215,8 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze);
>   
>   void nvme_quiesce_io_queues(struct nvme_ctrl *ctrl)
>   {
> +	if (!ctrl->tagset)
> +		return;
>   	if (!test_and_set_bit(NVME_CTRL_STOPPED, &ctrl->flags))
>   		blk_mq_quiesce_tagset(ctrl->tagset);
>   	else
> @@ -5224,6 +5226,8 @@ EXPORT_SYMBOL_GPL(nvme_quiesce_io_queues);
>   
>   void nvme_unquiesce_io_queues(struct nvme_ctrl *ctrl)
>   {
> +	if (!ctrl->tagset)
> +		return;
>   	if (test_and_clear_bit(NVME_CTRL_STOPPED, &ctrl->flags))
>   		blk_mq_unquiesce_tagset(ctrl->tagset);
>   }
> 

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Sagi Grimberg]

On 11/16/22 09:27, Christoph Hellwig wrote:
> The NVMe drivers support a mode where no tagset is allocated for the I/O
> queues and only the admin queue is usable.  In that case ctrl->tagset is
> NULL and we must not call the block per-tagset quiesce helpers that
> dereference it.
> 
> Fixes: 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset")
> Reported-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> It turns out we have a bunch of other quiesce calls that can happen when
> only the admin queue is live.  So bring back the original quick fix to
> check for the tagset pointer in the quiesce helpers.
> 
>   drivers/nvme/host/core.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3195ae17df309..fd2e26cb7a688 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -5215,6 +5215,8 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze);
>   
>   void nvme_quiesce_io_queues(struct nvme_ctrl *ctrl)
>   {
> +	if (!ctrl->tagset)
> +		return;

Can this be placed in the pci code instead?

>   	if (!test_and_set_bit(NVME_CTRL_STOPPED, &ctrl->flags))
>   		blk_mq_quiesce_tagset(ctrl->tagset);
>   	else
> @@ -5224,6 +5226,8 @@ EXPORT_SYMBOL_GPL(nvme_quiesce_io_queues);
>   
>   void nvme_unquiesce_io_queues(struct nvme_ctrl *ctrl)
>   {
> +	if (!ctrl->tagset)
> +		return;
>   	if (test_and_clear_bit(NVME_CTRL_STOPPED, &ctrl->flags))
>   		blk_mq_unquiesce_tagset(ctrl->tagset);
>   }

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 03:20:23PM +0200, Sagi Grimberg wrote:
>>   {
>> +	if (!ctrl->tagset)
>> +		return;
>
> Can this be placed in the pci code instead?

There's nothing PCIe-specific about this case.  All fabrics drivers
support controllers with I/O queues and thus the I/O tagset, e.g.
discovery controllers.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Sagi Grimberg]

>>>    {
>>> +	if (!ctrl->tagset)
>>> +		return;
>>
>> Can this be placed in the pci code instead?
> 
> There's nothing PCIe-specific about this case.  All fabrics drivers
> support controllers with I/O queues and thus the I/O tagset, e.g.
> discovery controllers.

All the other drivers check that they have IO queues before
quiescing/unquiescing them.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 03:23:55PM +0200, Sagi Grimberg wrote:
> All the other drivers check that they have IO queues before
> quiescing/unquiescing them.

The core firmware activation code does not.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Chaitanya Kulkarni]

Chao,

On 11/15/22 23:52, Chao Leng wrote:
> Reviewed-by: Chao Leng <lengchao@huawei.com>
> 
> On 2022/11/16 15:27, Christoph Hellwig wrote:
>> The NVMe drivers support a mode where no tagset is allocated for the I/O

Please avoid top posting.

-ck

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Sagi Grimberg]

>> All the other drivers check that they have IO queues before
>> quiescing/unquiescing them.
> 
> The core firmware activation code does not.

Well, ideally it should, but maybe it would be more readable
if we condition on ctrl->queue_count > 1.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Sun, Nov 20, 2022 at 01:38:28PM +0200, Sagi Grimberg wrote:
>
>>> All the other drivers check that they have IO queues before
>>> quiescing/unquiescing them.
>>
>> The core firmware activation code does not.
>
> Well, ideally it should, but maybe it would be more readable
> if we condition on ctrl->queue_count > 1.

I find this more confusing to be honest, but if we get a consensus for
that I can live with it.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Sun, Nov 20, 2022 at 01:38:28PM +0200, Sagi Grimberg wrote:
>
>>> All the other drivers check that they have IO queues before
>>> quiescing/unquiescing them.
>>
>> The core firmware activation code does not.
>
> Well, ideally it should, but maybe it would be more readable
> if we condition on ctrl->queue_count > 1.

This would be the queue_count version.  I have to say I much prefer
the tagset check, as it is self-documenting and matches what
nvme_queue_scan, nvme_cancel_tagset and nvme_cancel_admin_tagset
do.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Tue, Nov 22, 2022 at 01:31:54PM +0100, Christoph Hellwig wrote:
> This would be the queue_count version.  I have to say I much prefer
> the tagset check, as it is self-documenting and matches what
> nvme_queue_scan, nvme_cancel_tagset and nvme_cancel_admin_tagset
> do.

And here is the actual patch:

---
From 167d14d9613014142ee285c2912dadc089d80e86 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 16 Nov 2022 08:14:46 +0100
Subject: nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

The NVMe drivers support a mode where no tagset is allocated for the I/O
queues and only the admin queue is usable.  In that case
ctrl->queue_count is smaller than two and we must not call the block
per-tagset quiesce helpers that dereference it.

Fixes: 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset")
Reported-by: Gerd Bayer <gbayer@linux.ibm.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3195ae17df309..6c8a110d6738a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5215,6 +5215,8 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze);
 
 void nvme_quiesce_io_queues(struct nvme_ctrl *ctrl)
 {
+	if (ctrl->queue_count < 2)
+		return;
 	if (!test_and_set_bit(NVME_CTRL_STOPPED, &ctrl->flags))
 		blk_mq_quiesce_tagset(ctrl->tagset);
 	else
@@ -5224,6 +5226,8 @@ EXPORT_SYMBOL_GPL(nvme_quiesce_io_queues);
 
 void nvme_unquiesce_io_queues(struct nvme_ctrl *ctrl)
 {
+	if (ctrl->queue_count < 2)
+		return;
 	if (test_and_clear_bit(NVME_CTRL_STOPPED, &ctrl->flags))
 		blk_mq_unquiesce_tagset(ctrl->tagset);
 }
-- 
2.30.2

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Christoph Hellwig]

On Tue, Nov 22, 2022 at 01:31:54PM +0100, Christoph Hellwig wrote:
> This would be the queue_count version.  I have to say I much prefer
> the tagset check, as it is self-documenting and matches what
> nvme_queue_scan, nvme_cancel_tagset and nvme_cancel_admin_tagset
> do.

So no comments so far.  Unless I hear more feedback I'd prefer to
go with the ->tagset checks that matches what the other code is
doing and also is more readable.

Re: [PATCH] nvme: don't call blk_mq_{,un}quiesce_tagset when ctrl->tagset is NULL

[Sagi Grimberg]

>> This would be the queue_count version.  I have to say I much prefer
>> the tagset check, as it is self-documenting and matches what
>> nvme_queue_scan, nvme_cancel_tagset and nvme_cancel_admin_tagset
>> do.
> 
> So no comments so far.  Unless I hear more feedback I'd prefer to
> go with the ->tagset checks that matches what the other code is
> doing and also is more readable.

OK, I'm not opposed to that anymore. We can take care of this
area on a different occasion.