[PATCH v2 0/3] limit set the host state by sysfs

[PATCH v2 0/3] limit set the host state by sysfs

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Diff v2 vs v1:
1. Remove set scsi host state by sysfs, as set host state by sysfs
may lead to functional issue. Scsi host state has it's own running
state machine.
2. Introduce 'blocked' sysfs api for set scsi host blocking IO. Use
this function we can do some test.

Ye Bin (3):
  scsi: forbid to set scsi host state by sysfs
  scsi: introduce 'blocked' sysfs api
  scsi: blocking IO when host is set blocked

 drivers/scsi/scsi_lib.c   |  2 ++
 drivers/scsi/scsi_sysfs.c | 58 ++++++++++++++++++++++-----------------
 include/scsi/scsi_host.h  |  8 ++++++
 3 files changed, 43 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] scsi: forbid to set scsi host state by sysfs

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Actually, set scsi host state by sysfs may lead to functional issues.
So forbid to set scsi host state.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_sysfs.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ee28f73af4d4..903aa9de46e5 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -195,30 +195,6 @@ store_scan(struct device *dev, struct device_attribute *attr,
 };
 static DEVICE_ATTR(scan, S_IWUSR, NULL, store_scan);
 
-static ssize_t
-store_shost_state(struct device *dev, struct device_attribute *attr,
-		  const char *buf, size_t count)
-{
-	int i;
-	struct Scsi_Host *shost = class_to_shost(dev);
-	enum scsi_host_state state = 0;
-
-	for (i = 0; i < ARRAY_SIZE(shost_states); i++) {
-		const int len = strlen(shost_states[i].name);
-		if (strncmp(shost_states[i].name, buf, len) == 0 &&
-		   buf[len] == '\n') {
-			state = shost_states[i].value;
-			break;
-		}
-	}
-	if (!state)
-		return -EINVAL;
-
-	if (scsi_host_set_state(shost, state))
-		return -EINVAL;
-	return count;
-}
-
 static ssize_t
 show_shost_state(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -233,7 +209,7 @@ show_shost_state(struct device *dev, struct device_attribute *attr, char *buf)
 
 /* DEVICE_ATTR(state) clashes with dev_attr_state for sdev */
 static struct device_attribute dev_attr_hstate =
-	__ATTR(state, S_IRUGO | S_IWUSR, show_shost_state, store_shost_state);
+	__ATTR(state, S_IRUGO, show_shost_state, NULL);
 
 static ssize_t
 show_shost_mode(unsigned int mode, char *buf)
-- 
2.31.1

[PATCH v2 2/3] scsi: introduce 'blocked' sysfs api

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Introduce 'blocked' sysfs api to control scsi host blocking IO.
Use this founction for test. Perhaps we can use this to do some fault
recovery or firmware upgrades, as long as the driver support is good,
it may be insensitive to the upper layer.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_sysfs.c | 32 ++++++++++++++++++++++++++++++++
 include/scsi/scsi_host.h  |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 903aa9de46e5..cad1981ab528 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -345,6 +345,37 @@ store_shost_eh_deadline(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR(eh_deadline, S_IRUGO | S_IWUSR, show_shost_eh_deadline, store_shost_eh_deadline);
 
+static ssize_t
+store_shost_blocked(struct device *dev, struct device_attribute *attr,
+		    const char *buf, size_t count)
+{
+	int err;
+	bool blocked;
+	struct Scsi_Host *shost = class_to_shost(dev);
+
+	err = kstrtobool(buf, &blocked);
+	if (err)
+		return err;
+
+	if (shost->host_blockio != blocked) {
+		shost->host_blockio = blocked;
+		if (!blocked)
+			scsi_run_host_queues(shost);
+	}
+
+	return count;
+}
+
+static ssize_t
+show_shost_blocked(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+
+	return snprintf(buf, 20, "%d\n", shost->host_blockio);
+}
+static DEVICE_ATTR(blocked, S_IRUGO | S_IWUSR,
+		   show_shost_blocked, store_shost_blocked);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(can_queue, "%d\n");
@@ -397,6 +428,7 @@ static struct attribute *scsi_sysfs_shost_attrs[] = {
 	&dev_attr_host_reset.attr,
 	&dev_attr_eh_deadline.attr,
 	&dev_attr_nr_hw_queues.attr,
+	&dev_attr_blocked.attr,
 	NULL
 };
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 587cc767bb67..3e916dbac1cb 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -659,6 +659,9 @@ struct Scsi_Host {
 	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
 	unsigned no_scsi2_lun_in_cdb:1;
 
+	/* True host will blocking IO */
+	unsigned host_blockio:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */
-- 
2.31.1

Re: [PATCH v2 2/3] scsi: introduce 'blocked' sysfs api

[Mike Christie]

On 3/28/23 9:34 AM, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Introduce 'blocked' sysfs api to control scsi host blocking IO.
> Use this founction for test. Perhaps we can use this to do some fault
> recovery or firmware upgrades, as long as the driver support is good,

If it's for testing only then do people like a debugfs type of interface
is better?

If it's for actual production use like firmware upgrades and they can't
handle IO while they are doing the upgrade, then I think you need to do
more than just set a bit to prevent new IO. You also need to handle cmds
that have passed the scsi_queue_rq ready checks and have not been processed
by the driver's queuecommand. Also there are some issues like cmds can
still timeout and so you will get scsi_host_template->eh* calls still.


>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 587cc767bb67..3e916dbac1cb 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -659,6 +659,9 @@ struct Scsi_Host {
>  	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
>  	unsigned no_scsi2_lun_in_cdb:1;
>  
> +	/* True host will blocking IO */
> +	unsigned host_blockio:1;
> +

Maybe rename to host_user_blocked to match the host_self_blocked naming.

I would make the comment similar to host_self_blocked's comment but
instead of the host requesting it userspace did.

>  	/*
>  	 * Optional work queue to be utilized by the transport
>  	 */

[PATCH v2 3/3] scsi: blocking IO when host is set blocked

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

As previous patch introduce 'blocked' sysfs api to set 'host_blockio'.
If 'host_blockio' is true will blocking IO.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_lib.c  | 2 ++
 include/scsi/scsi_host.h | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..20d618300a46 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1724,6 +1724,8 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	}
 
 	ret = BLK_STS_RESOURCE;
+	if (unlikely(scsi_host_blocked(shost)))
+		goto out_put_budget;
 	if (!scsi_target_queue_ready(shost, sdev))
 		goto out_put_budget;
 	if (unlikely(scsi_host_in_recovery(shost))) {
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 3e916dbac1cb..9fc30d0c48de 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -747,6 +747,11 @@ static inline int scsi_host_in_recovery(struct Scsi_Host *shost)
 		shost->tmf_in_progress;
 }
 
+static inline int scsi_host_blocked(struct Scsi_Host *shost)
+{
+	return shost->host_blockio;
+}
+
 extern int scsi_queue_work(struct Scsi_Host *, struct work_struct *);
 extern void scsi_flush_work(struct Scsi_Host *);
 
-- 
2.31.1

Re: [PATCH v2 3/3] scsi: blocking IO when host is set blocked

[Mike Christie]

On 3/28/23 9:34 AM, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> As previous patch introduce 'blocked' sysfs api to set 'host_blockio'.
> If 'host_blockio' is true will blocking IO.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  drivers/scsi/scsi_lib.c  | 2 ++
>  include/scsi/scsi_host.h | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b7c569a42aa4..20d618300a46 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1724,6 +1724,8 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	}
>  
>  	ret = BLK_STS_RESOURCE;
> +	if (unlikely(scsi_host_blocked(shost)))
> +		goto out_put_budget;

You can just put this check in scsi_host_queue_ready with the
host_self_blocked check.