Re: [PATCH v2 rfc 1/3] nvme: split nvme_remove_namespaces

[Christoph Hellwig <hch@lst.de>]

I find the split rather confusing.  So I looked into alternatives
and found that the state change should just be a no-op for the PCIe
reset case.

So what about something like this ontop of your series?

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4b3bd9b85656e5..feee55903b1968 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -168,9 +168,7 @@ static void nvme_do_delete_ctrl(struct nvme_ctrl *ctrl)
 
 	flush_work(&ctrl->reset_work);
 	nvme_stop_ctrl(ctrl);
-	nvme_prep_remove_namespaces(ctrl);
-	nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING_NOIO);
-	nvme_do_remove_namespaces(ctrl);
+	nvme_remove_namespaces(ctrl);
 	ctrl->ops->delete_ctrl(ctrl);
 	nvme_uninit_ctrl(ctrl);
 }
@@ -4104,8 +4102,16 @@ static void nvme_scan_work(struct work_struct *work)
 	up_write(&ctrl->namespaces_rwsem);
 }
 
-void nvme_prep_remove_namespaces(struct nvme_ctrl *ctrl)
+/*
+ * This function iterates the namespace list unlocked to allow recovery from
+ * controller failure. It is up to the caller to ensure the namespace list is
+ * not modified by scan work while this function is executing.
+ */
+void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {
+	struct nvme_ns *ns, *next;
+	LIST_HEAD(ns_list);
+
 	/*
 	 * make sure to requeue I/O to all namespaces as these
 	 * might result from the scan itself and must complete
@@ -4124,13 +4130,9 @@ void nvme_prep_remove_namespaces(struct nvme_ctrl *ctrl)
 	 */
 	if (ctrl->state == NVME_CTRL_DEAD)
 		nvme_kill_queues(ctrl);
-}
-EXPORT_SYMBOL_GPL(nvme_prep_remove_namespaces);
 
-void nvme_do_remove_namespaces(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns, *next;
-	LIST_HEAD(ns_list);
+	/* this is a no-op when called from the controller reset handler */
+	nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING_NOIO);
 
 	down_write(&ctrl->namespaces_rwsem);
 	list_splice_init(&ctrl->namespaces, &ns_list);
@@ -4139,18 +4141,6 @@ void nvme_do_remove_namespaces(struct nvme_ctrl *ctrl)
 	list_for_each_entry_safe(ns, next, &ns_list, list)
 		nvme_ns_remove(ns);
 }
-EXPORT_SYMBOL_GPL(nvme_do_remove_namespaces);
-
-/*
- * This function iterates the namespace list unlocked to allow recovery from
- * controller failure. It is up to the caller to ensure the namespace list is
- * not modified by scan work while this function is executing.
- */
-void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
-{
-	nvme_prep_remove_namespaces(ctrl);
-	nvme_do_remove_namespaces(ctrl);
-}
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
 static int nvme_class_uevent(struct device *dev, struct kobj_uevent_env *env)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 1e74e0d62e2b11..900b35d47ec7ba 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -168,16 +168,17 @@ void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
 static bool nvme_path_is_disabled(struct nvme_ns *ns)
 {
 	/*
-	 * We don't treat NVME_CTRL_DELETING as a disabled path
-	 * as I/O should still be able to complete assuming that
-	 * the controller is connected, otherwise it'll fail
-	 * immediately and return to the requeue list. only fail
-	 * for NVME_CTRL_DELETING_NOIO
+	 * We don't treat NVME_CTRL_DELETING as a disabled path as I/O should
+	 * still be able to complete assuming that the controller is connected.
+	 * Otherwise it will fail immediately and return to the requeue list.
 	 */
-	return (ns->ctrl->state != NVME_CTRL_LIVE &&
-		ns->ctrl->state != NVME_CTRL_DELETING) ||
-		test_bit(NVME_NS_ANA_PENDING, &ns->flags) ||
-		test_bit(NVME_NS_REMOVING, &ns->flags);
+	if (ns->ctrl->state != NVME_CTRL_LIVE &&
+	    ns->ctrl->state != NVME_CTRL_DELETING)
+		return true;
+	if (test_bit(NVME_NS_ANA_PENDING, &ns->flags) ||
+	    test_bit(NVME_NS_REMOVING, &ns->flags))
+		return true;
+	return false;
 }
 
 static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2ba5d0cee6df25..c22117cd9b41e2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -552,8 +552,6 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
 void nvme_start_ctrl(struct nvme_ctrl *ctrl);
 void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_identify(struct nvme_ctrl *ctrl);
-void nvme_prep_remove_namespaces(struct nvme_ctrl *ctrl);
-void nvme_do_remove_namespaces(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
 int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0f974f932ac4e0..74cced620b0484 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2620,7 +2620,7 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->online_queues < 2) {
 		dev_warn(dev->ctrl.device, "IO queues not created\n");
 		nvme_kill_queues(&dev->ctrl);
-		nvme_remove_namespaces(&dev->ctrl);
+		nvme_remove_namespaces(ctrl);
 		nvme_free_tagset(dev);
 	} else {
 		nvme_start_queues(&dev->ctrl);
@@ -2899,9 +2899,7 @@ static void nvme_remove(struct pci_dev *pdev)
 
 	flush_work(&dev->ctrl.reset_work);
 	nvme_stop_ctrl(&dev->ctrl);
-	nvme_prep_remove_namespaces(&dev->ctrl);
-	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING_NOIO);
-	nvme_do_remove_namespaces(&dev->ctrl);
+	nvme_remove_namespaces(&dev->ctrl);
 	nvme_dev_disable(dev, true);
 	nvme_release_cmb(dev);
 	nvme_free_host_mem(dev);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme