[Qemu-devel] [PATCH 0/9] block: Fixes for bdrv_drain

[Qemu-devel] [PATCH 0/9] block: Fixes for bdrv_drain

[Fam Zheng]

Previously bdrv_drain and bdrv_drain_all don't handle ioctl, flush and discard
requests (which are fundamentally the same as read and write requests that
change disk state).  Forgetting such requests leaves us in risk of violating
the invariant that bdrv_drain() callers rely on - all asynchronous requests
must have completed after bdrv_drain returns.

Enrich the tracked request types, and add tracked_request_begin/end pairs to
all three code paths. As a prerequisite, ioctl code is moved into coroutine
too.

The last two patches take care of QED's "need check" timer, so that after
bdrv_drain returns, the driver is in a consistent state.

Fam


Fam Zheng (9):
  block: Add more types for tracked request
  block: Track flush requests
  block: Track discard requests
  iscsi: Emulate commands in iscsi_aio_ioctl as iscsi_ioctl
  block: Add ioctl parameter fields to BlockRequest
  block: Emulate bdrv_ioctl with bdrv_aio_ioctl and track both
  block: Drop BlockDriver.bdrv_ioctl
  block: Introduce BlockDriver.bdrv_drain callback
  qed: Implement .bdrv_drain

 block/io.c                | 144 +++++++++++++++++++++++++++++++++++++++-------
 block/iscsi.c             |  72 ++++++++++++-----------
 block/qed.c               |  13 +++++
 block/raw-posix.c         |   9 ---
 block/raw_bsd.c           |   6 --
 include/block/block.h     |  16 ++++--
 include/block/block_int.h |  17 +++++-
 7 files changed, 200 insertions(+), 77 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/9] block: Add more types for tracked request

[Fam Zheng]

We'll track more request types besides read and write, change the
boolean field to an enum.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c                |  9 +++++----
 include/block/block_int.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index 5311473..ca0859b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -344,13 +344,14 @@ static void tracked_request_end(BdrvTrackedRequest *req)
 static void tracked_request_begin(BdrvTrackedRequest *req,
                                   BlockDriverState *bs,
                                   int64_t offset,
-                                  unsigned int bytes, bool is_write)
+                                  unsigned int bytes,
+                                  enum BdrvTrackedRequestType type)
 {
     *req = (BdrvTrackedRequest){
         .bs = bs,
         .offset         = offset,
         .bytes          = bytes,
-        .is_write       = is_write,
+        .type           = type,
         .co             = qemu_coroutine_self(),
         .serialising    = false,
         .overlap_offset = offset,
@@ -967,7 +968,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
         bytes = ROUND_UP(bytes, align);
     }
 
-    tracked_request_begin(&req, bs, offset, bytes, false);
+    tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ);
     ret = bdrv_aligned_preadv(bs, &req, offset, bytes, align,
                               use_local_qiov ? &local_qiov : qiov,
                               flags);
@@ -1286,7 +1287,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
      * Pad qiov with the read parts and be sure to have a tracked request not
      * only for bdrv_aligned_pwritev, but also for the reads of the RMW cycle.
      */
-    tracked_request_begin(&req, bs, offset, bytes, true);
+    tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_WRITE);
 
     if (!qiov) {
         ret = bdrv_co_do_zero_pwritev(bs, offset, bytes, flags, &req);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a480f94..936a3fc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -59,11 +59,19 @@
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
+enum BdrvTrackedRequestType {
+    BDRV_TRACKED_READ,
+    BDRV_TRACKED_WRITE,
+    BDRV_TRACKED_FLUSH,
+    BDRV_TRACKED_IOCTL,
+    BDRV_TRACKED_DISCARD,
+};
+
 typedef struct BdrvTrackedRequest {
     BlockDriverState *bs;
     int64_t offset;
     unsigned int bytes;
-    bool is_write;
+    enum BdrvTrackedRequestType type;
 
     bool serialising;
     int64_t overlap_offset;
-- 
2.4.3

[Qemu-devel] [PATCH 2/9] block: Track flush requests

[Fam Zheng]

Both bdrv_flush and bdrv_aio_flush eventually call bdrv_co_flush, add
tracked_request_begin and tracked_request_end pair in that function so
that all flush requests are now tracked.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index ca0859b..223c4e9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2309,18 +2309,20 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque)
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
     int ret;
+    BdrvTrackedRequest req;
 
     if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
         bdrv_is_sg(bs)) {
         return 0;
     }
 
+    tracked_request_begin(&req, bs, 0, 0, BDRV_TRACKED_FLUSH);
     /* Write back cached data to the OS even with cache=unsafe */
     BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
     if (bs->drv->bdrv_co_flush_to_os) {
         ret = bs->drv->bdrv_co_flush_to_os(bs);
         if (ret < 0) {
-            return ret;
+            goto out;
         }
     }
 
@@ -2360,14 +2362,17 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
         ret = 0;
     }
     if (ret < 0) {
-        return ret;
+        goto out;
     }
 
     /* Now flush the underlying protocol.  It will also have BDRV_O_NO_FLUSH
      * in the case of cache=unsafe, so there are no useless flushes.
      */
 flush_parent:
-    return bs->file ? bdrv_co_flush(bs->file->bs) : 0;
+    ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
+out:
+    tracked_request_end(&req);
+    return ret;
 }
 
 int bdrv_flush(BlockDriverState *bs)
-- 
2.4.3

[Qemu-devel] [PATCH 3/9] block: Track discard requests

[Fam Zheng]

Both bdrv_discard and bdrv_aio_discard will call into bdrv_co_discard,
so add tracked_request_begin/end calls around the loop.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 223c4e9..abb3aaa 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2415,7 +2415,8 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
 int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors)
 {
-    int max_discard, ret;
+    BdrvTrackedRequest req;
+    int max_discard, ret = 0;
 
     if (!bs->drv) {
         return -ENOMEDIUM;
@@ -2437,6 +2438,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
+    tracked_request_begin(&req, bs, sector_num, nb_sectors,
+                          BDRV_TRACKED_DISCARD);
     bdrv_set_dirty(bs, sector_num, nb_sectors);
 
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
@@ -2470,20 +2473,23 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
             acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
                                             bdrv_co_io_em_complete, &co);
             if (acb == NULL) {
-                return -EIO;
+                ret = -EIO;
+                goto out;
             } else {
                 qemu_coroutine_yield();
                 ret = co.ret;
             }
         }
         if (ret && ret != -ENOTSUP) {
-            return ret;
+            goto out;
         }
 
         sector_num += num;
         nb_sectors -= num;
     }
-    return 0;
+out:
+    tracked_request_end(&req);
+    return ret;
 }
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
-- 
2.4.3

[Qemu-devel] [PATCH 4/9] iscsi: Emulate commands in iscsi_aio_ioctl as iscsi_ioctl

[Fam Zheng]

iscsi_ioctl emulates SG_GET_VERSION_NUM and SG_GET_SCSI_ID. Now that
bdrv_ioctl() will be emulated with .bdrv_aio_ioctl, replicate the logic
into iscsi_aio_ioctl to make them consistent.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 93f1ee4..94cbdf2 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -96,6 +96,7 @@ typedef struct IscsiAIOCB {
     int status;
     int64_t sector_num;
     int nb_sectors;
+    int ret;
 #ifdef __linux__
     sg_io_hdr_t *ioh;
 #endif
@@ -726,6 +727,37 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     iscsi_schedule_bh(acb);
 }
 
+static void iscsi_ioctl_bh_completion(void *opaque)
+{
+    IscsiAIOCB *acb = opaque;
+
+    qemu_bh_delete(acb->bh);
+    acb->common.cb(acb->common.opaque, acb->ret);
+}
+
+static void iscsi_ioctl_handle_emulated(IscsiAIOCB *acb, int req, void *buf)
+{
+    BlockDriverState *bs = acb->common.bs;
+    IscsiLun *iscsilun = bs->opaque;
+    int ret = 0;
+
+    switch (req) {
+    case SG_GET_VERSION_NUM:
+        *(int *)buf = 30000;
+        break;
+    case SG_GET_SCSI_ID:
+        ((struct sg_scsi_id *)buf)->scsi_type = iscsilun->type;
+        break;
+    default:
+        ret = -EINVAL;
+    }
+    assert(!acb->bh);
+    acb->bh = aio_bh_new(bdrv_get_aio_context(bs),
+                         iscsi_ioctl_bh_completion, acb);
+    acb->ret = ret;
+    qemu_bh_schedule(acb->bh);
+}
+
 static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockCompletionFunc *cb, void *opaque)
@@ -735,8 +767,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     struct iscsi_data data;
     IscsiAIOCB *acb;
 
-    assert(req == SG_IO);
-
     acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque);
 
     acb->iscsilun = iscsilun;
@@ -745,6 +775,11 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     acb->buf         = NULL;
     acb->ioh         = buf;
 
+    if (req != SG_IO) {
+        iscsi_ioctl_handle_emulated(acb, req, buf);
+        return &acb->common;
+    }
+
     acb->task = malloc(sizeof(struct scsi_task));
     if (acb->task == NULL) {
         error_report("iSCSI: Failed to allocate task for scsi command. %s",
-- 
2.4.3

[Qemu-devel] [PATCH 5/9] block: Add ioctl parameter fields to BlockRequest

[Fam Zheng]

The two fields that will be used by ioctl handling code later are added
as union, because it's used exclusively by ioctl code which dosn't need
the four fields in the other struct of the union.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/block.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 84f05ad..b3d55aa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -340,10 +340,18 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb);
 
 typedef struct BlockRequest {
     /* Fields to be filled by multiwrite caller */
-    int64_t sector;
-    int nb_sectors;
-    int flags;
-    QEMUIOVector *qiov;
+    union {
+        struct {
+            int64_t sector;
+            int nb_sectors;
+            int flags;
+            QEMUIOVector *qiov;
+        };
+        struct {
+            int req;
+            void *buf;
+        };
+    };
     BlockCompletionFunc *cb;
     void *opaque;
 
-- 
2.4.3

[Qemu-devel] [PATCH 6/9] block: Emulate bdrv_ioctl with bdrv_aio_ioctl and track both

[Fam Zheng]

Currently all drivers that support .bdrv_aio_ioctl also implement
.bdrv_ioctl redundantly.  To track ioctl requests in block layer it is
easier if we unify the two paths, because we'll need to run it in a
coroutine, as required by tracked_request_begin. While we're at it, use
.bdrv_aio_ioctl plus aio_poll() to emulate bdrv_ioctl().

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 9 deletions(-)

diff --git a/block/io.c b/block/io.c
index abb3aaa..358c3c4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2518,26 +2518,112 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
     return rwco.ret;
 }
 
+typedef struct {
+    CoroutineIOCompletion *co;
+    QEMUBH *bh;
+} BdrvIoctlCompletionData;
+
+static void bdrv_ioctl_bh_cb(void *opaque)
+{
+    BdrvIoctlCompletionData *data = opaque;
+
+    bdrv_co_io_em_complete(data->co, -ENOTSUP);
+    qemu_bh_delete(data->bh);
+}
+
+static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
+{
+    BlockDriver *drv = bs->drv;
+    BdrvTrackedRequest tracked_req;
+    CoroutineIOCompletion co = {
+        .coroutine = qemu_coroutine_self(),
+    };
+    BlockAIOCB *acb;
+
+    tracked_request_begin(&tracked_req, bs, 0, 0, BDRV_TRACKED_IOCTL);
+    if (!drv || !drv->bdrv_aio_ioctl) {
+        co.ret = -ENOTSUP;
+        goto out;
+    }
+
+    acb = drv->bdrv_aio_ioctl(bs, req, buf, bdrv_co_io_em_complete, &co);
+    if (!acb) {
+        BdrvIoctlCompletionData *data = g_new(BdrvIoctlCompletionData, 1);
+        data->bh = aio_bh_new(bdrv_get_aio_context(bs),
+                                bdrv_ioctl_bh_cb, data);
+        data->co = &co;
+        qemu_bh_schedule(data->bh);
+    }
+    qemu_coroutine_yield();
+out:
+    tracked_request_end(&tracked_req);
+    return co.ret;
+}
+
+typedef struct {
+    BlockDriverState *bs;
+    int req;
+    void *buf;
+    int ret;
+} BdrvIoctlCoData;
+
+static void coroutine_fn bdrv_co_ioctl_entry(void *opaque)
+{
+    BdrvIoctlCoData *data = opaque;
+    data->ret = bdrv_co_do_ioctl(data->bs, data->req, data->buf);
+}
+
 /* needed for generic scsi interface */
-
 int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
-    BlockDriver *drv = bs->drv;
+    BdrvIoctlCoData data = {
+        .bs = bs,
+        .req = req,
+        .buf = buf,
+        .ret = -EINPROGRESS,
+    };
 
-    if (drv && drv->bdrv_ioctl)
-        return drv->bdrv_ioctl(bs, req, buf);
-    return -ENOTSUP;
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_co_ioctl_entry(&data);
+    } else {
+        Coroutine *co = qemu_coroutine_create(bdrv_co_ioctl_entry);
+        qemu_coroutine_enter(co, &data);
+    }
+    while (data.ret == -EINPROGRESS) {
+        aio_poll(bdrv_get_aio_context(bs), true);
+    }
+    return data.ret;
+}
+
+static void coroutine_fn bdrv_co_aio_ioctl_entry(void *opaque)
+{
+    BlockAIOCBCoroutine *acb = opaque;
+    acb->req.error = bdrv_co_do_ioctl(acb->common.bs,
+                                      acb->req.req, acb->req.buf);
+    bdrv_co_complete(acb);
 }
 
 BlockAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockCompletionFunc *cb, void *opaque)
 {
-    BlockDriver *drv = bs->drv;
+    BlockAIOCBCoroutine *acb = qemu_aio_get(&bdrv_em_co_aiocb_info,
+                                            bs, cb, opaque);
+    acb->need_bh = true;
+    acb->req.error = -EINPROGRESS;
+    acb->req.req = req;
+    acb->req.buf = buf;
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_co_aio_ioctl_entry(acb);
+    } else {
+        Coroutine *co = qemu_coroutine_create(bdrv_co_aio_ioctl_entry);
+        qemu_coroutine_enter(co, acb);
+    }
 
-    if (drv && drv->bdrv_aio_ioctl)
-        return drv->bdrv_aio_ioctl(bs, req, buf, cb, opaque);
-    return NULL;
+    bdrv_co_maybe_schedule_bh(acb);
+    return &acb->common;
 }
 
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
-- 
2.4.3

[Qemu-devel] [PATCH 7/9] block: Drop BlockDriver.bdrv_ioctl

[Fam Zheng]

Now the callback is not used any more, drop the field along with all
implementations in block drivers, which are iscsi and raw.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c             | 33 ---------------------------------
 block/raw-posix.c         |  9 ---------
 block/raw_bsd.c           |  6 ------
 include/block/block_int.h |  1 -
 4 files changed, 49 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 94cbdf2..8134cc8 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -844,38 +844,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     return &acb->common;
 }
 
-static void ioctl_cb(void *opaque, int status)
-{
-    int *p_status = opaque;
-    *p_status = status;
-}
-
-static int iscsi_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
-{
-    IscsiLun *iscsilun = bs->opaque;
-    int status;
-
-    switch (req) {
-    case SG_GET_VERSION_NUM:
-        *(int *)buf = 30000;
-        break;
-    case SG_GET_SCSI_ID:
-        ((struct sg_scsi_id *)buf)->scsi_type = iscsilun->type;
-        break;
-    case SG_IO:
-        status = -EINPROGRESS;
-        iscsi_aio_ioctl(bs, req, buf, ioctl_cb, &status);
-
-        while (status == -EINPROGRESS) {
-            aio_poll(iscsilun->aio_context, true);
-        }
-
-        return 0;
-    default:
-        return -1;
-    }
-    return 0;
-}
 #endif
 
 static int64_t
@@ -1807,7 +1775,6 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_co_flush_to_disk = iscsi_co_flush,
 
 #ifdef __linux__
-    .bdrv_ioctl       = iscsi_ioctl,
     .bdrv_aio_ioctl   = iscsi_aio_ioctl,
 #endif
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3a527f0..70ff7e9 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2228,13 +2228,6 @@ static int fd_open(BlockDriverState *bs)
     return 0;
 }
 
-static int hdev_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
-{
-    BDRVRawState *s = bs->opaque;
-
-    return ioctl(s->fd, req, buf);
-}
-
 static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockCompletionFunc *cb, void *opaque)
@@ -2400,7 +2393,6 @@ static BlockDriver bdrv_host_device = {
 
     /* generic scsi device */
 #ifdef __linux__
-    .bdrv_ioctl         = hdev_ioctl,
     .bdrv_aio_ioctl     = hdev_aio_ioctl,
 #endif
 };
@@ -2684,7 +2676,6 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_lock_medium   = cdrom_lock_medium,
 
     /* generic scsi device */
-    .bdrv_ioctl         = hdev_ioctl,
     .bdrv_aio_ioctl     = hdev_aio_ioctl,
 };
 #endif /* __linux__ */
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 63ee911..4361c68 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -174,11 +174,6 @@ static void raw_lock_medium(BlockDriverState *bs, bool locked)
     bdrv_lock_medium(bs->file->bs, locked);
 }
 
-static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
-{
-    return bdrv_ioctl(bs->file->bs, req, buf);
-}
-
 static BlockAIOCB *raw_aio_ioctl(BlockDriverState *bs,
                                  unsigned long int req, void *buf,
                                  BlockCompletionFunc *cb,
@@ -268,7 +263,6 @@ BlockDriver bdrv_raw = {
     .bdrv_media_changed   = &raw_media_changed,
     .bdrv_eject           = &raw_eject,
     .bdrv_lock_medium     = &raw_lock_medium,
-    .bdrv_ioctl           = &raw_ioctl,
     .bdrv_aio_ioctl       = &raw_aio_ioctl,
     .create_opts          = &raw_create_opts,
     .bdrv_has_zero_init   = &raw_has_zero_init
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 936a3fc..d3b2411 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -226,7 +226,6 @@ struct BlockDriver {
     void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
 
     /* to control generic scsi devices */
-    int (*bdrv_ioctl)(BlockDriverState *bs, unsigned long int req, void *buf);
     BlockAIOCB *(*bdrv_aio_ioctl)(BlockDriverState *bs,
         unsigned long int req, void *buf,
         BlockCompletionFunc *cb, void *opaque);
-- 
2.4.3

[Qemu-devel] [PATCH 8/9] block: Introduce BlockDriver.bdrv_drain callback

[Fam Zheng]

Drivers can have internal request sources that generate IO, like the
need_check_timer in QED. Since we want quiesced periods that contain
nested event loops in block layer, we need to have a way to disable such
event sources.

Block drivers must implement the "bdrv_drain" callback if it has any
internal sources that can generate I/O activity, like a timer or a
worker thread (even in a library) that can schedule QEMUBH in an
asynchronous callback.

Update the comments of bdrv_drain and bdrv_drained_begin accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c                | 6 +++++-
 include/block/block_int.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 358c3c4..a740827 100644
--- a/block/io.c
+++ b/block/io.c
@@ -234,7 +234,8 @@ bool bdrv_requests_pending(BlockDriverState *bs)
 }
 
 /*
- * Wait for pending requests to complete on a single BlockDriverState subtree
+ * Wait for pending requests to complete on a single BlockDriverState subtree,
+ * and suspend block driver's internal I/O until next request arrives.
  *
  * Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState
  * AioContext.
@@ -247,6 +248,9 @@ void bdrv_drain(BlockDriverState *bs)
 {
     bool busy = true;
 
+    if (bs->drv && bs->drv->bdrv_drain) {
+        bs->drv->bdrv_drain(bs);
+    }
     while (busy) {
         /* Keep iterating */
          bdrv_flush_io_queue(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d3b2411..ade5387 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,6 +294,12 @@ struct BlockDriver {
      */
     int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
+    /**
+     * Drain and stop any internal sources of requests in the driver, and
+     * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+     */
+    void (*bdrv_drain)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH 9/9] qed: Implement .bdrv_drain

[Fam Zheng]

The "need_check_timer" is used to clear the "NEED_CHECK" flag in the
image header after a grace period once metadata update has finished. In
compliance to the bdrv_drain semantics we should make sure it remains
deleted once .bdrv_drain is called.

We cannot reuse qed_need_check_timer_cb because here it doesn't satisfy
the assertion.  Do the "plug" and "flush" calls manually.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qed.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index 5ea05d4..9b88895 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -375,6 +375,18 @@ static void bdrv_qed_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void bdrv_qed_drain(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+
+    /* Cancel timer and start doing I/O that were meant to happen as if it
+     * fired, that way we get bdrv_drain() taking care of the ongoing requests
+     * correctly. */
+    qed_cancel_need_check_timer(s);
+    qed_plug_allocating_write_reqs(s);
+    bdrv_aio_flush(s->bs, qed_clear_need_check, s);
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
@@ -1676,6 +1688,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_check               = bdrv_qed_check,
     .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
     .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
+    .bdrv_drain               = bdrv_qed_drain,
 };
 
 static void bdrv_qed_init(void)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4/9] iscsi: Emulate commands in iscsi_aio_ioctl as iscsi_ioctl

[Kevin Wolf]

Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> iscsi_ioctl emulates SG_GET_VERSION_NUM and SG_GET_SCSI_ID. Now that
> bdrv_ioctl() will be emulated with .bdrv_aio_ioctl, replicate the logic
> into iscsi_aio_ioctl to make them consistent.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/iscsi.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 93f1ee4..94cbdf2 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -96,6 +96,7 @@ typedef struct IscsiAIOCB {
>      int status;
>      int64_t sector_num;
>      int nb_sectors;
> +    int ret;
>  #ifdef __linux__
>      sg_io_hdr_t *ioh;
>  #endif
> @@ -726,6 +727,37 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
>      iscsi_schedule_bh(acb);
>  }
>  
> +static void iscsi_ioctl_bh_completion(void *opaque)
> +{
> +    IscsiAIOCB *acb = opaque;
> +
> +    qemu_bh_delete(acb->bh);
> +    acb->common.cb(acb->common.opaque, acb->ret);
> +}

qemu_aio_unref(acb)?

Kevin

Re: [Qemu-devel] [PATCH 3/9] block: Track discard requests

[Kevin Wolf]

Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> Both bdrv_discard and bdrv_aio_discard will call into bdrv_co_discard,
> so add tracked_request_begin/end calls around the loop.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 223c4e9..abb3aaa 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2415,7 +2415,8 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>  int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                                   int nb_sectors)
>  {
> -    int max_discard, ret;
> +    BdrvTrackedRequest req;
> +    int max_discard, ret = 0;
>  
>      if (!bs->drv) {
>          return -ENOMEDIUM;
> @@ -2437,6 +2438,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>          return 0;
>      }
>  
> +    tracked_request_begin(&req, bs, sector_num, nb_sectors,
> +                          BDRV_TRACKED_DISCARD);
>      bdrv_set_dirty(bs, sector_num, nb_sectors);
>  
>      max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
> @@ -2470,20 +2473,23 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>              acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
>                                              bdrv_co_io_em_complete, &co);
>              if (acb == NULL) {
> -                return -EIO;
> +                ret = -EIO;
> +                goto out;
>              } else {
>                  qemu_coroutine_yield();
>                  ret = co.ret;
>              }
>          }
>          if (ret && ret != -ENOTSUP) {
> -            return ret;
> +            goto out;
>          }
>  
>          sector_num += num;
>          nb_sectors -= num;
>      }
> -    return 0;
> +out:
> +    tracked_request_end(&req);
> +    return ret;
>  }

I would prefer an explicit ret = 0 before the out label because
otherwise you're relying on the previous value that has been set
somewhere in the loop. As far as I can tell, it's still correct, but it
makes it needlessly hard to tell whether success is 0 or >= 0.

Kevin

Re: [Qemu-devel] [PATCH 8/9] block: Introduce BlockDriver.bdrv_drain callback

[Kevin Wolf]

Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> Drivers can have internal request sources that generate IO, like the
> need_check_timer in QED. Since we want quiesced periods that contain
> nested event loops in block layer, we need to have a way to disable such
> event sources.
> 
> Block drivers must implement the "bdrv_drain" callback if it has any
> internal sources that can generate I/O activity, like a timer or a
> worker thread (even in a library) that can schedule QEMUBH in an
> asynchronous callback.
> 
> Update the comments of bdrv_drain and bdrv_drained_begin accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 6 +++++-
>  include/block/block_int.h | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 358c3c4..a740827 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -234,7 +234,8 @@ bool bdrv_requests_pending(BlockDriverState *bs)
>  }
>  
>  /*
> - * Wait for pending requests to complete on a single BlockDriverState subtree
> + * Wait for pending requests to complete on a single BlockDriverState subtree,
> + * and suspend block driver's internal I/O until next request arrives.
>   *
>   * Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState
>   * AioContext.
> @@ -247,6 +248,9 @@ void bdrv_drain(BlockDriverState *bs)
>  {
>      bool busy = true;
>  
> +    if (bs->drv && bs->drv->bdrv_drain) {
> +        bs->drv->bdrv_drain(bs);
> +    }

Does this need to be recursive? Recursing into bs->file and bs->backing
would be consistent with the current bdrv_requests_pending(); but I'm
planning to send a patch to extend this to all children.

Kevin

Re: [Qemu-devel] [PATCH 0/9] block: Fixes for bdrv_drain

[Kevin Wolf]

Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> Previously bdrv_drain and bdrv_drain_all don't handle ioctl, flush and discard
> requests (which are fundamentally the same as read and write requests that
> change disk state).  Forgetting such requests leaves us in risk of violating
> the invariant that bdrv_drain() callers rely on - all asynchronous requests
> must have completed after bdrv_drain returns.
> 
> Enrich the tracked request types, and add tracked_request_begin/end pairs to
> all three code paths. As a prerequisite, ioctl code is moved into coroutine
> too.
> 
> The last two patches take care of QED's "need check" timer, so that after
> bdrv_drain returns, the driver is in a consistent state.

Patches 1-3, 5-7 and 9:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 3/9] block: Track discard requests

[Fam Zheng]

On Wed, 10/28 10:54, Kevin Wolf wrote:
> > -    return 0;
> > +out:
> > +    tracked_request_end(&req);
> > +    return ret;
> >  }
> 
> I would prefer an explicit ret = 0 before the out label because
> otherwise you're relying on the previous value that has been set
> somewhere in the loop. As far as I can tell, it's still correct, but it
> makes it needlessly hard to tell whether success is 0 or >= 0.

Good point, will fix.

Fam

Re: [Qemu-devel] [PATCH 4/9] iscsi: Emulate commands in iscsi_aio_ioctl as iscsi_ioctl

[Fam Zheng]

On Wed, 10/28 10:51, Kevin Wolf wrote:
> Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> > iscsi_ioctl emulates SG_GET_VERSION_NUM and SG_GET_SCSI_ID. Now that
> > bdrv_ioctl() will be emulated with .bdrv_aio_ioctl, replicate the logic
> > into iscsi_aio_ioctl to make them consistent.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/iscsi.c | 39 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 37 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 93f1ee4..94cbdf2 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -96,6 +96,7 @@ typedef struct IscsiAIOCB {
> >      int status;
> >      int64_t sector_num;
> >      int nb_sectors;
> > +    int ret;
> >  #ifdef __linux__
> >      sg_io_hdr_t *ioh;
> >  #endif
> > @@ -726,6 +727,37 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
> >      iscsi_schedule_bh(acb);
> >  }
> >  
> > +static void iscsi_ioctl_bh_completion(void *opaque)
> > +{
> > +    IscsiAIOCB *acb = opaque;
> > +
> > +    qemu_bh_delete(acb->bh);
> > +    acb->common.cb(acb->common.opaque, acb->ret);
> > +}
> 
> qemu_aio_unref(acb)?
> 

Yes, will fix.

Fam

Re: [Qemu-devel] [PATCH 8/9] block: Introduce BlockDriver.bdrv_drain callback

[Fam Zheng]

On Wed, 10/28 11:13, Kevin Wolf wrote:
> Am 26.10.2015 um 07:24 hat Fam Zheng geschrieben:
> > Drivers can have internal request sources that generate IO, like the
> > need_check_timer in QED. Since we want quiesced periods that contain
> > nested event loops in block layer, we need to have a way to disable such
> > event sources.
> > 
> > Block drivers must implement the "bdrv_drain" callback if it has any
> > internal sources that can generate I/O activity, like a timer or a
> > worker thread (even in a library) that can schedule QEMUBH in an
> > asynchronous callback.
> > 
> > Update the comments of bdrv_drain and bdrv_drained_begin accordingly.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/io.c                | 6 +++++-
> >  include/block/block_int.h | 6 ++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 358c3c4..a740827 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -234,7 +234,8 @@ bool bdrv_requests_pending(BlockDriverState *bs)
> >  }
> >  
> >  /*
> > - * Wait for pending requests to complete on a single BlockDriverState subtree
> > + * Wait for pending requests to complete on a single BlockDriverState subtree,
> > + * and suspend block driver's internal I/O until next request arrives.
> >   *
> >   * Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState
> >   * AioContext.
> > @@ -247,6 +248,9 @@ void bdrv_drain(BlockDriverState *bs)
> >  {
> >      bool busy = true;
> >  
> > +    if (bs->drv && bs->drv->bdrv_drain) {
> > +        bs->drv->bdrv_drain(bs);
> > +    }
> 
> Does this need to be recursive? Recursing into bs->file and bs->backing
> would be consistent with the current bdrv_requests_pending(); but I'm
> planning to send a patch to extend this to all children.
> 

Yes, I'll change it to recurse into all children.

Fam