diff mbox

[v2,1/6] block: skip implicit nodes in snapshots, blockjobs

Message ID 20170809140256.25584-2-el13635@mail.ntua.gr (mailing list archive)
State New, archived
Headers show

Commit Message

Manos Pitsidianakis Aug. 9, 2017, 2:02 p.m. UTC
Implicit filter nodes added at the top of nodes can interfere with block
jobs. This is not a problem when they are added by other jobs since
adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
the next commit which introduces an implicitly created throttle filter
node below BlockBackend, which we want to be skipped during automatic
operations on the graph since the user does not necessarily know about
their existence.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c                   | 10 ++++++++++
 block/qapi.c              | 14 +++++---------
 blockdev.c                | 34 ++++++++++++++++++++++++++++++++++
 include/block/block_int.h |  9 +++++++++
 4 files changed, 58 insertions(+), 9 deletions(-)

Comments

Alberto Garcia Aug. 15, 2017, 1:24 p.m. UTC | #1
On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:
> @@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>          return;
>      }
>  
> +    /* Skip implicit filter nodes */
> +    bs = bdrv_get_first_explicit(bs);
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);

This change works here because right now the only implicit node that we
could have in practice is the throttle node, but I wonder if this is
good enough for any kind of implicit node in general.

> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +    BdrvChild *child = QLIST_FIRST(&bs->children);
> +    assert(child && !QLIST_NEXT(child, next));
> +    return child->bs;
> +}

This aborts if the bs has a number of children != 1. That's not
something that I would expect from a function named like that.

Considering that you're only using it in bdrv_get_first_explicit(), why
don't you simply move the code there?

The other question is of course whether we can rely for the future on
the assumption that implicit nodes only have one children.

Berto
Manos Pitsidianakis Aug. 15, 2017, 1:50 p.m. UTC | #2
On Tue, Aug 15, 2017 at 03:24:38PM +0200, Alberto Garcia wrote:
>On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:
>> @@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>>          return;
>>      }
>>
>> +    /* Skip implicit filter nodes */
>> +    bs = bdrv_get_first_explicit(bs);
>> +
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>
>This change works here because right now the only implicit node that we
>could have in practice is the throttle node, but I wonder if this is
>good enough for any kind of implicit node in general.

It does feel a bit sloppy but block jobs should work the same with
implicit nodes and without, so all we can do is ignore them.

>
>> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
>> +{
>> +    BdrvChild *child = QLIST_FIRST(&bs->children);
>> +    assert(child && !QLIST_NEXT(child, next));
>> +    return child->bs;
>> +}
>
>This aborts if the bs has a number of children != 1. That's not
>something that I would expect from a function named like that.
>
>Considering that you're only using it in bdrv_get_first_explicit(), why
>don't you simply move the code there?

It felt useful to have a function that also returns file->bs (we have 
backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)
>
>The other question is of course whether we can rely for the future on
>the assumption that implicit nodes only have one children.

This is only to get either bs->backing or bs->file (we can't have both
anyway). I wanted to document it with something like "Used for filter 
drivers with only one child" which fits with what implicit nodes we have 
so far (mirror, commit, throttle and in the future backup)
Alberto Garcia Aug. 21, 2017, 2:44 p.m. UTC | #3
On Tue 15 Aug 2017 03:50:12 PM CEST, Manos Pitsidianakis wrote:
>>> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
>>> +{
>>> +    BdrvChild *child = QLIST_FIRST(&bs->children);
>>> +    assert(child && !QLIST_NEXT(child, next));
>>> +    return child->bs;
>>> +}
>>
>>This aborts if the bs has a number of children != 1. That's not
>>something that I would expect from a function named like that.
>>
>>Considering that you're only using it in bdrv_get_first_explicit(),
>>why don't you simply move the code there?
>
> It felt useful to have a function that also returns file->bs (we have 
> backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)
>>
>>The other question is of course whether we can rely for the future on
>>the assumption that implicit nodes only have one children.
>
> This is only to get either bs->backing or bs->file (we can't have both
> anyway).

You can have other children (see Quorum for example).

Berto
diff mbox

Patch

diff --git a/block.c b/block.c
index 3615a6809e..e35d546c08 100644
--- a/block.c
+++ b/block.c
@@ -4945,3 +4945,13 @@  bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
 
     return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
 }
+
+/* Get first explicit node down a bs chain. */
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs)
+{
+    while (bs && bs->drv && bs->implicit) {
+        bs = child_bs(bs);
+        assert(bs);
+    }
+    return bs;
+}
diff --git a/block/qapi.c b/block/qapi.c
index 7fa2437923..847b044d13 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -147,9 +147,8 @@  BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
         /* Skip automatically inserted nodes that the user isn't aware of for
          * query-block (blk != NULL), but not for query-named-block-nodes */
-        while (blk && bs0->drv && bs0->implicit) {
-            bs0 = backing_bs(bs0);
-            assert(bs0);
+        if (blk) {
+            bs0 = bdrv_get_first_explicit(bs0);
         }
     }
 
@@ -336,9 +335,7 @@  static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     char *qdev;
 
     /* Skip automatically inserted nodes that the user isn't aware of */
-    while (bs && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
-    }
+    bs = bdrv_get_first_explicit(bs);
 
     info->device = g_strdup(blk_name(blk));
     info->type = g_strdup("unknown");
@@ -465,9 +462,8 @@  static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
     /* Skip automatically inserted nodes that the user isn't aware of in
      * a BlockBackend-level command. Stay at the exact node for a node-level
      * command. */
-    while (blk_level && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
-        assert(bs);
+    if (blk_level) {
+        bs = bdrv_get_first_explicit(bs);
     }
 
     if (bdrv_get_node_name(bs)[0]) {
diff --git a/blockdev.c b/blockdev.c
index 23475abb72..fc7b65c3f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1300,6 +1300,10 @@  SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
     if (!bs) {
         return NULL;
     }
+
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -1508,6 +1512,9 @@  static void internal_snapshot_prepare(BlkActionState *common,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
@@ -1664,6 +1671,9 @@  static void external_snapshot_prepare(BlkActionState *common,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    state->old_bs = bdrv_get_first_explicit(state->old_bs);
+
     /* Acquire AioContext now so any threads operating on old_bs stop */
     state->aio_context = bdrv_get_aio_context(state->old_bs);
     aio_context_acquire(state->aio_context);
@@ -1844,6 +1854,9 @@  static void drive_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
@@ -1908,6 +1921,9 @@  static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     target = bdrv_lookup_bs(backup->target, backup->target, errp);
     if (!target) {
         return;
@@ -2988,6 +3004,9 @@  void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3095,6 +3114,9 @@  void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3209,6 +3231,9 @@  static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         return NULL;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3484,6 +3509,9 @@  void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3638,6 +3666,9 @@  void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     target_bs = bdrv_lookup_bs(target, target, errp);
     if (!target_bs) {
         return;
@@ -3786,6 +3817,9 @@  void qmp_change_backing_file(const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7571c0aaaf..8353b83e86 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -699,6 +699,13 @@  static inline BlockDriverState *backing_bs(BlockDriverState *bs)
     return bs->backing ? bs->backing->bs : NULL;
 }
 
+static inline BlockDriverState *child_bs(BlockDriverState *bs)
+{
+    BdrvChild *child = QLIST_FIRST(&bs->children);
+    assert(child && !QLIST_NEXT(child, next));
+    return child->bs;
+}
+
 
 /* Essential block drivers which must always be statically linked into qemu, and
  * which therefore can be accessed without using bdrv_find_format() */
@@ -980,4 +987,6 @@  void bdrv_dec_in_flight(BlockDriverState *bs);
 
 void blockdev_close_all_bdrv_states(void);
 
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_H */