nvme-rdma: Queue ns scanning after a sucessful reconnection

RFC PATCH New Sagi Grimberg 3 participants 5 messages Jul 31, 2016 – Aug 8, 2016

Subsystems (2)

InfiniBand NVMe

lore

Share & tools

lore

Sagi Grimberg

author maintainer

2016-07-31 15:55 UTC #0 lore raw

RFC PATCH 1/1 nvme-rdma: Queue ns scanning after a sucessful reconnection

On an ordered target shutdown, the target can send a AEN on a namespace removal, this will trigger the host to queue ns-list query. The shutdown will trigger error recovery which will attepmt periodic reconnect.

We can hit a race where the ns rescanning fails (error recovery kicked in and we're not connected) causing removing all the namespaces and when we reconnect we won't see any namespaces for this controller.

So, queue a namespace rescan after we successfully reconnected to the target.

Note, that unlike user initiated controller reset, we don't need to trigger namespace scanning (until the point I noticed the above at least) because we reconnect to an existing controller. However due to the interaction with the aen mechanism we queue ns scan here as well.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
I'm open to other suggestions if anyone has any...

 drivers/nvme/host/rdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/nvme/host/rdma.c +3 -1

index f8539dd75504..5cb069ab27ed 100644
@@ -743,8 +743,10 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
 	WARN_ON_ONCE(!changed);
 
-	if (ctrl->queue_count > 1)
+	if (ctrl->queue_count > 1) {
 		nvme_start_queues(&ctrl->ctrl);
+		nvme_queue_scan(&ctrl->ctrl);
+	}
 
 	dev_info(ctrl->ctrl.device, "Successfully reconnected\n");

Signature · 1 line

1.9.1

Christoph Hellwig

maintainer

2016-08-01 11:14 UTC #1 lore raw

RFC PATCH 1/1 nvme-rdma: Queue ns scanning after a sucessful reconnection

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

J Freyensee

2016-08-04 17:08 UTC #2 lore raw

RFC PATCH 1/1 nvme-rdma: Queue ns scanning after a sucessful reconnection

In reply to Sagi Grimberg

Quoted 23 lines · Sagi Grimberg

On an ordered target shutdown, the target can send a AEN on a namespace
removal, this will trigger the host to queue ns-list query. The shutdown
will trigger error recovery which will attepmt periodic reconnect.
We can hit a race where the ns rescanning fails (error recovery kicked
in and we're not connected) causing removing all the namespaces and when
we reconnect we won't see any namespaces for this controller.
So, queue a namespace rescan after we successfully reconnected to the target.
Note, that unlike user initiated controller reset, we don't need to trigger
namespace scanning (until the point I noticed the above at least) because we
reconnect to an existing controller. However due to the interaction with
the aen mechanism we queue ns scan here as well.

Quoted 3 lines

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
I'm open to other suggestions if anyone has any...

this sounds like a fix that should really go in the core target code instead of RDMA code as this could affect any implementation layer.

If the target is shutting down I'm not sure why it would enter error recovery which would attempt a reconnect. ?If the target is shutting down, shut down. ?Maybe the keep-alive timer needs to stop (nvmet_stop_keep_alive_timer()???). I could see the benefit of the target emitting an AEN to tell the host to rescan for namespaces so it doesn't keep a stale list of namespaces after shutdown.

My 2.5 cents...

Quoted 22 lines

?drivers/nvme/host/rdma.c | 4 +++-
?1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvme/host/rdma.c +0 -0
index f8539dd75504..5cb069ab27ed 100644
@@ -743,8 +743,10 @@ static void nvme_rdma_reconnect_ctrl_work(struct
 work_struct *work)
 ?	changed = nvme_change_ctrl_state(&ctrl->ctrl,
 NVME_CTRL_LIVE);
 ?	WARN_ON_ONCE(!changed);
?
drivers/nvme/host/rdma.c +1 -1
-	if (ctrl->queue_count > 1)
+	if (ctrl->queue_count > 1) {
? nvme_start_queues(&ctrl->ctrl);
drivers/nvme/host/rdma.c +2 -0
+		nvme_queue_scan(&ctrl->ctrl);
+	}
?
? dev_info(ctrl->ctrl.device, "Successfully reconnected\n");
?

Sagi Grimberg

author maintainer

2016-08-07 09:12 UTC #3 lore raw

RFC PATCH 1/1 nvme-rdma: Queue ns scanning after a sucessful reconnection

Quoted 24 lines

Quoted 24 lines
On an ordered target shutdown, the target can send a AEN on a namespace
removal, this will trigger the host to queue ns-list query. The shutdown
will trigger error recovery which will attepmt periodic reconnect.
We can hit a race where the ns rescanning fails (error recovery kicked
in and we're not connected) causing removing all the namespaces and when
we reconnect we won't see any namespaces for this controller.
So, queue a namespace rescan after we successfully reconnected to the target.
Note, that unlike user initiated controller reset, we don't need to trigger
namespace scanning (until the point I noticed the above at least) because we
reconnect to an existing controller. However due to the interaction with
the aen mechanism we queue ns scan here as well.

Quoted 6 lines

Quoted 3 lines
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
I'm open to other suggestions if anyone has any...
this sounds like a fix that should really go in the core target code instead of RDMA code as this could affect any implementation layer.

But it fixes the host behavior (nvme-rdma).

Quoted 6 lines

If the target is shutting down I'm not sure why it would enter error recovery which would attempt a reconnect. If the target is shutting down, shut down. Maybe the keep-alive timer needs to stop (nvmet_stop_keep_alive_timer()???). I could see the benefit of the target emitting an AEN to tell the host to rescan for namespaces so it doesn't keep a stale list of namespaces after shutdown.

Its not the target entering error recovery, its the host. What would you expect from a host that suddenly was disconnected by the target? Not sure what other than periodic reconnects would be adequate...

The patch here just requeue ns scanning when we successfully reconnected to the target that was restored.

J Freyensee

2016-08-08 19:43 UTC #4 lore raw

RFC PATCH 1/1 nvme-rdma: Queue ns scanning after a sucessful reconnection

Quoted 34 lines · Sagi Grimberg

Quoted 34 lines
Quoted 33 lines
On an ordered target shutdown, the target can send a AEN on a namespace
removal, this will trigger the host to queue ns-list query. The shutdown
will trigger error recovery which will attepmt periodic reconnect.
We can hit a race where the ns rescanning fails (error recovery kicked
in and we're not connected) causing removing all the namespaces and
when
we reconnect we won't see any namespaces for this controller.
So, queue a namespace rescan after we successfully reconnected to the
target.
Note, that unlike user initiated controller reset, we don't need to
trigger
namespace scanning (until the point I noticed the above at least) because we
reconnect to an existing controller. However due to the interaction
with
the aen mechanism we queue ns scan here as well.
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
I'm open to other suggestions if anyone has any...

Quoted 5 lines

Quoted 3 lines
this sounds like a fix that should really go in the core target code
instead of RDMA code as this could affect any implementation layer.
But it fixes the host behavior (nvme-rdma).

Actually I think I meant host but didn't have enough coffee for my brain when I looked at this :-/...

OK, makes sense.