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 <[email protected]>
---
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;
}
}
}
--
2.32.0
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 <[email protected]>
> ---
> 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;
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z and LinuxONE
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
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 <[email protected]>
>> ---
>> 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 <[email protected]>
> ---
> 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 ...