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

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

[Simon Arlott]

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.

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.

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@vger.kernel.org
Signed-off-by: Simon Arlott <simon@octiron.net>
---
 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);
+		}
 	}
 }
 
-- 
2.17.1

-- 
Simon Arlott

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

[Bart Van Assche]

On 2020-06-17 11:49, Simon Arlott wrote:
> @@ -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);
> +		}
>  	}
>  }

Is introduction of a new kernel module parameter essential? Or in other
words, has it been considered to apply the new behavior to all SSDs?

Thanks,

Bart.

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

[Simon Arlott]

On 17/06/2020 20:19, Bart Van Assche wrote:
> On 2020-06-17 11:49, Simon Arlott wrote:
>> @@ -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);
>> +		}
>>  	}
>>  }
> 
> Is introduction of a new kernel module parameter essential?

It is system-dependent whether or not a reboot is going to result in a
loss of power, so it's required to be able to stop the HDDs too.

They're already always stopped on shutdown where power is definitely
going to be lost. I can't do that by default on a reboot because the
usual convention is that the power stays on during a reboot and it would
be expected that the HDDs keep spinning.

> Or in other
> words, has it been considered to apply the new behavior to all SSDs?

The default value is 1, so it does apply to all SSDs. I want to be able
to configure it to apply to HDDs too.

-- 
Simon Arlott

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

[Christoph Hellwig]

On Wed, Jun 17, 2020 at 07:49:57PM +0100, Simon Arlott wrote:
> 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.

I don't think that is true in general.  At least for most current server
class and older desktop and laptop class systems they use the same
format factors and enclosures, although they are slightly divering now.

So I think this needs to be quirked based on the platform and/or
enclosure.

I don't have ATA SSDs any more, but at least in my laptops the NVMe
SSDs do see unsafe shutdowns during reboots.

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@vger.kernel.org
> Signed-off-by: Simon Arlott <simon@octiron.net>
> ---
>  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

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

[Simon Arlott]

On 18/06/2020 08:21, Christoph Hellwig wrote:
> On Wed, Jun 17, 2020 at 07:49:57PM +0100, Simon Arlott wrote:
>> 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.
> 
> I don't think that is true in general.  At least for most current server
> class and older desktop and laptop class systems they use the same
> format factors and enclosures, although they are slightly divering now.
> 
> So I think this needs to be quirked based on the platform and/or
> enclosure.

Are you referring to the behaviour for handling HDDs or SSDs?

For HDDs, the default "1" option could mean "automatic" and apply to
rotational disks when power loss is expected.

For SSDs, I don't think an extra stop should ever be an issue.

-- 
Simon Arlott

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

[Simon Arlott]

On 18/06/2020 09:36, Damien Le Moal wrote:
> 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...

On a shutdown the kernel will send a stop command to the SSD. It does
not currently do this for a reboot so I observe the unexpected power
loss counters increasing.

> 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.

No, I'm not experiencing any data loss or corruption that I'm aware of.

We can argue whether or not any given SSD correctly processes commands
to flush the cache, but they are expecting to be stopped before power
is removed.

>> 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.

I haven't verified it, but the BIOS leaves the power off for several
seconds which should be long enough for the HDDs to spin down.

I'm less concerned about those suddenly losing power but it would be
nice to have a stop command sent to them too.

-- 
Simon Arlott

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

[Christoph Hellwig]

On Thu, Jun 18, 2020 at 01:25:18PM +0100, Simon Arlott wrote:
> On 18/06/2020 08:21, Christoph Hellwig wrote:
> > On Wed, Jun 17, 2020 at 07:49:57PM +0100, Simon Arlott wrote:
> >> 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.
> > 
> > I don't think that is true in general.  At least for most current server
> > class and older desktop and laptop class systems they use the same
> > format factors and enclosures, although they are slightly divering now.
> > 
> > So I think this needs to be quirked based on the platform and/or
> > enclosure.
> 
> Are you referring to the behaviour for handling HDDs or SSDs?

All of the above.

> 
> For HDDs, the default "1" option could mean "automatic" and apply to
> rotational disks when power loss is expected.
> 
> For SSDs, I don't think an extra stop should ever be an issue.

Extra shutdowns will usually cause additional P/E cycles.

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

[Damien Le Moal]

On 2020/06/18 21:26, Simon Arlott wrote:
> On 18/06/2020 09:36, Damien Le Moal wrote:
>> 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...
> 
> On a shutdown the kernel will send a stop command to the SSD. It does
> not currently do this for a reboot so I observe the unexpected power
> loss counters increasing.
> 
>> 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.
> 
> No, I'm not experiencing any data loss or corruption that I'm aware of.
> 
> We can argue whether or not any given SSD correctly processes commands
> to flush the cache, but they are expecting to be stopped before power
> is removed.
> 
>>> 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.
> 
> I haven't verified it, but the BIOS leaves the power off for several
> seconds which should be long enough for the HDDs to spin down.
> 
> I'm less concerned about those suddenly losing power but it would be
> nice to have a stop command sent to them too.

OK. So maybe the patch should be as simple as changing SYSTEM_RESTART state to
SYSTEM_POWER_OFF if reboot=p is set, no ? Since that is consistent with the fact
that reboot=p will cause power to go off, exactly the same as a regular
shutdown, it seems cleaner and safer to use SYSTEM_POWER_OFF for the entire
system, not just scsi disks.

Thoughts ?

-- 
Damien Le Moal
Western Digital Research

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

[Pavel Machek]

Hi!

> > 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...

Many SSDs are buggy, and will eventually corrupt themselves if you do enough
sudden power loss experiments.

HDDs don't like their power cut, either. You can hear the difference 
between normal power off and power cut...

> > Cc: stable@vger.kernel.org

This needs lot more testing before going to stable.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Henrique de Moraes Holschuh]

On Thu, 18 Jun 2020, Damien Le Moal wrote:
> 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.

Cache flushes do not matter that much when SSDs and sudden power cuts
are involved.  Power cuts at the wrong time harm the FLASH itself, it is
not about still-in-flight data.

Keep in mind that SSDs do a _lot_ of background writing, and power cuts
during a FLASH write or erase can cause from weakened cells, to much
larger damage.  It is possible to harden the chip or the design against
this, but it is *expensive*.  And even if warded off by hardening and no
FLASH damage happens, an erase/program cycle must be done on the whole
erase block to clean up the incomplete program cycle.

Due to this background activity, an unexpected power cut could damage
data *anywhere* in an SSD: it could hit some filesystem area that was
being scrubbed in background by the SSD, or internal SSD metadata.

So, you want that SSD to know it must be quiescent-for-poweroff for
*real* before you allow the system to do anything that could power it
off.

And, as I have found out the hard way years ago, you also want to give
the SSD enough *extra* time to actually quiesce, even if it claims to be
already prepared for poweroff [1].

When you do not follow these rules, well, excellent datacenter-class
SSDs have super-capacitor power banks that actually work.  Most SSDs do
not, although they hopefully came a long way and hopefully modern SSDs
are not as easily to brick as they were reported to be three or four
years ago.


[1] I have long lost the will and energy to pursue this, so *this* is a
throw-away anecdote for anyone that cares: I reported here a few years
ago that many models of *SATA* based SSDs from Crucial/Micron, Samsung
and Intel were complaining (through their SMART attributes) that Linux
was causing unsafe shutdowns.

https://lkml.org/lkml/2017/4/10/1181

TL;DR: wait one *extra* second after the SSD acknowleged the STOP
command as complete before you trust the SSD device is safe to be
powered down (i.e. before reboot, suspend, poweroff/shutdown, and device
removal/detach).  This worked around the issue for every vendor and
model of SSD we tested.

-- 
  Henrique Holschuh

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

[Simon Arlott]

On 23/06/2020 14:36, Pavel Machek wrote:
> Many SSDs are buggy, and will eventually corrupt themselves if you do enough
> sudden power loss experiments.
> 
> HDDs don't like their power cut, either. You can hear the difference 
> between normal power off and power cut...

I will change the patch so that it doesn't distinguish between types of
disks.

The default will have to be the existing behaviour (don't stop disks)
because most reboots shouldn't result in a loss of power.

-- 
Simon Arlott

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

[Simon Arlott]

On 19/06/2020 00:31, Damien Le Moal wrote:
> On 2020/06/18 21:26, Simon Arlott wrote:
>> I haven't verified it, but the BIOS leaves the power off for several
>> seconds which should be long enough for the HDDs to spin down.
>> 
>> I'm less concerned about those suddenly losing power but it would be
>> nice to have a stop command sent to them too.
> 
> OK. So maybe the patch should be as simple as changing SYSTEM_RESTART state to
> SYSTEM_POWER_OFF if reboot=p is set, no ? Since that is consistent with the fact
> that reboot=p will cause power to go off, exactly the same as a regular
> shutdown, it seems cleaner and safer to use SYSTEM_POWER_OFF for the entire
> system, not just scsi disks.

That could be a bit misleading because the power isn't going to stay
off. Some of the network drivers have specific WOL behaviour changes
for a power off.

Power cycling the PSU is not something that every BIOS will do, so it's
not that simple. It could be a module parameter but I'd be concerned
that some other code will assuming the system should be powered off and
all of my reboots will become power off events.

-- 
Simon Arlott

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

[Simon Arlott]

On 23/06/2020 21:42, Henrique de Moraes Holschuh wrote:
> [1] I have long lost the will and energy to pursue this, so *this* is a
> throw-away anecdote for anyone that cares: I reported here a few years
> ago that many models of *SATA* based SSDs from Crucial/Micron, Samsung
> and Intel were complaining (through their SMART attributes) that Linux
> was causing unsafe shutdowns.
> 
> https://lkml.org/lkml/2017/4/10/1181
> 
> TL;DR: wait one *extra* second after the SSD acknowleged the STOP
> command as complete before you trust the SSD device is safe to be
> powered down (i.e. before reboot, suspend, poweroff/shutdown, and device
> removal/detach).  This worked around the issue for every vendor and
> model of SSD we tested.

Looking through that thread, it looks like a simple 1 second delay on
shutdown/reboot patch hasn't been proposed yet?

In my case none of the SSDs are recording unexpected power loss if they
are stopped before the reboot, but the reboot won't necessarily be
instantaneous after the last stop command returns.

-- 
Simon Arlott

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

[Henrique de Moraes Holschuh]

On Sun, 28 Jun 2020, Simon Arlott wrote:
> On 23/06/2020 21:42, Henrique de Moraes Holschuh wrote:
> > [1] I have long lost the will and energy to pursue this, so *this* is a
> > throw-away anecdote for anyone that cares: I reported here a few years
> > ago that many models of *SATA* based SSDs from Crucial/Micron, Samsung
> > and Intel were complaining (through their SMART attributes) that Linux
> > was causing unsafe shutdowns.
> > 
> > https://lkml.org/lkml/2017/4/10/1181
> > 
> > TL;DR: wait one *extra* second after the SSD acknowleged the STOP
> > command as complete before you trust the SSD device is safe to be
> > powered down (i.e. before reboot, suspend, poweroff/shutdown, and device
> > removal/detach).  This worked around the issue for every vendor and
> > model of SSD we tested.
> 
> Looking through that thread, it looks like a simple 1 second delay on
> shutdown/reboot patch hasn't been proposed yet?

It should work, yes.  And it likely would help with whatever $RANDOM
other hardware that has the same issues but has no way to make itself
noticed, so *I* would appreciate it as something I could tell the kernel
to *always* do.

But for "sd" devices, it would be likely more complete to also ensure
the delay for device removal (not just on reboot and power off).

> In my case none of the SSDs are recording unexpected power loss if they
> are stopped before the reboot, but the reboot won't necessarily be
> instantaneous after the last stop command returns.

Yes, it is a race.  If either the SSD happens to need less "extra" time,
or the computer takes a bit longer to reboot/power off, all is well.
Otherwise, the SSD loses the race, and gets powered down at an
inappropriate time.

-- 
  Henrique Holschuh

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

[Damien Le Moal]

On 2020/06/29 3:23, Simon Arlott wrote:
> On 19/06/2020 00:31, Damien Le Moal wrote:
>> On 2020/06/18 21:26, Simon Arlott wrote:
>>> I haven't verified it, but the BIOS leaves the power off for several
>>> seconds which should be long enough for the HDDs to spin down.
>>>
>>> I'm less concerned about those suddenly losing power but it would be
>>> nice to have a stop command sent to them too.
>>
>> OK. So maybe the patch should be as simple as changing SYSTEM_RESTART state to
>> SYSTEM_POWER_OFF if reboot=p is set, no ? Since that is consistent with the fact
>> that reboot=p will cause power to go off, exactly the same as a regular
>> shutdown, it seems cleaner and safer to use SYSTEM_POWER_OFF for the entire
>> system, not just scsi disks.
> 
> That could be a bit misleading because the power isn't going to stay
> off. Some of the network drivers have specific WOL behaviour changes
> for a power off.

The point is that the power goes off, same as for a SYSTEM_POWER_OFF shutdown.
It do not think it matters how long it will be off before your BIOS restarts the
laptop power. And actually enabling the NICs WOL function would I think actually
be a very good thing: if the PSU power cycling fails, the machine can still be
remotely woken-up as configured by the user.

> Power cycling the PSU is not something that every BIOS will do, so it's
> not that simple. It could be a module parameter but I'd be concerned
> that some other code will assuming the system should be powered off and
> all of my reboots will become power off events.

Or define a SYSTEM_RESTART_P that essentially does what SYSTEM_POWER_OFF does +
the addition of telling the BIOS to restart the PSU, if the machines supports
it. What happens if one does a reboot=p on a machine that does not support it ?
Does this become a shutdown, or does it become a normal reset ?


-- 
Damien Le Moal
Western Digital Research

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

[Ming Lei]

On Wed, Jun 24, 2020 at 5:01 AM Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
>
> On Thu, 18 Jun 2020, Damien Le Moal wrote:
> > 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.
>
> Cache flushes do not matter that much when SSDs and sudden power cuts
> are involved.  Power cuts at the wrong time harm the FLASH itself, it is
> not about still-in-flight data.
>
> Keep in mind that SSDs do a _lot_ of background writing, and power cuts

What is the __lot__ of SSD's BG writing? GC?

> during a FLASH write or erase can cause from weakened cells, to much
> larger damage.  It is possible to harden the chip or the design against
> this, but it is *expensive*.  And even if warded off by hardening and no
> FLASH damage happens, an erase/program cycle must be done on the whole
> erase block to clean up the incomplete program cycle.

It should have been SSD's(including FW) responsibility to avoid data loss when
the SSD is doing its own BG writing, because power cut can happen any time
from SSD's viewpoint.

>
> Due to this background activity, an unexpected power cut could damage
> data *anywhere* in an SSD: it could hit some filesystem area that was
> being scrubbed in background by the SSD, or internal SSD metadata.
>
> So, you want that SSD to know it must be quiescent-for-poweroff for
> *real* before you allow the system to do anything that could power it
> off.
>
> And, as I have found out the hard way years ago, you also want to give
> the SSD enough *extra* time to actually quiesce, even if it claims to be
> already prepared for poweroff [1].
>
> When you do not follow these rules, well, excellent datacenter-class
> SSDs have super-capacitor power banks that actually work.  Most SSDs do
> not, although they hopefully came a long way and hopefully modern SSDs
> are not as easily to brick as they were reported to be three or four
> years ago.

I remember that DC SSDs often don't support BG GC.


Thanks,
Ming Lei

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Hi!

> > during a FLASH write or erase can cause from weakened cells, to much
> > larger damage.  It is possible to harden the chip or the design against
> > this, but it is *expensive*.  And even if warded off by hardening and no
> > FLASH damage happens, an erase/program cycle must be done on the whole
> > erase block to clean up the incomplete program cycle.
> 
> It should have been SSD's(including FW) responsibility to avoid data loss when
> the SSD is doing its own BG writing, because power cut can happen any time
> from SSD's viewpoint.

It should be their responsibility. But we know how well that works
(not well), so we try hard (and should try hard) to power SSDs down
cleanly.

In a similar way, it is ext4's responsibility not to corrupt itself,
and we still prefer clean shutdowns.

Plus, HDDs normally do handle unexpected power offs well, but it puts
extra stress on their hardware, so we should avoid that...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Laight]

From: Pavel Machek
> Sent: 02 July 2020 22:17
> > > during a FLASH write or erase can cause from weakened cells, to much
> > > larger damage.  It is possible to harden the chip or the design against
> > > this, but it is *expensive*.  And even if warded off by hardening and no
> > > FLASH damage happens, an erase/program cycle must be done on the whole
> > > erase block to clean up the incomplete program cycle.
> >
> > It should have been SSD's(including FW) responsibility to avoid data loss when
> > the SSD is doing its own BG writing, because power cut can happen any time
> > from SSD's viewpoint.
> 
> It should be their responsibility. But we know how well that works
> (not well), so we try hard (and should try hard) to power SSDs down
> cleanly.

I hope modern SSD disks are better than very old CF drives.

I had one where the entire contents got scrambled after an unexpected
power removal.
I suspect it was in the middle of a 'wear levelling' activity.
Even though it was only a FAT filesystem I was glad I didn't
actually need to recover any of the data.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

Hi!

> > Sent: 02 July 2020 22:17
> > > > during a FLASH write or erase can cause from weakened cells, to much
> > > > larger damage.  It is possible to harden the chip or the design against
> > > > this, but it is *expensive*.  And even if warded off by hardening and no
> > > > FLASH damage happens, an erase/program cycle must be done on the whole
> > > > erase block to clean up the incomplete program cycle.
> > >
> > > It should have been SSD's(including FW) responsibility to avoid data loss when
> > > the SSD is doing its own BG writing, because power cut can happen any time
> > > from SSD's viewpoint.
> > 
> > It should be their responsibility. But we know how well that works
> > (not well), so we try hard (and should try hard) to power SSDs down
> > cleanly.
> 
> I hope modern SSD disks are better than very old CF drives.

Testing showed there were not few yars ago.

> I had one where the entire contents got scrambled after an unexpected
> power removal.

If you have SSD you are willing to kill, I believe you can get to same
result with a bit of patience.

Best regards,
								Pavel-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Henrique de Moraes Holschuh]

On Thu, 18 Jun 2020, Christoph Hellwig wrote:
> > For SSDs, I don't think an extra stop should ever be an issue.
> 
> Extra shutdowns will usually cause additional P/E cycles.

I am not so sure.  We're talking about enforcing clean shutdowns here
(from the SSD PoV).

A system reboot takes enough time that the SSD is likely to do about the
same amount of P cycles commiting to FLASH any important data that it
would trigger by a shutdown sequence, simply because it should not keep
important data in RAM for too long.  By extension, it would not increase
E cycles either.

OTOH, unclean shutdowns *always* cause extra P/E, and that's if you're
lucky enough for it to not cause anything much worse.

-- 
  Henrique Holschuh

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

[Henrique de Moraes Holschuh]

On Tue, 30 Jun 2020, Ming Lei wrote:
> On Wed, Jun 24, 2020 at 5:01 AM Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > Cache flushes do not matter that much when SSDs and sudden power cuts
> > are involved.  Power cuts at the wrong time harm the FLASH itself, it is
> > not about still-in-flight data.
> >
> > Keep in mind that SSDs do a _lot_ of background writing, and power cuts
> 
> What is the __lot__ of SSD's BG writing? GC?

GC, and scrubbing.

> > during a FLASH write or erase can cause from weakened cells, to much
> > larger damage.  It is possible to harden the chip or the design against
> > this, but it is *expensive*.  And even if warded off by hardening and no
> > FLASH damage happens, an erase/program cycle must be done on the whole
> > erase block to clean up the incomplete program cycle.
> 
> It should have been SSD's(including FW) responsibility to avoid data loss when
> the SSD is doing its own BG writing, because power cut can happen any time
> from SSD's viewpoint.

Oh, I fully agree.  And yet, we had devices from several large vendors
complaining about unclean shutdowns.  So, "it should have been", as
usual, amounts to very little in the end.

> > When you do not follow these rules, well, excellent datacenter-class
> > SSDs have super-capacitor power banks that actually work.  Most SSDs do
> > not, although they hopefully came a long way and hopefully modern SSDs
> > are not as easily to brick as they were reported to be three or four
> > years ago.
> 
> I remember that DC SSDs often don't support BG GC.

And have proper supercap local power banks, etc.  I'd say they're not
really relevant to this thread.

-- 
  Henrique Holschuh

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

[Christoph Hellwig]

On Sun, Jul 05, 2020 at 06:31:25PM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Jun 2020, Christoph Hellwig wrote:
> > > For SSDs, I don't think an extra stop should ever be an issue.
> > 
> > Extra shutdowns will usually cause additional P/E cycles.
> 
> I am not so sure.  We're talking about enforcing clean shutdowns here
> (from the SSD PoV).
> 
> A system reboot takes enough time that the SSD is likely to do about the
> same amount of P cycles commiting to FLASH any important data that it
> would trigger by a shutdown sequence, simply because it should not keep
> important data in RAM for too long.  By extension, it would not increase
> E cycles either.
> 
> OTOH, unclean shutdowns *always* cause extra P/E, and that's if you're
> lucky enough for it to not cause anything much worse.

The point is - with a normal system that doesn't required your odd
reboot method we'll normally not shut down the SSD at all, and that
won't require a P/E cycle.

But the whole thing is a moot point - if you quirk your system to
require a poweroff to reboot the kernel should trat it as a power off
as far as shutdown/remove callbacks are concerned and everything will
just work as intended.