lkml.org 
[lkml]   [2021]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] scsi: Do not break scan luns loop if add single lun failed
From
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;


--
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

\
 
 \ /
  Last update: 2021-12-30 19:01    [W:0.051 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site