On Thu, Sep 29, 2022 at 09:57:19PM -0500, Mike Christie wrote:
> On 9/23/22 7:02 PM, Uday Shankar wrote:
> > ---
> > Looking for feedback on the "allowed source states" list. The bug I hit
> > is solved by prohibiting transitions out of SDEV_BLOCKED, but I think
> > most others shouldn't be allowed either.
>
> I think it's ok to be restrictive:
>
> 1. BLOCKED is just broken. When the transport classes and scsi_lib transition
> out of that state they do a lot more than just set the set. We are leaving
> the kernel in mismatched state where the device state is running but the
> block layerand transport classes are not ready for IO.
>
> 2. CREATED does not happen. We go into RUNNING then do scsi_sysfs_add_sdev so
> userspace should not see the CREATED state.
>
> 3. I'm not 100% sure about SDEV_QUIESCE though. It looks like it has similar issues
> as BLOCKED where scsi_device_resume will undo things it did in scsi_device_quiesce,
> so we can't just set the state to RUNNING and expect things to work. I'm not
> sure about the scsi_transport_spi cases.
>
> It would be best for James or Hannes to comment because they know that code well.
Adding Hannes to CC.
> 4. The transport classes are the ones that put devs in SDEV_TRANSPORT_OFFLINE
> and then transition them when they are ready. The block layer is at least in
> the correct state, but the transport classes may not be ready for IO since they
> are not expecting IO to be queued to them at that time.
>
> >
> > drivers/scsi/scsi_sysfs.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 9dad2fd5297f..b38c30fe681d 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -822,6 +822,14 @@ store_state_field(struct device *dev, struct device_attribute *attr,
> > }
> >
> > mutex_lock(&sdev->state_mutex);
> > + switch (sdev->sdev_state) {
> > + case SDEV_RUNNING:
> > + case SDEV_OFFLINE:
> > + break;
> > + default:
> > + mutex_unlock(&sdev->state_mutex);
> > + return -EINVAL;
> > + }
> > if (sdev->sdev_state == SDEV_RUNNING && state == SDEV_RUNNING) {
> > ret = 0;
> > } else {
> >
> > base-commit: 7f615c1b5986ff08a725ee489e838c90f8197bcd
>
