Re: [PATCH V3 15/25] smartpqi: fix host qdepth limit

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Don, dear Mahesh,


Am 10.12.20 um 21:35 schrieb Don Brace:
> From: Mahesh Rajashekhara <mahesh.rajashekhara@microchip.com>
> 
> * 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@microchip.com>
> Reviewed-by: Scott Teel <scott.teel@microchip.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com>
> Signed-off-by: Mahesh Rajashekhara <mahesh.rajashekhara@microchip.com>
> Signed-off-by: Don Brace <don.brace@microchip.com>
> ---
>   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