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

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

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Now, we can set the host state by sysfs with any value. Actually, it
doesn't make sense. May cause some functional issues.
This patchset introduce 'blocked' state to blocking IO, we can use this
state for testing. 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.

Ye Bin (5):
  scsi: fix switch host state race between by sysfs and others
  scsi: introduce SHOST_BLOCKED state to support blocking IO
  scsi: limit to set the host state
  scsi: blocking IO when host is blocked
  scsi: run queue after set host state from blocked to running

 drivers/scsi/hosts.c      | 11 +++++++++++
 drivers/scsi/scsi_lib.c   |  4 ++++
 drivers/scsi/scsi_sysfs.c | 18 +++++++++++++++++-
 include/scsi/scsi_host.h  |  6 ++++++
 4 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/5] scsi: fix switch host state race between by sysfs and others

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Now, switch host state by sysfs isn't hold 'shost->host_lock' lock.
It may race with other process, lead to host mixed state.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_sysfs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ee28f73af4d4..cc0ae5e3def3 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -202,6 +202,7 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
 	int i;
 	struct Scsi_Host *shost = class_to_shost(dev);
 	enum scsi_host_state state = 0;
+	unsigned long flags;
 
 	for (i = 0; i < ARRAY_SIZE(shost_states); i++) {
 		const int len = strlen(shost_states[i].name);
@@ -214,8 +215,13 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
 	if (!state)
 		return -EINVAL;
 
-	if (scsi_host_set_state(shost, state))
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsi_host_set_state(shost, state)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
 		return -EINVAL;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	return count;
 }
 
-- 
2.31.1

Re: [PATCH 1/5] scsi: fix switch host state race between by sysfs and others

[Bart Van Assche]

On 3/24/23 18:17, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Now, switch host state by sysfs isn't hold 'shost->host_lock' lock.
> It may race with other process, lead to host mixed state.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>   drivers/scsi/scsi_sysfs.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ee28f73af4d4..cc0ae5e3def3 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -202,6 +202,7 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
>   	int i;
>   	struct Scsi_Host *shost = class_to_shost(dev);
>   	enum scsi_host_state state = 0;
> +	unsigned long flags;
>   
>   	for (i = 0; i < ARRAY_SIZE(shost_states); i++) {
>   		const int len = strlen(shost_states[i].name);
> @@ -214,8 +215,13 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
>   	if (!state)
>   		return -EINVAL;
>   
> -	if (scsi_host_set_state(shost, state))
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	if (scsi_host_set_state(shost, state)) {
> +		spin_unlock_irqrestore(shost->host_lock, flags);
>   		return -EINVAL;
> +	}
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +
>   	return count;
>   }

Please make sure that there is only one spin_unlock_irqrestore() call in 
this function.

Thanks,

Bart.

[PATCH 2/5] scsi: introduce SHOST_BLOCKED state to support blocking IO

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

SHOST_BLOCKED state to blocking io in block layer. This state use for
test, Only running state and blocked state can be switched to each
other.

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

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 9b6fbbe15d92..3b497fd4d329 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -90,6 +90,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
 		switch (oldstate) {
 		case SHOST_CREATED:
 		case SHOST_RECOVERY:
+		case SHOST_BLOCKED:
 			break;
 		default:
 			goto illegal;
@@ -99,6 +100,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
 	case SHOST_RECOVERY:
 		switch (oldstate) {
 		case SHOST_RUNNING:
+		case SHOST_BLOCKED:
 			break;
 		default:
 			goto illegal;
@@ -109,6 +111,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
 		switch (oldstate) {
 		case SHOST_CREATED:
 		case SHOST_RUNNING:
+		case SHOST_BLOCKED:
 		case SHOST_CANCEL_RECOVERY:
 			break;
 		default:
@@ -144,6 +147,14 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
 			goto illegal;
 		}
 		break;
+	case SHOST_BLOCKED:
+		switch (oldstate) {
+		case SHOST_RUNNING:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
 	}
 	shost->shost_state = state;
 	return 0;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index cc0ae5e3def3..b14f95ac594e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -69,6 +69,7 @@ static const struct {
 	{ SHOST_RECOVERY, "recovery" },
 	{ SHOST_CANCEL_RECOVERY, "cancel/recovery" },
 	{ SHOST_DEL_RECOVERY, "deleted/recovery", },
+	{ SHOST_BLOCKED, "blocked", },
 };
 const char *scsi_host_state_name(enum scsi_host_state state)
 {
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 587cc767bb67..9e99317b11fa 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -527,6 +527,7 @@ enum scsi_host_state {
 	SHOST_RECOVERY,
 	SHOST_CANCEL_RECOVERY,
 	SHOST_DEL_RECOVERY,
+	SHOST_BLOCKED,
 };
 
 struct Scsi_Host {
-- 
2.31.1

Re: [PATCH 2/5] scsi: introduce SHOST_BLOCKED state to support blocking IO

[Bart Van Assche]

On 3/24/23 18:17, Ye Bin wrote:
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 9b6fbbe15d92..3b497fd4d329 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -90,6 +90,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>   		switch (oldstate) {
>   		case SHOST_CREATED:
>   		case SHOST_RECOVERY:
> +		case SHOST_BLOCKED:
>   			break;
>   		default:
>   			goto illegal;
> @@ -99,6 +100,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>   	case SHOST_RECOVERY:
>   		switch (oldstate) {
>   		case SHOST_RUNNING:
> +		case SHOST_BLOCKED:
>   			break;
>   		default:
>   			goto illegal;
> @@ -109,6 +111,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>   		switch (oldstate) {
>   		case SHOST_CREATED:
>   		case SHOST_RUNNING:
> +		case SHOST_BLOCKED:
>   		case SHOST_CANCEL_RECOVERY:
>   			break;
>   		default:
> @@ -144,6 +147,14 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>   			goto illegal;
>   		}
>   		break;
> +	case SHOST_BLOCKED:
> +		switch (oldstate) {
> +		case SHOST_RUNNING:
> +			break;
> +		default:
> +			goto illegal;
> +		}
> +		break;
>   	}

If a host is blocked, error recovery happens and completes, the host 
will be unblocked. I don't think that is acceptable.

The "blocked" property is orthogonal to the host state so a new boolean 
member variable should be introduced in struct Scsi_Host instead of 
introducing a new SCSI host state.

Thanks,

Bart.

[PATCH 3/5] scsi: limit to set the host state

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Now, we can set the host state with any value. Actually, it doesn't
make sense. As previous patch introduce SHOST_BLOCKED state, set this
state, it will blocking IO. So this patch limit to set the host with
running/blocked state.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_sysfs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b14f95ac594e..42c5936c7711 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -203,6 +203,7 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
 	int i;
 	struct Scsi_Host *shost = class_to_shost(dev);
 	enum scsi_host_state state = 0;
+	enum scsi_host_state old_state;
 	unsigned long flags;
 
 	for (i = 0; i < ARRAY_SIZE(shost_states); i++) {
@@ -216,8 +217,13 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
 	if (!state)
 		return -EINVAL;
 
+	if (state != SHOST_RUNNING && state != SHOST_BLOCKED)
+		return -EINVAL;
+
 	spin_lock_irqsave(shost->host_lock, flags);
-	if (scsi_host_set_state(shost, state)) {
+	old_state = shost->shost_state;
+	if ((old_state != SHOST_RUNNING && old_state != SHOST_BLOCKED) ||
+	    scsi_host_set_state(shost, state)) {
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		return -EINVAL;
 	}
-- 
2.31.1

Re: [PATCH 3/5] scsi: limit to set the host state

[Bart Van Assche]

On 3/24/23 18:17, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Now, we can set the host state with any value. Actually, it doesn't
> make sense. As previous patch introduce SHOST_BLOCKED state, set this
> state, it will blocking IO. So this patch limit to set the host with
> running/blocked state.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>   drivers/scsi/scsi_sysfs.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index b14f95ac594e..42c5936c7711 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -203,6 +203,7 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
>   	int i;
>   	struct Scsi_Host *shost = class_to_shost(dev);
>   	enum scsi_host_state state = 0;
> +	enum scsi_host_state old_state;
>   	unsigned long flags;
>   
>   	for (i = 0; i < ARRAY_SIZE(shost_states); i++) {
> @@ -216,8 +217,13 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
>   	if (!state)
>   		return -EINVAL;
>   
> +	if (state != SHOST_RUNNING && state != SHOST_BLOCKED)
> +		return -EINVAL;
> +
>   	spin_lock_irqsave(shost->host_lock, flags);
> -	if (scsi_host_set_state(shost, state)) {
> +	old_state = shost->shost_state;
> +	if ((old_state != SHOST_RUNNING && old_state != SHOST_BLOCKED) ||
> +	    scsi_host_set_state(shost, state)) {
>   		spin_unlock_irqrestore(shost->host_lock, flags);
>   		return -EINVAL;
>   	}

Please make sure that the "state != SHOST_RUNNING && state != 
SHOST_BLOCKED" check occurs only once and also that there is one 
spin_lock_irqsave() call in this function and only one 
spin_unlock_irqrestore() call.

Thanks,

Bart.

[PATCH 4/5] scsi: blocking IO when host is blocked

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Unlike recovery state may block other process, blocking IO only
when host is blocked.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..492487717f41 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1731,6 +1731,10 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 			ret = BLK_STS_OFFLINE;
 		goto out_dec_target_busy;
 	}
+
+	if (unlikely(scsi_host_blocked(shost)))
+		goto out_dec_target_busy;
+
 	if (!scsi_host_queue_ready(q, shost, sdev, cmd))
 		goto out_dec_target_busy;
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 9e99317b11fa..571321bbb706 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -745,6 +745,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->shost_state == SHOST_BLOCKED;
+}
+
 extern int scsi_queue_work(struct Scsi_Host *, struct work_struct *);
 extern void scsi_flush_work(struct Scsi_Host *);
 
-- 
2.31.1

Re: [PATCH 4/5] scsi: blocking IO when host is blocked

[Bart Van Assche]

On 3/24/23 18:17, Ye Bin wrote:
> +	if (unlikely(scsi_host_blocked(shost)))
> +		goto out_dec_target_busy;
I this check would be moved earlier then the atomic_inc() and 
atomic_dec() of scsi_target(sdev)->target_busy could be skipped.

Thanks,

Bart.

[PATCH 5/5] scsi: run queue after set host state from blocked to running

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

As when host is blocked, all request is blocked. After set the host
state with running, e will need to ensure that these requests are
started.

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

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 42c5936c7711..202d58f4f267 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -229,6 +229,9 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
+	if (old_state == SHOST_BLOCKED && state == SHOST_RUNNING)
+		scsi_run_host_queues(shost);
+
 	return count;
 }
 
-- 
2.31.1