Re: [PATCH] scsi: sd: stop SSD (non-rotational) disks before reboot

[Damien Le Moal]

On 2020/06/18 3:50, Simon Arlott wrote:
> I need to use "reboot=p" on my desktop because one of the PCIe devices
> does not appear after a warm boot. This results in a very cold boot
> because the BIOS turns the PSU off and on.
> 
> The scsi sd shutdown process does not send a stop command to disks
> before the reboot happens (stop commands are only sent for a shutdown).
> 
> The result is that all of my SSDs experience a sudden power loss on
> every reboot, which is undesirable behaviour. These events are recorded
> in the SMART attributes.

Why is it undesirable for an SSD ? The sequence you are describing is not
different from doing "shutdown -h now" and then pressing down the power button
again immediately after power is cut...
Are you experiencing data loss or corruption ? If yes, since a clean reboot or
shutdown issues a synchronize cache to all devices, a corruption would mean that
your SSD is probably not correctly processing flush cache commands.

> Avoiding a stop of the disk on a reboot is appropriate for HDDs because
> they're likely to continue to be powered (and should not be told to spin
> down only to spin up again) but the default behaviour for SSDs should
> be changed to stop them before the reboot.

If your BIOS turns the PSU down and up, then the HDDs too will lose power... The
difference will be that the disks will still be spinning from inertia on the
power up, and so the HDD spin up processing will be faster than for a pure cold
boot sequence.

> 
> Add a "stop_before_reboot" module parameter that can be used to control
> the shutdown behaviour of disks before a reboot. The default will be
> to stop non-rotational disks (SSDs) only, but it can be configured to
> stop all disks if it is known that power will be lost completely on a
> reboot.
> 
>   sd_mod.stop_before_reboot=<integer>
>     0 = never
>     1 = non-rotational disks only (default)
>     2 = all disks
> 
> The behaviour on shutdown is unchanged: all disks are unconditionally
> stopped.
> 
> The disk I/O will be mostly quiescent at reboot time (and there is a
> sync first) but this should be added to stable kernels to protect all
> SSDs from unexpected power loss during a reboot by default. There is
> the potential for an unexpected power loss to corrupt data depending
> on the SSD model/firmware.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Simon Arlott <simon@xxxxxxxxxxx>
> ---
>  Documentation/scsi/scsi-parameters.rst |  7 +++++++
>  drivers/scsi/sd.c                      | 22 +++++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/scsi/scsi-parameters.rst b/Documentation/scsi/scsi-parameters.rst
> index 9aba897c97ac..fd64d0d43861 100644
> --- a/Documentation/scsi/scsi-parameters.rst
> +++ b/Documentation/scsi/scsi-parameters.rst
> @@ -101,6 +101,13 @@ parameters may be changed at runtime by the command
>  			allowing boot to proceed.  none ignores them, expecting
>  			user space to do the scan.
>  
> +	sd_mod.stop_before_reboot=
> +			[SCSI] configure stop action for disks before a reboot
> +			Format: <integer>
> +			0 = never
> +			1 = non-rotational disks only (default)
> +			2 = all disks> +
>  	sim710=		[SCSI,HW]
>  			See header of drivers/scsi/sim710.c.
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d90fefffe31b..1cd652e037ab 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -98,6 +98,12 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_MOD);
>  MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);
>  MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);
>  
> +static unsigned int stop_before_reboot = 1;
> +
> +module_param(stop_before_reboot, uint, 0644);
> +MODULE_PARM_DESC(stop_before_reboot,
> +		 "stop disks before reboot (1=non-rotational, 2=all)");
> +
>  #if !defined(CONFIG_DEBUG_BLOCK_EXT_DEVT)
>  #define SD_MINORS	16
>  #else
> @@ -3576,9 +3582,19 @@ static void sd_shutdown(struct device *dev)
>  		sd_sync_cache(sdkp, NULL);
>  	}
>  
> -	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> -		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
> -		sd_start_stop_device(sdkp, 0);
> +	if (sdkp->device->manage_start_stop) {
> +		bool stop_disk = (system_state != SYSTEM_RESTART);
> +
> +		if (stop_before_reboot > 1) { /* stop all disks */
> +			stop_disk = true;
> +		} else if (stop_before_reboot) { /* non-rotational only */
> +			stop_disk |= blk_queue_nonrot(sdkp->disk->queue);
> +		}> +
> +		if (stop_disk) {
> +			sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
> +			sd_start_stop_device(sdkp, 0);
> +		}
>  	}
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research