[PATCH] scsi: Avoid unnecessary iterations on __scsi_scan_target

[PATCH] scsi: Avoid unnecessary iterations on __scsi_scan_target

[Anjali Kulkarni]

There is a loop in scsi_scan_channel(), which calls
__scsi_scan_target() 255 times, even when it has found a target/device on a
lun in the first iteration; this ends up adding a 2 secs delay to the boot
time. The for loop in scsi_scan_channel() adding 2 secs to boot time is as
follows:

for (id = 0; id < shost->max_id; ++id) {
     ...
     __scsi_scan_target(&shost->shost_gendev, channel,
                               order_id, lun, rescan);
}

__scsi_scan_target() calls scsi_probe_and_add_lun() which calls
scsi_probe_lun(), hence scsi_probe_lun() ends up getting called 255 times.
Each call of scsi_probe_lun() takes 0.007865 secs.
0.007865 multiplied by 255 = 2.00557 secs.
By adding a break in above for loop when a valid device on lun is found,
we can avoid this 2 secs delay improving boot time by 2 secs.

The flow of code is depicted in the following sequence of events:

do_scan_async() ->
     do_scsi_scan_host()->
         scsi_scan_host_selected() ->
             scsi_scan_channel()
                 __scsi_scan_target() -> this is called shost->max_id times
                                         (255 times)
                     scsi_probe_and_add_lun-> this is called 255 times
                         scsi_probe_lun : called 255 times, each call
                                          takes 0.007865 secs.

The with break and without break boot time differences can be seen with
systemd-analyze:

With break:

[root@vm-sec root]# systemd-analyze
Startup finished in 2.038s (kernel) + 1.210s (initrd) + 14.198s 
(userspace) = 17.446s

Without break:

[root@vm-sec root]# systemd-analyze
Startup finished in 2.036s (kernel) + 3.342s (initrd) + 14.255s 
(userspace) = 19.634s

Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
---
  drivers/scsi/scsi_scan.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f2437a7570ce..a519b944b512 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1527,23 +1527,23 @@ void scsi_rescan_device(struct device *dev)
  }
  EXPORT_SYMBOL(scsi_rescan_device);

-static void __scsi_scan_target(struct device *parent, unsigned int channel,
+static int __scsi_scan_target(struct device *parent, unsigned int channel,
          unsigned int id, u64 lun, enum scsi_scan_mode rescan)
  {
      struct Scsi_Host *shost = dev_to_shost(parent);
      blist_flags_t bflags = 0;
-    int res;
+    int res = SCSI_SCAN_NO_RESPONSE;
      struct scsi_target *starget;

      if (shost->this_id == id)
          /*
           * Don't scan the host adapter
           */
-        return;
+        return res;

      starget = scsi_alloc_target(parent, channel, id);
      if (!starget)
-        return;
+        return res;
      scsi_autopm_get_target(starget);

      if (lun != SCAN_WILD_CARD) {
@@ -1551,6 +1551,9 @@ static void __scsi_scan_target(struct device 
*parent, unsigned int channel,
           * Scan for a specific host/chan/id/lun.
           */
          scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL);
+        res = scsi_probe_and_add_lun(starget, lun, NULL, NULL,
+                rescan, NULL);
+
          goto out_reap;
      }

@@ -1578,6 +1581,7 @@ static void __scsi_scan_target(struct device 
*parent, unsigned int channel,
      scsi_target_reap(starget);

      put_device(&starget->dev);
+    return res;
  }

  /**
@@ -1646,8 +1650,9 @@ static void scsi_scan_channel(struct Scsi_Host 
*shost, unsigned int channel,
                  order_id = shost->max_id - id - 1;
              else
                  order_id = id;
-            __scsi_scan_target(&shost->shost_gendev, channel,
-                    order_id, lun, rescan);
+            if (__scsi_scan_target(&shost->shost_gendev, channel,
+                order_id, lun, rescan) != SCSI_SCAN_NO_RESPONSE)
+                break;
          }
      else
          __scsi_scan_target(&shost->shost_gendev, channel,
-- 
2.26.2

Re: [PATCH] scsi: Avoid unnecessary iterations on __scsi_scan_target

[James Bottomley]

On Fri, 2020-06-26 at 16:53 -0700, Anjali Kulkarni wrote:
> There is a loop in scsi_scan_channel(), which calls
> __scsi_scan_target() 255 times, even when it has found a
> target/device on a
> lun in the first iteration; this ends up adding a 2 secs delay to the
> boot
> time. The for loop in scsi_scan_channel() adding 2 secs to boot time
> is as
> follows:
> 
> for (id = 0; id < shost->max_id; ++id) {
>      ...
>      __scsi_scan_target(&shost->shost_gendev, channel,
>                                order_id, lun, rescan);
> }
> 
> __scsi_scan_target() calls scsi_probe_and_add_lun() which calls
> scsi_probe_lun(), hence scsi_probe_lun() ends up getting called 255
> times.
> Each call of scsi_probe_lun() takes 0.007865 secs.
> 0.007865 multiplied by 255 = 2.00557 secs.
> By adding a break in above for loop when a valid device on lun is
> found,
> we can avoid this 2 secs delay improving boot time by 2 secs.
> 
> The flow of code is depicted in the following sequence of events:
> 
> do_scan_async() ->
>      do_scsi_scan_host()->
>          scsi_scan_host_selected() ->
>              scsi_scan_channel()
>                  __scsi_scan_target() -> this is called shost->max_id 
> times
>                                          (255 times)
>                      scsi_probe_and_add_lun-> this is called 255
> times
>                          scsi_probe_lun : called 255 times, each call
>                                           takes 0.007865 secs.

What HBA is this?  This code is for legacy scanning of busses which
require it, which is pretty much only SPI.  The max_id of even the
latest SPI bus should only be 16, so where is 255 coming from?

James

Re: [PATCH] scsi: Avoid unnecessary iterations on __scsi_scan_target

[Anjali Kulkarni]

Thanks James.

This is on a VM on OCI cloud, using virtio scsi.

Here is output of lspci on the VM:

00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02)
00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE 
[Natoma/Triton II]
00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB 
[Natoma/Triton II] (rev 01)
00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
00:02.0 VGA compatible controller: Device 1234:1111 (rev 02)
00:03.0 Ethernet controller: Red Hat, Inc. Virtio network device
00:04.0 SCSI storage controller: Red Hat, Inc. Virtio SCSI

With 16 iterations, we could reduce time by 0.13 secs.

Anjali

On 6/26/20 17:03, James Bottomley wrote:
> On Fri, 2020-06-26 at 16:53 -0700, Anjali Kulkarni wrote:
>> There is a loop in scsi_scan_channel(), which calls
>> __scsi_scan_target() 255 times, even when it has found a
>> target/device on a
>> lun in the first iteration; this ends up adding a 2 secs delay to the
>> boot
>> time. The for loop in scsi_scan_channel() adding 2 secs to boot time
>> is as
>> follows:
>>
>> for (id = 0; id < shost->max_id; ++id) {
>>       ...
>>       __scsi_scan_target(&shost->shost_gendev, channel,
>>                                 order_id, lun, rescan);
>> }
>>
>> __scsi_scan_target() calls scsi_probe_and_add_lun() which calls
>> scsi_probe_lun(), hence scsi_probe_lun() ends up getting called 255
>> times.
>> Each call of scsi_probe_lun() takes 0.007865 secs.
>> 0.007865 multiplied by 255 = 2.00557 secs.
>> By adding a break in above for loop when a valid device on lun is
>> found,
>> we can avoid this 2 secs delay improving boot time by 2 secs.
>>
>> The flow of code is depicted in the following sequence of events:
>>
>> do_scan_async() ->
>>       do_scsi_scan_host()->
>>           scsi_scan_host_selected() ->
>>               scsi_scan_channel()
>>                   __scsi_scan_target() -> this is called shost->max_id
>> times
>>                                           (255 times)
>>                       scsi_probe_and_add_lun-> this is called 255
>> times
>>                           scsi_probe_lun : called 255 times, each call
>>                                            takes 0.007865 secs.
> What HBA is this?  This code is for legacy scanning of busses which
> require it, which is pretty much only SPI.  The max_id of even the
> latest SPI bus should only be 16, so where is 255 coming from?
>
> James
>

Re: [PATCH] scsi: Avoid unnecessary iterations on __scsi_scan_target

[James Bottomley]

On Fri, 2020-06-26 at 17:17 -0700, Anjali Kulkarni wrote:
> Thanks James.
> 
> This is on a VM on OCI cloud, using virtio scsi.
> 
> Here is output of lspci on the VM:
> 
> 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma]
> (rev 02)
> 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA
> [Natoma/Triton II]
> 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE 
> [Natoma/Triton II]
> 00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB 
> [Natoma/Triton II] (rev 01)
> 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
> 00:02.0 VGA compatible controller: Device 1234:1111 (rev 02)
> 00:03.0 Ethernet controller: Red Hat, Inc. Virtio network device
> 00:04.0 SCSI storage controller: Red Hat, Inc. Virtio SCSI

I think the right thing to do would be to fix virtio-SCSI.  It already
has the guest to host communication to know how many targets are
configured.  There's shouldn't be any need to do scanning.

James