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

From: Matt Lupfer
Date: Wed Sep 04 2019 - 13:39:41 EST


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