Make NVME shutdown two-pass - Version 6

Make NVME shutdown two-pass - Version 6

[Jeremy Allison]

This is version 6 of a patchset originally written by
Tanjore Suresh <tansuresh@google.com> to make shutdown
of nvme devices two-pass.

Changes from Version 5:

1). Function nvme_request_shutdown() name changed to
nvme_ctrl_shutdown_start() as requested by Sagi Grimberg.
2). Git commit message reformatting to 75 columns
as requested by Bjorn Helgaas <bhelgaas@google.com>.

NB. An alternate patchset from David Jeffery <djeffery@redhat.com>
has been proposed that covers much of the same ground.

I'm happy to collaborate on merging these two patchsets
if that is requested by the Linux nvme community.

-------------------------------------------------------------
Currently the Linux nvme driver shutdown code steps
through each connected drive, sets the NVME_CC_SHN_NORMAL
(normal shutdown) flag and then polls the given drive
waiting for the response NVME_CSTS_SHST_CMPLT flag
(shutdown complete).

Each drive is taking around 13 seconds to respond to this.

The customer has 20+ drives on the box so this time adds
up on shutdown when the nvme driver is being shut down.

This patchset changes shutdown to proceed in parallel,
so the NVME_CC_SHN_NORMAL (normal shutdown) flag is
sent to all drives first, and then it polls waiting
for the NVME_CSTS_SHST_CMPLT flag (shutdown complete)
for all drives.

In the specific customer case it reduces the NVME
shutdown time from over 300 seconds to around 15
seconds.
-------------------------------------------------------------

Jeremy Allison
CIQ / Samba Team.

[PATCH 1/5] driver core: Support two-pass driver shutdown.

[Jeremy Allison]

From: Tanjore Suresh <tansuresh@google.com>

This changes the bus driver interface with an additional entry point
to enable devices to implement two-pass shutdown. The existing
synchronous interface to shutdown is called, and if a shutdown_wait
method is defined the device is moved to an alternate list.

Once the shutdown method is called for all devices, the
shutdown_wait method is then called synchronously for
all devices on the alternate list.

Signed-off-by: Tanjore Suresh <tansuresh@google.com>
Signed-off-by: Jeremy Allison <jallison@ciq.com>
---
 drivers/base/core.c        | 35 +++++++++++++++++++++++++++++++++++
 include/linux/device/bus.h |  6 +++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14d46af40f9a..c8b0d4c9abed 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4725,6 +4725,7 @@ EXPORT_SYMBOL_GPL(device_change_owner);
 void device_shutdown(void)
 {
 	struct device *dev, *parent;
+	LIST_HEAD(shutdown_wait_list);
 
 	wait_for_device_probe();
 	device_block_probing();
@@ -4769,10 +4770,17 @@ void device_shutdown(void)
 				dev_info(dev, "shutdown_pre\n");
 			dev->class->shutdown_pre(dev);
 		}
+
 		if (dev->bus && dev->bus->shutdown) {
 			if (initcall_debug)
 				dev_info(dev, "shutdown\n");
 			dev->bus->shutdown(dev);
+			/*
+			 * Only put the device on the shutdown_wait_list
+			 * if a shutdown_wait() method is also defined.
+			 */
+			if (dev->bus->shutdown_wait)
+				list_add(&dev->kobj.entry, &shutdown_wait_list);
 		} else if (dev->driver && dev->driver->shutdown) {
 			if (initcall_debug)
 				dev_info(dev, "shutdown\n");
@@ -4789,6 +4797,33 @@ void device_shutdown(void)
 		spin_lock(&devices_kset->list_lock);
 	}
 	spin_unlock(&devices_kset->list_lock);
+
+	/*
+	 * Second pass only for devices that have configured
+	 * a shutdown_wait() method.
+	 */
+	while (!list_empty(&shutdown_wait_list)) {
+		dev = list_entry(shutdown_wait_list.next, struct device,
+				kobj.entry);
+		parent = get_device(dev->parent);
+		get_device(dev);
+		/*
+		 * Make sure the device is off the  list
+		 */
+		list_del_init(&dev->kobj.entry);
+		if (parent)
+			device_lock(parent);
+		device_lock(dev);
+		if (initcall_debug)
+			dev_info(dev,
+			"shutdown_wait called\n");
+		dev->bus->shutdown_wait(dev);
+		device_unlock(dev);
+		if (parent)
+			device_unlock(parent);
+		put_device(dev);
+		put_device(parent);
+	}
 }
 
 /*
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 5ef4ec1c36c3..a4132c44dac3 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -48,7 +48,10 @@ struct fwnode_handle;
  *		will never get called until they do.
  * @remove:	Called when a device removed from this bus.
  * @shutdown:	Called at shut-down time to quiesce the device.
- *
+ * @shutdown_wait:	If this method exists, devices are stored on a separate
+ *			list after shutdown() has been called and
+ *			shutdown_wait() is called synchronously on each device
+ *			in this list in turn.
  * @online:	Called to put the device back online (after offlining it).
  * @offline:	Called to put the device offline for hot-removal. May fail.
  *
@@ -87,6 +90,7 @@ struct bus_type {
 	void (*sync_state)(struct device *dev);
 	void (*remove)(struct device *dev);
 	void (*shutdown)(struct device *dev);
+	void (*shutdown_wait)(struct device *dev);
 
 	int (*online)(struct device *dev);
 	int (*offline)(struct device *dev);
-- 
2.39.3

[PATCH 2/5] PCI: Support two-pass shutdown

[Jeremy Allison]

From: Tanjore Suresh <tansuresh@google.com>

Enhance the base PCI driver to add support for two-pass shutdown. Add
shutdown_wait() method.

Assume a device takes n secs to shutdown. If a machine has been populated
with M such devices, the total time spent in shutting down all the devices
will be M * n secs if the shutdown is done synchronously. For example, if
NVMe PCI Controllers take 5 secs to shutdown and if there are 16 such NVMe
controllers in a system, system will spend a total of 80 secs to shutdown
all NVMe devices in that system.

In order to speed up the shutdown time, a two-pass interface to shutdown
has been implemented. The caller calls the shutdown method for each device
in turn to allow a shutdown request to be sent, then the caller walks the
list of devices and calls shutdown_wait() to synchronously wait for the
shutdown to complete.

In the NVMe case above, all 16 devices will process the shutdown in
parallel taking the total time to shutdown down to the time for one NVMe
PCI Controller to shut down.

This will significantly reduce the machine reboot time.

Signed-off-by: Tanjore Suresh <tansuresh@google.com>
Signed-off-by: Jeremy Allison <jallison@ciq.com>
---
 drivers/pci/pci-driver.c | 9 +++++++++
 include/linux/pci.h      | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 51ec9e7e784f..257bbb04c806 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -547,6 +547,14 @@ static int pci_restore_standard_config(struct pci_dev *pci_dev)
 }
 #endif /* CONFIG_PM_SLEEP */
 
+static void pci_device_shutdown_wait(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct pci_driver *drv = pci_dev->driver;
+
+	if (drv && drv->shutdown_wait)
+		drv->shutdown_wait(pci_dev);
+}
 #ifdef CONFIG_PM
 
 /* Auxiliary functions used for system resume and run-time resume */
@@ -1682,6 +1690,7 @@ struct bus_type pci_bus_type = {
 	.probe		= pci_device_probe,
 	.remove		= pci_device_remove,
 	.shutdown	= pci_device_shutdown,
+	.shutdown_wait	= pci_device_shutdown_wait,
 	.dev_groups	= pci_dev_groups,
 	.bus_groups	= pci_bus_groups,
 	.drv_groups	= pci_drv_groups,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index add9368e6314..5ef014ac84f2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -917,6 +917,7 @@ struct module;
  *		Useful for enabling wake-on-lan (NIC) or changing
  *		the power state of a device before reboot.
  *		e.g. drivers/net/e100.c.
+ * @shutdown_wait: Optional driver callback to allow two-pass shutdown.
  * @sriov_configure: Optional driver callback to allow configuration of
  *		number of VFs to enable via sysfs "sriov_numvfs" file.
  * @sriov_set_msix_vec_count: PF Driver callback to change number of MSI-X
@@ -947,6 +948,7 @@ struct pci_driver {
 	int  (*suspend)(struct pci_dev *dev, pm_message_t state);	/* Device suspended */
 	int  (*resume)(struct pci_dev *dev);	/* Device woken up */
 	void (*shutdown)(struct pci_dev *dev);
+	void (*shutdown_wait)(struct pci_dev *dev);
 	int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
 	int  (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */
 	u32  (*sriov_get_vf_total_msix)(struct pci_dev *pf);
-- 
2.39.3

Re: [PATCH 2/5] PCI: Support two-pass shutdown

[Bjorn Helgaas]

On Wed, Feb 07, 2024 at 01:40:41PM -0800, Jeremy Allison wrote:
> From: Tanjore Suresh <tansuresh@google.com>
> 
> Enhance the base PCI driver to add support for two-pass shutdown. Add
> shutdown_wait() method.
> 
> Assume a device takes n secs to shutdown. If a machine has been populated
> with M such devices, the total time spent in shutting down all the devices
> will be M * n secs if the shutdown is done synchronously. For example, if
> NVMe PCI Controllers take 5 secs to shutdown and if there are 16 such NVMe
> controllers in a system, system will spend a total of 80 secs to shutdown
> all NVMe devices in that system.
> 
> In order to speed up the shutdown time, a two-pass interface to shutdown
> has been implemented. The caller calls the shutdown method for each device
> in turn to allow a shutdown request to be sent, then the caller walks the
> list of devices and calls shutdown_wait() to synchronously wait for the
> shutdown to complete.
> 
> In the NVMe case above, all 16 devices will process the shutdown in
> parallel taking the total time to shutdown down to the time for one NVMe
> PCI Controller to shut down.
> 
> This will significantly reduce the machine reboot time.
> 
> Signed-off-by: Tanjore Suresh <tansuresh@google.com>
> Signed-off-by: Jeremy Allison <jallison@ciq.com>

I already acked this, so you can include that ack in subsequent
revisions unless you make significant changes:
https://lore.kernel.org/r/20240130175430.GA527253@bhelgaas

Thanks for rewrapping the commit log!

> ---
>  drivers/pci/pci-driver.c | 9 +++++++++
>  include/linux/pci.h      | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 51ec9e7e784f..257bbb04c806 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -547,6 +547,14 @@ static int pci_restore_standard_config(struct pci_dev *pci_dev)
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  
> +static void pci_device_shutdown_wait(struct device *dev)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	struct pci_driver *drv = pci_dev->driver;
> +
> +	if (drv && drv->shutdown_wait)
> +		drv->shutdown_wait(pci_dev);
> +}
>  #ifdef CONFIG_PM
>  
>  /* Auxiliary functions used for system resume and run-time resume */
> @@ -1682,6 +1690,7 @@ struct bus_type pci_bus_type = {
>  	.probe		= pci_device_probe,
>  	.remove		= pci_device_remove,
>  	.shutdown	= pci_device_shutdown,
> +	.shutdown_wait	= pci_device_shutdown_wait,
>  	.dev_groups	= pci_dev_groups,
>  	.bus_groups	= pci_bus_groups,
>  	.drv_groups	= pci_drv_groups,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index add9368e6314..5ef014ac84f2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -917,6 +917,7 @@ struct module;
>   *		Useful for enabling wake-on-lan (NIC) or changing
>   *		the power state of a device before reboot.
>   *		e.g. drivers/net/e100.c.
> + * @shutdown_wait: Optional driver callback to allow two-pass shutdown.
>   * @sriov_configure: Optional driver callback to allow configuration of
>   *		number of VFs to enable via sysfs "sriov_numvfs" file.
>   * @sriov_set_msix_vec_count: PF Driver callback to change number of MSI-X
> @@ -947,6 +948,7 @@ struct pci_driver {
>  	int  (*suspend)(struct pci_dev *dev, pm_message_t state);	/* Device suspended */
>  	int  (*resume)(struct pci_dev *dev);	/* Device woken up */
>  	void (*shutdown)(struct pci_dev *dev);
> +	void (*shutdown_wait)(struct pci_dev *dev);
>  	int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
>  	int  (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */
>  	u32  (*sriov_get_vf_total_msix)(struct pci_dev *pf);
> -- 
> 2.39.3
> 
> 

Re: [PATCH 2/5] PCI: Support two-pass shutdown

[Jeremy Allison]

On Wed, Feb 07, 2024 at 03:59:50PM -0600, Bjorn Helgaas wrote:
>On Wed, Feb 07, 2024 at 01:40:41PM -0800, Jeremy Allison wrote:
>> From: Tanjore Suresh <tansuresh@google.com>
>>
>> Enhance the base PCI driver to add support for two-pass shutdown. Add
>> shutdown_wait() method.
>>
>> Assume a device takes n secs to shutdown. If a machine has been populated
>> with M such devices, the total time spent in shutting down all the devices
>> will be M * n secs if the shutdown is done synchronously. For example, if
>> NVMe PCI Controllers take 5 secs to shutdown and if there are 16 such NVMe
>> controllers in a system, system will spend a total of 80 secs to shutdown
>> all NVMe devices in that system.
>>
>> In order to speed up the shutdown time, a two-pass interface to shutdown
>> has been implemented. The caller calls the shutdown method for each device
>> in turn to allow a shutdown request to be sent, then the caller walks the
>> list of devices and calls shutdown_wait() to synchronously wait for the
>> shutdown to complete.
>>
>> In the NVMe case above, all 16 devices will process the shutdown in
>> parallel taking the total time to shutdown down to the time for one NVMe
>> PCI Controller to shut down.
>>
>> This will significantly reduce the machine reboot time.
>>
>> Signed-off-by: Tanjore Suresh <tansuresh@google.com>
>> Signed-off-by: Jeremy Allison <jallison@ciq.com>
>
>I already acked this, so you can include that ack in subsequent
>revisions unless you make significant changes:
>https://lore.kernel.org/r/20240130175430.GA527253@bhelgaas
>
>Thanks for rewrapping the commit log!

Thanks, I'm still learning the right conventions. I'll
add the ack's for the next version.

[PATCH 3/5] nvme: Change 'bool shutdown' to an enum shutdown_type in nvme_dev_disable()

[Jeremy Allison]

Convert nvme_dev_disable() and nvme_disable_prepare_reset()
inside drivers/nvme/host/pci.c to use this:

bool shutdown = false == NVME_PCI_DISABLE_RESET
bool shutdown = true == (NVME_PCI_DISABLE_SHUTDOWN_SYNC ||
			 NVME_PCI_DISABLE_SHUTDOWN_TWOPASS)

The third state NVME_PCI_DISABLE_SHUTDOWN_TWOPASS
will be used later to implement two-pass PCI nvme
shutdown.

Signed-off-by: Jeremy Allison <jallison@ciq.com>
---
 drivers/nvme/host/pci.c | 47 +++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e6267a6aa380..4dfc6258de07 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -108,7 +108,14 @@ MODULE_PARM_DESC(noacpi, "disable acpi bios quirks");
 struct nvme_dev;
 struct nvme_queue;
 
-static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
+enum shutdown_type {
+	NVME_PCI_DISABLE_RESET = 0,
+	NVME_PCI_DISABLE_SHUTDOWN = 1,
+	NVME_PCI_DISABLE_SHUTDOWN_TWOPASS = 2
+};
+
+static void nvme_dev_disable(struct nvme_dev *dev,
+		enum shutdown_type shutdown_type);
 static void nvme_delete_io_queues(struct nvme_dev *dev);
 static void nvme_update_attrs(struct nvme_dev *dev);
 
@@ -1331,7 +1338,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
 			 "I/O tag %d (%04x) QID %d timeout, disable controller\n",
 			 req->tag, nvme_cid(req), nvmeq->qid);
 		nvme_req(req)->flags |= NVME_REQ_CANCELLED;
-		nvme_dev_disable(dev, true);
+		nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 		return BLK_EH_DONE;
 	case NVME_CTRL_RESETTING:
 		return BLK_EH_RESET_TIMER;
@@ -1393,7 +1400,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
 	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
 		return BLK_EH_DONE;
 
-	nvme_dev_disable(dev, false);
+	nvme_dev_disable(dev, NVME_PCI_DISABLE_RESET);
 	if (nvme_try_sched_reset(&dev->ctrl))
 		nvme_unquiesce_io_queues(&dev->ctrl);
 	return BLK_EH_DONE;
@@ -2574,11 +2581,14 @@ static bool nvme_pci_ctrl_is_dead(struct nvme_dev *dev)
 	return (csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY);
 }
 
-static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
+static void nvme_dev_disable(struct nvme_dev *dev,
+		enum shutdown_type shutdown_type)
 {
 	enum nvme_ctrl_state state = nvme_ctrl_state(&dev->ctrl);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	bool dead;
+	bool shutdown = (shutdown_type == NVME_PCI_DISABLE_SHUTDOWN ||
+			 shutdown_type == NVME_PCI_DISABLE_SHUTDOWN_TWOPASS);
 
 	mutex_lock(&dev->shutdown_lock);
 	dead = nvme_pci_ctrl_is_dead(dev);
@@ -2623,11 +2633,12 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	mutex_unlock(&dev->shutdown_lock);
 }
 
-static int nvme_disable_prepare_reset(struct nvme_dev *dev, bool shutdown)
+static int nvme_disable_prepare_reset(struct nvme_dev *dev,
+		enum shutdown_type shutdown_type)
 {
 	if (!nvme_wait_reset(&dev->ctrl))
 		return -EBUSY;
-	nvme_dev_disable(dev, shutdown);
+	nvme_dev_disable(dev, shutdown_type);
 	return 0;
 }
 
@@ -2705,7 +2716,7 @@ static void nvme_reset_work(struct work_struct *work)
 	 * moving on.
 	 */
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
-		nvme_dev_disable(dev, false);
+		nvme_dev_disable(dev, NVME_PCI_DISABLE_RESET);
 	nvme_sync_queues(&dev->ctrl);
 
 	mutex_lock(&dev->shutdown_lock);
@@ -2783,7 +2794,7 @@ static void nvme_reset_work(struct work_struct *work)
 	dev_warn(dev->ctrl.device, "Disabling device after reset failure: %d\n",
 		 result);
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
-	nvme_dev_disable(dev, true);
+	nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 	nvme_sync_queues(&dev->ctrl);
 	nvme_mark_namespaces_dead(&dev->ctrl);
 	nvme_unquiesce_io_queues(&dev->ctrl);
@@ -3075,7 +3086,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 out_disable:
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
-	nvme_dev_disable(dev, true);
+	nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 	nvme_free_host_mem(dev);
 	nvme_dev_remove_admin(dev);
 	nvme_dbbuf_dma_free(dev);
@@ -3101,7 +3112,7 @@ static void nvme_reset_prepare(struct pci_dev *pdev)
 	 * state as pci_dev device lock is held, making it impossible to race
 	 * with ->remove().
 	 */
-	nvme_disable_prepare_reset(dev, false);
+	nvme_disable_prepare_reset(dev, NVME_PCI_DISABLE_RESET);
 	nvme_sync_queues(&dev->ctrl);
 }
 
@@ -3117,7 +3128,7 @@ static void nvme_shutdown(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
 
-	nvme_disable_prepare_reset(dev, true);
+	nvme_disable_prepare_reset(dev, NVME_PCI_DISABLE_SHUTDOWN);
 }
 
 /*
@@ -3134,13 +3145,13 @@ static void nvme_remove(struct pci_dev *pdev)
 
 	if (!pci_device_is_present(pdev)) {
 		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
-		nvme_dev_disable(dev, true);
+		nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 	}
 
 	flush_work(&dev->ctrl.reset_work);
 	nvme_stop_ctrl(&dev->ctrl);
 	nvme_remove_namespaces(&dev->ctrl);
-	nvme_dev_disable(dev, true);
+	nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 	nvme_free_host_mem(dev);
 	nvme_dev_remove_admin(dev);
 	nvme_dbbuf_dma_free(dev);
@@ -3203,7 +3214,7 @@ static int nvme_suspend(struct device *dev)
 	if (pm_suspend_via_firmware() || !ctrl->npss ||
 	    !pcie_aspm_enabled(pdev) ||
 	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
-		return nvme_disable_prepare_reset(ndev, true);
+		return nvme_disable_prepare_reset(ndev, NVME_PCI_DISABLE_SHUTDOWN);
 
 	nvme_start_freeze(ctrl);
 	nvme_wait_freeze(ctrl);
@@ -3246,7 +3257,7 @@ static int nvme_suspend(struct device *dev)
 		 * Clearing npss forces a controller reset on resume. The
 		 * correct value will be rediscovered then.
 		 */
-		ret = nvme_disable_prepare_reset(ndev, true);
+		ret = nvme_disable_prepare_reset(ndev, NVME_PCI_DISABLE_SHUTDOWN);
 		ctrl->npss = 0;
 	}
 unfreeze:
@@ -3258,7 +3269,7 @@ static int nvme_simple_suspend(struct device *dev)
 {
 	struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev));
 
-	return nvme_disable_prepare_reset(ndev, true);
+	return nvme_disable_prepare_reset(ndev, NVME_PCI_DISABLE_SHUTDOWN);
 }
 
 static int nvme_simple_resume(struct device *dev)
@@ -3296,10 +3307,10 @@ static pci_ers_result_t nvme_error_detected(struct pci_dev *pdev,
 		dev_warn(dev->ctrl.device,
 			"frozen state error detected, reset controller\n");
 		if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING)) {
-			nvme_dev_disable(dev, true);
+			nvme_dev_disable(dev, NVME_PCI_DISABLE_SHUTDOWN);
 			return PCI_ERS_RESULT_DISCONNECT;
 		}
-		nvme_dev_disable(dev, false);
+		nvme_dev_disable(dev, NVME_PCI_DISABLE_RESET);
 		return PCI_ERS_RESULT_NEED_RESET;
 	case pci_channel_io_perm_failure:
 		dev_warn(dev->ctrl.device,
-- 
2.39.3

Re: [PATCH 3/5] nvme: Change 'bool shutdown' to an enum shutdown_type in nvme_dev_disable()

[Christoph Hellwig]

>  	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
> -		return nvme_disable_prepare_reset(ndev, true);
> +		return nvme_disable_prepare_reset(ndev, NVME_PCI_DISABLE_SHUTDOWN);


> -		ret = nvme_disable_prepare_reset(ndev, true);
> +		ret = nvme_disable_prepare_reset(ndev, NVME_PCI_DISABLE_SHUTDOWN);

Please avoid overly long lines.

The rest looks good to me.

[PATCH 4/5] nvme: Add a new exported function nvme_ctrl_shutdown_start()

[Jeremy Allison]

Sets the shutdown bit but doesn't wait for ready.
Use from nvme_disable_ctrl(). Export nvme_wait_ready()
so we can call it from drivers/nvme/host/pci.c.

Signed-off-by: Jeremy Allison <jallison@ciq.com>
---
 drivers/nvme/host/core.c | 22 ++++++++++++++++------
 drivers/nvme/host/nvme.h |  3 +++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 60537c9224bf..b7695f472fab 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2260,7 +2260,7 @@ const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
-static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
 		u32 timeout, const char *op)
 {
 	unsigned long timeout_jiffies = jiffies + timeout * HZ;
@@ -2286,18 +2286,28 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(nvme_wait_ready);
+
+int nvme_ctrl_shutdown_start(struct nvme_ctrl *ctrl)
+{
+	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
+	ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
+}
+EXPORT_SYMBOL_GPL(nvme_ctrl_shutdown_start);
 
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	int ret;
 
-	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
-	if (shutdown)
-		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
-	else
+	if (shutdown) {
+		ret = nvme_ctrl_shutdown_start(ctrl);
+	} else {
+		ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
 		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
+		ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
+	}
 
-	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7b87763e2f8a..f52cb3d1d9c7 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -759,6 +759,9 @@ blk_status_t nvme_host_path_error(struct request *req);
 bool nvme_cancel_request(struct request *req, void *data);
 void nvme_cancel_tagset(struct nvme_ctrl *ctrl);
 void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl);
+int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+		u32 timeout, const char *op);
+int nvme_ctrl_shutdown_start(struct nvme_ctrl *ctrl);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown);
-- 
2.39.3

Re: [PATCH 4/5] nvme: Add a new exported function nvme_ctrl_shutdown_start()

[Christoph Hellwig]

Normally we'd just say add a new helper instead of exported function.

On Wed, Feb 07, 2024 at 01:40:43PM -0800, Jeremy Allison wrote:
> Sets the shutdown bit but doesn't wait for ready.
> Use from nvme_disable_ctrl(). Export nvme_wait_ready()
> so we can call it from drivers/nvme/host/pci.c.

The nvme_wait_ready export seems unrelated to the nvme_disable_ctrl
changes.  But looking at the next patch, it seems like a helper that
wraps nvme_wait_ready with the shutdown-specific flags would seem
useful over open coding them anyway.

>  
>  int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
>  {
>  	int ret;
>  
> -	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
> -	if (shutdown)
> -		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
> -	else
> +	if (shutdown) {
> +		ret = nvme_ctrl_shutdown_start(ctrl);
> +	} else {
> +		ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
>  		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
> +		ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);

Please avoid the overly long line here.

Looking at the result after the series, nvme_disable_ctrl now has two
entirely separate code flows for the shutdown vs !shutdown case.  To me
this suggested we should just split it into a nvme_disable_ctrl that does
the non-shutdown disable, and a nvme_shutdown_ctl that does
nvme_ctrl_shutdown_start + the shutdown version of nvme_wait_ready and
let the callers do the 'if (shutdown)' for the cases where it is needed.

[PATCH 5/5] nvme: Add two-pass shutdown support

[Jeremy Allison]

This works with the two-pass shutdown mechanism setup for the PCI
drivers and participates to provide the shutdown_wait
method at the pci_driver structure level.

This patch changes the nvme shutdown() method to pass
down the NVME_PCI_DISABLE_SHUTDOWN_TWOPASS enum value instead
of NVME_PCI_DISABLE_SHUTDOWN. nvme_dev_disable() is changed
to call nvme_ctrl_shutdown_start() instead of nvme_disable_ctrl()
in this case.

nvme_ctrl_shutdown_start() sets the shutdown bit,
but does not wait for completion.

The nvme_shutdown_wait() callback is added to synchronously
wait for the NVME_CSTS_SHST_CMPLT bit meaning the nvme
device has shutdown.

This change speeds up the shutdown in a system which hosts
many controllers.

Based on work by Tanjore Suresh <tansuresh@google.com>

Signed-off-by: Jeremy Allison <jallison@ciq.com>
---
 drivers/nvme/host/pci.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4dfc6258de07..5cf452651091 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2607,7 +2607,14 @@ static void nvme_dev_disable(struct nvme_dev *dev,
 
 	if (!dead && dev->ctrl.queue_count > 0) {
 		nvme_delete_io_queues(dev);
-		nvme_disable_ctrl(&dev->ctrl, shutdown);
+		/*
+		 * NVME_PCI_DISABLE_SHUTDOWN_TWOPASS requests shutdown
+		 * but doesn't wait for completion.
+		 */
+		if (shutdown_type == NVME_PCI_DISABLE_SHUTDOWN_TWOPASS)
+			nvme_ctrl_shutdown_start(&dev->ctrl);
+		else
+			nvme_disable_ctrl(&dev->ctrl, shutdown);
 		nvme_poll_irqdisable(&dev->queues[0]);
 	}
 	nvme_suspend_io_queues(dev);
@@ -2625,7 +2632,7 @@ static void nvme_dev_disable(struct nvme_dev *dev,
 	 * must flush all entered requests to their failed completion to avoid
 	 * deadlocking blk-mq hot-cpu notifier.
 	 */
-	if (shutdown) {
+	if (shutdown_type == NVME_PCI_DISABLE_SHUTDOWN) {
 		nvme_unquiesce_io_queues(&dev->ctrl);
 		if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q))
 			nvme_unquiesce_admin_queue(&dev->ctrl);
@@ -3128,7 +3135,32 @@ static void nvme_shutdown(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
 
-	nvme_disable_prepare_reset(dev, NVME_PCI_DISABLE_SHUTDOWN);
+	nvme_disable_prepare_reset(dev, NVME_PCI_DISABLE_SHUTDOWN_TWOPASS);
+}
+
+static void nvme_shutdown_wait(struct pci_dev *pdev)
+{
+	struct nvme_dev *dev = pci_get_drvdata(pdev);
+
+	mutex_lock(&dev->shutdown_lock);
+	/*
+	 * Finish waiting for the shutdown request
+	 * initiated in nvme_shutdown() above using
+	 * NVME_PCI_DISABLE_SHUTDOWN_TWOPASS.
+	 */
+	nvme_wait_ready(&dev->ctrl, NVME_CSTS_SHST_MASK,
+			NVME_CSTS_SHST_CMPLT,
+			dev->ctrl.shutdown_timeout, "shutdown");
+	/*
+	 * The driver will not be starting up queues again if shutting down so
+	 * must flush all entered requests to their failed completion to avoid
+	 * deadlocking blk-mq hot-cpu notifier.
+	 */
+	nvme_unquiesce_io_queues(&dev->ctrl);
+	if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q))
+		nvme_unquiesce_admin_queue(&dev->ctrl);
+
+	mutex_unlock(&dev->shutdown_lock);
 }
 
 /*
@@ -3522,6 +3554,7 @@ static struct pci_driver nvme_driver = {
 	.probe		= nvme_probe,
 	.remove		= nvme_remove,
 	.shutdown	= nvme_shutdown,
+	.shutdown_wait	= nvme_shutdown_wait,
 	.driver		= {
 		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
 #ifdef CONFIG_PM_SLEEP
-- 
2.39.3

Re: [PATCH 5/5] nvme: Add two-pass shutdown support

[Christoph Hellwig]

> +		/*
> +		 * NVME_PCI_DISABLE_SHUTDOWN_TWOPASS requests shutdown
> +		 * but doesn't wait for completion.
> +		 */

Nit: please use up the normal 80 character line length for comments if
possible.  Same for at least one other comment further down.

Otherwise this looks good.

Re: Make NVME shutdown two-pass - Version 6

[Bjorn Helgaas]

On Wed, Feb 07, 2024 at 01:40:39PM -0800, Jeremy Allison wrote:
> This is version 6 of a patchset originally written by
> Tanjore Suresh <tansuresh@google.com> to make shutdown
> of nvme devices two-pass.
> 
> Changes from Version 5:
> 
> 1). Function nvme_request_shutdown() name changed to
> nvme_ctrl_shutdown_start() as requested by Sagi Grimberg.
> 2). Git commit message reformatting to 75 columns
> as requested by Bjorn Helgaas <bhelgaas@google.com>.
> 
> NB. An alternate patchset from David Jeffery <djeffery@redhat.com>
> has been proposed that covers much of the same ground.

Would be useful to have a link to David's patchset for comparison.

Re: Make NVME shutdown two-pass - Version 6

[Jeremy Allison]

On Wed, Feb 07, 2024 at 04:00:48PM -0600, Bjorn Helgaas wrote:
>On Wed, Feb 07, 2024 at 01:40:39PM -0800, Jeremy Allison wrote:
>> This is version 6 of a patchset originally written by
>> Tanjore Suresh <tansuresh@google.com> to make shutdown
>> of nvme devices two-pass.
>>
>> Changes from Version 5:
>>
>> 1). Function nvme_request_shutdown() name changed to
>> nvme_ctrl_shutdown_start() as requested by Sagi Grimberg.
>> 2). Git commit message reformatting to 75 columns
>> as requested by Bjorn Helgaas <bhelgaas@google.com>.
>>
>> NB. An alternate patchset from David Jeffery <djeffery@redhat.com>
>> has been proposed that covers much of the same ground.
>
>Would be useful to have a link to David's patchset for comparison.

Sorry, here it is:

https://lore.kernel.org/linux-nvme/20240207184100.18066-1-djeffery@redhat.com/

Re: Make NVME shutdown two-pass - Version 6

[David Jeffery]

On Wed, Feb 7, 2024 at 5:03 PM Jeremy Allison <jra@samba.org> wrote:
>
> On Wed, Feb 07, 2024 at 04:00:48PM -0600, Bjorn Helgaas wrote:
> >On Wed, Feb 07, 2024 at 01:40:39PM -0800, Jeremy Allison wrote:
> >> This is version 6 of a patchset originally written by
> >> Tanjore Suresh <tansuresh@google.com> to make shutdown
> >> of nvme devices two-pass.
...
> >>
> >> NB. An alternate patchset from David Jeffery <djeffery@redhat.com>
> >> has been proposed that covers much of the same ground.
> >
> >Would be useful to have a link to David's patchset for comparison.
>
> Sorry, here it is:
>
> https://lore.kernel.org/linux-nvme/20240207184100.18066-1-djeffery@redhat.com/
>

Hi Jeremy,

I'm fine with merging our efforts. From reviewing your patch set, the
main problem I have is with the core patch compared to my initial 2
patches. Yours runs shutdown_wait calls after shutdown for all
devices. This design won't work for example with the scsi system sd
async shutdown. Scsi HBA shutdown calls may stop the HBA or firmware,
leaving the async flush for sd in an unknowable state as to if the
command succeeded or even made it to storage. My patches by ensuring
all shutdown for child devices is finished before a parent device's
shutdown may be called allows the ordering to work.

I'm not attached to a particular async API. If nvme and core devs are
fine with the shutdown_wait api, I can alter my core shutdown and
scsi/sd patches to match.

David Jeffery

Re: Make NVME shutdown two-pass - Version 6

[Jeremy Allison]

On Thu, Feb 08, 2024 at 01:02:37PM -0500, David Jeffery wrote:
>
>Hi Jeremy,
>
>I'm fine with merging our efforts. From reviewing your patch set, the
>main problem I have is with the core patch compared to my initial 2
>patches. Yours runs shutdown_wait calls after shutdown for all
>devices. This design won't work for example with the scsi system sd
>async shutdown. Scsi HBA shutdown calls may stop the HBA or firmware,
>leaving the async flush for sd in an unknowable state as to if the
>command succeeded or even made it to storage. My patches by ensuring
>all shutdown for child devices is finished before a parent device's
>shutdown may be called allows the ordering to work.
>
>I'm not attached to a particular async API. If nvme and core devs are
>fine with the shutdown_wait api, I can alter my core shutdown and
>scsi/sd patches to match.
>
>David Jeffery

Great point David, thanks ! I wasn't testing with the scsi system
shutdown, only PCI nvme. I didn't know the scsi system suffered
from slow shutdown also.

As I've already done the work to get the bus driver and nvme devs
to agree on the shutdown()/shutdown_wait() two-pass interface
(and they have already signed off on that), I'd suggest keeping
that interface.

Looks like the key change is the addition of the ability of
a device to mark its parent as busy to keep the parent-child
ordering.

If you're willing to merge your patches and required fixes
for the scsi system on top of the new agreed on API then I'll
be happy to help review and test any updated patchsets to make
sure my nvme PCI case gets fixed too. It might be easiest to
add in the scsi changes needed on top of my existing working
code for nvme/PCI (I've already had confirmation from an nvme
list user that my patchset improves his shutdown times
significantly) but I'll let you make the decisions on that.
I'm not wedded to any specific code :-).

Thanks for collaborating on this.

Cheers,

	Jeremy.