Re: [PATCH] scsi: Do not break scan luns loop if add single lun failed

[Wenchao Hao]

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@xxxxxxxxxx>
> > ---
> >   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;