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

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

[Matt Lupfer]

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@ddn.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);
+
+		if (result == 0 && inq_result[0] >> 5) {
+			/* PQ indicates the LUN is not attached */
+			scsi_remove_device(sdev);
+		}
+	}
+
+	kfree(inq_result);
+}
+
 static void virtscsi_handle_event(struct work_struct *work)
 {
 	struct virtio_scsi_event_node *event_node =
@@ -335,6 +365,7 @@ static void virtscsi_handle_event(struct work_struct *work)
 	    cpu_to_virtio32(vscsi->vdev, VIRTIO_SCSI_T_EVENTS_MISSED)) {
 		event->event &= ~cpu_to_virtio32(vscsi->vdev,
 						   VIRTIO_SCSI_T_EVENTS_MISSED);
+		virtscsi_rescan_hotunplug(vscsi);
 		scsi_scan_host(virtio_scsi_host(vscsi->vdev));
 	}
 
-- 
2.23.0

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

[Michael S. Tsirkin]

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@ddn.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...

> +
> +		if (result == 0 && inq_result[0] >> 5) {
> +			/* PQ indicates the LUN is not attached */
> +			scsi_remove_device(sdev);
> +		}
> +	}
> +
> +	kfree(inq_result);
> +}
> +
>  static void virtscsi_handle_event(struct work_struct *work)
>  {
>  	struct virtio_scsi_event_node *event_node =
> @@ -335,6 +365,7 @@ static void virtscsi_handle_event(struct work_struct *work)
>  	    cpu_to_virtio32(vscsi->vdev, VIRTIO_SCSI_T_EVENTS_MISSED)) {
>  		event->event &= ~cpu_to_virtio32(vscsi->vdev,
>  						   VIRTIO_SCSI_T_EVENTS_MISSED);
> +		virtscsi_rescan_hotunplug(vscsi);
>  		scsi_scan_host(virtio_scsi_host(vscsi->vdev));
>  	}
>  
> -- 
> 2.23.0

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@ddn.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