linux-kernel - Re: [PATCH] scsi: virtio_scsi: unplug LUNs when events missed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190904173848.GD4571@tesla>
Date:   Wed, 4 Sep 2019 17:38:53 +0000
From:   Matt Lupfer <mlupfer@....com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
CC:     Jason Wang <jasowang@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        "James E. J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] scsi: virtio_scsi: unplug LUNs when events missed

On Wed, Sep 04, 2019 at 05:14:33AM -0400, Michael S. Tsirkin wrote:
> On Tue, Sep 03, 2019 at 05:04:20PM +0000, Matt Lupfer wrote:
>> The event handler calls scsi_scan_host() when events are missed, which
>> will hotplug new LUNs.  However, this function won't remove any
>> unplugged LUNs.  The result is that hotunplug doesn't work properly when
>> the number of unplugged LUNs exceeds the event queue size (currently 8).
>>
>> Scan existing LUNs when events are missed to check if they are still
>> present.  If not, remove them.
>>
>> Signed-off-by: Matt Lupfer <mlupfer@....com>
>> ---
>>  drivers/scsi/virtio_scsi.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index 297e1076e571..18df77bf371b 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
>> @@ -324,6 +324,36 @@ static void virtscsi_handle_param_change(struct virtio_scsi *vscsi,
>>  	scsi_device_put(sdev);
>>  }
>>
>> +static void virtscsi_rescan_hotunplug(struct virtio_scsi *vscsi)
>> +{
>> +	struct scsi_device *sdev;
>> +	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>> +	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>> +	int result, inquiry_len, inq_result_len = 256;
>> +	char *inq_result = kmalloc(inq_result_len, GFP_KERNEL);
>> +
>> +	shost_for_each_device(sdev, shost) {
>> +		inquiry_len = sdev->inquiry_len ? sdev->inquiry_len : 36;
>> +
>> +		memset(scsi_cmd, 0, sizeof(scsi_cmd));
>> +		scsi_cmd[0] = INQUIRY;
>> +		scsi_cmd[4] = (unsigned char) inquiry_len;
>> +
>> +		memset(inq_result, 0, inq_result_len);
>> +
>> +		result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
>> +					  inq_result, inquiry_len, NULL,
>> +					  2, 3, NULL);
>
>
> Where do the weird 2 and 3 values come from?
>
> Most callers seem to use SD_TIMEOUT, SD_MAX_RETRIES...
>

The value of 3 retries is from scsi_probe_lun() in scsi_scan.c.

The value of 2 seconds is arbitrary, but equals SCSI_TIMEOUT.
scsi_inq_timeout in scsi_scan.c is complicated for reasons unknown to
me, but is quite a bit longer, more in line with SD_TIMEOUT.

I will send a V2 patch with the SD_TIMEOUT and SD_MAX_RETRIES macros
from drivers/scsi/sd.h.

Thanks for taking a look.

Matt