[Qemu-devel] [RFC PATCH 0/4] Virtio command timeouts (qemu part)

[Qemu-devel] [RFC PATCH 0/4] Virtio command timeouts (qemu part)

[Hannes Reinecke]

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(-)

-- 
2.12.0

[Qemu-devel] [PATCH 1/4] scsi: make default command timeout user-settable

[Hannes Reinecke]

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 <hare@suse.com>
---
 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(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a53f058621..dd01ff7e06 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -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(),
 };
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index a55ff87c22..fd02a0f4b2 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -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(),
 };
 
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 6b85786dbf..a976e85cfa 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -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;
-- 
2.12.0

[Qemu-devel] [PATCH 2/4] scsi: use host default timeouts for SCSI commands

[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 <hare@suse.com>
---
 hw/scsi/scsi-disk.c    | 3 +--
 hw/scsi/scsi-generic.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index dd01ff7e06..4ac4c872fe 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -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(),
 };
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index fd02a0f4b2..998b6a4558 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -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(),
 };
 
-- 
2.12.0

[Qemu-devel] [PATCH 3/4] scsi: per-request timeouts

[Hannes Reinecke]

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

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 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(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f5574469c8..206000d873 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -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:
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4ac4c872fe..6a6f091a9c 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -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,
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 998b6a4558..31c3a75bad 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -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[] = {
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index a976e85cfa..6c51ddc3ef 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -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;
-- 
2.12.0

[Qemu-devel] [PATCH 4/4] virtio: implement VIRTIO_SCSI_F_TIMEOUT feature

[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 <hare@suse.com>
---
 hw/scsi/virtio-scsi.c                        | 16 ++++++++++++++++
 include/standard-headers/linux/virtio_scsi.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 46a3e3f280..f1c2f7cb5b 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -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)) {
diff --git a/include/standard-headers/linux/virtio_scsi.h b/include/standard-headers/linux/virtio_scsi.h
index ab66166b6a..b00e2b95ab 100644
--- a/include/standard-headers/linux/virtio_scsi.h
+++ b/include/standard-headers/linux/virtio_scsi.h
@@ -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
-- 
2.12.0