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

[Matt Lupfer]

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