Mailing List Archive: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic

[PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic

dwagner at suse

Aug 2, 2021, 4:15 AM

Post #1 of 10 (15 views)

From: James Smart <jsmart2021@gmail.com>

commit fe35ec58f0d3 ("block: update hctx map when use multiple maps")
exposed an issue where we may hang trying to wait for queue freeze
during I/O. We call blk_mq_update_nr_hw_queues which may attempt to freeze
the queue. However we never started queue freeze when starting the
reset, which means that we have inflight pending requests that entered the
queue that we will not complete once the queue is quiesced.

So start a freeze before we quiesce the queue, and unfreeze the queue
after we successfully connected the I/O queues (the unfreeze is already
present in the code). blk_mq_update_nr_hw_queues will be called only
after we are sure that the queue was already frozen.

This follows to how the pci driver handles resets.

This patch added logic introduced in commit 9f98772ba307 "nvme-rdma: fix
controller reset hang during traffic".

Signed-off-by: James Smart <jsmart2021@gmail.com>
CC: Sagi Grimberg <sagi@grimberg.me>
[dwagner: call nvme_unfreeze() unconditionally in
nvme_fc_recreate_io_queues() to match the nvme_start_freeze()]
Tested-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/fc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 133b87db4f1d..b292af0fd655 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2486,6 +2486,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
* (but with error status).
*/
if (ctrl->ctrl.queue_count > 1) {
+ nvme_start_freeze(&ctrl->ctrl);
nvme_stop_queues(&ctrl->ctrl);
nvme_sync_io_queues(&ctrl->ctrl);
blk_mq_tagset_busy_iter(&ctrl->tag_set,
@@ -2966,8 +2967,8 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl)
return -ENODEV;
}
blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
- nvme_unfreeze(&ctrl->ctrl);
}
+ nvme_unfreeze(&ctrl->ctrl);

ret = nvme_fc_create_hw_io_queues(ctrl, ctrl->ctrl.sqsize + 1);
if (ret)
--
2.29.2

[PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

dwagner at suse

Aug 2, 2021, 6:15 AM

Post #2 of 10 (15 views)

From: James Smart <jsmart2021@gmail.com>

commit fe35ec58f0d3 ("block: update hctx map when use multiple maps")
exposed an issue where we may hang trying to wait for queue freeze
during I/O. We call blk_mq_update_nr_hw_queues which may attempt to freeze
the queue. However we never started queue freeze when starting the
reset, which means that we have inflight pending requests that entered the
queue that we will not complete once the queue is quiesced.

So start a freeze before we quiesce the queue, and unfreeze the queue
after we successfully connected the I/O queues (the unfreeze is already
present in the code). blk_mq_update_nr_hw_queues will be called only
after we are sure that the queue was already frozen.

This follows to how the pci driver handles resets.

This patch added logic introduced in commit 9f98772ba307 "nvme-rdma: fix
controller reset hang during traffic".

Signed-off-by: James Smart <jsmart2021@gmail.com>
CC: Sagi Grimberg <sagi@grimberg.me>
[dwagner: call nvme_unfreeze() unconditionally in
nvme_fc_recreate_io_queues() to match the nvme_start_freeze()]
Tested-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/fc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 133b87db4f1d..b292af0fd655 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2486,6 +2486,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
* (but with error status).
*/
if (ctrl->ctrl.queue_count > 1) {
+ nvme_start_freeze(&ctrl->ctrl);
nvme_stop_queues(&ctrl->ctrl);
nvme_sync_io_queues(&ctrl->ctrl);
blk_mq_tagset_busy_iter(&ctrl->tag_set,
@@ -2966,8 +2967,8 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl)
return -ENODEV;
}
blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
- nvme_unfreeze(&ctrl->ctrl);
}
+ nvme_unfreeze(&ctrl->ctrl);

ret = nvme_fc_create_hw_io_queues(ctrl, ctrl->ctrl.sqsize + 1);
if (ret)
--
2.29.2

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

himanshu.madhani at oracle

Aug 2, 2021, 2:15 PM

Post #3 of 10 (15 views)

On 8/2/21 6:26 AM, Daniel Wagner wrote:
> From: James Smart <jsmart2021@gmail.com>
>
> commit fe35ec58f0d3 ("block: update hctx map when use multiple maps")
> exposed an issue where we may hang trying to wait for queue freeze
> during I/O. We call blk_mq_update_nr_hw_queues which may attempt to freeze
> the queue. However we never started queue freeze when starting the
> reset, which means that we have inflight pending requests that entered the
> queue that we will not complete once the queue is quiesced.
>
> So start a freeze before we quiesce the queue, and unfreeze the queue
> after we successfully connected the I/O queues (the unfreeze is already
> present in the code). blk_mq_update_nr_hw_queues will be called only
> after we are sure that the queue was already frozen.
>
> This follows to how the pci driver handles resets.
>
> This patch added logic introduced in commit 9f98772ba307 "nvme-rdma: fix
> controller reset hang during traffic".
>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> CC: Sagi Grimberg <sagi@grimberg.me>
> [.dwagner: call nvme_unfreeze() unconditionally in
> nvme_fc_recreate_io_queues() to match the nvme_start_freeze()]
> Tested-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> ---
> drivers/nvme/host/fc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 133b87db4f1d..b292af0fd655 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2486,6 +2486,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
> * (but with error status).
> */
> if (ctrl->ctrl.queue_count > 1) {
> + nvme_start_freeze(&ctrl->ctrl);
> nvme_stop_queues(&ctrl->ctrl);
> nvme_sync_io_queues(&ctrl->ctrl);
> blk_mq_tagset_busy_iter(&ctrl->tag_set,
> @@ -2966,8 +2967,8 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl)
> return -ENODEV;
> }
> blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
> - nvme_unfreeze(&ctrl->ctrl);
> }
> + nvme_unfreeze(&ctrl->ctrl);
>
> ret = nvme_fc_create_hw_io_queues(ctrl, ctrl->ctrl.sqsize + 1);
> if (ret)
>

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani Oracle Linux Engineering

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

Aug 4, 2021, 1:05 AM

Post #4 of 10 (15 views)

On 8/2/21 1:26 PM, Daniel Wagner wrote:
> From: James Smart <jsmart2021@gmail.com>
>
> commit fe35ec58f0d3 ("block: update hctx map when use multiple maps")
> exposed an issue where we may hang trying to wait for queue freeze
> during I/O. We call blk_mq_update_nr_hw_queues which may attempt to freeze
> the queue. However we never started queue freeze when starting the
> reset, which means that we have inflight pending requests that entered the
> queue that we will not complete once the queue is quiesced.
>
> So start a freeze before we quiesce the queue, and unfreeze the queue
> after we successfully connected the I/O queues (the unfreeze is already
> present in the code). blk_mq_update_nr_hw_queues will be called only
> after we are sure that the queue was already frozen.
>
> This follows to how the pci driver handles resets.
>
> This patch added logic introduced in commit 9f98772ba307 "nvme-rdma: fix
> controller reset hang during traffic".
>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> CC: Sagi Grimberg <sagi@grimberg.me>
> [.dwagner: call nvme_unfreeze() unconditionally in
> nvme_fc_recreate_io_queues() to match the nvme_start_freeze()]
> Tested-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> ---
> drivers/nvme/host/fc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 133b87db4f1d..b292af0fd655 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2486,6 +2486,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
> * (but with error status).
> */
> if (ctrl->ctrl.queue_count > 1) {
> + nvme_start_freeze(&ctrl->ctrl);
> nvme_stop_queues(&ctrl->ctrl);
> nvme_sync_io_queues(&ctrl->ctrl);
> blk_mq_tagset_busy_iter(&ctrl->tag_set,
> @@ -2966,8 +2967,8 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl)
> return -ENODEV;
> }
> blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
> - nvme_unfreeze(&ctrl->ctrl);
> }
> + nvme_unfreeze(&ctrl->ctrl);
>
> ret = nvme_fc_create_hw_io_queues(ctrl, ctrl->ctrl.sqsize + 1);
> if (ret)
>
There still is now an imbalance, as we're always calling
'nvme_unfreeze()' (irrespective on the number of queues), but will only
call 'nvme_start_freeze()' if we have more than one queue.

This might lead to an imbalance on the mq_freeze_depth counter.
Wouldn't it be better to move the call to 'nvme_start_freeze()' out of
the if() condition to avoid the imbalance?

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

dwagner at suse

Aug 4, 2021, 2:15 AM

Post #5 of 10 (15 views)

On Wed, Aug 04, 2021 at 09:23:49AM +0200, Hannes Reinecke wrote:
> There still is now an imbalance, as we're always calling
> 'nvme_unfreeze()' (irrespective on the number of queues), but will only
> call 'nvme_start_freeze()' if we have more than one queue.
>
> This might lead to an imbalance on the mq_freeze_depth counter.
> Wouldn't it be better to move the call to 'nvme_start_freeze()' out of
> the if() condition to avoid the imbalance?

What about something like nmve_is_frozen() which would avoid the tracking
of the queue state open coded as it is right?

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

sagi at grimberg

Aug 10, 2021, 6:15 PM

Post #6 of 10 (15 views)

> On Wed, Aug 04, 2021 at 09:23:49AM +0200, Hannes Reinecke wrote:
>> There still is now an imbalance, as we're always calling
>> 'nvme_unfreeze()' (irrespective on the number of queues), but will only
>> call 'nvme_start_freeze()' if we have more than one queue.
>>
>> This might lead to an imbalance on the mq_freeze_depth counter.
>> Wouldn't it be better to move the call to 'nvme_start_freeze()' out of
>> the if() condition to avoid the imbalance?
>
> What about something like nmve_is_frozen() which would avoid the tracking
> of the queue state open coded as it is right?

Why is there a conditional freeze?

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

dwagner at suse

Aug 11, 2021, 4:15 AM

Post #7 of 10 (15 views)

On Tue, Aug 10, 2021 at 06:05:04PM -0700, Sagi Grimberg wrote:
> Why is there a conditional freeze?

The freeze only happens if I/O queues have been created/started. If I
understand this correctly, this is the corner case we were only able to
establish the admin queue and fail later to setup the I/O queues.

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

jsmart2021 at gmail

Aug 12, 2021, 2:15 PM

Post #8 of 10 (15 views)

On 8/4/2021 12:23 AM, Hannes Reinecke wrote:
> On 8/2/21 1:26 PM, Daniel Wagner wrote:
>> From: James Smart <jsmart2021@gmail.com>
>>
>> commit fe35ec58f0d3 ("block: update hctx map when use multiple maps")
>> exposed an issue where we may hang trying to wait for queue freeze
>> during I/O. We call blk_mq_update_nr_hw_queues which may attempt to freeze
>> the queue. However we never started queue freeze when starting the
>> reset, which means that we have inflight pending requests that entered the
>> queue that we will not complete once the queue is quiesced.
>>
>> So start a freeze before we quiesce the queue, and unfreeze the queue
>> after we successfully connected the I/O queues (the unfreeze is already
>> present in the code). blk_mq_update_nr_hw_queues will be called only
>> after we are sure that the queue was already frozen.
>>
>> This follows to how the pci driver handles resets.
>>
>> This patch added logic introduced in commit 9f98772ba307 "nvme-rdma: fix
>> controller reset hang during traffic".
>>
>> Signed-off-by: James Smart <jsmart2021@gmail.com>
>> CC: Sagi Grimberg <sagi@grimberg.me>
>> [.dwagner: call nvme_unfreeze() unconditionally in
>> nvme_fc_recreate_io_queues() to match the nvme_start_freeze()]
>> Tested-by: Daniel Wagner <dwagner@suse.de>
>> Reviewed-by: Daniel Wagner <dwagner@suse.de>
>> ---
>> drivers/nvme/host/fc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
>> index 133b87db4f1d..b292af0fd655 100644
>> --- a/drivers/nvme/host/fc.c
>> +++ b/drivers/nvme/host/fc.c
>> @@ -2486,6 +2486,7 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
>> * (but with error status).
>> */
>> if (ctrl->ctrl.queue_count > 1) {
>> + nvme_start_freeze(&ctrl->ctrl);
>> nvme_stop_queues(&ctrl->ctrl);
>> nvme_sync_io_queues(&ctrl->ctrl);
>> blk_mq_tagset_busy_iter(&ctrl->tag_set,
>> @@ -2966,8 +2967,8 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl)
>> return -ENODEV;
>> }
>> blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
>> - nvme_unfreeze(&ctrl->ctrl);
>> }
>> + nvme_unfreeze(&ctrl->ctrl);
>>
>> ret = nvme_fc_create_hw_io_queues(ctrl, ctrl->ctrl.sqsize + 1);
>> if (ret)
>>
> There still is now an imbalance, as we're always calling
> 'nvme_unfreeze()' (irrespective on the number of queues), but will only
> call 'nvme_start_freeze()' if we have more than one queue.
>
> This might lead to an imbalance on the mq_freeze_depth counter.
> Wouldn't it be better to move the call to 'nvme_start_freeze()' out of
> the if() condition to avoid the imbalance?
>
> Cheers,

Daniel,

try this. Moves the location of the freeze and will always pair with the
unfreeze.

--- fc.c 2021-08-12 12:33:33.273278611 -0700
+++ fc.c.new 2021-08-12 13:01:16.185817238 -0700
@@ -2965,9 +2965,10 @@ nvme_fc_recreate_io_queues(struct nvme_f
prior_ioq_cnt, nr_io_queues);
nvme_wait_freeze(&ctrl->ctrl);
blk_mq_update_nr_hw_queues(&ctrl->tag_set, nr_io_queues);
- nvme_unfreeze(&ctrl->ctrl);
}

+ nvme_unfreeze(&ctrl->ctrl);
+
return 0;

out_delete_hw_queues:
@@ -3206,6 +3207,9 @@ nvme_fc_delete_association(struct nvme_f
ctrl->iocnt = 0;
spin_unlock_irqrestore(&ctrl->lock, flags);

+ if (ctrl->ctrl.queue_count > 1)
+ nvme_start_freeze(&ctrl->ctrl);
+
__nvme_fc_abort_outstanding_ios(ctrl, false);

/* kill the aens as they are a separate path */

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

dwagner at suse

Aug 18, 2021, 6:16 AM

Post #9 of 10 (15 views)

On Thu, Aug 12, 2021 at 01:03:07PM -0700, James Smart wrote:
> try this. Moves the location of the freeze and will always pair with the
> unfreeze.

Yes, this works. Do you send a proper patch out or should I just pick up
this patch and add a commit message?

Re: [PATCH v4 6/8] nvme-fc: fix controller reset hang during traffic [ In reply to ]

dwagner at suse

Aug 18, 2021, 6:16 AM

Post #10 of 10 (15 views)

On Wed, Aug 18, 2021 at 01:43:09PM +0200, Daniel Wagner wrote:
> On Thu, Aug 12, 2021 at 01:03:07PM -0700, James Smart wrote:
> > try this. Moves the location of the freeze and will always pair with the
> > unfreeze.
>
> Yes, this works. Do you send a proper patch out or should I just pick up
> this patch and add a commit message?

Forget it, the commit message is still the same. Let me respin this series.