On Mon, 2016-12-12 at 10:20 +0800, Wei Fang wrote:
> A race between scanning and fc_remote_port_delete() may result in a
> permanent stop if the device gets blocked before scsi_sysfs_add_sdev()
> and unblocked after. The reason is that blocking a device sets both
> the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED. However,
> scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
> the device to be ignored by scsi_target_unblock() and thus never have
> its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
> running but has a stopped queue.
>
> We actually have two places where SDEV_RUNNING is set: once in
> scsi_add_lun() which respects the blocked flag and once in
> scsi_sysfs_add_sdev() which doesn't. Since the second set is entirely
> spurious, simply remove it to fix the problem.
>
> Reported-by: Zengxi Chen <chenzengxi@xxxxxxxxxx>
> Signed-off-by: Wei Fang <fangwei1@xxxxxxxxxx>
> ---
> Changes v1->v2:
> - don't modify scsi_internal_device_unblock(), just remove changing
> state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
> James Bottomley and Ewan D. Milne.
> Changes v2->v3
> - Use a clearer description of this problem
>
> drivers/scsi/scsi_sysfs.c | 4 ----
> include/scsi/scsi_device.h | 2 +-
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 0734927..82dfe07 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> struct request_queue *rq = sdev->request_queue;
> struct scsi_target *starget = sdev->sdev_target;
>
> - error = scsi_device_set_state(sdev, SDEV_RUNNING);
> - if (error)
> - return error;
> -
> error = scsi_target_add(starget);
> if (error)
> return error;
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 8990e58..8bfb37f 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -31,7 +31,7 @@ struct scsi_mode_data {
> enum scsi_device_state {
> SDEV_CREATED = 1, /* device created but not added to sysfs
> * Only internal commands allowed (for inq) */
> - SDEV_RUNNING, /* device properly configured
> + SDEV_RUNNING, /* device properly configured and not blocked
> * All commands allowed */
> SDEV_CANCEL, /* beginning to delete device
> * Only error handler commands allowed */
Well, James said not to bother with the comment, but OK.
I take it this has passed your testing. I have not heard back yet
from the site that reported this problem to me on their reproducer.
The change looks good to me.
Reviewed-by: Ewan D. Milne <emilne@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
