Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[James Bottomley <jejb@linux.vnet.ibm.com>]

On Wed, 2016-12-07 at 15:30 -0500, Ewan D. Milne wrote:
> On Wed, 2016-12-07 at 12:09 -0800, James Bottomley wrote:
> > Hm, it looks like the state set in scsi_sysfs_add_sdev() is bogus. 
> >  We expect the state to have been properly set before that (in
> > scsi_add_lun), so can we not simply remove it?
> > 
> > James
> > 
> 
> I was considering that, but...
> 
> enum scsi_device_state {
>         SDEV_CREATED = 1,       /* device created but not added to
> sysfs                                                                
>                                                                      
>       
>                                  * Only internal commands allowed
> (for inq) */
> 
> So it seems the intent was for the state to not change until then.

I think this is historical.  There was a change somewhere that moved
the sysfs state handling out of the sdev stat to is_visible, so the
sdev state no-longer reflects  it.

> The call to set the SDEV_RUNNING state earlier in scsi_add_lun()
> was added with:
> 
> commit 6f4267e3bd1211b3d09130e626b0b3d885077610
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Fri Aug 22 16:53:31 2008 -0500
> 
>     [SCSI] Update the SCSI state model to allow blocking in the
> created state
> 
> Which allows the device to go into ->BLOCK (which is needed, since it
> actually happens).
> 
> Should we remove the call from scsi_sysfs_add_sdev() and change the
> comment in scsi_device.h to reflect the intent?

Assuming someone with the problem actually tests it, yes.

> I have not verified the async vs. non-async scan path yet but it 
> looks like it would be OK.

I did.  The async device addition occurs after scsi_add_lun(), so it
rules the state change in both cases.

James


> -Ewan
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>