Support streaming to an intermediate layer

Hi!

Half a year later :) here's the new version of the intermediate block streaming patches.

Quick summary: the feature was already working since June but there were two problems that made it impossible to merge the patches. Here's the full description:

   https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

And here's the TL;DR version:

1) Opening a BDS in read-write mode with bdrv_reopen() has the side effect of reopening its backing chain in read-only mode.

2) bdrv_reopen() can trigger the completion of an existing block job. If that job modifies the backing chain in which that reopen call is operating, it can crash QEMU.

Now, these problems are only relevant if there are two block jobs running at the same time in the same backing chain. This is a nice feature, but it's not essential for the intermediate block streaming operation. Therefore I decided to disable it in this version of the series.

The way it works is that if you run block-stream on an intermediate node, it will block the active layer as well, preventing other block jobs in the same chain.

Other than that there are no significant changes compared to v8, but a couple of things are different:

- The code has been rebased
- The patch that introduced block_job_next() has been split into
several: one that adds the function and the rest that use it in different parts of the QEMU code.
- Some patches have already been merged into QEMU so they have been removed from the series.
- The test that ran several block-stream operations in parallel now verifies that they are forbidden.

I think that's all. Comments and questions are welcome!

Berto

v9:
- Rebase the code
- Block the active layer in order to forbid other block jobs in the same chain.
- Split the patch that adds block_job_next() into 4 (new patches 1-4).
- Replace the test that performs several block-stream operations in parallel with one that check that they're forbidden.
- Remove patches that have already been merged.

v8: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05754.html
- Rebased on top of Stefan's block branch (0a35bce416)
- The loop that pauses the block jobs in bdrv_drain_all() is now split in two: one that iterates the list of block jobs to stop them, and one that iterates the root bds in order to get the aio contexts.

v7: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html
- Rebased against the current master
- Updated bdrv_drain_all() to use the new block_job_next() API.

v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html
- fix the no-op test following Max's suggestions

v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html
- Fix a few typos
- Minor documentation updates
- Update test_stream_partial() to test no-ops
- New test case: test_stream_parallel()
- New test case: test_stream_overlapping()

v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html
- Refactor find_block_job to use the error from bdrv_lookup_bs()
- Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be dealing with nodes now.
- Fix @device comment in the BlockJobInfo documentation
- stream_start(): simplify the bdrv_reopen() call and use bdrv_get_device_or_node_name() for error messages.
- Use a different variable name for BlockDriverState *i
- Documentation fixes in docs/live-block-ops.txt
- Update iotest 30 since now test_device_not_found() returns GenericError
- Fix test case test_stream_partial()
- Add new test case test_stream_intermediate()
- Fix typos

v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html
- Keep a list of block jobs and make qmp_query_block_jobs() iterate over it.

v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html
- The 'block-stream' command does not have a 'node-name' parameter anymore and reuses 'device' for that purpose.
- Block jobs can now be owned by any intermediate node, and not just by the ones at the root. query-block-jobs is updated to reflect that change.
- The 'device' parameter of all 'block-job-*' commands can now take a node name.
- The BlockJobInfo type and all BLOCK_JOB_* events report the node name in the 'device' field if the node does not have a device name.
- All intermediate nodes are blocked (and checked for blockers) during the streaming operation.

v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html

Alberto Garcia (11):
block: keep a list of block jobs
block: use the block job list in bdrv_drain_all()
block: use the block job list in qmp_query_block_jobs()
block: use the block job list in bdrv_close()
block: allow block jobs in any arbitrary node
block: Support streaming to an intermediate layer
block: Add QMP support for streaming to an intermediate layer
docs: Document how to stream to an intermediate layer
qemu-iotests: test streaming to an intermediate layer
qemu-iotests: test overlapping block-stream operations
qemu-iotests: test non-overlapping block-stream operations

 block.c                    |  29 ++++---------
 block/io.c                 |  21 ++++++----
 block/stream.c             |  39 ++++++++++++++++-
 blockdev.c                 |  68 ++++++++++++++++++------------
 blockjob.c                 |  18 +++++++-
 docs/live-block-ops.txt    |  31 +++++++++-----
 docs/qmp-events.txt        |   8 ++--
 include/block/block_int.h  |   5 ++-
 include/block/blockjob.h   |  14 +++++++
 qapi/block-core.json       |  30 ++++++++------
 tests/qemu-iotests/030     | 101 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |   4 +-
 12 files changed, 276 insertions(+), 92 deletions(-)

Signature · 1 line

2.8.0.rc3

The current way to obtain the list of existing block jobs is to iterate over all root nodes and check which ones own a job.

Since we want to be able to support block jobs in other nodes as well, this patch keeps a list of jobs that is updated every time one is created or destroyed.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

bdrv_drain_all() pauses all block jobs by using bdrv_next() to iterate over all top-level BlockDriverStates. Therefore the code is unable to find block jobs in other nodes.

This patch uses block_job_next() to iterate over all block jobs.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/io.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

block/io.c +9 -3

index c4869b9..884ed1e 100644
@@ -288,15 +288,21 @@ void bdrv_drain_all(void)
     /* Always run first iteration so any pending completion BHs run */
     bool busy = true;
     BlockDriverState *bs = NULL;
+    BlockJob *job = NULL;
     GSList *aio_ctxs = NULL, *ctx;
 
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = bdrv_get_aio_context(job->bs);
+
+        aio_context_acquire(aio_context);
+        block_job_pause(job);
+        aio_context_release(aio_context);
+    }
+
     while ((bs = bdrv_next(bs))) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
-        if (bs->job) {
-            block_job_pause(bs->job);
-        }
         bdrv_drain_recurse(bs);
         aio_context_release(aio_context);
 

block/io.c +1 -3

         aio_context_acquire(aio_context);
-        if (bs->job) {
-            block_job_resume(bs->job);
-        }
+        block_job_resume(job);
         aio_context_release(aio_context);
     }
     g_slist_free(aio_ctxs);
 

block/io.c void bdrv_drain_all(void) · line 343

Quoted 5 lines

block/io.c +2 -3

-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    while ((job = block_job_next(job))) {
+        AioContext *aio_context = bdrv_get_aio_context(job->bs);

Technically, the "bs = NULL;" before didn't do anything either. But in my opinion, it made the code more readable, therefore I'd really like a "job = NULL;" before this loop, too.

qmp_query_block_jobs() uses bdrv_next() to look for block jobs, but this function can only find those in top-level BlockDriverStates.

This patch uses block_job_next() instead.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

bdrv_close_all() cancels all block jobs by iterating over all BlockDriverStates. This patch simplifies the code by iterating directly over the block jobs using block_job_next().

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

block.c +1 -15

index d36eb75..48638c9 100644
@@ -2182,8 +2182,7 @@ static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockDriverState *bs;
-    AioContext *aio_context;
+    BlockJob *job;
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */

@@ -2193,23 +2192,11 @@ void bdrv_close_all(void)
     blockdev_close_all_bdrv_states();
 
     /* Cancel all block jobs */
-    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
-        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
-            aio_context = bdrv_get_aio_context(bs);
-
-            aio_context_acquire(aio_context);
-            if (bs->job) {
-                block_job_cancel_sync(bs->job);
-                aio_context_release(aio_context);
-                break;
-            }
-            aio_context_release(aio_context);
-        }
-

block.c +0 -0

 }
 

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c           | 18 ++++++++++--------
 blockjob.c           |  5 +++--
 docs/qmp-events.txt  |  8 ++++----
 qapi/block-core.json | 20 ++++++++++----------
 4 files changed, 27 insertions(+), 24 deletions(-)

blockdev.c +4 -2

index edbcc19..d1f5dfb 100644
@@ -3685,8 +3685,10 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     aio_context_release(aio_context);
 }
 
-/* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context,
+/* Get the block job for a given device or node name
+ * and acquire its AioContext */
+static BlockJob *find_block_job(const char *device_or_node,
+                                AioContext **aio_context,
                                 Error **errp)
 {
     BlockBackend *blk;

blockdev.c +0 -0

     if (*aio_context) {
         aio_context_release(*aio_context);
         *aio_context = NULL;

blockjob.c +2 -1

index 3557048..2ab4794 100644
@@ -67,7 +67,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     BlockJob *job;
 
     if (bs->job) {
-        error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
+        error_setg(errp, "Node '%s' is in use",
+                   bdrv_get_device_or_node_name(bs));
         return NULL;
     }
     bdrv_ref(bs);

This makes sure that the image we are steaming into is open in read-write mode during the operation.

The block job is created on the destination image, but operation blockers are also set on the active layer. We do this in order to prevent other block jobs from running in parallel in the same chain. See here for details on why that is currently not supported:

[Qemu-block] RFC: Status of the intermediate block streaming work
https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

Finally, this also unblocks the stream operation in backing files.

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c                   |  4 +++-
 block/stream.c            | 39 ++++++++++++++++++++++++++++++++++++++-
 blockdev.c                |  2 +-
 include/block/block_int.h |  5 ++++-
 4 files changed, 46 insertions(+), 4 deletions(-)

block.c +3 -1

index 48638c9..f7698a1 100644
@@ -1252,9 +1252,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
             backing_hd->drv ? backing_hd->drv->format_name : "");
 
     bdrv_op_block_all(backing_hd, bs->backing_blocker);
-    /* Otherwise we won't be able to commit due to check in bdrv_commit */
+    /* Otherwise we won't be able to commit or stream */
     bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
                     bs->backing_blocker);
+    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
+                    bs->backing_blocker);
 out:
     bdrv_refresh_limits(bs, NULL);
 }

block/stream.c +23 -1

index 332b9a1..ac02ddf 100644
@@ -35,8 +35,10 @@ typedef struct StreamBlockJob {
     BlockJob common;
     RateLimit limit;
     BlockDriverState *base;
+    BlockDriverState *active;
     BlockdevOnError on_error;
     char *backing_file_str;
+    int bs_flags;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockDriverState *bs,

@@ -66,6 +68,11 @@ static void stream_complete(BlockJob *job, void *opaque)
     StreamCompleteData *data = opaque;
     BlockDriverState *base = s->base;
 
+    if (s->active) {
+        bdrv_op_unblock_all(s->active, s->common.blocker);
+        bdrv_unref(s->active);
+    }
+
     if (!block_job_is_cancelled(&s->common) && data->reached_end &&
         data->ret == 0) {
         const char *base_id = NULL, *base_fmt = NULL;

@@ -79,6 +86,11 @@ static void stream_complete(BlockJob *job, void *opaque)
         bdrv_set_backing_hd(job->bs, base);
     }
 
+    /* Reopen the image back in read-only mode if necessary */
+    if (s->bs_flags != bdrv_get_flags(job->bs)) {
+        bdrv_reopen(job->bs, s->bs_flags, NULL);
+    }
+
     g_free(s->backing_file_str);
     block_job_completed(&s->common, data->ret);
     g_free(data);

@@ -216,13 +228,15 @@ static const BlockJobDriver stream_job_driver = {
     .set_speed     = stream_set_speed,
 };
 
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
+void stream_start(BlockDriverState *bs, BlockDriverState *active,
+                  BlockDriverState *base,
                   const char *backing_file_str, int64_t speed,
                   BlockdevOnError on_error,
                   BlockCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
     StreamBlockJob *s;
+    int orig_bs_flags;
 
     if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
          on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&

@@ -231,13 +245,36 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
+    /* Make sure that the image is opened in read-write mode */
+    orig_bs_flags = bdrv_get_flags(bs);
+    if (!(orig_bs_flags & BDRV_O_RDWR)) {
+        if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+            return;
+        }
+    }
+
     s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;

block/stream.c +7 -0

+        /* Hold a reference to the active layer, otherwise
+         * bdrv_close_all() will destroy it if we shut down the VM */
+        bdrv_ref(active);
+    }
+
     s->base = base;
+    s->active = active;
     s->backing_file_str = g_strdup(backing_file_str);
+    s->bs_flags = orig_bs_flags;
 
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(stream_run);

blockdev.c +1 -1

index d1f5dfb..2e7712e 100644
@@ -3038,7 +3038,7 @@ void qmp_block_stream(const char *device,
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
-    stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
+    stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);

include/block/block_int.h +4 -1

index 10d8759..0a53d5f 100644
@@ -597,6 +597,8 @@ int is_windows_drive(const char *filename);
 /**
  * stream_start:
  * @bs: Block device to operate on.
+ * @active: The active layer of the chain where @bs belongs, or %NULL
+ *          if @bs is already the topmost device.
  * @base: Block device that will become the new base, or %NULL to
  * flatten the whole backing file chain onto @bs.
  * @base_id: The file name that will be written to @bs as the new

@@ -613,7 +615,8 @@ int is_windows_drive(const char *filename);
  * streaming job, the backing file of @bs will be changed to
  * @base_id in the written image and to @base in the live BlockDriverState.
  */
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
+void stream_start(BlockDriverState *bs, BlockDriverState *active,
+                  BlockDriverState *base,
                   const char *base_id, int64_t speed, BlockdevOnError on_error,
                   BlockCompletionFunc *cb,
                   void *opaque, Error **errp);

This patch makes the 'device' parameter of the 'block-stream' command accept a node name as well as a device name.

In addition to that, operation blockers will be checked in all intermediate nodes between the top and the base node.

Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and no longer returns DeviceNotFound, iotest 030 is updated to expect GenericError instead.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c             | 31 +++++++++++++++++++++++--------
 qapi/block-core.json   | 10 +++++++---
 tests/qemu-iotests/030 |  2 +-
 3 files changed, 31 insertions(+), 12 deletions(-)

blockdev.c +6 -7

index 2e7712e..bfdc0e3 100644
@@ -2989,6 +2989,7 @@ void qmp_block_stream(const char *device,
     BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
+    BlockDriverState *active;
     AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;

@@ -2997,21 +2998,19 @@ void qmp_block_stream(const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = bdrv_lookup_bs(device, device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
+    blk = blk_by_name(device);
+    if (blk && !blk_is_available(blk)) {
         error_setg(errp, "Device '%s' has no medium", device);
         goto out;
     }
-    bs = blk_bs(blk);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;

blockdev.c +11 -0

+
+    if (active == NULL) {
+        error_setg(errp, "Cannot find top level node for '%s'", device);
+        goto out;
+    }
+
+    /* Check for op blockers in the top-level node too */
+    if (bdrv_op_is_blocked(active, BLOCK_OP_TYPE_STREAM, errp)) {
+        goto out;
+    }
+
     /* if we are streaming the entire chain, the result will have no backing
      * file, and specifying one is therefore an error */
     if (base_bs == NULL && has_backing_file) {

blockdev.c +0 -0

                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);

qapi/block-core.json +7 -3

index 59107ed..e20193a 100644
@@ -1405,6 +1405,10 @@
 # with query-block-jobs.  The operation can be stopped before it has completed
 # using the block-job-cancel command.
 #
+# The node that receives the data is called the top image, can be located
+# in any part of the whole chain and can be specified using its device
+# or node name.
+#
 # If a base file is specified then sectors are not copied from that base file and
 # its backing chain.  When streaming completes the image file will have the base
 # file as its backing file.  This can be used to stream a subset of the backing

@@ -1413,12 +1417,12 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @device: the device or node name of the top image
 #
 # @base:   #optional the common backing file name
 #
-# @backing-file: #optional The backing file string to write into the active
-#                          layer. This filename is not validated.
+# @backing-file: #optional The backing file string to write into the top
+#                          image. This filename is not validated.
 #
 #                          If a pathname string is such that it cannot be
 #                          resolved by QEMU, that means that subsequent QMP or

tests/qemu-iotests/030 +1 -1

index 3ac2443..107049b 100755
@@ -126,7 +126,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-stream', device='nonexistent')
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
 
 class TestSmallerBackingFile(iotests.QMPTestCase):

This test case checks that it's not possible to perform two block-stream operations if there are nodes involved in both.

Signed-off-by Alberto Garcia <[email protected]>

---
 tests/qemu-iotests/030     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 62 insertions(+), 2 deletions(-)

tests/qemu-iotests/030 +20 -0

index 1c53f80..6348872 100755
@@ -145,6 +145,66 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
 
 
+class TestMultipleOps(iotests.QMPTestCase):
+    num_imgs = 9 # It must be > 5 or some tests will break
+    image_len = num_imgs * 1024 * 1024
+    imgs = []
+
+    def setUp(self):
+        opts = []
+        self.imgs = []
+
+        # Initialize file names and command-line options
+        for i in range(self.num_imgs):
+            img_depth = self.num_imgs - i - 1
+            opts.append("backing." * img_depth + "node-name=node%d" % i)
+            self.imgs.append(os.path.join(iotests.test_dir, 'img-%d.img' % i))
+
+        # Create all images
+        iotests.create_image(self.imgs[0], self.image_len)
+        for i in range(1, self.num_imgs):
+            qemu_img('create', '-f', iotests.imgfmt,
+                     '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i])

tests/qemu-iotests/030 +30 -0

+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for img in self.imgs:
+            os.remove(img)
+
+    # Test that it's not possible to perform two block-stream
+    # operations if there are nodes involved in both.
+    def test_overlapping(self):
+        self.assert_no_active_block_jobs()
+
+        # Set a speed limit to make sure that this job blocks the rest
+        result = self.vm.qmp('block-stream', device='node4', base=self.imgs[0], speed=32768)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-stream', device='node5', base=self.imgs[1])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node3', base=self.imgs[2])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('block-stream', device='node4')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.wait_until_completed(drive='node4')
+        self.assert_no_active_block_jobs()
+
+        self.vm.shutdown()
+
 class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * backing_len

tests/qemu-iotests/030.out +2 -2

index 96961ed..b6f2576 100644
@@ -1,5 +1,5 @@
-...............
+................
 ----------------------------------------------------------------------
-Ran 15 tests
+Ran 16 tests
 
 OK

Even if there are no common nodes involved, we currently don't support several operations at the same time in the same backing chain.

Signed-off-by Alberto Garcia <[email protected]>

---
 tests/qemu-iotests/030     | 21 +++++++++++++++++++++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 23 insertions(+), 2 deletions(-)

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3700

Quoted 8 lines

blockdev.c +2 -2

@@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);
+    if (!bs) {

If we get here, *errp is already set... [1]

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3708

Quoted 3 lines

blockdev.c +2 -1

-    if (!blk_is_available(blk)) {
+    blk = blk_by_name(device_or_node);
+    if (blk && !blk_is_available(blk)) {

I'd just drop this. The reason blk_is_available() was added here because it became possible for BBs not to have a BDS.

Now that you get the BDS directly through bdrv_lookup_bs(), it's no longer necessary.

blockjob.c void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, · line 82

Quoted 6 lines

blockjob.c +1 -1

@@ -78,7 +79,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
     job->driver        = driver;
-    job->id            = g_strdup(bdrv_get_device_name(bs));
+    job->id            = g_strdup(bdrv_get_device_or_node_name(bs));

Hm, since this is only used for events, it's not too bad that a block job will have a different name if it was created on a root BDS by specifying its node name. But it still is kind of strange.

tests/qemu-iotests/030 class TestSingleDrive(iotests.QMPTestCase): · line 177

Quoted 3 lines

tests/qemu-iotests/030 +3 -0

+        # Attach the drive to the VM
+        self.vm = iotests.VM()
+        self.vm.add_drive("blkdebug::" + self.imgs[-1], ','.join(opts))

Any special reason for blkdebug? For me it works just fine without.

block/stream.c void stream_start(BlockDriverState *bs, BlockDriverState *base, · line 268

Quoted 8 lines

block/stream.c +8 -0

+    /* If we are streaming to an intermediate image, we need to block
+     * the active layer. Due to a race condition, having several block
+     * jobs running in the same chain is broken and we currently don't
+     * support it. See here for details:
+     * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html
+     */
+    if (active) {
+        bdrv_op_block_all(active, s->common.blocker);

block_job_create() unblocks BLOCK_OP_TYPE_DATAPLANE. Maybe this should do the same?

blockdev.c void qmp_block_stream(const char *device, · line 3033

Quoted 5 lines

blockdev.c +5 -0

+    /* Look for the top-level node that contains 'bs' in its chain */
+    active = NULL;
+    do {
+        active = bdrv_next(active);
+    } while (active && !bdrv_chain_contains(active, bs));

Alternatively, you could iterate up directly from @bs. Just look for the BdrvChild in bs->parents with .role == &child_backing.

More interesting question: What happens if you have e.g. a qcow2 file as a quorum child, and then want to stream inside of the qcow2 backing chain?

So maybe you should first walk up along &child_backing and then along &child_file/&child_format. I think a generic bdrv_get_root_bs() wouldn't hurt.

blockdev.c void qmp_block_stream(const char *device, · line 3056

Quoted 2 lines

blockdev.c +1 -1

-    stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
+    stream_start(bs, active, base_bs, base_name, has_speed ? speed : 0,

stream_start() behaves differently for active == NULL and active == bs. I think both kinds of behavior work, but it looks weird for active == bs. Should we pass NULL in that case instead?

The current way to obtain the list of existing block jobs is to iterate over all root nodes and check which ones own a job.

Since we want to be able to support block jobs in other nodes as well, this patch keeps a list of jobs that is updated every time one is created or destroyed.

Signed-off-by Alberto Garcia <[email protected]>

bdrv_drain_all() pauses all block jobs by using bdrv_next() to iterate over all top-level BlockDriverStates. Therefore the code is unable to find block jobs in other nodes.

This patch uses block_job_next() to iterate over all block jobs.

Signed-off-by Alberto Garcia <[email protected]>

qmp_query_block_jobs() uses bdrv_next() to look for block jobs, but this function can only find those in top-level BlockDriverStates.

This patch uses block_job_next() instead.

Signed-off-by Alberto Garcia <[email protected]>

bdrv_close_all() cancels all block jobs by iterating over all BlockDriverStates. This patch simplifies the code by iterating directly over the block jobs using block_job_next().

Signed-off-by Alberto Garcia <[email protected]>

 block.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

block.c +6 -19

index d36eb75..48638c9 100644
@@ -2182,8 +2182,7 @@ static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockDriverState *bs;
-    AioContext *aio_context;
+    BlockJob *job;
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */

@@ -2193,23 +2192,11 @@ void bdrv_close_all(void)
     blockdev_close_all_bdrv_states();
 
     /* Cancel all block jobs */
-    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
-        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
-            aio_context = bdrv_get_aio_context(bs);
-
-            aio_context_acquire(aio_context);
-            if (bs->job) {
-                block_job_cancel_sync(bs->job);
-                aio_context_release(aio_context);
-                break;
-            }
-            aio_context_release(aio_context);
-        }
-
-        /* All the remaining BlockDriverStates are referenced directly or
-         * indirectly from block jobs, so there needs to be at least one BDS
-         * directly used by a block job */
-        assert(bs);
+    while ((job = block_job_next(NULL))) {
+        AioContext *aio_context = bdrv_get_aio_context(job->bs);
+        aio_context_acquire(aio_context);
+        block_job_cancel_sync(job);
+        aio_context_release(aio_context);
     }
 }
 

Signature · 1 line

2.8.0.rc3

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c           | 18 ++++++++++--------
 blockjob.c           |  5 +++--
 docs/qmp-events.txt  |  8 ++++----
 qapi/block-core.json | 20 ++++++++++----------
 4 files changed, 27 insertions(+), 24 deletions(-)

blockdev.c +4 -2

index edbcc19..d1f5dfb 100644
@@ -3685,8 +3685,10 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     aio_context_release(aio_context);
 }
 
-/* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context,
+/* Get the block job for a given device or node name
+ * and acquire its AioContext */
+static BlockJob *find_block_job(const char *device_or_node,
+                                AioContext **aio_context,
                                 Error **errp)
 {
     BlockBackend *blk;

This makes sure that the image we are steaming into is open in

read-write mode during the operation.

The block job is created on the destination image, but operation blockers are also set on the active layer. We do this in order to prevent other block jobs from running in parallel in the same chain. See here for details on why that is currently not supported:

[Qemu-block] RFC: Status of the intermediate block streaming work
https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

Finally, this also unblocks the stream operation in backing files.

Signed-off-by Alberto Garcia <[email protected]>

This patch makes the 'device' parameter of the 'block-stream' command accept a node name as well as a device name.

In addition to that, operation blockers will be checked in all intermediate nodes between the top and the base node.

Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and no longer returns DeviceNotFound, iotest 030 is updated to expect GenericError instead.

Signed-off-by Alberto Garcia <[email protected]>

---
 blockdev.c             | 31 +++++++++++++++++++++++--------
 qapi/block-core.json   | 10 +++++++---
 tests/qemu-iotests/030 |  2 +-
 3 files changed, 31 insertions(+), 12 deletions(-)

blockdev.c +6 -7

index 2e7712e..bfdc0e3 100644
@@ -2989,6 +2989,7 @@ void qmp_block_stream(const char *device,
     BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
+    BlockDriverState *active;
     AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;

@@ -2997,21 +2998,19 @@ void qmp_block_stream(const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = bdrv_lookup_bs(device, device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
+    blk = blk_by_name(device);
+    if (blk && !blk_is_available(blk)) {
         error_setg(errp, "Device '%s' has no medium", device);
         goto out;
     }
-    bs = blk_bs(blk);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;

@@ -3027,6 +3026,22 @@ void qmp_block_stream(const char *device,
         base_name = base;
     }
 

Quoted 6 lines

Quoted 5 lines

blockdev.c +5 -0

+    /* Look for the top-level node that contains 'bs' in its chain */
+    active = NULL;
+    do {
+        active = bdrv_next(active);
+    } while (active && !bdrv_chain_contains(active, bs));

Quoted 2 lines

Alternatively, you could iterate up directly from @bs. Just look for the BdrvChild in bs->parents with .role == &child_backing.

Yes, but the BdrvChild in bs->parents does not contain any pointer to the actual parent node, so unless we want to change that structure I wouldn't go for this solution.

Quoted 7 lines

More interesting question: What happens if you have e.g. a qcow2 file as a quorum child, and then want to stream inside of the qcow2 backing chain?

So maybe you should first walk up along &child_backing and then along &child_file/&child_format. I think a generic bdrv_get_root_bs() wouldn't hurt.

You're right. I'm not sure if that would have other consequences that we should consider, so maybe we can add that later, with its own set of tests.

Also, this is all necessary because of the problem with bdrv_reopen(). If that is ever fixed there's no need to block the active layer at all.

Currently, block jobs can only be owned by root nodes. This patch allows block jobs to be in any arbitrary node, by making the following changes:

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

- The "device" parameter used by all commands that operate on block jobs can also be a node name now.

- The node name is used as a fallback to fill in the BlockJobInfo structure and all BLOCK_JOB_* events if there is no device name for that job.

However, I'd like to give you a heads-up that this will technically conflict with my series that removes BlockDriverState.blk because that changes the bdrv_next() signature.

Nothing dramatic, but I guess it would make sense to decide where in the queue of patches this series should go. My suggestion would be on top of "blockdev: (Nearly) free clean-up work".

This is essentially the same as I'm doing here:
http://repo.or.cz/qemu/kevin.git/commitdiff/6b545b21e3dfe2e3927cfb6bbdcc1b233c67630c

I think I like having a separate block_job_cancel_sync_all() function like I did instead of inlining it in bdrv_close_all(), though that's a matter of taste.

This patch errors out if we can't find the active layer. Sounds safe and appropriate for an initial version. The real solution isn't to improve the magic to find the root node, but to remove the need to find it (by getting the new op blockers).

blockdev.c void qmp_block_stream(const char *device, · line 3038

Quoted 4 lines

blockdev.c +4 -0

+    if (active == NULL) {
+        error_setg(errp, "Cannot find top level node for '%s'", device);
+        goto out;
+    }

Hm... On the one hand, I really like that you don't expect the user to provide the active layer in QMP. This allows us to remove this wart once we have the new op blockers.

On the other hand, this code assumes that there is only a single top-level node. This isn't necessarily true any more these days.

Quoted 8 lines

blockdev.c void qmp_block_stream(const char *device, · line 3038

Quoted 4 lines

blockdev.c +4 -0

+    if (active == NULL) {
+        error_setg(errp, "Cannot find top level node for '%s'", device);
+        goto out;
+    }

Hm... On the one hand, I really like that you don't expect the user to provide the active layer in QMP. This allows us to remove this wart once we have the new op blockers.

Exactly, I still plan to stick to the API we discussed last year.

Quoted 2 lines

On the other hand, this code assumes that there is only a single top-level node. This isn't necessarily true any more these days.

Hmm... if you give me an example I can test that scenario.

Quoted 4 lines

This patch errors out if we can't find the active layer. Sounds safe and appropriate for an initial version. The real solution isn't to improve the magic to find the root node, but to remove the need to find it (by getting the new op blockers).

Well, I agree with the fact that what we really want is not to block the active layer at all, but I don't see how any new op blockers are going to solve that.

The situation is that we can't allow two block jobs in the same chain at the moment, and I only see three solutions:

a) each job blocks the whole chain (what this series does).

b) each job checks that no other job is running on the same chain. Maybe cleaner? But it would require modifying all other jobs.

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

Quoted 10 lines

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

I think that's not enough, see point 2) here:

https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

"I've been taking a look at the bdrv_drained_begin/end() API, but as I
understand it it prevents requests from a different AioContext. Since all BDS in the same chain share the same context it does not really help here."

The solution to that should be simply changing the order of things:

1. bdrv_drained_begin()
2. bdrv_reopen_queue()
3. bdrv_reopen_multiple()
* Instead of bdrv_drain_all(), assert that no requests are pending

* We don't run requests, so we can't complete a block job and manipulate the graph any more
4. then bdrv_drained_end()

Quoted 3 lines

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

Careful, we know this is a part of our API that we have already messed up and we don't want to make things worse while adding new things before we've cleaned it up.

Let's keep in mind where we are planning to go with block jobs: They should become background jobs, not tied to any block device. The close connection to a single BDS already doesn't make a lot of sense today because most block jobs involve at least two BDSes.

In the final state, we will have a job ID that uniquely identifies the job, and each command that starts a job will take an ID from the client. For compatibility, we'll use the block device name as the job ID when using old commands that don't get an explicit ID yet.

In the existing qemu version, you can't start two block jobs on the same device, and in future versions, you're supposed to specify an ID each time. This is why the default can always be supposed to work without conflicts. If in newer versions, the client mixes both ways (which it shouldn't do), starting a new block job may fail because the device name is already in use as an ID for another job.

Now we can probably make the same argument for node names, so we can extend the interface and still keep it compatible.

Where we need to be careful is that with device names and node names, we have potentially two names describing the same BDS and therefore the same job. This must not happen, because we won't be able to represent that in the generic background job API. Any job has just a single ID there.

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3699

Quoted 7 lines

blockdev.c +1 -2

@@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);

Specifically, this one is bad. It allows two different ways to specify the same job.

Quoted 25 lines · Kevin Wolf

Quoted 3 lines

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

Careful, we know this is a part of our API that we have already messed up and we don't want to make things worse while adding new things before we've cleaned it up.

Let's keep in mind where we are planning to go with block jobs: They should become background jobs, not tied to any block device. The close connection to a single BDS already doesn't make a lot of sense today because most block jobs involve at least two BDSes.

In the final state, we will have a job ID that uniquely identifies the job, and each command that starts a job will take an ID from the client. For compatibility, we'll use the block device name as the job ID when using old commands that don't get an explicit ID yet.

In the existing qemu version, you can't start two block jobs on the same device, and in future versions, you're supposed to specify an ID each time. This is why the default can always be supposed to work without conflicts. If in newer versions, the client mixes both ways (which it shouldn't do), starting a new block job may fail because the device name is already in use as an ID for another job.

Quoted 8 lines

Now we can probably make the same argument for node names, so we can extend the interface and still keep it compatible.

Where we need to be careful is that with device names and node names, we have potentially two names describing the same BDS and therefore the same job. This must not happen, because we won't be able to represent that in the generic background job API. Any job has just a single ID there.

Ok, what can be done in this case is to keep the name that the client used when the job was created.

block-stream virti0 <-- job id is 'virtio0'
block-stream node5 <-- job id is 'node5'

In this case it wouldn't matter if 'node5' is the one attached to 'virtio0'.

Quoted 10 lines

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3699

Quoted 7 lines

blockdev.c +1 -2

@@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);

Specifically, this one is bad. It allows two different ways to specify the same job.

I think we can simply iterate over all block jobs (now that we have a function to do that) and return the one with the matching ID.

Berto

Quoted 56 lines · Kevin Wolf

Quoted 26 lines

Quoted 4 lines

- Block jobs can now be identified by the node name of their BlockDriverState in addition to the device name. Since both device and node names live in the same namespace there's no ambiguity.

Careful, we know this is a part of our API that we have already messed up and we don't want to make things worse while adding new things before we've cleaned it up.

Let's keep in mind where we are planning to go with block jobs: They should become background jobs, not tied to any block device. The close connection to a single BDS already doesn't make a lot of sense today because most block jobs involve at least two BDSes.

In the final state, we will have a job ID that uniquely identifies the job, and each command that starts a job will take an ID from the client. For compatibility, we'll use the block device name as the job ID when using old commands that don't get an explicit ID yet.

In the existing qemu version, you can't start two block jobs on the same device, and in future versions, you're supposed to specify an ID each time. This is why the default can always be supposed to work without conflicts. If in newer versions, the client mixes both ways (which it shouldn't do), starting a new block job may fail because the device name is already in use as an ID for another job.

Quoted 8 lines

Now we can probably make the same argument for node names, so we can extend the interface and still keep it compatible.

Where we need to be careful is that with device names and node names, we have potentially two names describing the same BDS and therefore the same job. This must not happen, because we won't be able to represent that in the generic background job API. Any job has just a single ID there.

Ok, what can be done in this case is to keep the name that the client used when the job was created.

block-stream virti0 <-- job id is 'virtio0'
block-stream node5 <-- job id is 'node5'

In this case it wouldn't matter if 'node5' is the one attached to 'virtio0'.

Quoted 10 lines

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3699

Quoted 7 lines

blockdev.c +1 -2

@@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);

Specifically, this one is bad. It allows two different ways to specify the same job.

Quoted 5 lines

I think we can simply iterate over all block jobs (now that we have a function to do that) and return the one with the matching ID.

Berto

I think it's time to add a proper ID field to Block Jobs. Currently, we just use the node-name as the ID, but as you are aware this is a poor mechanism for fetching the job.

I'd really like to avoid continue using any kind of node-name or block/device-name for job IDs, and instead start using either a user-provided value or a QEMU auto-generated one.

Then, for legacy support, we'd have an "id" field (that's the new proper globally unique ID) and an old legacy "node" field or similar.

Then, for events/etc we can report two things back:

(1) Legacy: the name of the node we were created under. This is like it works currently, and it should keep libvirt happy.
(2) New: the proper, real ID that all management utilities should begin using in the future.

I've got some patches that work towards this angle, but they're currently intermingled with a bunch of experimental job re-factoring stuff. If you give me a few days I can submit a proposal series to re-do the job ID situation.

Quoted 11 lines

Quoted 10 lines

blockdev.c static BlockJob *find_block_job(const char *device, AioContext **aio_context, · line 3699

Quoted 7 lines

blockdev.c +1 -2

@@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);

Specifically, this one is bad. It allows two different ways to specify the same job.

Quoted 2 lines

I think we can simply iterate over all block jobs (now that we have a function to do that) and return the one with the matching ID.

I think it's time to add a proper ID field to Block Jobs. Currently, we just use the node-name as the ID, but as you are aware this is a poor mechanism for fetching the job.

I'd really like to avoid continue using any kind of node-name or block/device-name for job IDs, and instead start using either a user-provided value or a QEMU auto-generated one.

Then, for legacy support, we'd have an "id" field (that's the new proper globally unique ID) and an old legacy "node" field or similar.

Then, for events/etc we can report two things back:

(1) Legacy: the name of the node we were created under. This is like it works currently, and it should keep libvirt happy.
(2) New: the proper, real ID that all management utilities should begin using in the future.

Quoted 32 lines

Am 03.05.2016 um 15:33 hat Alberto Garcia geschrieben:

Quoted 12 lines · Kevin Wolf

Quoted 10 lines

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

Quoted 8 lines

I think that's not enough, see point 2) here:

https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

"I've been taking a look at the bdrv_drained_begin/end() API, but as I
understand it it prevents requests from a different AioContext. Since all BDS in the same chain share the same context it does not really help here."

Yes, that's the part I meant with pulling up the calls.

If I understand correctly, the problem is that first bdrv_reopen_queue() queues a few BDSes for reopen, then bdrv_drain_all() completes all running requests and can indirectly trigger a graph modification, and then bdrv_reopen_multiple() uses the queue which doesn't match reality any more.

The solution to that should be simply changing the order of things:

Quoted 7 lines

1. bdrv_drained_begin()
2. bdrv_reopen_queue()
3. bdrv_reopen_multiple()
* Instead of bdrv_drain_all(), assert that no requests are pending
* We don't run requests, so we can't complete a block job and manipulate the graph any more
4. then bdrv_drained_end()

This doesn't work. Here's what happens:

1) Block job (a) starts (block-stream).

2) Block job (b) starts (block-stream, or block-commit).

3) job (b) calls bdrv_reopen() and does the drain call.

4) job (b) creates reopen_queue and calls bdrv_reopen_multiple(). There are no pending requests at this point, but job (a) is sleeping.

5) bdrv_reopen_multiple() iterates over reopen_queue and calls bdrv_reopen_prepare() -> bdrv_flush() -> bdrv_co_flush() -> qemu_coroutine_yield().

6) job (a) resumes, finishes the job and removes nodes from the graph.

7) job (b) continues with bdrv_reopen_multiple() but now reopen_queue contains invalid pointers.

Quoted 59 lines

Am 12.05.2016 um 15:47 hat Alberto Garcia geschrieben:

Quoted 47 lines · Kevin Wolf

Quoted 32 lines

Am 03.05.2016 um 15:33 hat Alberto Garcia geschrieben:

Quoted 12 lines · Kevin Wolf

Quoted 10 lines

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

Quoted 8 lines

I think that's not enough, see point 2) here:

https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

"I've been taking a look at the bdrv_drained_begin/end() API, but as I
understand it it prevents requests from a different AioContext. Since all BDS in the same chain share the same context it does not really help here."

Yes, that's the part I meant with pulling up the calls.

If I understand correctly, the problem is that first bdrv_reopen_queue() queues a few BDSes for reopen, then bdrv_drain_all() completes all running requests and can indirectly trigger a graph modification, and then bdrv_reopen_multiple() uses the queue which doesn't match reality any more.

The solution to that should be simply changing the order of things:

Quoted 7 lines

1. bdrv_drained_begin()
2. bdrv_reopen_queue()
3. bdrv_reopen_multiple()
* Instead of bdrv_drain_all(), assert that no requests are pending
* We don't run requests, so we can't complete a block job and manipulate the graph any more
4. then bdrv_drained_end()

This doesn't work. Here's what happens:

1) Block job (a) starts (block-stream).

2) Block job (b) starts (block-stream, or block-commit).

Quoted 8 lines

3) job (b) calls bdrv_reopen() and does the drain call.

4) job (b) creates reopen_queue and calls bdrv_reopen_multiple(). There are no pending requests at this point, but job (a) is sleeping.

5) bdrv_reopen_multiple() iterates over reopen_queue and calls bdrv_reopen_prepare() -> bdrv_flush() -> bdrv_co_flush() -> qemu_coroutine_yield().

I think between here and the next step is what I don't understand.

Quoted 4 lines

bdrv_reopen_multiple() is not called in coroutine context, right? All block jobs use block_job_defer_to_main_loop() before they call bdrv_reopen(), as far as I can see. So bdrv_flush() shouldn't take the shortcut, but use a nested event loop.

When bdrv_flush() is not called in coroutine context it does qemu_coroutine_create() + qemu_coroutine_enter().

Quoted 3 lines

What is it that calls into job (a) from that event loop? It can't be a request completion because we already drained all requests. Is it a timer?

If I didn't get it wrong it's this bit in bdrv_co_flush() [...]

Quoted 7 lines

Quoted 4 lines

6) job (a) resumes, finishes the job and removes nodes from the graph.

7) job (b) continues with bdrv_reopen_multiple() but now reopen_queue contains invalid pointers.

I don't fully understand the problem yet, but as a shot in the dark, would pausing block jobs in bdrv_drained_begin() help?

Yeah, my impression is that pausing all jobs during bdrv_reopen() should be enough.

Am 12.05.2016 um 17:13 hat Alberto Garcia geschrieben:

Quoted 60 lines · Kevin Wolf

Quoted 59 lines

Am 12.05.2016 um 15:47 hat Alberto Garcia geschrieben:

Quoted 47 lines · Kevin Wolf

Quoted 32 lines

Am 03.05.2016 um 15:33 hat Alberto Garcia geschrieben:

Quoted 12 lines · Kevin Wolf

Quoted 10 lines

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

Quoted 8 lines

I think that's not enough, see point 2) here:

https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

"I've been taking a look at the bdrv_drained_begin/end() API, but as I
understand it it prevents requests from a different AioContext. Since all BDS in the same chain share the same context it does not really help here."

Yes, that's the part I meant with pulling up the calls.

If I understand correctly, the problem is that first bdrv_reopen_queue() queues a few BDSes for reopen, then bdrv_drain_all() completes all running requests and can indirectly trigger a graph modification, and then bdrv_reopen_multiple() uses the queue which doesn't match reality any more.

The solution to that should be simply changing the order of things:

Quoted 7 lines

1. bdrv_drained_begin()
2. bdrv_reopen_queue()
3. bdrv_reopen_multiple()
* Instead of bdrv_drain_all(), assert that no requests are pending
* We don't run requests, so we can't complete a block job and manipulate the graph any more
4. then bdrv_drained_end()

This doesn't work. Here's what happens:

1) Block job (a) starts (block-stream).

2) Block job (b) starts (block-stream, or block-commit).

Quoted 8 lines

3) job (b) calls bdrv_reopen() and does the drain call.

4) job (b) creates reopen_queue and calls bdrv_reopen_multiple(). There are no pending requests at this point, but job (a) is sleeping.

5) bdrv_reopen_multiple() iterates over reopen_queue and calls bdrv_reopen_prepare() -> bdrv_flush() -> bdrv_co_flush() -> qemu_coroutine_yield().

I think between here and the next step is what I don't understand.

Quoted 7 lines

Quoted 4 lines

bdrv_reopen_multiple() is not called in coroutine context, right? All block jobs use block_job_defer_to_main_loop() before they call bdrv_reopen(), as far as I can see. So bdrv_flush() shouldn't take the shortcut, but use a nested event loop.

When bdrv_flush() is not called in coroutine context it does qemu_coroutine_create() + qemu_coroutine_enter().

Right, but if the coroutine yields, we jump back to the caller, which looks like this:

    co = qemu_coroutine_create(bdrv_flush_co_entry);
    qemu_coroutine_enter(co, &rwco);
    while (rwco.ret == NOT_DONE) {
        aio_poll(aio_context, true);
    }

So this loops until the flush has completed. The only way I can see how something else (job (a)) can run is if aio_poll() calls it.

Quoted 5 lines

Quoted 2 lines

I don't fully understand the problem yet, but as a shot in the dark, would pausing block jobs in bdrv_drained_begin() help?

Yeah, my impression is that pausing all jobs during bdrv_reopen() should be enough.

If you base your patches on top of my queue (which I think you already do for the greatest part), the nicest way to implement this would probably be that BlockBackends give their users a callback for .drained_begin/end and the jobs implement that as pausing themselves.

We could, of course, directly pause block jobs in bdrv_drained_begin(), but that would feel a bit hackish. So maybe do that for a quick attempt whether it helps, and if it does, we can write the real thing then.

Quoted 78 lines

Am 12.05.2016 um 17:13 hat Alberto Garcia geschrieben:

Quoted 60 lines · Kevin Wolf

Quoted 59 lines

Am 12.05.2016 um 15:47 hat Alberto Garcia geschrieben:

Quoted 47 lines · Kevin Wolf

Quoted 32 lines

Am 03.05.2016 um 15:33 hat Alberto Garcia geschrieben:

Quoted 12 lines · Kevin Wolf

Quoted 10 lines

Quoted 4 lines

c) we fix bdrv_reopen() so we can actually run both jobs at the same time. I'm wondering if pausing all block jobs between bdrv_reopen_prepare() and bdrv_reopen_commit() would do the trick. Opinions?

I would have to read up the details of the problem again, but I think with bdrv_drained_begin/end() we actually have the right tool now to fix it properly. We may need to pull up the drain (bdrv_drain_all() today) from bdrv_reopen_multiple() to its caller and just assert it in the function itself, but there shouldn't be much more to it than that.

Quoted 8 lines

I think that's not enough, see point 2) here:

https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html

"I've been taking a look at the bdrv_drained_begin/end() API, but as I
understand it it prevents requests from a different AioContext. Since all BDS in the same chain share the same context it does not really help here."

Yes, that's the part I meant with pulling up the calls.

If I understand correctly, the problem is that first bdrv_reopen_queue() queues a few BDSes for reopen, then bdrv_drain_all() completes all running requests and can indirectly trigger a graph modification, and then bdrv_reopen_multiple() uses the queue which doesn't match reality any more.

The solution to that should be simply changing the order of things:

Quoted 7 lines

1. bdrv_drained_begin()
2. bdrv_reopen_queue()
3. bdrv_reopen_multiple()
* Instead of bdrv_drain_all(), assert that no requests are pending
* We don't run requests, so we can't complete a block job and manipulate the graph any more
4. then bdrv_drained_end()

This doesn't work. Here's what happens:

1) Block job (a) starts (block-stream).

2) Block job (b) starts (block-stream, or block-commit).

Quoted 8 lines

3) job (b) calls bdrv_reopen() and does the drain call.

4) job (b) creates reopen_queue and calls bdrv_reopen_multiple(). There are no pending requests at this point, but job (a) is sleeping.

5) bdrv_reopen_multiple() iterates over reopen_queue and calls bdrv_reopen_prepare() -> bdrv_flush() -> bdrv_co_flush() -> qemu_coroutine_yield().

I think between here and the next step is what I don't understand.

Quoted 7 lines

Quoted 4 lines

bdrv_reopen_multiple() is not called in coroutine context, right? All block jobs use block_job_defer_to_main_loop() before they call bdrv_reopen(), as far as I can see. So bdrv_flush() shouldn't take the shortcut, but use a nested event loop.

When bdrv_flush() is not called in coroutine context it does qemu_coroutine_create() + qemu_coroutine_enter().

Right, but if the coroutine yields, we jump back to the caller, which looks like this:

    co = qemu_coroutine_create(bdrv_flush_co_entry);
    qemu_coroutine_enter(co, &rwco);
    while (rwco.ret == NOT_DONE) {
        aio_poll(aio_context, true);
    }

Quoted 2 lines

So this loops until the flush has completed. The only way I can see how something else (job (a)) can run is if aio_poll() calls it.

If I'm not wrong that can happen if job (a) is sleeping.

If job (a) is launched with a rate limit, then the bdrv_drain() call will not drain it completely but return when the block job hits the I/O limit -> block_job_sleep_ns() -> co_aio_sleep_ns().

Quoted 11 lines

Quoted 5 lines

Quoted 2 lines

I don't fully understand the problem yet, but as a shot in the dark, would pausing block jobs in bdrv_drained_begin() help?

Yeah, my impression is that pausing all jobs during bdrv_reopen() should be enough.

If you base your patches on top of my queue (which I think you already do for the greatest part), the nicest way to implement this would probably be that BlockBackends give their users a callback for .drained_begin/end and the jobs implement that as pausing themselves.

Quoted 4 lines

We could, of course, directly pause block jobs in bdrv_drained_begin(), but that would feel a bit hackish. So maybe do that for a quick attempt whether it helps, and if it does, we can write the real thing then.

I'll try that.

On a related note, bdrv_drain_all() already pauses all block jobs. The problem is that when it ends it resumes them, so it can happen that there's again pending I/O requests before bdrv_drain_all() returns.

That sounds like a problem to me, or am I overlooking anything?

Quoted 9 lines

Quoted 9 lines

Right, but if the coroutine yields, we jump back to the caller, which looks like this:

    co = qemu_coroutine_create(bdrv_flush_co_entry);
    qemu_coroutine_enter(co, &rwco);
    while (rwco.ret == NOT_DONE) {
        aio_poll(aio_context, true);
    }

Quoted 8 lines

Quoted 2 lines

So this loops until the flush has completed. The only way I can see how something else (job (a)) can run is if aio_poll() calls it.

If I'm not wrong that can happen if job (a) is sleeping.

If job (a) is launched with a rate limit, then the bdrv_drain() call will not drain it completely but return when the block job hits the I/O limit -> block_job_sleep_ns() -> co_aio_sleep_ns().

So timers. Currently, bdrv_drained_begin/end disables the FD handlers, but not timers. Not sure if that's easily fixable in a generic way, though, so maybe we need to disable timers one by one.

Pausing a block job does not actually cause the timer to be cancelled. Maybe we need to change block_job_sleep_ns() so that the function returns only when the job isn't paused any more. Maybe something like:

    job->busy = false;
    if (!block_job_is_paused(job)) {
        co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns);
        /* The job could be paused now */
    }
    if (block_job_is_paused(job)) {
        qemu_coroutine_yield();
    }
    job->busy = true;

Then it should be safe to assume that if block jobs are paused (even if they were sleeping), they won't issue new requests any more until they are resumed.