Dear Don, dear Mahesh,
Am 10.12.20 um 21:35 schrieb Don Brace:
From: Mahesh Rajashekhara <mahesh.rajashekhara@xxxxxxxxxxxxx>
* Correct scsi-mid-layer sending more requests than
exposed host Q depth causing firmware ASSERT issue.
* Add host Qdepth counter.
This supposedly fixes the regression between Linux 5.4 and 5.9, which we
reported in [1].
kernel: smartpqi 0000:89:00.0: controller is offline: status code
0x6100c
kernel: smartpqi 0000:89:00.0: controller offline
Thank you for looking into this issue and fixing it. We are going to
test this.
For easily finding these things in the git history or the WWW, it would
be great if these log messages could be included (in the future).
Also, that means, that the regression is still present in Linux 5.10,
released yesterday, and this commit does not apply to these versions.
Mahesh, do you have any idea, what commit caused the regression and why
the issue started to show up?
James, Martin, how are regressions handled for the SCSI subsystem?
Regarding the diff, personally, I find the commit message much too
terse. `pqi_scsi_queue_command()` will return `SCSI_MLQUEUE_HOST_BUSY`
for the case of too many requests. Will that be logged by Linux in some
log level? In my opinion it points to a performance problem, and should
be at least logged as a notice or warning.
Can `ctrl_info->scsi_ml_can_queue` be queried somehow maybe in the logs?
`sudo find /sys -name queue` did not display something interesting.
[1]: https://marc.info/?l=linux-scsi&m=160271263114829&w=2
"Linux 5.9: smartpqi: controller is offline: status code 0x6100c"
Reviewed-by: Scott Benesh <scott.benesh@xxxxxxxxxxxxx>
Reviewed-by: Scott Teel <scott.teel@xxxxxxxxxxxxx>
Reviewed-by: Kevin Barnett <kevin.barnett@xxxxxxxxxxxxx>
Signed-off-by: Mahesh Rajashekhara <mahesh.rajashekhara@xxxxxxxxxxxxx>
Signed-off-by: Don Brace <don.brace@xxxxxxxxxxxxx>
---
drivers/scsi/smartpqi/smartpqi.h | 2 ++
drivers/scsi/smartpqi/smartpqi_init.c | 19 ++++++++++++++++---
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
index 0b94c755a74c..c3b103b15924 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -1345,6 +1345,8 @@ struct pqi_ctrl_info {
struct work_struct ofa_quiesce_work;
u32 ofa_bytes_requested;
u16 ofa_cancel_reason;
+
+ atomic_t total_scmds_outstanding;
};
What is the difference between the already existing
atomic_t scsi_cmds_outstanding;
and the new counter?
atomic_t total_scmds_outstanding;
The names are quite similar, so different names or a comment might be
useful.
enum pqi_ctrl_mode {
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index 082b17e9bd80..4e088f47d95f 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -5578,6 +5578,8 @@ static inline bool pqi_is_bypass_eligible_request(struct scsi_cmnd *scmd)
void pqi_prep_for_scsi_done(struct scsi_cmnd *scmd)
{
struct pqi_scsi_dev *device;
+ struct pqi_ctrl_info *ctrl_info;
+ struct Scsi_Host *shost;
if (!scmd->device) {
set_host_byte(scmd, DID_NO_CONNECT);
@@ -5590,7 +5592,11 @@ void pqi_prep_for_scsi_done(struct scsi_cmnd *scmd)
return;
}
+ shost = scmd->device->host;
The function already has a variable `device`, which is assigned
“hostdata” though:
device = scmd->device->hostdata;
This confuses me. Maybe this should be cleaned up in a followup commit,
and the variable device be reused above in the `shost` assignment.
+ ctrl_info = shost_to_hba(shost);
+
atomic_dec(&device->scsi_cmds_outstanding);
+ atomic_dec(&ctrl_info->total_scmds_outstanding);
}
static bool pqi_is_parity_write_stream(struct pqi_ctrl_info *ctrl_info,
@@ -5678,6 +5684,7 @@ static int pqi_scsi_queue_command(struct Scsi_Host *shost, struct scsi_cmnd *scm
bool raid_bypassed;
device = scmd->device->hostdata;
+ ctrl_info = shost_to_hba(shost);
if (!device) {
set_host_byte(scmd, DID_NO_CONNECT);
@@ -5686,8 +5693,11 @@ static int pqi_scsi_queue_command(struct Scsi_Host *shost, struct scsi_cmnd *scm
}
atomic_inc(&device->scsi_cmds_outstanding);
-
- ctrl_info = shost_to_hba(shost);
I believe, style changes (re-ordering) in commits fixing regressions
make it harder to backport it.
+ if (atomic_inc_return(&ctrl_info->total_scmds_outstanding) >
+ ctrl_info->scsi_ml_can_queue) {
+ rc = SCSI_MLQUEUE_HOST_BUSY;
+ goto out;
+ }
if (pqi_ctrl_offline(ctrl_info) || pqi_device_in_remove(device)) {
set_host_byte(scmd, DID_NO_CONNECT);
@@ -5730,8 +5740,10 @@ static int pqi_scsi_queue_command(struct Scsi_Host *shost, struct scsi_cmnd *scm
}
out:
- if (rc)
+ if (rc) {
atomic_dec(&device->scsi_cmds_outstanding);
+ atomic_dec(&ctrl_info->total_scmds_outstanding);
+ }
return rc;
}
@@ -8054,6 +8066,7 @@ static struct pqi_ctrl_info *pqi_alloc_ctrl_info(int numa_node)
INIT_WORK(&ctrl_info->event_work, pqi_event_worker);
atomic_set(&ctrl_info->num_interrupts, 0);
+ atomic_set(&ctrl_info->total_scmds_outstanding, 0);
INIT_DELAYED_WORK(&ctrl_info->rescan_work, pqi_rescan_worker);
INIT_DELAYED_WORK(&ctrl_info->update_time_work, pqi_update_time_worker);
Kind regards,
Paul
