On 6/5/19 10:14 PM, Bart Van Assche wrote:
> The ability to modify the SCSI device state was introduced by commit
> 638127e579a4 ("[PATCH] Fix error handler offline behaviour"; v2.6.12). That
> same commit introduced the following device states:
>
> { SDEV_CREATED, "created" },
> { SDEV_RUNNING, "running" },
> { SDEV_CANCEL, "cancel" },
> { SDEV_DEL, "deleted" },
> { SDEV_QUIESCE, "quiesce" },
> { SDEV_OFFLINE, "offline" },
>
> The SDEV_BLOCK state was introduced later to avoid that an FC cable pull
> would immediately result in an I/O error (commit 1094e682310e; "[PATCH]
> suspending I/Os to a device"; v2.6.12). That same patch introduced the
> ability to set the SDEV_BLOCK state from user space. I'm not sure whether
> that ability was introduced on purpose or accidentally.
>
> This patch makes sure that SDEV_BLOCK is only used for its original
> purpose, namely to allow transport drivers and LLDs to block further
> .queuecommand() calls while transport layer or adapter recovery is in
> progress.
>
> Notes:
> - While SDEV_BLOCK blocks all SCSI commands, in the SDEV_QUIESCE
> state only those block layer requests are blocked for which RQF_PREEMPT
> has not been set. RQF_PREEMPT is not set for I/O requests submitted by
> e.g. a filesystem but is set for all requests pass-through requests.
> See also __scsi_execute().
> - By doing a web search for ("blocked" OR "quiesce") AND
> "/sys/class/scsi_device" AND "device/state" I found several storage
> configuration guides. The instructions I found in these guides
> tell users to write the value "running" or "offline" in the SCSI
> device state sysfs attribute and no other values.
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Cc: Johannes Thumshirn <jthumshirn@xxxxxxx>
> Cc: James Smart <james.smart@xxxxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/scsi/scsi_sysfs.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ff0aea7ac87f..a49ee113b3c4 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -769,6 +769,13 @@ store_state_field(struct device *dev, struct device_attribute *attr,
> }
> if (!state)
> return -EINVAL;
> + /*
> + * The state SDEV_BLOCK should not be set from userspace. Translate
> + * SDEV_BLOCK into SDEV_QUIESCE in case the SDEV_BLOCK state transition
> + * is requested from user space.
> + */
> + if (state == SDEV_BLOCK)
> + state = SDEV_QUIESCE;
>
> mutex_lock(&sdev->state_mutex);
> ret = scsi_device_set_state(sdev, state);
>
Why not simply '-EPERM' ?
Translating a state into something else seems counterproductive.
And, while we're at it:
The only meaningful states to be set from userspace are 'RUNNING',
'OFFLINE', and 'BLOCK'.
Everything else relates to the internal state machine and really
shouldn't be touched from userspace at all.
So I'd rather filter out everything _but_ the three states, avoid
similar issue in the future.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
