| Message ID | 20211225232911.1117843-1-haowenchao@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: Do not break scan luns loop if add single lun failed | expand |
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? > > 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 ? > */ > 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. > - break; > + continue; > } > } > } Wouldn't this change existing semantics for LLDDs intentionally returning -ENXIO from their slave_alloc() callback in certain cases?: > 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). 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;
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; > >
On 2021/12/26 7: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. > > 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 > */ > sdev_printk(KERN_ERR, sdev, > "Unexpected response" > " from lun %llu while scanning, scan" > " aborted\n", (unsigned long long)lun); > - break; > + continue; > } > } > } > ping ...
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 */ sdev_printk(KERN_ERR, sdev, "Unexpected response" " from lun %llu while scanning, scan" " aborted\n", (unsigned long long)lun); - break; + continue; } } }
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. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/scsi/scsi_scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)