On 7/18/23 8:25 AM, John Garry wrote:
> On 14/07/2023 22:33, Mike Christie wrote:
>> If scsi_execute_cmd returns < 0, it doesn't initialize the sshdr, so we
>> shouldn't access the sshdr. If it returns 0, then the cmd executed
>> successfully, so there is no need to check the sshdr. This has us access
>> the sshdr when get a return value > 0.
>>
>> Signed-off-by: Mike Christie<michael.christie@xxxxxxxxxx>
>> Reviewed-by: Christoph Hellwig<hch@xxxxxx>
>> ---
>> drivers/scsi/device_handler/scsi_dh_rdac.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
>> index c5538645057a..cdefaa9f614e 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
>> @@ -541,6 +541,7 @@ static void send_mode_select(struct work_struct *work)
>> const struct scsi_exec_args exec_args = {
>> .sshdr = &sshdr,
>> };
>> + int rc;
>> spin_lock(&ctlr->ms_lock);
>> list_splice_init(&ctlr->ms_head, &list);
>> @@ -558,14 +559,18 @@ static void send_mode_select(struct work_struct *work)
>> (char *) h->ctlr->array_name, h->ctlr->index,
>> (retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
>> - if (scsi_execute_cmd(sdev, cdb, opf, &h->ctlr->mode_select, data_size,
>> - RDAC_TIMEOUT * HZ, RDAC_RETRIES, &exec_args)) {
>> + rc = scsi_execute_cmd(sdev, cdb, opf, &h->ctlr->mode_select, data_size,
>> + RDAC_TIMEOUT * HZ, RDAC_RETRIES, &exec_args);
>> + if (rc < 0) {
>> + err = SCSI_DH_IO;
>> + } else if (rc > 0) {
>> err = mode_select_handle_sense(sdev, &sshdr);
>> if (err == SCSI_DH_RETRY && retry_cnt--)
>> goto retry;
>> if (err == SCSI_DH_IMM_RETRY)
>> goto retry;
>> }
>> +
>> if (err == SCSI_DH_OK) {
> As I see, err is initially set to SCSI_DH_OK when declared. Then if we need to retry and 2nd call to scsi_execute_cmd() passes, such that rc == 0, then err still holds the old value. This seems same as pre-existing behaviour - is this proper?
I guess this has been working by accident.
When that happens we end up returning one of the retryable error codes
to dm-multipath. It will then recall us, and before we re-run this
function we will run check_ownership and see that the last call worked.
