[RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Dongli Zhang]

This RFC is to trigger the discussion about to poll and kick the
virtqueue on purpose in virtio-scsi timeout handler.

The virtio-scsi relies on the virtio vring shared between VM and host.
The VM side produces requests to vring and kicks the virtqueue, while the
host side produces responses to vring and interrupts the VM side.

By default the virtio-scsi handler depends on the host timeout handler
by BLK_EH_RESET_TIMER to give host a chance to perform EH.

However, this is not helpful for the case that the responses are available
on vring but the notification from host to VM is lost.


Would you mind share your feedback on this idea to poll the vring on
purpose in timeout handler to give VM a chance to recover from stalled
IO? In addition, how about to kick for an extra time, in case the
stalled IO is due to the loss of VM-to-host notification?


I have tested the IO can be recovered after interrupt is lost on purpose
with below debug patch.

https://github.com/finallyjustice/patchset/blob/master/scsi-virtio_scsi-to-lose-an-interrupt-on-purpose.patch


In addition, the virtio-blk may implement the timeout handler as well,
with the helper function in below patchset from Stefan.

https://lore.kernel.org/linux-block/20210520141305.355961-1-stefanha@redhat.com/T/#t


Thank you very much!

Dongli Zhang

--------

Considering there might be loss of interrupt or kick issue, the timeout
handler may poll or kick the virtqueue on purpose, in order to recover the
IO.

If the response is already available on vring, it indicates the host side
has already processed the request and it is not helpful by giving host a
chance to perform EH.

There will be syslog like below by the timeout handler:

[  135.830746] virtio_scsi: Virtio SCSI HBA 0: I/O 196 QID 3 timeout, completion polled

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/scsi/virtio_scsi.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b9c86a7e3b97..633950b6336a 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -36,6 +36,9 @@
 #define VIRTIO_SCSI_EVENT_LEN 8
 #define VIRTIO_SCSI_VQ_BASE 2
 
+static bool __read_mostly timed_out_enabled;
+module_param(timed_out_enabled, bool, 0644);
+
 /* Command queue element */
 struct virtio_scsi_cmd {
 	struct scsi_cmnd *sc;
@@ -732,9 +735,38 @@ static void virtscsi_commit_rqs(struct Scsi_Host *shost, u16 hwq)
  * The host guarantees to respond to each command, although I/O
  * latencies might be higher than on bare metal.  Reset the timer
  * unconditionally to give the host a chance to perform EH.
+ *
+ * If 'timed_out_enabled' is set, the timeout handler polls or kicks the
+ * virtqueue on purpose, in order to recover the IO, in case there is loss
+ * of interrupt or kick issue with virtio.
  */
 static enum blk_eh_timer_return virtscsi_eh_timed_out(struct scsi_cmnd *scmnd)
 {
+	struct Scsi_Host *shost;
+	struct virtio_scsi *vscsi;
+	struct virtio_scsi_vq *req_vq;
+
+	if (!timed_out_enabled)
+		return BLK_EH_RESET_TIMER;
+
+	shost = scmnd->device->host;
+	vscsi = shost_priv(shost);
+	req_vq = virtscsi_pick_vq_mq(vscsi, scmnd);
+
+	virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
+
+	if (test_bit(SCMD_STATE_COMPLETE, &scmnd->state)) {
+		pr_warn("Virtio SCSI HBA %u: I/O %u QID %d timeout, completion polled\n",
+			shost->host_no, scmnd->tag, req_vq->vq->index);
+		return BLK_EH_DONE;
+	}
+
+	/*
+	 * To kick the virtqueue in case the timeout is due to the loss of
+	 * one prior notification.
+	 */
+	virtqueue_notify(req_vq->vq);
+
 	return BLK_EH_RESET_TIMER;
 }
 
-- 
2.17.1

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Hannes Reinecke]

On 5/23/21 8:38 AM, Dongli Zhang wrote:
> This RFC is to trigger the discussion about to poll and kick the
> virtqueue on purpose in virtio-scsi timeout handler.
> 
> The virtio-scsi relies on the virtio vring shared between VM and host.
> The VM side produces requests to vring and kicks the virtqueue, while the
> host side produces responses to vring and interrupts the VM side.
> 
> By default the virtio-scsi handler depends on the host timeout handler
> by BLK_EH_RESET_TIMER to give host a chance to perform EH.
> 
> However, this is not helpful for the case that the responses are available
> on vring but the notification from host to VM is lost.
> 
How can this happen?
If responses are lost the communication between VM and host is broken, 
and we should rather reset the virtio rings themselves.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Sun, May 23, 2021 at 09:39:51AM +0200, Hannes Reinecke wrote:
> On 5/23/21 8:38 AM, Dongli Zhang wrote:
> > This RFC is to trigger the discussion about to poll and kick the
> > virtqueue on purpose in virtio-scsi timeout handler.
> > 
> > The virtio-scsi relies on the virtio vring shared between VM and host.
> > The VM side produces requests to vring and kicks the virtqueue, while the
> > host side produces responses to vring and interrupts the VM side.
> > 
> > By default the virtio-scsi handler depends on the host timeout handler
> > by BLK_EH_RESET_TIMER to give host a chance to perform EH.
> > 
> > However, this is not helpful for the case that the responses are available
> > on vring but the notification from host to VM is lost.
> > 
> How can this happen?
> If responses are lost the communication between VM and host is broken, and
> we should rather reset the virtio rings themselves.

I agree. In principle it's fine to poll the virtqueue at any time, but I
don't understand the failure scenario here. It's not clear to me why the
device-to-driver vq notification could be lost.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Dongli Zhang]

Hi Stefan and Hannes,

On 5/24/21 6:24 AM, Stefan Hajnoczi wrote:
> On Sun, May 23, 2021 at 09:39:51AM +0200, Hannes Reinecke wrote:
>> On 5/23/21 8:38 AM, Dongli Zhang wrote:
>>> This RFC is to trigger the discussion about to poll and kick the
>>> virtqueue on purpose in virtio-scsi timeout handler.
>>>
>>> The virtio-scsi relies on the virtio vring shared between VM and host.
>>> The VM side produces requests to vring and kicks the virtqueue, while the
>>> host side produces responses to vring and interrupts the VM side.
>>>
>>> By default the virtio-scsi handler depends on the host timeout handler
>>> by BLK_EH_RESET_TIMER to give host a chance to perform EH.
>>>
>>> However, this is not helpful for the case that the responses are available
>>> on vring but the notification from host to VM is lost.
>>>
>> How can this happen?
>> If responses are lost the communication between VM and host is broken, and
>> we should rather reset the virtio rings themselves.
> 
> I agree. In principle it's fine to poll the virtqueue at any time, but I
> don't understand the failure scenario here. It's not clear to me why the
> device-to-driver vq notification could be lost.
> 

One example is the CPU hotplug issue before the commit bf0beec0607d ("blk-mq:
drain I/O when all CPUs in a hctx are offline") was available. The issue is
equivalent to loss of interrupt. Without the CPU hotplug fix, while NVMe driver
relies on the timeout handler to complete inflight IO requests, the PV
virtio-scsi may hang permanently.

In addition, as the virtio/vhost/QEMU are complex software, we are not able to
guarantee there is no further lost of interrupt/kick issue in the future. It is
really painful if we encounter such issue in production environment.


About to reset vring, if this is just due to loss of interrupt, I do not think
it is necessary to reset the entire vring. To poll the vring should be enough.
The NVMe PCI does the same by assuming there may be loss of interrupt.

Once there is request timeout, the NVMe PCI driver first polls the ring buffer
and confirm if the request is completed, instead of reset/abort immediately.


1254 static enum blk_eh_timer_return nvme_timeout(struct request *req, bool
reserved)
... ...
1280         /*
1281          * Did we miss an interrupt?
1282          */
1283         if (test_bit(NVMEQ_POLLED, &nvmeq->flags))
1284                 nvme_poll(req->mq_hctx);
1285         else
1286                 nvme_poll_irqdisable(nvmeq);
1287
1288         if (blk_mq_request_completed(req)) {
1289                 dev_warn(dev->ctrl.device,
1290                          "I/O %d QID %d timeout, completion polled\n",
1291                          req->tag, nvmeq->qid);
1292                 return BLK_EH_DONE;
1293         }


Thank you very much!

Dongli Zhang

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

On Mon, May 24, 2021 at 11:33:33PM -0700, Dongli Zhang wrote:
> On 5/24/21 6:24 AM, Stefan Hajnoczi wrote:
> > On Sun, May 23, 2021 at 09:39:51AM +0200, Hannes Reinecke wrote:
> >> On 5/23/21 8:38 AM, Dongli Zhang wrote:
> >>> This RFC is to trigger the discussion about to poll and kick the
> >>> virtqueue on purpose in virtio-scsi timeout handler.
> >>>
> >>> The virtio-scsi relies on the virtio vring shared between VM and host.
> >>> The VM side produces requests to vring and kicks the virtqueue, while the
> >>> host side produces responses to vring and interrupts the VM side.
> >>>
> >>> By default the virtio-scsi handler depends on the host timeout handler
> >>> by BLK_EH_RESET_TIMER to give host a chance to perform EH.
> >>>
> >>> However, this is not helpful for the case that the responses are available
> >>> on vring but the notification from host to VM is lost.
> >>>
> >> How can this happen?
> >> If responses are lost the communication between VM and host is broken, and
> >> we should rather reset the virtio rings themselves.
> > 
> > I agree. In principle it's fine to poll the virtqueue at any time, but I
> > don't understand the failure scenario here. It's not clear to me why the
> > device-to-driver vq notification could be lost.
> > 
> 
> One example is the CPU hotplug issue before the commit bf0beec0607d ("blk-mq:
> drain I/O when all CPUs in a hctx are offline") was available. The issue is
> equivalent to loss of interrupt. Without the CPU hotplug fix, while NVMe driver
> relies on the timeout handler to complete inflight IO requests, the PV
> virtio-scsi may hang permanently.
> 
> In addition, as the virtio/vhost/QEMU are complex software, we are not able to
> guarantee there is no further lost of interrupt/kick issue in the future. It is
> really painful if we encounter such issue in production environment.

Any number of hardware or software bugs might exist that we don't know
about, yet we don't pre-emptively add workarounds for them because where
do you draw the line?

I checked other SCSI/block drivers and found it's rare to poll in the
timeout function so there does not seem to be a consensus that it's
useful to do this.

That said, it's technically fine to do it, the virtqueue APIs are there
and can be used like this. So if you and others think this is necessary,
then it's a pretty small change and I'm not against merging a patch like
this.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Hannes Reinecke]

On 5/25/21 6:47 PM, Stefan Hajnoczi wrote:
> On Mon, May 24, 2021 at 11:33:33PM -0700, Dongli Zhang wrote:
>> On 5/24/21 6:24 AM, Stefan Hajnoczi wrote:
>>> On Sun, May 23, 2021 at 09:39:51AM +0200, Hannes Reinecke wrote:
>>>> On 5/23/21 8:38 AM, Dongli Zhang wrote:
>>>>> This RFC is to trigger the discussion about to poll and kick the
>>>>> virtqueue on purpose in virtio-scsi timeout handler.
>>>>>
>>>>> The virtio-scsi relies on the virtio vring shared between VM and host.
>>>>> The VM side produces requests to vring and kicks the virtqueue, while the
>>>>> host side produces responses to vring and interrupts the VM side.
>>>>>
>>>>> By default the virtio-scsi handler depends on the host timeout handler
>>>>> by BLK_EH_RESET_TIMER to give host a chance to perform EH.
>>>>>
>>>>> However, this is not helpful for the case that the responses are available
>>>>> on vring but the notification from host to VM is lost.
>>>>>
>>>> How can this happen?
>>>> If responses are lost the communication between VM and host is broken, and
>>>> we should rather reset the virtio rings themselves.
>>>
>>> I agree. In principle it's fine to poll the virtqueue at any time, but I
>>> don't understand the failure scenario here. It's not clear to me why the
>>> device-to-driver vq notification could be lost.
>>>
>>
>> One example is the CPU hotplug issue before the commit bf0beec0607d ("blk-mq:
>> drain I/O when all CPUs in a hctx are offline") was available. The issue is
>> equivalent to loss of interrupt. Without the CPU hotplug fix, while NVMe driver
>> relies on the timeout handler to complete inflight IO requests, the PV
>> virtio-scsi may hang permanently.
>>
>> In addition, as the virtio/vhost/QEMU are complex software, we are not able to
>> guarantee there is no further lost of interrupt/kick issue in the future. It is
>> really painful if we encounter such issue in production environment.
> 
> Any number of hardware or software bugs might exist that we don't know
> about, yet we don't pre-emptively add workarounds for them because where
> do you draw the line?
> 
> I checked other SCSI/block drivers and found it's rare to poll in the
> timeout function so there does not seem to be a consensus that it's
> useful to do this.
> 
Not only this; it's downright dangerous attempting to do that in SCSI.
In SCSI we don't have fixed lifetime guarantees that NVMe has, so there 
will be a race condition between timeout and command completion.
Plus there is no interface in SCSI allowing to 'poll' for completions in 
a meaningful manner.

> That said, it's technically fine to do it, the virtqueue APIs are there
> and can be used like this. So if you and others think this is necessary,
> then it's a pretty small change and I'm not against merging a patch like
> this.
> 
I would rather _not_ put more functionality into the virtio_scsi timeout 
handler; this only serves to assume that the timeout handler has some 
functionality in virtio.
Which it patently hasn't, as the prime reason for a timeout handler is 
to _abort_ a command, which we can't on virtio.
Well, we can on virtio, but qemu as the main user will re-route the I/O 
from virtio into doing async-I/O, and there is no way how we can abort 
outstanding asynchronous I/O.
Or any other ioctl, for that matter.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [RFC] virtio_scsi: to poll and kick the virtqueue in timeout handler

[Dongli Zhang]

On 5/25/21 10:24 AM, Hannes Reinecke wrote:
> On 5/25/21 6:47 PM, Stefan Hajnoczi wrote:
>> On Mon, May 24, 2021 at 11:33:33PM -0700, Dongli Zhang wrote:
>>> On 5/24/21 6:24 AM, Stefan Hajnoczi wrote:
>>>> On Sun, May 23, 2021 at 09:39:51AM +0200, Hannes Reinecke wrote:
>>>>> On 5/23/21 8:38 AM, Dongli Zhang wrote:
>>>>>> This RFC is to trigger the discussion about to poll and kick the
>>>>>> virtqueue on purpose in virtio-scsi timeout handler.
>>>>>>
>>>>>> The virtio-scsi relies on the virtio vring shared between VM and host.
>>>>>> The VM side produces requests to vring and kicks the virtqueue, while the
>>>>>> host side produces responses to vring and interrupts the VM side.
>>>>>>
>>>>>> By default the virtio-scsi handler depends on the host timeout handler
>>>>>> by BLK_EH_RESET_TIMER to give host a chance to perform EH.
>>>>>>
>>>>>> However, this is not helpful for the case that the responses are available
>>>>>> on vring but the notification from host to VM is lost.
>>>>>>
>>>>> How can this happen?
>>>>> If responses are lost the communication between VM and host is broken, and
>>>>> we should rather reset the virtio rings themselves.
>>>>
>>>> I agree. In principle it's fine to poll the virtqueue at any time, but I
>>>> don't understand the failure scenario here. It's not clear to me why the
>>>> device-to-driver vq notification could be lost.
>>>>
>>>
>>> One example is the CPU hotplug issue before the commit bf0beec0607d ("blk-mq:
>>> drain I/O when all CPUs in a hctx are offline") was available. The issue is
>>> equivalent to loss of interrupt. Without the CPU hotplug fix, while NVMe driver
>>> relies on the timeout handler to complete inflight IO requests, the PV
>>> virtio-scsi may hang permanently.
>>>
>>> In addition, as the virtio/vhost/QEMU are complex software, we are not able to
>>> guarantee there is no further lost of interrupt/kick issue in the future. It is
>>> really painful if we encounter such issue in production environment.
>>
>> Any number of hardware or software bugs might exist that we don't know
>> about, yet we don't pre-emptively add workarounds for them because where
>> do you draw the line?
>>
>> I checked other SCSI/block drivers and found it's rare to poll in the
>> timeout function so there does not seem to be a consensus that it's
>> useful to do this.
>>
> Not only this; it's downright dangerous attempting to do that in SCSI.
> In SCSI we don't have fixed lifetime guarantees that NVMe has, so there will be
> a race condition between timeout and command completion.

Thank you very much for the explanation. Yes, we cannot do that due to the race.

Dongli Zhang


> Plus there is no interface in SCSI allowing to 'poll' for completions in a
> meaningful manner.
> 
>> That said, it's technically fine to do it, the virtqueue APIs are there
>> and can be used like this. So if you and others think this is necessary,
>> then it's a pretty small change and I'm not against merging a patch like
>> this.
>>
> I would rather _not_ put more functionality into the virtio_scsi timeout
> handler; this only serves to assume that the timeout handler has some
> functionality in virtio.
> Which it patently hasn't, as the prime reason for a timeout handler is to
> _abort_ a command, which we can't on virtio.
> Well, we can on virtio, but qemu as the main user will re-route the I/O from
> virtio into doing async-I/O, and there is no way how we can abort outstanding
> asynchronous I/O.
> Or any other ioctl, for that matter.
> 
> Cheers,
> 
> Hannes