Re: [PATCH] scsi: Update max_hw_sectors on rescan

[John Garry <john.g.garry@oracle.com>]

On 18/01/2024 17:22, Brian King wrote:
> On 1/18/24 9:44 AM, John Garry wrote:
>> On 17/01/2024 21:36, Brian King wrote:
>>> This addresses an issue discovered on ibmvfc LUNs. For this driver,
>>> max_sectors is negotiated with the VIOS. This gets done at initialization
>>> time, then LUNs get scanned and things generally work fine. However,
>>> this attribute can be changed on the VIOS, either due to a sysadmin
>>> change or potentially a VIOS code level change. If this decreases
>>> to a smaller value, due to one of these reasons, the next time the
>>> ibmvfc driver performs an NPIV login, it will only be able to use
>>> the smaller value. In the case of a VIOS reboot, when the VIOS goes
>>> down, all paths through that VIOS will go to devloss state. When
>>> the VIOS comes back up, ibmvfc negotiates max_sectors and will only
>>> be able to get the smaller value and it will update shost->max_sectors.
>>
>> Are you saying that the driver will manually update shost->max_sectors after adding the scsi host? I didn't think that was permitted.
> 
> That is what happens. The characteristics of the underlying hardware can change across
> a virtual adapter reset.

That's unfortunate.

I don't think that it's a good idea to change shost->max_sectors after 
adding the scsi host or to add core code to condone doing it. Indeed, 
there is code there to limit shost->max_sectors from DMA mapping 
constraints in scsi_add_host() path, which should not be ignored.

Would it be possible to initially set shost->max_sectors for this 
adapter at the lowest anticipated value for that adapter and don't 
change thereafter?

Thanks,
John

> 
> Thanks,
> 
> Brian
> 
>>
>> Thanks,
>> John
>>
>>> However, when LUNs are scanned, the devloss paths will be found
>>> and brought back online, still using the old max_hw_sectors. This
>>> change ensures that max_hw_sectors gets updated.
>>>
>>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>>> ---
>>>    drivers/scsi/scsi_scan.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>>> index 44680f65ea14..01f2b38daab3 100644
>>> --- a/drivers/scsi/scsi_scan.c
>>> +++ b/drivers/scsi/scsi_scan.c
>>> @@ -1162,6 +1162,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>>>        blist_flags_t bflags;
>>>        int res = SCSI_SCAN_NO_RESPONSE, result_len = 256;
>>>        struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>>> +    struct request_queue *q;
>>>          /*
>>>         * The rescan flag is used as an optimization, the first scan of a
>>> @@ -1182,6 +1183,10 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>>>                    *bflagsp = scsi_get_device_flags(sdev,
>>>                                     sdev->vendor,
>>>                                     sdev->model);
>>> +            q = sdev->request_queue;
>>> +            if (queue_max_hw_sectors(q) > shost->max_sectors)
>>> +                blk_queue_max_hw_sectors(q, shost->max_sectors);
>>> +
>>>                return SCSI_SCAN_LUN_PRESENT;
>>>            }
>>>            scsi_device_put(sdev);
>>> @@ -2006,4 +2011,3 @@ void scsi_forget_host(struct Scsi_Host *shost)
>>>        }
>>>        spin_unlock_irqrestore(shost->host_lock, flags);
>>>    }
>>> -
>>
>