Re: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown

[Damien Le Moal <dlemoal@kernel.org>]

On 2023/09/22 13:08, Bart Van Assche wrote:
> On 9/22/23 12:10, Damien Le Moal wrote:
>> Looking at the code, scsi_remove_host() calls scsi_forget_host() which calls
>> __scsi_remove_device() for any device that is not in the SDEV_DEL state.
>> __scsi_remove_device() then sets the state to SDEV_CANCEL. So it appears that
>> the state should always be CANCEL and not running. However, my tests showed it
>> to be running. I am not fully understanding how sd_remove() end up being called...
> 
> I think this is how sd_sync_cache() gets called from inside
> scsi_remove_host():
> 
> scsi_remove_host()
>    -> scsi_forget_host()
>      -> __scsi_remove_device()
>        -> device_del(&sdev->sdev_gendev)
>          -> bus_remove_device()
>            -> device_release_driver()
>              -> __device_release_driver()
>                -> sd_remove()
>                  -> sd_shutdown()
>                    -> sd_sync_cache()
> 
> In other words, it is guaranteed that scsi_device_set_state(sdev, 
> SDEV_CANCEL) has been called before sd_remove() if it is called by 
> scsi_remove_host().
> 
>> I think we should investigate this further though, to make sure that we can
>> always safely call sd_shutdown(). __scsi_remove_device() has this comment:
>>
>> /*
>>   * If blocked, we go straight to DEL and restart the queue so
>>   * any commands issued during driver shutdown (like sync
>>   * cache) are errored immediately.
>>   */
>>
>> Which kind of give a hint that we should probably not blindy always try to call
>> sd_shutdown().
> 
> Does that comment perhaps refer to the SDEV_BLOCK / SDEV_CREATED_BLOCK
> states? Anyway, I'm wondering whether there are better ways to prevent
> that it is attempted to queue SCSI commands if a SCSI device is
> suspended, e.g. by checking the suspended state from inside
> scsi_device_state_check() or scsi_dispatch_cmd().

Using information in the device ->power structure is not reliable without
holding the device lock(), so we should not do that. But we can add a
"suspended" scsi_device flag that we maintain on execution of
sd_suspend_system() and sd_resume_system(). Many drivers do that...
Thoughts ?


-- 
Damien Le Moal
Western Digital Research