Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: YX4xKGD5wCErJqsZPthEppAmnjqt9gXe8NA5PYoxHR/d52vDwiop3ViignY1CDWBsiJDsXzkgw
 LQTC9v50hT946gjpBf1wyZAY0pPJS8CR530qd21zAb9UF36YI+hDkNkHS7B5Z08V849KskKzp8
 M/MhVPPGn/IQk4hvuinPf0RY+AkH7rfk5qM/kFg8ICXJpyZP69Ao68yo28tchj6ecYul6x/oLy
 /nBtuYnX5wwUvb4zg2zlMaUIuiJkYkYIawR5CxjfkNc8dfLlbfMva++hs+Q1c4X+yGcKFIAox1
 GRM=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Simon Arlott <simon@octiron.net>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH] scsi: sd: stop SSD (non-rotational) disks before reboot
Thread-Topic: [PATCH] scsi: sd: stop SSD (non-rotational) disks before reboot
Thread-Index: AQHWRNgorE8Ue5/pcEOqVFi0S2b5Lw==
Date:   Thu, 18 Jun 2020 08:36:20 +0000
Message-ID: <CY4PR04MB37511505492E9EC6A245CFB1E79B0@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <499138c8-b6d5-ef4a-2780-4f750ed337d3@0882a8b5-c6c3-11e9-b005-00805fc181fe>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c3f62fd0-fece-490b-d016-08d81362ad58
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jun 2020 08:36:20.6797
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: qa4u+YYyjghVi+cGisxluOsZZzElD+32Wlz1ixoWkFssvCwgnP3OwV0vEBN3YNUfin78Si3lC+pSrPwPGkhUZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR04MB1254
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/06/18 3:50, Simon Arlott wrote:=0A=
> I need to use "reboot=3Dp" on my desktop because one of the PCIe devices=
=0A=
> does not appear after a warm boot. This results in a very cold boot=0A=
> because the BIOS turns the PSU off and on.=0A=
> =0A=
> The scsi sd shutdown process does not send a stop command to disks=0A=
> before the reboot happens (stop commands are only sent for a shutdown).=
=0A=
> =0A=
> The result is that all of my SSDs experience a sudden power loss on=0A=
> every reboot, which is undesirable behaviour. These events are recorded=
=0A=
> in the SMART attributes.=0A=
=0A=
Why is it undesirable for an SSD ? The sequence you are describing is not=
=0A=
different from doing "shutdown -h now" and then pressing down the power but=
ton=0A=
again immediately after power is cut...=0A=
Are you experiencing data loss or corruption ? If yes, since a clean reboot=
 or=0A=
shutdown issues a synchronize cache to all devices, a corruption would mean=
 that=0A=
your SSD is probably not correctly processing flush cache commands.=0A=
=0A=
> Avoiding a stop of the disk on a reboot is appropriate for HDDs because=
=0A=
> they're likely to continue to be powered (and should not be told to spin=
=0A=
> down only to spin up again) but the default behaviour for SSDs should=0A=
> be changed to stop them before the reboot.=0A=
=0A=
If your BIOS turns the PSU down and up, then the HDDs too will lose power..=
. The=0A=
difference will be that the disks will still be spinning from inertia on th=
e=0A=
power up, and so the HDD spin up processing will be faster than for a pure =
cold=0A=
boot sequence.=0A=
=0A=
> =0A=
> Add a "stop_before_reboot" module parameter that can be used to control=
=0A=
> the shutdown behaviour of disks before a reboot. The default will be=0A=
> to stop non-rotational disks (SSDs) only, but it can be configured to=0A=
> stop all disks if it is known that power will be lost completely on a=0A=
> reboot.=0A=
> =0A=
>   sd_mod.stop_before_reboot=3D<integer>=0A=
>     0 =3D never=0A=
>     1 =3D non-rotational disks only (default)=0A=
>     2 =3D all disks=0A=
> =0A=
> The behaviour on shutdown is unchanged: all disks are unconditionally=0A=
> stopped.=0A=
> =0A=
> The disk I/O will be mostly quiescent at reboot time (and there is a=0A=
> sync first) but this should be added to stable kernels to protect all=0A=
> SSDs from unexpected power loss during a reboot by default. There is=0A=
> the potential for an unexpected power loss to corrupt data depending=0A=
> on the SSD model/firmware.=0A=
> =0A=
> Cc: stable@vger.kernel.org=0A=
> Signed-off-by: Simon Arlott <simon@octiron.net>=0A=
> ---=0A=
>  Documentation/scsi/scsi-parameters.rst |  7 +++++++=0A=
>  drivers/scsi/sd.c                      | 22 +++++++++++++++++++---=0A=
>  2 files changed, 26 insertions(+), 3 deletions(-)=0A=
> =0A=
> diff --git a/Documentation/scsi/scsi-parameters.rst b/Documentation/scsi/=
scsi-parameters.rst=0A=
> index 9aba897c97ac..fd64d0d43861 100644=0A=
> --- a/Documentation/scsi/scsi-parameters.rst=0A=
> +++ b/Documentation/scsi/scsi-parameters.rst=0A=
> @@ -101,6 +101,13 @@ parameters may be changed at runtime by the command=
=0A=
>  			allowing boot to proceed.  none ignores them, expecting=0A=
>  			user space to do the scan.=0A=
>  =0A=
> +	sd_mod.stop_before_reboot=3D=0A=
> +			[SCSI] configure stop action for disks before a reboot=0A=
> +			Format: <integer>=0A=
> +			0 =3D never=0A=
> +			1 =3D non-rotational disks only (default)=0A=
> +			2 =3D all disks> +=0A=
>  	sim710=3D		[SCSI,HW]=0A=
>  			See header of drivers/scsi/sim710.c.=0A=
>  =0A=
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c=0A=
> index d90fefffe31b..1cd652e037ab 100644=0A=
> --- a/drivers/scsi/sd.c=0A=
> +++ b/drivers/scsi/sd.c=0A=
> @@ -98,6 +98,12 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_MOD);=0A=
>  MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);=0A=
>  MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);=0A=
>  =0A=
> +static unsigned int stop_before_reboot =3D 1;=0A=
> +=0A=
> +module_param(stop_before_reboot, uint, 0644);=0A=
> +MODULE_PARM_DESC(stop_before_reboot,=0A=
> +		 "stop disks before reboot (1=3Dnon-rotational, 2=3Dall)");=0A=
> +=0A=
>  #if !defined(CONFIG_DEBUG_BLOCK_EXT_DEVT)=0A=
>  #define SD_MINORS	16=0A=
>  #else=0A=
> @@ -3576,9 +3582,19 @@ static void sd_shutdown(struct device *dev)=0A=
>  		sd_sync_cache(sdkp, NULL);=0A=
>  	}=0A=
>  =0A=
> -	if (system_state !=3D SYSTEM_RESTART && sdkp->device->manage_start_stop=
) {=0A=
> -		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");=0A=
> -		sd_start_stop_device(sdkp, 0);=0A=
> +	if (sdkp->device->manage_start_stop) {=0A=
> +		bool stop_disk =3D (system_state !=3D SYSTEM_RESTART);=0A=
> +=0A=
> +		if (stop_before_reboot > 1) { /* stop all disks */=0A=
> +			stop_disk =3D true;=0A=
> +		} else if (stop_before_reboot) { /* non-rotational only */=0A=
> +			stop_disk |=3D blk_queue_nonrot(sdkp->disk->queue);=0A=
> +		}> +=0A=
> +		if (stop_disk) {=0A=
> +			sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");=0A=
> +			sd_start_stop_device(sdkp, 0);=0A=
> +		}=0A=
>  	}=0A=
>  }=0A=
>  =0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=