From: Ye Bin <[email protected]>
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
From: Ye Bin <[email protected]>
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 <[email protected]>
---
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
From: Ye Bin <[email protected]>
Unlike recovery state may block other process, blocking IO only
when host is blocked.
Signed-off-by: Ye Bin <[email protected]>
---
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
From: Ye Bin <[email protected]>
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 <[email protected]>
---
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
From: Ye Bin <[email protected]>
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 <[email protected]>
---
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
On 3/24/23 18:17, Ye Bin wrote:
> From: Ye Bin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
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.
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.