[PATCH hci-8.1 2/5] scsi-disk: fix UNMAP racing with inactivation of block nodes #VSTOR-151532

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

scsi-disk turns a multi-descriptor UNMAP into one discard per
descriptor and submits the next one from the completion callback of
the previous. Nothing holds the BlockBackend in-flight counter between
two descriptors, so a drain only waits for the discard currently in
flight. When that discard completes inside drained_begin, the callback
submits the next one, blk_wait_while_drained() queues it, and the drain
returns with the UNMAP half done.

bdrv_inactivate_all() drains, inactivates the nodes and ends the drain,
which resumes the queued discard on an inactive node:

  bdrv_co_write_req_prepare: Assertion
  `!(bs->open_flags & BDRV_O_INACTIVE)' failed.

A guest that issues UNMAP at the end of a live migration, or around
blockdev-set-active, takes QEMU down this way.

Handle UNMAP the way commit 095c08a7ba handles IDE TRIM: process the
descriptors in a coroutine between blk_co_start_request() and
blk_end_request() and submit them with BDRV_REQ_NO_QUEUE. A drain now
waits for the whole command instead of a single descriptor, the same
trade-off IDE makes. The coroutine is wrapped in an AIOCB that stays in
r->req.aiocb, so cancellation keeps working as before.

A failed descriptor ends the loop, and the completion applies the
rerror/werror policy as before. werror=ignore is handled inside the
loop, so the remaining descriptors are still discarded as the callback
chain did.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/scsi/scsi-disk.c | 160 +++++++++++++++++++++++++++++++-------------
 1 file changed, 113 insertions(+), 47 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c608fc56036..8383b7cbe93 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1742,61 +1742,135 @@ static inline bool check_lba_range(SCSIDiskState *s,
             sector_num + nb_sectors <= s->qdev.max_lba + 1);
 }
 
-typedef struct UnmapCBData {
+typedef struct SCSIDiskCoAIOCB SCSIDiskCoAIOCB;
+
+typedef int coroutine_fn SCSIDiskCoFunc(SCSIDiskCoAIOCB *iocb);
+
+struct SCSIDiskCoAIOCB {
+    BlockAIOCB common;
     SCSIDiskReq *r;
-    uint8_t *inbuf;
-    int count;
-} UnmapCBData;
+    SCSIDiskCoFunc *fn;
+    int ret;
+    bool canceled;
+};
+
+static void scsi_disk_co_aio_cancel(BlockAIOCB *acb)
+{
+    SCSIDiskCoAIOCB *iocb = container_of(acb, SCSIDiskCoAIOCB, common);
+
+    iocb->canceled = true;
+}
 
-static void scsi_unmap_complete(void *opaque, int ret);
+static const AIOCBInfo scsi_disk_co_aiocb_info = {
+    .aiocb_size         = sizeof(SCSIDiskCoAIOCB),
+    .cancel_async       = scsi_disk_co_aio_cancel,
+};
 
-static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
+static void scsi_disk_co_aio_bh(void *opaque)
 {
-    SCSIDiskReq *r = data->r;
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskCoAIOCB *iocb = opaque;
+    BlockBackend *blk = iocb->r->req.dev->conf.blk;
 
-    assert(r->req.aiocb == NULL);
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+    qemu_aio_unref(iocb);
 
-    if (data->count > 0) {
-        uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
-        uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
-        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    /* Paired with blk_co_start_request in scsi_disk_co_aio_entry() */
+    blk_end_request(blk);
+}
 
-        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
-                         r->sector_count * BDRV_SECTOR_SIZE,
-                         BLOCK_ACCT_UNMAP);
+static void coroutine_fn scsi_disk_co_aio_entry(void *opaque)
+{
+    SCSIDiskCoAIOCB *iocb = opaque;
 
-        r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
-                                        r->sector * BDRV_SECTOR_SIZE,
-                                        r->sector_count * BDRV_SECTOR_SIZE,
-                                        scsi_unmap_complete, data);
-        data->count--;
-        data->inbuf += 16;
-        return;
-    }
+    /* Paired with blk_end_request in scsi_disk_co_aio_bh() */
+    blk_co_start_request(iocb->r->req.dev->conf.blk);
+    iocb->ret = iocb->fn(iocb);
+    aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
+                            scsi_disk_co_aio_bh, iocb);
+}
 
-    scsi_req_complete(&r->req, GOOD);
+static void scsi_disk_co_aio_complete(void *opaque, int ret)
+{
+    SCSIDiskReq *r = opaque;
+
+    assert(r->req.ctx == qemu_get_current_aio_context());
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+
+    if (!scsi_disk_req_check_error(r, ret, true)) {
+        scsi_req_complete(&r->req, GOOD);
+    }
     scsi_req_unref(&r->req);
-    g_free(data);
 }
 
-static void scsi_unmap_complete(void *opaque, int ret)
+/* fn must submit with BDRV_REQ_NO_QUEUE and stop once iocb->canceled */
+static void scsi_disk_co_aio_start(SCSIDiskReq *r, SCSIDiskCoFunc *fn)
 {
-    UnmapCBData *data = opaque;
-    SCSIDiskReq *r = data->r;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskCoAIOCB *iocb;
 
-    assert(r->req.aiocb != NULL);
-    r->req.aiocb = NULL;
+    iocb = blk_aio_get(&scsi_disk_co_aiocb_info, s->qdev.conf.blk,
+                       scsi_disk_co_aio_complete, r);
+    iocb->r = r;
+    iocb->fn = fn;
+    iocb->ret = 0;
+    iocb->canceled = false;
 
-    if (scsi_disk_req_check_error(r, ret, true)) {
-        scsi_req_unref(&r->req);
-        g_free(data);
-    } else {
-        block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
-        scsi_unmap_complete_noio(data, ret);
+    /* The matching unref is in scsi_disk_co_aio_complete */
+    scsi_req_ref(&r->req);
+    r->req.aiocb = &iocb->common;
+    aio_co_enter(qemu_get_current_aio_context(),
+                 qemu_coroutine_create(scsi_disk_co_aio_entry, iocb));
+}
+
+/* Report like scsi_handle_rw_error(), but let werror=ignore go on */
+static bool scsi_disk_ignore_error(SCSIDiskReq *r, int ret)
+{
+    BlockBackend *blk = r->req.dev->conf.blk;
+    BlockErrorAction action = blk_get_error_action(blk, false, -ret);
+
+    if (action != BLOCK_ERROR_ACTION_IGNORE) {
+        return false;
+    }
+    blk_error_action(blk, action, false, r->sector << BDRV_SECTOR_BITS,
+                     r->sector_count << BDRV_SECTOR_BITS, -ret);
+    return true;
+}
+
+static int coroutine_fn scsi_unmap_co(SCSIDiskCoAIOCB *iocb)
+{
+    SCSIDiskReq *r = iocb->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    BlockBackend *blk = s->qdev.conf.blk;
+    uint8_t *p = r->iov.iov_base;
+    int count = lduw_be_p(&p[2]) >> 4;
+    int i, ret;
+
+    for (i = 0; i < count; i++) {
+        uint64_t sector_num = ldq_be_p(&p[8 + i * 16]);
+        uint32_t nb_sectors = ldl_be_p(&p[8 + i * 16 + 8]);
+
+        if (iocb->canceled) {
+            return -ECANCELED;
+        }
+
+        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+
+        block_acct_start(blk_get_stats(blk), &r->acct,
+                         r->sector_count * BDRV_SECTOR_SIZE,
+                         BLOCK_ACCT_UNMAP);
+
+        ret = blk_co_pdiscard(blk, r->sector * BDRV_SECTOR_SIZE,
+                              r->sector_count * BDRV_SECTOR_SIZE,
+                              BDRV_REQ_NO_QUEUE);
+        if (ret < 0 && !scsi_disk_ignore_error(r, ret)) {
+            return ret;
+        }
+        block_acct_done(blk_get_stats(blk), &r->acct);
     }
+
+    return 0;
 }
 
 static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
@@ -1804,7 +1878,6 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     uint8_t *p = inbuf;
     int len = r->req.cmd.xfer;
-    UnmapCBData *data;
     int count;
     int i;
 
@@ -1843,14 +1916,7 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
         }
     }
 
-    data = g_new0(UnmapCBData, 1);
-    data->r = r;
-    data->inbuf = &p[8];
-    data->count = count;
-
-    /* The matching unref is in scsi_unmap_complete, before data is freed.  */
-    scsi_req_ref(&r->req);
-    scsi_unmap_complete_noio(data, 0);
+    scsi_disk_co_aio_start(r, scsi_unmap_co);
     return;
 
 invalid_param_len:
-- 
2.53.0