lkml.org 
[lkml]   [2022]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] scsi: Do not break scan luns loop if add single lun failed
From
Date
On 2021/12/31 1:55, Steffen Maier wrote:
> On 12/26/21 00:29, Wenchao Hao wrote:
>> Failed to add a single lun does not mean all luns are unaccessible,
>> if we break the scan luns loop, the other luns reported by REPORT LUNS
>> command would not be probed any more.
>>
>> In this case, we might loss some luns which are accessible.
>
> Could you please add more details about the specific use case, where
> this actually was a problem, for my understanding?
>

When REPORT LUNS returns 4 luns which are lun0, lun1, lun2 and lun3.
If lun1 becomes inaccessible during the scan flow,
scsi_probe_and_add_lun() for lun1 would failed, lun2 and lun3 are still
accessible. scsi_report_lun_scan() would print error log and return 0,
and scsi_sequential_lun_scan() would not be called.

In this scenario, lun2 and lun3 would not been probed and added any
more, so we loss them.

>>
>> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
>> ---
>>   drivers/scsi/scsi_scan.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 23e1c0acdeae..fee7ce082103 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1476,13 +1476,13 @@ static int scsi_report_lun_scan(struct
>> scsi_target *starget, blist_flags_t bflag
>>                   lun, NULL, NULL, rescan, NULL);
>>               if (res == SCSI_SCAN_NO_RESPONSE) {
>>                   /*
>> -                 * Got some results, but now none, abort.
>> +                 * Got some results, but now none, abort this lun
>
> abort => skip ?

Yes, "skip" looks better than "abort".

>
>>                    */
>>                   sdev_printk(KERN_ERR, sdev,
>>                       "Unexpected response"
>>                       " from lun %llu while scanning, scan"
>>                       " aborted\n", (unsigned long long)lun);
>
> That message would no longer be correct with your change, as it would
> not abort the scan any more.

I would change "abort" to "skip" which makes it better.

>
>> -                break;
>> +                continue;
>>               }
>>           }
>>       }
>
>
> Wouldn't this change existing semantics for LLDDs intentionally
> returning -ENXIO from their slave_alloc() callback in certain cases?:
>
>

Yes, it would print error message like "Unexpected response ..." for
every failed lun. I think it's reasonable, so we can know every failed
lun in one scan flow.

>> static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
> ...
>>     if (shost->hostt->slave_alloc) {
>>         ret = shost->hostt->slave_alloc(sdev);
>>         if (ret) {
>>             /*
>>              * if LLDD reports slave not present, don't clutter
>>              * console with alloc failure messages
>>              */
>>             if (ret == -ENXIO)
>>                 display_failure_msg = 0;
>>             goto out_device_destroy;
> ...
>> out_device_destroy:
>>     __scsi_remove_device(sdev);
>> out:
>>     if (display_failure_msg)
>>         printk(ALLOC_FAILURE_MSG, __func__);
>>     return NULL;
>
>
> scsi_probe_and_add_lun() [such as called by scsi_report_lun_scan() for
> the case at hand] converts this case into a SCSI_SCAN_NO_RESPONSE return
> value.
>
>> static int scsi_probe_and_add_lun(struct scsi_target *starget,
> ...
>>     int res = SCSI_SCAN_NO_RESPONSE, result_len = 256;
> ...
>>         sdev = scsi_alloc_sdev(starget, lun, hostdata);
>>     if (!sdev)
>>         goto out;
> ...
>>  out:
>>     return res;
>
>
> Such as being used by zfcp:
>
>> static int zfcp_scsi_slave_alloc(struct scsi_device *sdev)
>> {
> ...
>>     unit = zfcp_unit_find(port, zfcp_scsi_dev_lun(sdev));
>>     if (unit)
>>         put_device(&unit->dev);
>>
>>     if (!unit && !(allow_lun_scan && npiv)) {
>>         put_device(&port->dev);
>>         return -ENXIO;
>                       ^^^^^^
>
> which implements an initiator-based LUN masking that is necessary for
> shared HBAs virtualized without NPIV.
> https://www.ibm.com/docs/en/linux-on-systems?topic=devices-manually-configured-fcp-luns
>
>
> While things might still work, as zfcp now "just" gets (much) more
> callbacks to slave_alloc() it has to end with -ENXIO, the user may get
> flooded with the error(!) sdev_printk on "Unexpected response from LUN
> ..." in scsi_report_lun_scan().
> In the worst case, we could get this message now 64k - 1 times in a zfcp
> scenario connected to IBM DS8000 storage being able to map (all) 64k
> volumes to a single initiator (HBA), where the user via zfcp sysfs
> decided to use only the first lun reported (for the vHBA).
>

64k - 1 times error log seems terrible. While I do not understand what
"where the user via zfcp sysfs decided to use only the first lun
reported (for the vHBA)" means.

Why would all luns slave_alloc() failed? This don't seem like a normal
scenario.

> Other LLLDs also seem to intentionally return -ENXIO from slave_alloc()
> callbacks, such as but not limited to lpfc or qla2xxx:
>
>> int fc_slave_alloc(struct scsi_device *sdev)
>> {
>>     struct fc_rport *rport = starget_to_rport(scsi_target(sdev));
>>
>>     if (!rport || fc_remote_port_chkready(rport))
>>         return -ENXIO;
>
>> static int
>> qla2xxx_slave_alloc(struct scsi_device *sdev)
>> {
>>     struct fc_rport *rport = starget_to_rport(scsi_target(sdev));
>>
>>     if (!rport || fc_remote_port_chkready(rport))
>>         return -ENXIO;
>
>

\
 
 \ /
  Last update: 2022-01-04 13:10    [W:0.042 / U:0.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site