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

[Bart Van Assche <bvanassche@acm.org>]

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

Thanks,

Bart.