Skip to contentSkip to conversation

Virtio command timeouts (qemu part)

RFC PATCH New Hannes Reinecke 1 participant 5 messages May 12, 2017

lore
Share & tools
lore

Hannes Reinecke

author
#0 lore raw

cover Virtio command timeouts (qemu part)

Hi all,

we've run into a really awkward customer situation where the guest would hang forever due to an SG_IO ioctl on the host not returning. Looking into it we found that qemu will submit direct I/O requests with an _infinite_ timeout (well, actually UINT_MAX, which due to a kernel bug gets translated into (ULONG)-2, resulting in a timeout of 4.2 years :-).
And this particular I/O ran into a timeout on the wire due to a flaky connection. Which resulted in the 'normal' block-level timeout on the host being disabled, and the SCSI stack never sending any aborts as the block-layer was still waiting for the I/O timeout to expire.

Unfortunately I didn't find a way to create a stand-alone patch; the fix I'm proposing relies on fixes for qemu running on the host and the kernel side running on the guest.

The proposed fix consists of several parts:
- make the standard device-timeout user-settable via a 'timeout' attribute to 'scsi-disk' and 'scsi-generic'
- Add a kernel patch to implement a eh_timeout_handler() for virtio_scsi(); this patch just checks if the command is still pending and resets the timer if so.
- Add a request timeout to allow drivers to modify the timeout on a per-request base.
- Implement a new VIRTIO_SCSI_F_TIMEOUT feature allowing virtio-scsi to pass in a timeout via the otherwise unused 'crn' field.
- Add a kernel patch to implement the VIRTIO_SCSI_F_TIMEOUT feature so that the timeout is added per virtio request.

With that virtio-scsi on the guest can pass in the used timeout to the qemu on the host side, which then can use this timeout to issue I/O requests to the host.
The host can then properly aborting a command if the timeout is hit, and the aborted command will be returned to the guest.
The guest itself doesn't need to (and, in fact, in most cases can't) abort any commands anymore, so it just need to reset the I/O timer until the requests are returned.

However, as this is quite an elaborate construct I'd like to get some feedback for it.

Hannes Reinecke (4):
scsi: make default command timeout user-settable
scsi: use host default timeouts for SCSI commands
scsi: per-request timeouts
virtio: implement VIRTIO_SCSI_F_TIMEOUT feature

 hw/scsi/scsi-bus.c                           |  1 +
 hw/scsi/scsi-disk.c                          | 16 ++++++++++++----
 hw/scsi/scsi-generic.c                       | 11 +++++++++--
 hw/scsi/virtio-scsi.c                        | 16 ++++++++++++++++
 include/hw/scsi/scsi.h                       |  2 ++
 include/standard-headers/linux/virtio_scsi.h |  1 +
 6 files changed, 41 insertions(+), 6 deletions(-)
Signature · 1 line
2.12.0
author
#1 lore raw

PATCH 1/4 scsi: make default command timeout user-settable

Per default any SCSI commands are sent with an infinite timeout, which essentially disables any command abort mechanism on the host and causes the guest to stall.
This patch adds a new option 'timeout' for scsi-generic and scsi-disk which allows the user to set the timeout value to something sensible.

Signed-off-by Hannes Reinecke <[email protected]>
---
 hw/scsi/scsi-disk.c    | 4 +++-
 hw/scsi/scsi-generic.c | 5 ++++-
 include/hw/scsi/scsi.h | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)
hw/scsi/scsi-disk.c +3 -1
index a53f058621..dd01ff7e06 100644
@@ -2679,7 +2679,7 @@ static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
     /* The rest is as in scsi-generic.c.  */
     io_header->mx_sb_len = sizeof(r->req.sense);
     io_header->sbp = r->req.sense;
-    io_header->timeout = UINT_MAX;
+    io_header->timeout = s->qdev.timeout;
     io_header->usr_ptr = r;
     io_header->flags |= SG_FLAG_DIRECT_IO;
 

@@ -2898,6 +2898,8 @@ static Property scsi_hd_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout,
+                       MAX_UINT),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
hw/scsi/scsi-generic.c +4 -1
index a55ff87c22..fd02a0f4b2 100644
@@ -157,6 +157,8 @@ static int execute_command(BlockBackend *blk,
                            SCSIGenericReq *r, int direction,
                            BlockCompletionFunc *complete)
 {
+    SCSIDevice *s = r->req.dev;
+
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
     r->io_header.dxferp = r->buf;

@@ -165,7 +167,7 @@ static int execute_command(BlockBackend *blk,
     r->io_header.cmd_len = r->req.cmd.len;
     r->io_header.mx_sb_len = sizeof(r->req.sense);
     r->io_header.sbp = r->req.sense;
-    r->io_header.timeout = MAX_UINT;
+    r->io_header.timeout = s->timeout;
     r->io_header.usr_ptr = r;
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 

@@ -599,6 +601,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
 
 static Property scsi_generic_properties[] = {
     DEFINE_PROP_DRIVE("drive", SCSIDevice, conf.blk),
+    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout, MAX_UINT),
     DEFINE_PROP_END_OF_LIST(),
 };
include/hw/scsi/scsi.h +1 -0
index 6b85786dbf..a976e85cfa 100644
@@ -110,6 +110,7 @@ struct SCSIDevice
     uint64_t max_lba;
     uint64_t wwn;
     uint64_t port_wwn;
+    uint32_t timeout;
 };
 
 extern const VMStateDescription vmstate_scsi_device;
Signature · 1 line
2.12.0
author
#2 lore raw

PATCH 2/4 scsi: use host default timeouts for SCSI commands

In reply to Hannes Reinecke

Instead of disabling command aborts by setting the command timeout to infinity we should be setting it to '0' per default, allowing the host to fall back to its default values.

Signed-off-by Hannes Reinecke <[email protected]>
---
 hw/scsi/scsi-disk.c    | 3 +--
 hw/scsi/scsi-generic.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
hw/scsi/scsi-disk.c +1 -2
index dd01ff7e06..4ac4c872fe 100644
@@ -2898,8 +2898,7 @@ static Property scsi_hd_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
-    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout,
-                       MAX_UINT),
+    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
hw/scsi/scsi-generic.c +1 -1
index fd02a0f4b2..998b6a4558 100644
@@ -601,7 +601,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
 
 static Property scsi_generic_properties[] = {
     DEFINE_PROP_DRIVE("drive", SCSIDevice, conf.blk),
-    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout, MAX_UINT),
+    DEFINE_PROP_UINT32("timeout", SCSIDevice, timeout, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
Signature · 1 line
2.12.0
author
#3 lore raw

PATCH 3/4 scsi: per-request timeouts

In reply to Hannes Reinecke

Add a 'timeout' value per request to allow to specify individual per-request timeouts.

Signed-off-by Hannes Reinecke <[email protected]>
---
 hw/scsi/scsi-bus.c     |  1 +
 hw/scsi/scsi-disk.c    | 15 +++++++++++----
 hw/scsi/scsi-generic.c | 12 ++++++++----
 include/hw/scsi/scsi.h |  1 +
 4 files changed, 21 insertions(+), 8 deletions(-)
hw/scsi/scsi-bus.c +1 -0
index f5574469c8..206000d873 100644
@@ -690,6 +690,7 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
 
     req->cmd = cmd;
     req->resid = req->cmd.xfer;
+    req->timeout = d->timeout;
 
     switch (buf[0]) {
     case INQUIRY:
hw/scsi/scsi-disk.c +11 -4
index 4ac4c872fe..6a6f091a9c 100644
@@ -2503,7 +2503,9 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
         printf("\n");
     }
 #endif
-
+    if (req) {
+        req->timeout = d->timeout;
+    }
     return req;
 }
 

@@ -2679,7 +2681,7 @@ static BlockAIOCB *scsi_block_do_sgio(SCSIBlockReq *req,
     /* The rest is as in scsi-generic.c.  */
     io_header->mx_sb_len = sizeof(r->req.sense);
     io_header->sbp = r->req.sense;
-    io_header->timeout = s->qdev.timeout;
+    io_header->timeout = r->req.timeout;
     io_header->usr_ptr = r;
     io_header->flags |= SG_FLAG_DIRECT_IO;
 

@@ -2812,14 +2814,19 @@ static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
                                            void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
+    struct SCSIRequest *req;
 
     if (scsi_block_is_passthrough(s, buf)) {
-        return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
+        req = scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
                               hba_private);
     } else {
-        return scsi_req_alloc(&scsi_block_dma_reqops, &s->qdev, tag, lun,
+        req = scsi_req_alloc(&scsi_block_dma_reqops, &s->qdev, tag, lun,
                               hba_private);
     }
+    if (req) {
+        req->timeout = d->timeout;
+    }
+    return req;
 }
 
 static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
hw/scsi/scsi-generic.c +8 -4
index 998b6a4558..31c3a75bad 100644
@@ -157,8 +157,6 @@ static int execute_command(BlockBackend *blk,
                            SCSIGenericReq *r, int direction,
                            BlockCompletionFunc *complete)
 {
-    SCSIDevice *s = r->req.dev;
-
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
     r->io_header.dxferp = r->buf;

@@ -167,7 +165,7 @@ static int execute_command(BlockBackend *blk,
     r->io_header.cmd_len = r->req.cmd.len;
     r->io_header.mx_sb_len = sizeof(r->req.sense);
     r->io_header.sbp = r->req.sense;
-    r->io_header.timeout = s->timeout;
+    r->io_header.timeout = r->req.timeout;
     r->io_header.usr_ptr = r;
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 

@@ -596,7 +594,13 @@ const SCSIReqOps scsi_generic_req_ops = {
 static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
                                      uint8_t *buf, void *hba_private)
 {
-    return scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
+    struct SCSIRequest *req;
+
+    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
+    if (req) {
+        req->timeout = d->timeout;
+    }
+    return req;
 }
 
 static Property scsi_generic_properties[] = {
include/hw/scsi/scsi.h +1 -0
index a976e85cfa..6c51ddc3ef 100644
@@ -51,6 +51,7 @@ struct SCSIRequest {
     uint32_t          tag;
     uint32_t          lun;
     uint32_t          status;
+    uint32_t          timeout;
     void              *hba_private;
     size_t            resid;
     SCSICommand       cmd;
Signature · 1 line
2.12.0
author
#4 lore raw

PATCH 4/4 virtio: implement VIRTIO_SCSI_F_TIMEOUT feature

In reply to Hannes Reinecke

Implement a handler for the VIRTIO_SCSI_F_TIMEOUT feature, which allows to pass in the assigned command timeout in seconds or minutes. This allows to specify a timeout up to 3 hours.

Signed-off-by Hannes Reinecke <[email protected]>
---
 hw/scsi/virtio-scsi.c                        | 16 ++++++++++++++++
 include/standard-headers/linux/virtio_scsi.h |  1 +
 2 files changed, 17 insertions(+)
hw/scsi/virtio-scsi.c +16 -0
index 46a3e3f280..f1c2f7cb5b 100644
@@ -534,6 +534,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
 static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
     VirtIOSCSICommon *vs = &s->parent_obj;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     SCSIDevice *d;
     int rc;
 

@@ -560,6 +561,21 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
                              virtio_scsi_get_lun(req->req.cmd.lun),
                              req->req.cmd.cdb, req);
 
+    if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_TIMEOUT)) {
+        int timeout = (int)req->req.cmd.crn;
+
+        if (timeout < 60) {
+            /* Timeouts below 60 are in seconds */
+            req->sreq->timeout = timeout * 1000;
+        } else if (timeout == 255) {
+            /* 255 is infinite timeout */
+            req->sreq->timeout = UINT_MAX;
+        } else {
+            /* Otherwise the timeout is in minutes */
+            req->sreq->timeout = timeout * 1000 * 60;
+        }
+    }
+
     if (req->sreq->cmd.mode != SCSI_XFER_NONE
         && (req->sreq->cmd.mode != req->mode ||
             req->sreq->cmd.xfer > req->qsgl.size)) {
include/standard-headers/linux/virtio_scsi.h +1 -0
index ab66166b6a..b00e2b95ab 100644
@@ -120,6 +120,7 @@ struct virtio_scsi_config {
 #define VIRTIO_SCSI_F_HOTPLUG                  1
 #define VIRTIO_SCSI_F_CHANGE                   2
 #define VIRTIO_SCSI_F_T10_PI                   3
+#define VIRTIO_SCSI_F_TIMEOUT                  4
 
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
Signature · 1 line
2.12.0