[PATCH 0/6] nvme fabircs: move start freeze into reconnection work

[PATCH 0/6] nvme fabircs: move start freeze into reconnection work

[Ming Lei]

Hello,

The 1st patch unquiesces queues when removing namespaces.

The 2nd patch remove 'shutdown' parameters from two teardown helpers.

The other 4 patches moves start freeze into reconnection work context for
tcp and rdma: 1) freeze and unfreeze are paired 2) tcp/rdma io won't be
held too long during error recovery, and can be failfast quickly.
Suggested by Sagi Grimberg.


Ming Lei (6):
  nvme: unquiesce io queues when removing namespaces
  nvme: tcp: remove 'shutdown' parameters from two teardown helpers
  nvme: tcp: unquiesce queues after ctrl state is changed
  nvme: rdma: unquiesce queues until ctrl state is changed
  nvem: tcp: move start freeze into nvme_tcp_configure_io_queues
  nvem: rdma: move start freeze into nvme_rdma_configure_io_queues

 drivers/nvme/host/core.c | 10 +++++++---
 drivers/nvme/host/rdma.c |  7 ++++---
 drivers/nvme/host/tcp.c  | 32 ++++++++++++++++----------------
 3 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.40.1

[PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Ming Lei]

Error recovery can be interrupted by controller removal, then the
controller is left as quiesced, and IO hang can be caused.

Fix the issue by unquiescing controller unconditionally when removing
namespaces.

This way is reasonable and safe given forward progress can be made
when removing namespaces.

Reported-by: Chunguang Xu <brookxu.cn@gmail.com>
Closes: https://lore.kernel.org/linux-nvme/cover.1685350577.git.chunguang.xu@shopee.com/
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 47d7ba2827ff..98fa8315bc65 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3903,6 +3903,12 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 	 */
 	nvme_mpath_clear_ctrl_paths(ctrl);
 
+	/*
+	 * Unquiesce io queues so any pending IO won't hang, especially
+	 * those submitted from scan work
+	 */
+	nvme_unquiesce_io_queues(ctrl);
+
 	/* prevent racing with ns scanning */
 	flush_work(&ctrl->scan_work);
 
@@ -3912,10 +3918,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 	 * removing the namespaces' disks; fail all the queues now to avoid
 	 * potentially having to clean up the failed sync later.
 	 */
-	if (ctrl->state == NVME_CTRL_DEAD) {
+	if (ctrl->state == NVME_CTRL_DEAD)
 		nvme_mark_namespaces_dead(ctrl);
-		nvme_unquiesce_io_queues(ctrl);
-	}
 
 	/* this is a no-op when called from the controller reset handler */
 	nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING_NOIO);
-- 
2.40.1

[PATCH 2/6] nvme: tcp: remove 'shutdown' parameters from two teardown helpers

[Ming Lei]

'shutdown' is just for unquiescing queues, and we can do that outside
the caller, and turns out more readable given unquiesce isn't needed
in error recovery code path.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/tcp.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 3e7dd6f91832..e414cfb3433f 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1962,20 +1962,16 @@ static int nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
 	return error;
 }
 
-static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
-		bool remove)
+static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl)
 {
 	nvme_quiesce_admin_queue(ctrl);
 	blk_sync_queue(ctrl->admin_q);
 	nvme_tcp_stop_queue(ctrl, 0);
 	nvme_cancel_admin_tagset(ctrl);
-	if (remove)
-		nvme_unquiesce_admin_queue(ctrl);
 	nvme_tcp_destroy_admin_queue(ctrl, remove);
 }
 
-static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
-		bool remove)
+static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
 {
 	if (ctrl->queue_count <= 1)
 		return;
@@ -1985,8 +1981,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 	nvme_sync_io_queues(ctrl);
 	nvme_tcp_stop_io_queues(ctrl);
 	nvme_cancel_tagset(ctrl);
-	if (remove)
-		nvme_unquiesce_io_queues(ctrl);
 	nvme_tcp_destroy_io_queues(ctrl, remove);
 }
 
@@ -2114,10 +2108,10 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
-	nvme_tcp_teardown_io_queues(ctrl, false);
+	nvme_tcp_teardown_io_queues(ctrl);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl, false);
+	nvme_tcp_teardown_admin_queue(ctrl);
 	nvme_unquiesce_admin_queue(ctrl);
 	nvme_auth_stop(ctrl);
 
@@ -2133,10 +2127,14 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
-	nvme_tcp_teardown_io_queues(ctrl, shutdown);
+	nvme_tcp_teardown_io_queues(ctrl);
 	nvme_quiesce_admin_queue(ctrl);
 	nvme_disable_ctrl(ctrl, shutdown);
-	nvme_tcp_teardown_admin_queue(ctrl, shutdown);
+	nvme_tcp_teardown_admin_queue(ctrl);
+	if (shutdown) {
+		nvme_unquiesce_io_queues(ctrl);
+		nvme_unquiesce_admin_queue(ctrl);
+	}
 }
 
 static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
-- 
2.40.1

[PATCH 3/6] nvme: tcp: unquiesce queues after ctrl state is changed

[Ming Lei]

Unquiesce queues until controller state is changed successfully.

Prepare for moving start freeze into reconnect work for preventing new
request from entering queue in LIVE state.

Before removing namespace, unquiescing queues is done unconditionally, so
it is safe to do this way in case of state change failure.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/tcp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index e414cfb3433f..e68bf21af348 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2109,10 +2109,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	nvme_tcp_teardown_io_queues(ctrl);
-	/* unquiesce to fail fast pending requests */
-	nvme_unquiesce_io_queues(ctrl);
 	nvme_tcp_teardown_admin_queue(ctrl);
-	nvme_unquiesce_admin_queue(ctrl);
 	nvme_auth_stop(ctrl);
 
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
@@ -2122,6 +2119,10 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 		return;
 	}
 
+	/* unquiesce to fail fast pending requests */
+	nvme_unquiesce_io_queues(ctrl);
+	nvme_unquiesce_admin_queue(ctrl);
+
 	nvme_tcp_reconnect_or_remove(ctrl);
 }
 
-- 
2.40.1

[PATCH 4/6] nvme: rdma: unquiesce queues until ctrl state is changed

[Ming Lei]

Unquiesce queues until controller state is changed successfully.

Prepare for moving start freeze into reconnect work for preventing new
request from entering queue in LIVE state.

Before removing namespace, unquiescing queues is done unconditionally, so
it is safe to do this way in case of state change failure.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/rdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d433b2ec07a6..cdfa93d67b80 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1117,9 +1117,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 	nvme_stop_keep_alive(&ctrl->ctrl);
 	flush_work(&ctrl->ctrl.async_event_work);
 	nvme_rdma_teardown_io_queues(ctrl, false);
-	nvme_unquiesce_io_queues(&ctrl->ctrl);
 	nvme_rdma_teardown_admin_queue(ctrl, false);
-	nvme_unquiesce_admin_queue(&ctrl->ctrl);
 	nvme_auth_stop(&ctrl->ctrl);
 
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
@@ -1129,6 +1127,8 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 		return;
 	}
 
+	nvme_unquiesce_io_queues(&ctrl->ctrl);
+	nvme_unquiesce_admin_queue(&ctrl->ctrl);
 	nvme_rdma_reconnect_or_remove(ctrl);
 }
 
-- 
2.40.1

[PATCH 5/6] nvem: tcp: move start freeze into nvme_tcp_configure_io_queues

[Ming Lei]

Move start_freeze into nvme_tcp_configure_io_queues(), and there is
at least two benefits:

1) freeze and unfreeze are done in pair

2) IO during error recovery can be failfast quickly because nvme fabrics
unquiesces queues after teardown.

One side-effect is that !mpath request may timeout during connecting
because of queue topo change, but that looks not one big deal:

1) same problem exists with current code base

2) compared with !mpath, mpath use case is dominant

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/tcp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index e68bf21af348..798bfc29bc88 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1868,6 +1868,7 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
 		goto out_cleanup_connect_q;
 
 	if (!new) {
+		nvme_start_freeze(ctrl);
 		nvme_unquiesce_io_queues(ctrl);
 		if (!nvme_wait_freeze_timeout(ctrl, NVME_IO_TIMEOUT)) {
 			/*
@@ -1876,6 +1877,7 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
 			 * to be safe.
 			 */
 			ret = -ENODEV;
+			nvme_unfreeze(ctrl);
 			goto out_wait_freeze_timed_out;
 		}
 		blk_mq_update_nr_hw_queues(ctrl->tagset,
@@ -1976,7 +1978,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
 	if (ctrl->queue_count <= 1)
 		return;
 	nvme_quiesce_admin_queue(ctrl);
-	nvme_start_freeze(ctrl);
 	nvme_quiesce_io_queues(ctrl);
 	nvme_sync_io_queues(ctrl);
 	nvme_tcp_stop_io_queues(ctrl);
-- 
2.40.1

[PATCH 6/6] nvem: rdma: move start freeze into nvme_rdma_configure_io_queues

[Ming Lei]

Move start_freeze into nvme_rdma_configure_io_queues(), and there is
at least two benefits:

1) freeze and unfreeze are done in pair

2) IO during error recovery can be failfast quickly because nvme fabrics
unquiesces queues after teardown.

One side-effect is that !mpath request may timeout during connecting
because of queue topo change, but that looks not one big deal:

1) same problem exists with current code base

2) compared with !mpath, mpath use case is dominant

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index cdfa93d67b80..ec365ee9ad0e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -883,6 +883,7 @@ static int nvme_rdma_configure_io_queues(struct nvme_rdma_ctrl *ctrl, bool new)
 		goto out_cleanup_tagset;
 
 	if (!new) {
+		nvme_start_freeze(&ctrl->ctrl);
 		nvme_unquiesce_io_queues(&ctrl->ctrl);
 		if (!nvme_wait_freeze_timeout(&ctrl->ctrl, NVME_IO_TIMEOUT)) {
 			/*
@@ -891,6 +892,7 @@ static int nvme_rdma_configure_io_queues(struct nvme_rdma_ctrl *ctrl, bool new)
 			 * to be safe.
 			 */
 			ret = -ENODEV;
+			nvme_unfreeze(&ctrl->ctrl);
 			goto out_wait_freeze_timed_out;
 		}
 		blk_mq_update_nr_hw_queues(ctrl->ctrl.tagset,
@@ -940,7 +942,6 @@ static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
 		bool remove)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_start_freeze(&ctrl->ctrl);
 		nvme_quiesce_io_queues(&ctrl->ctrl);
 		nvme_sync_io_queues(&ctrl->ctrl);
 		nvme_rdma_stop_io_queues(ctrl);
-- 
2.40.1

Re: [PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Keith Busch]

On Mon, Jul 10, 2023 at 11:35:50PM +0800, Ming Lei wrote:
> Error recovery can be interrupted by controller removal, then the
> controller is left as quiesced, and IO hang can be caused.
> 
> Fix the issue by unquiescing controller unconditionally when removing
> namespaces.
> 
> This way is reasonable and safe given forward progress can be made
> when removing namespaces.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Sagi Grimberg]

Thanks Ming,

Can you please phrase the patch title as a bug fix, something like:
"nvme: fix possible hang when removing a controller during error recovery"

Other than that, looks good,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 5/6] nvem: tcp: move start freeze into nvme_tcp_configure_io_queues

[Sagi Grimberg]

Hey Ming, a few comments,

1. s/nvem/nvme/g
2. prefix is "nvme-tcp: "
3. Can you please phrase the patch title as a bug fix?

On 7/10/23 18:35, Ming Lei wrote:
> Move start_freeze into nvme_tcp_configure_io_queues(), and there is
> at least two benefits:
> 
> 1) freeze and unfreeze are done in pair
> 
> 2) IO during error recovery can be failfast quickly because nvme fabrics
> unquiesces queues after teardown.
> 
> One side-effect is that !mpath request may timeout during connecting
> because of queue topo change, but that looks not one big deal:
> 
> 1) same problem exists with current code base
> 
> 2) compared with !mpath, mpath use case is dominant

4. I think we need a "Fixes: " tag for the commit that added
the freeze functionality. IIRC it is:
2875b0aecabe ("nvme-tcp: fix controller reset hang during traffic")

Similar comments on patch 6/6.

> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/nvme/host/tcp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index e68bf21af348..798bfc29bc88 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1868,6 +1868,7 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
>   		goto out_cleanup_connect_q;
>   
>   	if (!new) {
> +		nvme_start_freeze(ctrl);
>   		nvme_unquiesce_io_queues(ctrl);
>   		if (!nvme_wait_freeze_timeout(ctrl, NVME_IO_TIMEOUT)) {
>   			/*
> @@ -1876,6 +1877,7 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
>   			 * to be safe.
>   			 */
>   			ret = -ENODEV;
> +			nvme_unfreeze(ctrl);
>   			goto out_wait_freeze_timed_out;
>   		}
>   		blk_mq_update_nr_hw_queues(ctrl->tagset,
> @@ -1976,7 +1978,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
>   	if (ctrl->queue_count <= 1)
>   		return;
>   	nvme_quiesce_admin_queue(ctrl);
> -	nvme_start_freeze(ctrl);
>   	nvme_quiesce_io_queues(ctrl);
>   	nvme_sync_io_queues(ctrl);
>   	nvme_tcp_stop_io_queues(ctrl);

Can you please separate patches 1,5,6 to a separate series as these are
bug fixes that should go to stable.

Re: [PATCH 2/6] nvme: tcp: remove 'shutdown' parameters from two teardown helpers

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/6] nvme: tcp: remove 'shutdown' parameters from two teardown helpers

[Sagi Grimberg]

> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Nit: title prefix is "nvme-tcp: "

Re: [PATCH 3/6] nvme: tcp: unquiesce queues after ctrl state is changed

[Sagi Grimberg]

> Unquiesce queues until controller state is changed successfully.
> 
> Prepare for moving start freeze into reconnect work for preventing new
> request from entering queue in LIVE state.
> 
> Before removing namespace, unquiescing queues is done unconditionally, so
> it is safe to do this way in case of state change failure.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/nvme/host/tcp.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index e414cfb3433f..e68bf21af348 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2109,10 +2109,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
>   	nvme_stop_keep_alive(ctrl);
>   	flush_work(&ctrl->async_event_work);
>   	nvme_tcp_teardown_io_queues(ctrl);
> -	/* unquiesce to fail fast pending requests */
> -	nvme_unquiesce_io_queues(ctrl);
>   	nvme_tcp_teardown_admin_queue(ctrl);
> -	nvme_unquiesce_admin_queue(ctrl);
>   	nvme_auth_stop(ctrl);

This is potentially a problem. It is possible that auth work is
running in the bg here, and without unquiescing the admin queue
the the auth_stop may take unnecessarily long until its I/O times
out.

I think it is fine to keep it as is...

>   
>   	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
> @@ -2122,6 +2119,10 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
>   		return;
>   	}
>   
> +	/* unquiesce to fail fast pending requests */
> +	nvme_unquiesce_io_queues(ctrl);
> +	nvme_unquiesce_admin_queue(ctrl);
> +
>   	nvme_tcp_reconnect_or_remove(ctrl);
>   }
>   

Re: [PATCH 4/6] nvme: rdma: unquiesce queues until ctrl state is changed

[Sagi Grimberg]

Same comment as the last patch, I don't think this
is necessary.

Re: [PATCH 3/6] nvme: tcp: unquiesce queues after ctrl state is changed

[Ming Lei]

On Tue, Jul 11, 2023 at 10:41:46AM +0300, Sagi Grimberg wrote:
> 
> > Unquiesce queues until controller state is changed successfully.
> > 
> > Prepare for moving start freeze into reconnect work for preventing new
> > request from entering queue in LIVE state.
> > 
> > Before removing namespace, unquiescing queues is done unconditionally, so
> > it is safe to do this way in case of state change failure.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   drivers/nvme/host/tcp.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index e414cfb3433f..e68bf21af348 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -2109,10 +2109,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> >   	nvme_stop_keep_alive(ctrl);
> >   	flush_work(&ctrl->async_event_work);
> >   	nvme_tcp_teardown_io_queues(ctrl);
> > -	/* unquiesce to fail fast pending requests */
> > -	nvme_unquiesce_io_queues(ctrl);
> >   	nvme_tcp_teardown_admin_queue(ctrl);
> > -	nvme_unquiesce_admin_queue(ctrl);
> >   	nvme_auth_stop(ctrl);
> 
> This is potentially a problem. It is possible that auth work is
> running in the bg here, and without unquiescing the admin queue
> the the auth_stop may take unnecessarily long until its I/O times
> out.

There isn't any IO queued to nvme before unquiesce since nvme_cancel_*tagset
cancels all inflight IOs.

> 
> I think it is fine to keep it as is...

If ctrl state is LIVE, nvme_check_ready() return true, then IO is
sent to socket before changing ctrl state to NVME_CTRL_CONNECTING.

So unquisce has to be done after state is changed to NVME_CTRL_CONNECTING
if we want to move start_freeze into re-connection work.

Thanks,
Ming

Re: [PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Christoph Hellwig]

On Mon, Jul 10, 2023 at 11:35:50PM +0800, Ming Lei wrote:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 47d7ba2827ff..98fa8315bc65 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3903,6 +3903,12 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>  	 */
>  	nvme_mpath_clear_ctrl_paths(ctrl);
>  
> +	/*
> +	 * Unquiesce io queues so any pending IO won't hang, especially
> +	 * those submitted from scan work
> +	 */
> +	nvme_unquiesce_io_queues(ctrl);

What quiesce does this pair with?

Re: [PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Ming Lei]

On Tue, Jul 11, 2023 at 10:27:19AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 10, 2023 at 11:35:50PM +0800, Ming Lei wrote:
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 47d7ba2827ff..98fa8315bc65 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3903,6 +3903,12 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
> >  	 */
> >  	nvme_mpath_clear_ctrl_paths(ctrl);
> >  
> > +	/*
> > +	 * Unquiesce io queues so any pending IO won't hang, especially
> > +	 * those submitted from scan work
> > +	 */
> > +	nvme_unquiesce_io_queues(ctrl);
> 
> What quiesce does this pair with?

The one done during teardown of error recovery, then removal comes and breaks
the in-progress error recovery.


Thanks,
Ming

Re: [PATCH 3/6] nvme: tcp: unquiesce queues after ctrl state is changed

[Sagi Grimberg]

On 7/11/23 10:53, Ming Lei wrote:
> On Tue, Jul 11, 2023 at 10:41:46AM +0300, Sagi Grimberg wrote:
>>
>>> Unquiesce queues until controller state is changed successfully.
>>>
>>> Prepare for moving start freeze into reconnect work for preventing new
>>> request from entering queue in LIVE state.
>>>
>>> Before removing namespace, unquiescing queues is done unconditionally, so
>>> it is safe to do this way in case of state change failure.
>>>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>    drivers/nvme/host/tcp.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index e414cfb3433f..e68bf21af348 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -2109,10 +2109,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
>>>    	nvme_stop_keep_alive(ctrl);
>>>    	flush_work(&ctrl->async_event_work);
>>>    	nvme_tcp_teardown_io_queues(ctrl);
>>> -	/* unquiesce to fail fast pending requests */
>>> -	nvme_unquiesce_io_queues(ctrl);
>>>    	nvme_tcp_teardown_admin_queue(ctrl);
>>> -	nvme_unquiesce_admin_queue(ctrl);
>>>    	nvme_auth_stop(ctrl);
>>
>> This is potentially a problem. It is possible that auth work is
>> running in the bg here, and without unquiescing the admin queue
>> the the auth_stop may take unnecessarily long until its I/O times
>> out.
> 
> There isn't any IO queued to nvme before unquiesce since nvme_cancel_*tagset
> cancels all inflight IOs.

There is a window after cancel and before unquiesce that requests can
come in.

>>
>> I think it is fine to keep it as is...
> 
> If ctrl state is LIVE, nvme_check_ready() return true, then IO is
> sent to socket before changing ctrl state to NVME_CTRL_CONNECTING.

a) the controller is not live because it was changed to RESETTING before
b) nvme_check_ready() will also fail because the queue cleared bit
NVME_TCP_Q_LIVE when the io queues stopped in
nvme_tcp_teardown_io_queues.

> So unquisce has to be done after state is changed to NVME_CTRL_CONNECTING
> if we want to move start_freeze into re-connection work.

See my response. It is perfectly safe to keep it as is.

Re: [PATCH 1/6] nvme: unquiesce io queues when removing namespaces

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 47d7ba2827ff..98fa8315bc65 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -3903,6 +3903,12 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>>>   	 */
>>>   	nvme_mpath_clear_ctrl_paths(ctrl);
>>>   
>>> +	/*
>>> +	 * Unquiesce io queues so any pending IO won't hang, especially
>>> +	 * those submitted from scan work
>>> +	 */
>>> +	nvme_unquiesce_io_queues(ctrl);
>>
>> What quiesce does this pair with?
> 
> The one done during teardown of error recovery, then removal comes and breaks
> the in-progress error recovery.

unquiesce is also safe because of NVME_CTRL_STOPPED flag. All the
drivers depend on it being this way as we unquiesce unconditionally
after reset/(re)connect.

We don't track enough state here to know if the io queues were quiesced
before or not (in the happy delete they weren't and in the unhappy
delete they were).