Re: [PATCH v7 3/4] driver core: shut down devices asynchronously

From: stuart hayes
Date: Mon Jul 01 2024 - 13:57:53 EST




On 6/27/2024 12:55 AM, Christoph Hellwig wrote:
On Wed, Jun 26, 2024 at 02:46:49PM -0500, Stuart Hayes wrote:
Add code to allow asynchronous shutdown of devices, ensuring that each
device is shut down before its parents & suppliers.

Add async_shutdown_enable to struct device_driver, and expose it via sysfs.
This can be used to view or change driver opt-in to asynchronous shutdown.
Only devices with drivers that have async_shutdown_enable enabled will be
shut down asynchronously.

This can dramatically reduce system shutdown/reboot time on systems that
have multiple devices that take many seconds to shut down (like certain
NVMe drives). On one system tested, the shutdown time went from 11 minutes
without this patch to 55 seconds with the patch.

We discussed this before, but there is no summary of it and I of course
forgot the conclusion:

- why don't we do this by default?

It is done by default in this version, for devices whose drivers opt-in.

In the previous discussion, you mentioned that you thought "safe" was the
only sensible option (where "safe" was driver opt-in to async shutdown)...
that is the default (and only) option with this version. Greg K-H also
requested opt-in as well, and suggested that "on" (driver opt-out) could
be removed.

- why is it safe to user enable it?

I guess it isn't necessarily safe, if there are any drivers that can't
handle their devices shutting down asynchronously. I thought it would be
nice to be able to enable driver opt-in from user space for testing, before
changing the default setting for the driver.


+ * @shutdown_after - used during device shutdown to ensure correct shutdown ordering.

Overly long line.

+static ssize_t async_shutdown_enable_store(struct device_driver *drv, const char *buf,

.. and here.

I can correct these lines. I thought that an 80 character line length limit
was no longer required, and saw another line a few lines above these that was
even longer... and the checkpatch script didn't flag it either.