[Qemu-devel] [PATCH v3 00/14] blockdev: Further BlockBackend work

[Qemu-devel] [PATCH v3 00/14] blockdev: Further BlockBackend work

[Max Reitz]

At this point, the main merit of this series is to remove
blk_hide_on_behalf_of_hmp_drive_del() and the bdrv_states list. A
follow-up to it will make bdrv_open() return a BDS pointer.


v3:
It certainly feels like pretty much a rewrite, and git-backport-diff
thinks so, too.

The general idea was to be less strict about making everyone use
blk_next() instead of bdrv_next(), which is for instance why patch 4
shrunk by a lot. Also, some functions do not make any sense on the BB
level (such as blk_invalidate_cache_all()), so they are no longer added
here.

I'll try my best to give a patch-by-patch diff (patch indices are given
as [this series]/[last series]):
- Patch  1/4: I split the original patch 4 into multiple patches and
              dropped the largest portion. This is the first patch of
              what is left.
- Patch  2/4: Second part of the original patch 4.
- Patch  3/-: Added, because it's needed for patch 11. Comes so early
              because we can use it in patches 4 and 9, too.
- Patch  4/1: Use blk_all_next() instead of QTAILQ_FOREACH() over
              blk_backends.
- Patch  -/2: Dropped. With all the modifications, I did not have any
              user left.
- Patch  5/3: blk_invalidate_cache_all() is no longer added by this
              series [Kevin], so this patch does not need to add it; it
              only needs to add blk_commit_all().
- Patch  6/4: Third and final part of the original patch 4.
- Patch  7/5: Actually pretty much unchanged, there is only a contextual
              conflict because blk_next_inserted() is missing now which
              git-backport-diff decided to consider a functional
              conflict.
- Patch  8/6: Unchanged, except I updated the comment in hmp_drive_del()
              now.
- Patch  9/7: Only bdrv_commit_all() and bdrv_flush_all() are moved;
              bdrv_invalidate_cache_all() and bdrv_drain_all() are kept
              where they are. [Kevin]
- Patch 10/-: Added, because it's needed for patch 12.
- Patch 11/-: Added, because it's needed for patch 12.
- Patch 12/-: Because we have to keep bdrv_invalidate_cache_all() and
              bdrv_drain_all() on the root BDS level, we will have to
              keep some form of bdrv_next(). Managing bdrv_states is
              ugly and error-prone, though, so it would be nicer if
              all root BDSs were visible through bdrv_next() without
              having to explicitly register them in a special list. This
              patch rewrites bdrv_next() so it iterates over all
              BB-attached and all monitor-owned BDSs, which I hope are
              then all the root BDSs we need.
- Patch 13/-: Now I decided to keep bdrv_next() but still remove
              bdrv_states, so this patch makes the remaining users of
              bdrv_states use bdrv_next() and thus makes patch 14
              possible.
- Patch 14/8: Rebase conflicts and no longer removes bdrv_next().


For the sake of completeness, here is the backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/14:[down] 'monitor: Use BB list for BB name completion'
002/14:[down] 'block: Use blk_next() where appropriate'
003/14:[down] 'block: Add blk_all_next()'
004/14:[0004] [FC] 'block: Add blk_name_taken()'
005/14:[down] 'block: Add blk_commit_all()'
006/14:[down] 'block: Use blk_{commit,flush}_all() consistently'
007/14:[0006] [FC] 'blockdev: Add list of monitor-owned BlockBackends'
008/14:[0006] [FC] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
009/14:[0204] [FC] 'block: Move some bdrv_*_all() functions to BB'
010/14:[down] 'block: Add bdrv_next_monitor_owned()'
011/14:[down] 'block: Add blk_next_root_bs()'
012/14:[down] 'block: Rewrite bdrv_next()'
013/14:[down] 'block: Use bdrv_next() instead of bdrv_states'
014/14:[down] 'block: Remove bdrv_states list'


Max Reitz (14):
  monitor: Use BB list for BB name completion
  block: Use blk_next() where appropriate
  block: Add blk_all_next()
  block: Add blk_name_taken()
  block: Add blk_commit_all()
  block: Use blk_{commit,flush}_all() consistently
  blockdev: Add list of monitor-owned BlockBackends
  blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()
  block: Move some bdrv_*_all() functions to BB
  block: Add bdrv_next_monitor_owned()
  block: Add blk_next_root_bs()
  block: Rewrite bdrv_next()
  block: Use bdrv_next() instead of bdrv_states
  block: Remove bdrv_states list

 block.c                                       |  84 ++++---------
 block/block-backend.c                         | 171 ++++++++++++++++++++------
 block/io.c                                    |  20 ---
 blockdev.c                                    |  31 +++--
 cpus.c                                        |   5 +-
 include/block/block.h                         |   4 +-
 include/block/block_int.h                     |   4 -
 include/sysemu/block-backend.h                |   7 +-
 monitor.c                                     |   7 +-
 qemu-char.c                                   |   3 +-
 stubs/Makefile.objs                           |   3 +-
 stubs/bdrv-next-monitor-owned.c               |   6 +
 stubs/{bdrv-commit-all.c => blk-commit-all.c} |   4 +-
 13 files changed, 203 insertions(+), 146 deletions(-)
 create mode 100644 stubs/bdrv-next-monitor-owned.c
 rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)

-- 
2.7.1

[Qemu-devel] [PATCH v3 01/14] monitor: Use BB list for BB name completion

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 monitor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 73eac17..7620e20 100644
--- a/monitor.c
+++ b/monitor.c
@@ -42,6 +42,7 @@
 #include "ui/console.h"
 #include "ui/input.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include "audio/audio.h"
 #include "disas/disas.h"
 #include "sysemu/balloon.h"
@@ -3467,7 +3468,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
     int i;
     const char *ptype, *str, *name;
     const mon_cmd_t *cmd;
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     if (nb_args <= 1) {
         /* command completion */
@@ -3522,8 +3523,8 @@ static void monitor_find_completion_by_table(Monitor *mon,
         case 'B':
             /* block device name completion */
             readline_set_completion_index(mon->rs, strlen(str));
-            for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-                name = bdrv_get_device_name(bs);
+            while ((blk = blk_next(blk)) != NULL) {
+                name = blk_name(blk);
                 if (str[0] == '\0' ||
                     !strncmp(name, str, strlen(str))) {
                     readline_add_completion(mon->rs, name);
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 01/14] monitor: Use BB list for BB name completion

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

In the long run, I guess we want to have to separate types, one of which
accepts only BlockBackend names and the other one BB and BDS names.
Though I think that most HMP commands currently require a BB, so this
patch is okay for now.

Kevin

Re: [Qemu-devel] [PATCH v3 01/14] monitor: Use BB list for BB name completion

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On 17.02.2016 11:09, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> In the long run, I guess we want to have to separate types, one of which
> accepts only BlockBackend names and the other one BB and BDS names.

Well, for the second type we shouldn't use bdrv_next() either but
bdrv_next_node() instead (and bdrv_get_node_name() instead of
bdrv_get_device_name()).

Max

> Though I think that most HMP commands currently require a BB, so this
> patch is okay for now.
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v3 02/14] block: Use blk_next() where appropriate

[Max Reitz]

blk_by_name() and blk_by_legacy_dinfo() are not supposed to iterate over
hidden BlockBackends (that is, BlockBackends that do not have a name).
As of a follow-up patch, blk_backends will actually contain all BBs, not
only non-hidden ones, so use blk_next() instead of iterating over that
list.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index ebdf78a..2146268 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -269,10 +269,10 @@ const char *blk_name(BlockBackend *blk)
  */
 BlockBackend *blk_by_name(const char *name)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
     assert(name);
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (!strcmp(name, blk->name)) {
             return blk;
         }
@@ -330,9 +330,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
  */
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (blk->legacy_dinfo == dinfo) {
             return blk;
         }
-- 
2.7.1

[Qemu-devel] [PATCH v3 03/14] block: Add blk_all_next()

[Max Reitz]

Some places in block/block-backend.c need to iterate over actually all
BlockBackends, so this adds a function to do so. blk_next() in contrast
only iterates over the BlockBackends which are owned by the monitor.
Right now, both do the same, but this will change as of a follow-up
patch.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 2146268..30decb4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -223,11 +223,21 @@ void blk_unref(BlockBackend *blk)
     }
 }
 
+/*
+ * Behaves similarly to blk_next() but iterates over all BlockBackends, even the
+ * ones which are hidden (i.e. are not referenced by the monitor).
+ */
+static BlockBackend *blk_all_next(BlockBackend *blk)
+{
+    return blk ? QTAILQ_NEXT(blk, link)
+               : QTAILQ_FIRST(&blk_backends);
+}
+
 void blk_remove_all_bs(void)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_all_next(blk)) != NULL) {
         AioContext *ctx = blk_get_aio_context(blk);
 
         aio_context_acquire(ctx);
-- 
2.7.1

[Qemu-devel] [PATCH v3 04/14] block: Add blk_name_taken()

[Max Reitz]

There may be BlockBackends which are not returned by blk_by_name(), but
do exist and have a name. blk_name_taken() allows testing whether a
specific name is in use already, independent of whether the BlockBackend
with that name is accessible through blk_by_name().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                        |  4 ++--
 block/block-backend.c          | 19 ++++++++++++++++++-
 include/sysemu/block-backend.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index efc3c43..a119840 100644
--- a/block.c
+++ b/block.c
@@ -847,8 +847,8 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
         return;
     }
 
-    /* takes care of avoiding namespaces collisions */
-    if (blk_by_name(node_name)) {
+    /* takes care of avoiding namespace collisions */
+    if (blk_name_taken(node_name)) {
         error_setg(errp, "node-name=%s is conflicting with a device id",
                    node_name);
         goto out;
diff --git a/block/block-backend.c b/block/block-backend.c
index 30decb4..45d4057 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -87,7 +87,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
         error_setg(errp, "Invalid device name");
         return NULL;
     }
-    if (blk_by_name(name)) {
+    if (blk_name_taken(name)) {
         error_setg(errp, "Device with id '%s' already exists", name);
         return NULL;
     }
@@ -291,6 +291,23 @@ BlockBackend *blk_by_name(const char *name)
 }
 
 /*
+ * This function should be used to check whether a certain BlockBackend name is
+ * already taken; blk_by_name() will only search in the list of monitor-owned
+ * BlockBackends which is not necessarily complete.
+ */
+bool blk_name_taken(const char *name)
+{
+    BlockBackend *blk = NULL;
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        if (!strcmp(name, blk->name)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/*
  * Return the BlockDriverState attached to @blk if any, else null.
  */
 BlockDriverState *blk_bs(BlockBackend *blk)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index ec30331..3fbf822 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -71,6 +71,7 @@ void blk_unref(BlockBackend *blk);
 void blk_remove_all_bs(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
+bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 04/14] block: Add blk_name_taken()

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> There may be BlockBackends which are not returned by blk_by_name(), but
> do exist and have a name.

Really? And if so, isn't this a bug?

I expect that a BB is always either visible to the user and has a name
that is resolved to this BB everywhere, or it's entirely internal and
doesn't need a name therefore.

Having a BB that is internal and therefore invisble, but has a name and
prevents the creation of another BB or BDS with the same name, must
certainly be confusing for the user.

> blk_name_taken() allows testing whether a
> specific name is in use already, independent of whether the BlockBackend
> with that name is accessible through blk_by_name().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Kevin

Re: [Qemu-devel] [PATCH v3 04/14] block: Add blk_name_taken()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On 17.02.2016 11:29, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> There may be BlockBackends which are not returned by blk_by_name(), but
>> do exist and have a name.
> 
> Really? And if so, isn't this a bug?

Depends on your definition of what the name is. :-)

As you said on IRC, denoting the monitor reference by that name seems
reasonable, so in that case it would be wrong behavior indeed.

> I expect that a BB is always either visible to the user and has a name
> that is resolved to this BB everywhere, or it's entirely internal and
> doesn't need a name therefore.
> 
> Having a BB that is internal and therefore invisble, but has a name and
> prevents the creation of another BB or BDS with the same name, must
> certainly be confusing for the user.

Yep, will change.

Max

>> blk_name_taken() allows testing whether a
>> specific name is in use already, independent of whether the BlockBackend
>> with that name is accessible through blk_by_name().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v3 05/14] block: Add blk_commit_all()

[Max Reitz]

Later, we will remove bdrv_commit_all() and move its contents here, and
in order to replace bdrv_commit_all() calls by calls to blk_commit_all()
before doing so, we need to add it as an alias now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 45d4057..62cff17 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1347,3 +1347,8 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 {
     return &blk->root_state;
 }
+
+int blk_commit_all(void)
+{
+    return bdrv_commit_all();
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3fbf822..8027671 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -128,6 +128,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_flush_all(void);
+int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
-- 
2.7.1

[Qemu-devel] [PATCH v3 06/14] block: Use blk_{commit, flush}_all() consistently

[Max Reitz]

Replace bdrv_commmit_all() and bdrv_flush_all() by their BlockBackend
equivalents.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c  | 2 +-
 cpus.c      | 5 +++--
 qemu-char.c | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8eff47d..46cd8a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1175,7 +1175,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
     int ret;
 
     if (!strcmp(device, "all")) {
-        ret = bdrv_commit_all();
+        ret = blk_commit_all();
     } else {
         BlockDriverState *bs;
         AioContext *aio_context;
diff --git a/cpus.c b/cpus.c
index 898426c..2d34518 100644
--- a/cpus.c
+++ b/cpus.c
@@ -29,6 +29,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "exec/gdbstub.h"
 #include "sysemu/dma.h"
 #include "sysemu/kvm.h"
@@ -726,7 +727,7 @@ static int do_vm_stop(RunState state)
     }
 
     bdrv_drain_all();
-    ret = bdrv_flush_all();
+    ret = blk_flush_all();
 
     return ret;
 }
@@ -1428,7 +1429,7 @@ int vm_stop_force_state(RunState state)
         bdrv_drain_all();
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
-        return bdrv_flush_all();
+        return blk_flush_all();
     }
 }
 
diff --git a/qemu-char.c b/qemu-char.c
index 1b7d5da..bd58df2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -25,6 +25,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/char.h"
@@ -561,7 +562,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
                  break;
             }
         case 's':
-            bdrv_commit_all();
+            blk_commit_all();
             break;
         case 'b':
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
-- 
2.7.1

[Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

The monitor does hold references to some BlockBackends so it should have
a list of those BBs; blk_backends is a different list, as it contains
references to all BBs (after a follow-up patch, that is), and that
should not be changed because we do need such a list.

monitor_remove_blk() is idempotent so that we can call it in
blockdev_auto_del() without having to care whether it had been called in
do_drive_del() before. monitor_add_blk() is idempotent for symmetry
reasons (monitor_remove_blk() is, so it would be strange for
monitor_add_blk() not to be).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 34 +++++++++++++++++++++++++++++++++-
 blockdev.c                     |  8 ++++++++
 include/sysemu/block-backend.h |  2 ++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 62cff17..430d7d5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -30,6 +30,8 @@ struct BlockBackend {
     BlockDriverState *bs;
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
     QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
+    QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
+    bool in_monitor_list;
 
     void *dev;                  /* attached device model, if any */
     /* TODO change to DeviceState when all users are qdevified */
@@ -71,6 +73,11 @@ static void drive_info_del(DriveInfo *dinfo);
 static QTAILQ_HEAD(, BlockBackend) blk_backends =
     QTAILQ_HEAD_INITIALIZER(blk_backends);
 
+/* All BlockBackends referenced by the monitor and which are iterated through by
+ * blk_next() */
+static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
+    QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
+
 /*
  * Create a new BlockBackend with @name, with a reference count of one.
  * @name must not be null or empty.
@@ -260,7 +267,32 @@ void blk_remove_all_bs(void)
  */
 BlockBackend *blk_next(BlockBackend *blk)
 {
-    return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&blk_backends);
+    return blk ? QTAILQ_NEXT(blk, monitor_link)
+               : QTAILQ_FIRST(&monitor_block_backends);
+}
+
+/*
+ * Add a BlockBackend into the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_add_blk(BlockBackend *blk)
+{
+    if (!blk->in_monitor_list) {
+        QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+        blk->in_monitor_list = true;
+    }
+}
+
+/*
+ * Remove a BlockBackend from the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_remove_blk(BlockBackend *blk)
+{
+    if (blk->in_monitor_list) {
+        QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
+        blk->in_monitor_list = false;
+    }
 }
 
 /*
diff --git a/blockdev.c b/blockdev.c
index 46cd8a9..ba1f648 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -147,6 +147,7 @@ void blockdev_auto_del(BlockBackend *blk)
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     }
 }
@@ -643,6 +644,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
     blk_set_on_error(blk, on_read_error, on_write_error);
 
+    monitor_add_blk(blk);
+
 err_no_bs_opts:
     qemu_opts_del(opts);
     QDECREF(interval_dict);
@@ -2813,6 +2816,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
         blk_remove_bs(blk);
     }
 
+    monitor_remove_blk(blk);
+
     /* if we have a device attached to this BlockDriverState
      * then we need to make the drive anonymous until the device
      * can be removed.  If this is a drive with no device backing
@@ -3899,6 +3904,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     if (bs && bdrv_key_required(bs)) {
         if (blk) {
+            monitor_remove_blk(blk);
             blk_unref(blk);
         } else {
             QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
@@ -3928,6 +3934,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (has_id) {
+        /* blk_by_name() never returns a BB that is not owned by the monitor */
         blk = blk_by_name(id);
         if (!blk) {
             error_setg(errp, "Cannot find block backend %s", id);
@@ -3975,6 +3982,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (blk) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     } else {
         QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8027671..078690a 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -73,6 +73,8 @@ const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
+void monitor_add_blk(BlockBackend *blk);
+void monitor_remove_blk(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> The monitor does hold references to some BlockBackends so it should have

s/does hold/holds/?

> a list of those BBs; blk_backends is a different list, as it contains
> references to all BBs (after a follow-up patch, that is), and that
> should not be changed because we do need such a list.
> 
> monitor_remove_blk() is idempotent so that we can call it in
> blockdev_auto_del() without having to care whether it had been called in
> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
> reasons (monitor_remove_blk() is, so it would be strange for
> monitor_add_blk() not to be).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I think hmp_drive_add() needs a monitor_remove_blk() in its error path.

Kevin

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On 17.02.2016 11:53, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> The monitor does hold references to some BlockBackends so it should have
> 
> s/does hold/holds/?

It was intentional, so I'd keep it unless you drop the question mark.

>> a list of those BBs; blk_backends is a different list, as it contains
>> references to all BBs (after a follow-up patch, that is), and that
>> should not be changed because we do need such a list.
>>
>> monitor_remove_blk() is idempotent so that we can call it in
>> blockdev_auto_del() without having to care whether it had been called in
>> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
>> reasons (monitor_remove_blk() is, so it would be strange for
>> monitor_add_blk() not to be).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> I think hmp_drive_add() needs a monitor_remove_blk() in its error path.

You're right, thanks.

In addition, if we really do say that a BB having a name equals being
referenced by the monitor, then maybe we don't need explicit calls to
monitor_add_blk() because any BB that is created with a non-NULL name
should be automatically added to the list of monitor BBs.

But that would mean that qemu-img's, qemu-nbd's and qemu-io's BBs would
have to be monitor-owned, too, and they'd all have to call
monitor_remove_blk() all over the place... Unless we'd allow NULL BB
names now and make them use it (I don't really see a reason why not;
them calling their BBs "hda" seems weird anyway), or implicitly call
monitor_remove_blk() in blk_delete(). Or maybe both, because the latter
seems convenient anyway.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
> On 17.02.2016 11:53, Kevin Wolf wrote:
> > Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> >> The monitor does hold references to some BlockBackends so it should have
> > 
> > s/does hold/holds/?
> 
> It was intentional, so I'd keep it unless you drop the question mark.

For me it seems to imply something like "contrary to your expectations",
but maybe that's just my non-native English needing a fix.

I don't find it surprising anyway that the monitor holds BB references.

> >> a list of those BBs; blk_backends is a different list, as it contains
> >> references to all BBs (after a follow-up patch, that is), and that
> >> should not be changed because we do need such a list.
> >>
> >> monitor_remove_blk() is idempotent so that we can call it in
> >> blockdev_auto_del() without having to care whether it had been called in
> >> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
> >> reasons (monitor_remove_blk() is, so it would be strange for
> >> monitor_add_blk() not to be).
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> > I think hmp_drive_add() needs a monitor_remove_blk() in its error path.
> 
> You're right, thanks.
> 
> In addition, if we really do say that a BB having a name equals being
> referenced by the monitor, then maybe we don't need explicit calls to
> monitor_add_blk() because any BB that is created with a non-NULL name
> should be automatically added to the list of monitor BBs.

While probably workable, I'd rather avoid this kind of magic where the
presence of a name parameter decides whether a reference is taken or
not. It makes the interface of the function a lot less obvious.

> But that would mean that qemu-img's, qemu-nbd's and qemu-io's BBs would
> have to be monitor-owned, too, and they'd all have to call
> monitor_remove_blk() all over the place... Unless we'd allow NULL BB
> names now and make them use it (I don't really see a reason why not;
> them calling their BBs "hda" seems weird anyway), or implicitly call
> monitor_remove_blk() in blk_delete(). Or maybe both, because the latter
> seems convenient anyway.

I'm not sure. It would be correct even when we start to create BBs
automatically, because monitor_remove_blk() doesn't do anything when
there is no monitor reference and the monitor reference is the last
thing that can be returned (hopefully). But I like reference counting to
be as explicit as possible.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3478 bytes --]

On 17.02.2016 17:20, Kevin Wolf wrote:
> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>> The monitor does hold references to some BlockBackends so it should have
>>>
>>> s/does hold/holds/?
>>
>> It was intentional, so I'd keep it unless you drop the question mark.
> 
> For me it seems to imply something like "contrary to your expectations",
> but maybe that's just my non-native English needing a fix.
> 
> I don't find it surprising anyway that the monitor holds BB references.

The contrast I tried to point out is that while we do have these
references in theory, and they are reflected by a refcount, too, we do
not actually have these references because the monitor does not yet have
a list of the BBs it owns.

So it's not an "emphasize the verb because it may be contrary to your
expectations", but an "emphasize it because it is contrary to what the
current code does" (which is not actually referencing these BBs).

Like: It is supposed to have references. It says it does. But it
actually doesn't. It does "hold" them, however, because they are
accounted for in the BBs' refcounts.

>>>> a list of those BBs; blk_backends is a different list, as it contains
>>>> references to all BBs (after a follow-up patch, that is), and that
>>>> should not be changed because we do need such a list.
>>>>
>>>> monitor_remove_blk() is idempotent so that we can call it in
>>>> blockdev_auto_del() without having to care whether it had been called in
>>>> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
>>>> reasons (monitor_remove_blk() is, so it would be strange for
>>>> monitor_add_blk() not to be).
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> I think hmp_drive_add() needs a monitor_remove_blk() in its error path.
>>
>> You're right, thanks.
>>
>> In addition, if we really do say that a BB having a name equals being
>> referenced by the monitor, then maybe we don't need explicit calls to
>> monitor_add_blk() because any BB that is created with a non-NULL name
>> should be automatically added to the list of monitor BBs.
> 
> While probably workable, I'd rather avoid this kind of magic where the
> presence of a name parameter decides whether a reference is taken or
> not. It makes the interface of the function a lot less obvious.

Still you want the name to be the monitor's reference to the BB. Thus,
if monitor_add_blk() should not be called implicitly by blk_new(), then
I'd instead move the @name parameter from blk_new() to monitor_add_blk().

Max

>> But that would mean that qemu-img's, qemu-nbd's and qemu-io's BBs would
>> have to be monitor-owned, too, and they'd all have to call
>> monitor_remove_blk() all over the place... Unless we'd allow NULL BB
>> names now and make them use it (I don't really see a reason why not;
>> them calling their BBs "hda" seems weird anyway), or implicitly call
>> monitor_remove_blk() in blk_delete(). Or maybe both, because the latter
>> seems convenient anyway.
> 
> I'm not sure. It would be correct even when we start to create BBs
> automatically, because monitor_remove_blk() doesn't do anything when
> there is no monitor reference and the monitor reference is the last
> thing that can be returned (hopefully). But I like reference counting to
> be as explicit as possible.
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]

On 20.02.2016 14:34, Max Reitz wrote:
> On 17.02.2016 17:20, Kevin Wolf wrote:
>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>
>>>> s/does hold/holds/?
>>>
>>> It was intentional, so I'd keep it unless you drop the question mark.
>>
>> For me it seems to imply something like "contrary to your expectations",
>> but maybe that's just my non-native English needing a fix.
>>
>> I don't find it surprising anyway that the monitor holds BB references.
> 
> The contrast I tried to point out is that while we do have these
> references in theory, and they are reflected by a refcount, too, we do
> not actually have these references because the monitor does not yet have
> a list of the BBs it owns.
> 
> So it's not an "emphasize the verb because it may be contrary to your
> expectations", but an "emphasize it because it is contrary to what the
> current code does" (which is not actually referencing these BBs).
> 
> Like: It is supposed to have references. It says it does. But it
> actually doesn't. It does "hold" them, however, because they are
> accounted for in the BBs' refcounts.
> 
>>>>> a list of those BBs; blk_backends is a different list, as it contains
>>>>> references to all BBs (after a follow-up patch, that is), and that
>>>>> should not be changed because we do need such a list.
>>>>>
>>>>> monitor_remove_blk() is idempotent so that we can call it in
>>>>> blockdev_auto_del() without having to care whether it had been called in
>>>>> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
>>>>> reasons (monitor_remove_blk() is, so it would be strange for
>>>>> monitor_add_blk() not to be).
>>>>>
>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>
>>>> I think hmp_drive_add() needs a monitor_remove_blk() in its error path.
>>>
>>> You're right, thanks.
>>>
>>> In addition, if we really do say that a BB having a name equals being
>>> referenced by the monitor, then maybe we don't need explicit calls to
>>> monitor_add_blk() because any BB that is created with a non-NULL name
>>> should be automatically added to the list of monitor BBs.
>>
>> While probably workable, I'd rather avoid this kind of magic where the
>> presence of a name parameter decides whether a reference is taken or
>> not. It makes the interface of the function a lot less obvious.
> 
> Still you want the name to be the monitor's reference to the BB. Thus,
> if monitor_add_blk() should not be called implicitly by blk_new(), then
> I'd instead move the @name parameter from blk_new() to monitor_add_blk().

...aaand I noticed that this is what you said for patch 8 anyway. Good.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 17.02.2016 17:20, Kevin Wolf wrote:
>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>
>>>> s/does hold/holds/?
>>>
>>> It was intentional, so I'd keep it unless you drop the question mark.
>> 
>> For me it seems to imply something like "contrary to your expectations",
>> but maybe that's just my non-native English needing a fix.
>> 
>> I don't find it surprising anyway that the monitor holds BB references.

Me neither.

> The contrast I tried to point out is that while we do have these
> references in theory, and they are reflected by a refcount, too, we do
> not actually have these references because the monitor does not yet have
> a list of the BBs it owns.

Oh yes, it has.  It's just outsources their actual storage to
block-backend.c, but that's detail.

> So it's not an "emphasize the verb because it may be contrary to your
> expectations", but an "emphasize it because it is contrary to what the
> current code does" (which is not actually referencing these BBs).

I disagree :)

> Like: It is supposed to have references. It says it does. But it
> actually doesn't. It does "hold" them, however, because they are
> accounted for in the BBs' refcounts.
[...]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

On 22.02.2016 09:24, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>>
>>>>> s/does hold/holds/?
>>>>
>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>>
>>> For me it seems to imply something like "contrary to your expectations",
>>> but maybe that's just my non-native English needing a fix.
>>>
>>> I don't find it surprising anyway that the monitor holds BB references.
> 
> Me neither.
> 
>> The contrast I tried to point out is that while we do have these
>> references in theory, and they are reflected by a refcount, too, we do
>> not actually have these references because the monitor does not yet have
>> a list of the BBs it owns.
> 
> Oh yes, it has.  It's just outsources their actual storage to
> block-backend.c, but that's detail.

In my opinion it's not a reference made by the monitor, then, especially
because it's done through magic. With this interpretation,
block-backend.c considers every BB to be monitor-owned (until
blk_hide...() is called).

Also, if blk_backends is supposed to be the list of monitor-owned BBs,
then it's a really bad name and I put all the blame on that.

>> So it's not an "emphasize the verb because it may be contrary to your
>> expectations", but an "emphasize it because it is contrary to what the
>> current code does" (which is not actually referencing these BBs).
> 
> I disagree :)

Then I'd say "It's contrary to my interpretation of what the current
code wants to do." Now it's my personal opinion! ;-)

I wouldn't mind removing the "does" from the commit message (obviously),
but that is not the only thing there which conflicts with your opinion.
It clearly states that blk_backends is not the list of monitor-owned
BlockBackends, whereas you are saying that it very much is.

So...? Rephrase it entirely? State that blk_backends is a bad name and
this commit is basically duplicating blk_backends as
monitor_block_backends, which is the correct name, and that a follow-up
commit will make blk_backends contain what it name implies it does? Or
just call the commit "Remove magic", because it adds explicit functions
for saying that a BB is monitor-owned or that it is not?

Max

>> Like: It is supposed to have references. It says it does. But it
>> actually doesn't. It does "hold" them, however, because they are
>> accounted for in the BBs' refcounts.
> [...]
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 22.02.2016 09:24, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>>>
>>>>>> s/does hold/holds/?
>>>>>
>>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>>>
>>>> For me it seems to imply something like "contrary to your expectations",
>>>> but maybe that's just my non-native English needing a fix.
>>>>
>>>> I don't find it surprising anyway that the monitor holds BB references.
>> 
>> Me neither.
>> 
>>> The contrast I tried to point out is that while we do have these
>>> references in theory, and they are reflected by a refcount, too, we do
>>> not actually have these references because the monitor does not yet have
>>> a list of the BBs it owns.
>> 
>> Oh yes, it has.  It's just outsources their actual storage to
>> block-backend.c, but that's detail.
>
> In my opinion it's not a reference made by the monitor, then, especially
> because it's done through magic. With this interpretation,
> block-backend.c considers every BB to be monitor-owned (until
> blk_hide...() is called).

block-backend.c holds a reference from blk_backends.  By *why* does it
do that?  Let's go through the uses of blk_backends.

0. blk_backends maintenance: blk_new(), blk_delete(),
   blk_hide_on_behalf_of_hmp_drive_del()

1. To permit lookup by name, with blk_by_name().

   In my view, block-backend.c holds this reference in trust for those
   parts of QEMU that reference by name rather than by pointer, most
   prominently the monitor.

   I can't see the point of backing up the reference by name with a
   reference by pointer in the monitor, like your patch seems to do.
   What's the difference between the two?  Can one ever exist without
   the other?  Why in the monitor, and not in any other part looking up
   by name?

2. To iterate over all named ones, with blk_next().

   Since this can only return named BBs, the reference held in trust for
   named lookup suffices.

3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()

   Since this must only return named BBs, the reference held in trust
   for named lookup suffices.

4. To do something so unimportant that it's not worth explaining, with
   blk_remove_bs().

   I made a point of giving every single external function a carefully
   worded contract, to hopefully shame future contributors into
   following suit.  Didn't work.

> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
> then it's a really bad name and I put all the blame on that.

Naming is hard.  Feel free to propose better comments and/or better
names.

>>> So it's not an "emphasize the verb because it may be contrary to your
>>> expectations", but an "emphasize it because it is contrary to what the
>>> current code does" (which is not actually referencing these BBs).
>> 
>> I disagree :)
>
> Then I'd say "It's contrary to my interpretation of what the current
> code wants to do." Now it's my personal opinion! ;-)
>
> I wouldn't mind removing the "does" from the commit message (obviously),
> but that is not the only thing there which conflicts with your opinion.
> It clearly states that blk_backends is not the list of monitor-owned
> BlockBackends, whereas you are saying that it very much is.
>
> So...? Rephrase it entirely? State that blk_backends is a bad name and
> this commit is basically duplicating blk_backends as
> monitor_block_backends, which is the correct name, and that a follow-up
> commit will make blk_backends contain what it name implies it does? Or
> just call the commit "Remove magic", because it adds explicit functions
> for saying that a BB is monitor-owned or that it is not?

Can you explain the *actual* difference between blk_backends and
monitor_block_backends?  "Actual" in the sense of difference in contents
over time.

>>> Like: It is supposed to have references. It says it does. But it
>>> actually doesn't. It does "hold" them, however, because they are
>>> accounted for in the BBs' refcounts.
>> [...]
>> 

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6073 bytes --]

On 23.02.2016 10:48, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 22.02.2016 09:24, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>>>>
>>>>>>> s/does hold/holds/?
>>>>>>
>>>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>>>>
>>>>> For me it seems to imply something like "contrary to your expectations",
>>>>> but maybe that's just my non-native English needing a fix.
>>>>>
>>>>> I don't find it surprising anyway that the monitor holds BB references.
>>>
>>> Me neither.
>>>
>>>> The contrast I tried to point out is that while we do have these
>>>> references in theory, and they are reflected by a refcount, too, we do
>>>> not actually have these references because the monitor does not yet have
>>>> a list of the BBs it owns.
>>>
>>> Oh yes, it has.  It's just outsources their actual storage to
>>> block-backend.c, but that's detail.
>>
>> In my opinion it's not a reference made by the monitor, then, especially
>> because it's done through magic. With this interpretation,
>> block-backend.c considers every BB to be monitor-owned (until
>> blk_hide...() is called).
> 
> block-backend.c holds a reference from blk_backends.  By *why* does it
> do that?  Let's go through the uses of blk_backends.
> 
> 0. blk_backends maintenance: blk_new(), blk_delete(),
>    blk_hide_on_behalf_of_hmp_drive_del()
> 
> 1. To permit lookup by name, with blk_by_name().
> 
>    In my view, block-backend.c holds this reference in trust for those
>    parts of QEMU that reference by name rather than by pointer, most
>    prominently the monitor.
> 
>    I can't see the point of backing up the reference by name with a
>    reference by pointer in the monitor, like your patch seems to do.
>    What's the difference between the two?  Can one ever exist without
>    the other?  Why in the monitor, and not in any other part looking up
>    by name?
> 
> 2. To iterate over all named ones, with blk_next().
> 
>    Since this can only return named BBs, the reference held in trust for
>    named lookup suffices.
> 
> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
> 
>    Since this must only return named BBs, the reference held in trust
>    for named lookup suffices.
> 
> 4. To do something so unimportant that it's not worth explaining, with
>    blk_remove_bs().
> 
>    I made a point of giving every single external function a carefully
>    worded contract, to hopefully shame future contributors into
>    following suit.  Didn't work.

Side note: I consider it very important and there was no other way to do
it before this series, because there is no list of all block backends.

>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
>> then it's a really bad name and I put all the blame on that.
> 
> Naming is hard.  Feel free to propose better comments and/or better
> names.

I did. It's "monitor_block_backends".

>>>> So it's not an "emphasize the verb because it may be contrary to your
>>>> expectations", but an "emphasize it because it is contrary to what the
>>>> current code does" (which is not actually referencing these BBs).
>>>
>>> I disagree :)
>>
>> Then I'd say "It's contrary to my interpretation of what the current
>> code wants to do." Now it's my personal opinion! ;-)
>>
>> I wouldn't mind removing the "does" from the commit message (obviously),
>> but that is not the only thing there which conflicts with your opinion.
>> It clearly states that blk_backends is not the list of monitor-owned
>> BlockBackends, whereas you are saying that it very much is.
>>
>> So...? Rephrase it entirely? State that blk_backends is a bad name and
>> this commit is basically duplicating blk_backends as
>> monitor_block_backends, which is the correct name, and that a follow-up
>> commit will make blk_backends contain what it name implies it does? Or
>> just call the commit "Remove magic", because it adds explicit functions
>> for saying that a BB is monitor-owned or that it is not?
> 
> Can you explain the *actual* difference between blk_backends and
> monitor_block_backends?  "Actual" in the sense of difference in contents
> over time.

First difference: The name. That's enough reason for me.

Second difference: blk_new() adds any BB to blk_backends automatically.
It doesn't do so for monitor_block_backends.

Third difference: Often the monitor doesn't take care of signalling that
it's releasing its reference, only sometimes (where "sometimes" means
every time blk_hide...() is called). blk_delete() will instead
automatically remove it from blk_backends, because it's assuming that
the last reference had been held by the monitor.

The second and third difference are what I referred to as "magic". You
could also call them "convenience", if you prefer, but in any case as we
can see by the existence blk_hide...(), removing the monitor reference
in blk_delete() appears to be wrong. Both should be separate operations,
and this is what this patch does.

Assuming that any blk_new() is ultimately done by the monitor, we maybe
actually do not need an own monitor_add_blk() function; except that
Kevin stated that he does deem it useful when I proposed inlining it
(back) into blk_new().

All in all, you have convinced me that the commit message is incorrect.
It should state that blk_backends is effectively repurposed to contain
the list of all BBs, and that a more explicit monitor_block_backends
list will take its place, with explicit operations for the monitor to
signal when it takes or releases the reference to a BB.

However, I still believe this patch itself to be useful.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 6656 bytes --]

Am 23.02.2016 um 14:52 hat Max Reitz geschrieben:
> On 23.02.2016 10:48, Markus Armbruster wrote:
> > Max Reitz <mreitz@redhat.com> writes:
> > 
> >> On 22.02.2016 09:24, Markus Armbruster wrote:
> >>> Max Reitz <mreitz@redhat.com> writes:
> >>>
> >>>> On 17.02.2016 17:20, Kevin Wolf wrote:
> >>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
> >>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
> >>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> >>>>>>>> The monitor does hold references to some BlockBackends so it should have
> >>>>>>>
> >>>>>>> s/does hold/holds/?
> >>>>>>
> >>>>>> It was intentional, so I'd keep it unless you drop the question mark.
> >>>>>
> >>>>> For me it seems to imply something like "contrary to your expectations",
> >>>>> but maybe that's just my non-native English needing a fix.
> >>>>>
> >>>>> I don't find it surprising anyway that the monitor holds BB references.
> >>>
> >>> Me neither.
> >>>
> >>>> The contrast I tried to point out is that while we do have these
> >>>> references in theory, and they are reflected by a refcount, too, we do
> >>>> not actually have these references because the monitor does not yet have
> >>>> a list of the BBs it owns.
> >>>
> >>> Oh yes, it has.  It's just outsources their actual storage to
> >>> block-backend.c, but that's detail.
> >>
> >> In my opinion it's not a reference made by the monitor, then, especially
> >> because it's done through magic. With this interpretation,
> >> block-backend.c considers every BB to be monitor-owned (until
> >> blk_hide...() is called).
> > 
> > block-backend.c holds a reference from blk_backends.  By *why* does it
> > do that?  Let's go through the uses of blk_backends.
> > 
> > 0. blk_backends maintenance: blk_new(), blk_delete(),
> >    blk_hide_on_behalf_of_hmp_drive_del()
> > 
> > 1. To permit lookup by name, with blk_by_name().
> > 
> >    In my view, block-backend.c holds this reference in trust for those
> >    parts of QEMU that reference by name rather than by pointer, most
> >    prominently the monitor.
> > 
> >    I can't see the point of backing up the reference by name with a
> >    reference by pointer in the monitor, like your patch seems to do.
> >    What's the difference between the two?  Can one ever exist without
> >    the other?  Why in the monitor, and not in any other part looking up
> >    by name?
> > 
> > 2. To iterate over all named ones, with blk_next().
> > 
> >    Since this can only return named BBs, the reference held in trust for
> >    named lookup suffices.
> > 
> > 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
> > 
> >    Since this must only return named BBs, the reference held in trust
> >    for named lookup suffices.
> > 
> > 4. To do something so unimportant that it's not worth explaining, with
> >    blk_remove_bs().
> > 
> >    I made a point of giving every single external function a carefully
> >    worded contract, to hopefully shame future contributors into
> >    following suit.  Didn't work.
> 
> Side note: I consider it very important and there was no other way to do
> it before this series, because there is no list of all block backends.
> 
> >> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
> >> then it's a really bad name and I put all the blame on that.
> > 
> > Naming is hard.  Feel free to propose better comments and/or better
> > names.
> 
> I did. It's "monitor_block_backends".
> 
> >>>> So it's not an "emphasize the verb because it may be contrary to your
> >>>> expectations", but an "emphasize it because it is contrary to what the
> >>>> current code does" (which is not actually referencing these BBs).
> >>>
> >>> I disagree :)
> >>
> >> Then I'd say "It's contrary to my interpretation of what the current
> >> code wants to do." Now it's my personal opinion! ;-)
> >>
> >> I wouldn't mind removing the "does" from the commit message (obviously),
> >> but that is not the only thing there which conflicts with your opinion.
> >> It clearly states that blk_backends is not the list of monitor-owned
> >> BlockBackends, whereas you are saying that it very much is.
> >>
> >> So...? Rephrase it entirely? State that blk_backends is a bad name and
> >> this commit is basically duplicating blk_backends as
> >> monitor_block_backends, which is the correct name, and that a follow-up
> >> commit will make blk_backends contain what it name implies it does? Or
> >> just call the commit "Remove magic", because it adds explicit functions
> >> for saying that a BB is monitor-owned or that it is not?
> > 
> > Can you explain the *actual* difference between blk_backends and
> > monitor_block_backends?  "Actual" in the sense of difference in contents
> > over time.
> 
> First difference: The name. That's enough reason for me.
> 
> Second difference: blk_new() adds any BB to blk_backends automatically.
> It doesn't do so for monitor_block_backends.
> 
> Third difference: Often the monitor doesn't take care of signalling that
> it's releasing its reference, only sometimes (where "sometimes" means
> every time blk_hide...() is called). blk_delete() will instead
> automatically remove it from blk_backends, because it's assuming that
> the last reference had been held by the monitor.
> 
> The second and third difference are what I referred to as "magic". You
> could also call them "convenience", if you prefer, but in any case as we
> can see by the existence blk_hide...(), removing the monitor reference
> in blk_delete() appears to be wrong. Both should be separate operations,
> and this is what this patch does.
> 
> Assuming that any blk_new() is ultimately done by the monitor, we maybe
> actually do not need an own monitor_add_blk() function; except that
> Kevin stated that he does deem it useful when I proposed inlining it
> (back) into blk_new().

This assumption is wrong. Or more precisely, it may be correct now, but
we want block jobs and NBD servers to create their own BBs in the
future. These will be BBs that are created by blk_new(), but not
referenced at all by the monitor.

Kevin

> All in all, you have convinced me that the commit message is incorrect.
> It should state that blk_backends is effectively repurposed to contain
> the list of all BBs, and that a more explicit monitor_block_backends
> list will take its place, with explicit operations for the monitor to
> signal when it takes or releases the reference to a BB.
> 
> However, I still believe this patch itself to be useful.
> 
> Max
> 



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 23.02.2016 10:48, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 22.02.2016 09:24, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>>>>>
>>>>>>>> s/does hold/holds/?
>>>>>>>
>>>>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>>>>>
>>>>>> For me it seems to imply something like "contrary to your expectations",
>>>>>> but maybe that's just my non-native English needing a fix.
>>>>>>
>>>>>> I don't find it surprising anyway that the monitor holds BB references.
>>>>
>>>> Me neither.
>>>>
>>>>> The contrast I tried to point out is that while we do have these
>>>>> references in theory, and they are reflected by a refcount, too, we do
>>>>> not actually have these references because the monitor does not yet have
>>>>> a list of the BBs it owns.
>>>>
>>>> Oh yes, it has.  It's just outsources their actual storage to
>>>> block-backend.c, but that's detail.
>>>
>>> In my opinion it's not a reference made by the monitor, then, especially
>>> because it's done through magic. With this interpretation,
>>> block-backend.c considers every BB to be monitor-owned (until
>>> blk_hide...() is called).
>> 
>> block-backend.c holds a reference from blk_backends.  By *why* does it
>> do that?  Let's go through the uses of blk_backends.
>> 
>> 0. blk_backends maintenance: blk_new(), blk_delete(),
>>    blk_hide_on_behalf_of_hmp_drive_del()
>> 
>> 1. To permit lookup by name, with blk_by_name().
>> 
>>    In my view, block-backend.c holds this reference in trust for those
>>    parts of QEMU that reference by name rather than by pointer, most
>>    prominently the monitor.
>> 
>>    I can't see the point of backing up the reference by name with a
>>    reference by pointer in the monitor, like your patch seems to do.
>>    What's the difference between the two?  Can one ever exist without
>>    the other?  Why in the monitor, and not in any other part looking up
>>    by name?
>> 
>> 2. To iterate over all named ones, with blk_next().
>> 
>>    Since this can only return named BBs, the reference held in trust for
>>    named lookup suffices.
>> 
>> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
>> 
>>    Since this must only return named BBs, the reference held in trust
>>    for named lookup suffices.
>> 
>> 4. To do something so unimportant that it's not worth explaining, with
>>    blk_remove_bs().
>> 
>>    I made a point of giving every single external function a carefully
>>    worded contract, to hopefully shame future contributors into
>>    following suit.  Didn't work.
>
> Side note: I consider it very important and there was no other way to do
> it before this series, because there is no list of all block backends.
>
>>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
>>> then it's a really bad name and I put all the blame on that.
>> 
>> Naming is hard.  Feel free to propose better comments and/or better
>> names.
>
> I did. It's "monitor_block_backends".
>
>>>>> So it's not an "emphasize the verb because it may be contrary to your
>>>>> expectations", but an "emphasize it because it is contrary to what the
>>>>> current code does" (which is not actually referencing these BBs).
>>>>
>>>> I disagree :)
>>>
>>> Then I'd say "It's contrary to my interpretation of what the current
>>> code wants to do." Now it's my personal opinion! ;-)
>>>
>>> I wouldn't mind removing the "does" from the commit message (obviously),
>>> but that is not the only thing there which conflicts with your opinion.
>>> It clearly states that blk_backends is not the list of monitor-owned
>>> BlockBackends, whereas you are saying that it very much is.
>>>
>>> So...? Rephrase it entirely? State that blk_backends is a bad name and
>>> this commit is basically duplicating blk_backends as
>>> monitor_block_backends, which is the correct name, and that a follow-up
>>> commit will make blk_backends contain what it name implies it does? Or
>>> just call the commit "Remove magic", because it adds explicit functions
>>> for saying that a BB is monitor-owned or that it is not?
>> 
>> Can you explain the *actual* difference between blk_backends and
>> monitor_block_backends?  "Actual" in the sense of difference in contents
>> over time.
>
> First difference: The name. That's enough reason for me.
>
> Second difference: blk_new() adds any BB to blk_backends automatically.
> It doesn't do so for monitor_block_backends.
>
> Third difference: Often the monitor doesn't take care of signalling that
> it's releasing its reference, only sometimes (where "sometimes" means
> every time blk_hide...() is called). blk_delete() will instead
> automatically remove it from blk_backends, because it's assuming that
> the last reference had been held by the monitor.

The reference held in trust for lookup by name exists as long as lookup
by name is permitted.

Therefore, it goes away when the BB dies or when it loses its name.

The only way for a BB to lose its name is (was?) the messed up HMP
drive_del: it wants to kill the BB right away, but can't, so it needs to
hide it instead until it can.

New functionality may lead to a more complex live cycle where BBs may
acquire and lose their name in new ways.

Note that I carefully avoid calling the reference the monitor's
reference.  Because you made me realize it's not the monitor's alone!
Lookup by name has more customers than just the monitor.

> The second and third difference are what I referred to as "magic". You
> could also call them "convenience", if you prefer, but in any case as we
> can see by the existence blk_hide...(), removing the monitor reference
> in blk_delete() appears to be wrong. Both should be separate operations,
> and this is what this patch does.

I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del()
shows that the name going away in blk_delete() is wrong.  It must go
away there, because the thing it names goes away.

Additional operations to add / remove a BBs name may make sense; I'd
have to see their users.  See "more complex live cycle" above.

> Assuming that any blk_new() is ultimately done by the monitor, we maybe
> actually do not need an own monitor_add_blk() function; except that
> Kevin stated that he does deem it useful when I proposed inlining it
> (back) into blk_new().

As Kevin noted, that's not a good assumption.

> All in all, you have convinced me that the commit message is incorrect.
> It should state that blk_backends is effectively repurposed to contain
> the list of all BBs, and that a more explicit monitor_block_backends
> list will take its place, with explicit operations for the monitor to
> signal when it takes or releases the reference to a BB.

A member of monitor_block_backends is always a member of blk_backends.
Correct?

Is a member of blk_backends with a name always a member of
monitor_block_backends?

> However, I still believe this patch itself to be useful.
>
> Max

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

Am 24.02.2016 um 10:28 hat Markus Armbruster geschrieben:
> Max Reitz <mreitz@redhat.com> writes:
> 
> > On 23.02.2016 10:48, Markus Armbruster wrote:
> >> Max Reitz <mreitz@redhat.com> writes:
> >> 
> >>> On 22.02.2016 09:24, Markus Armbruster wrote:
> >>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>
> >>>>> On 17.02.2016 17:20, Kevin Wolf wrote:
> >>>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
> >>>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
> >>>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> >>>>>>>>> The monitor does hold references to some BlockBackends so it should have
> >>>>>>>>
> >>>>>>>> s/does hold/holds/?
> >>>>>>>
> >>>>>>> It was intentional, so I'd keep it unless you drop the question mark.
> >>>>>>
> >>>>>> For me it seems to imply something like "contrary to your expectations",
> >>>>>> but maybe that's just my non-native English needing a fix.
> >>>>>>
> >>>>>> I don't find it surprising anyway that the monitor holds BB references.
> >>>>
> >>>> Me neither.
> >>>>
> >>>>> The contrast I tried to point out is that while we do have these
> >>>>> references in theory, and they are reflected by a refcount, too, we do
> >>>>> not actually have these references because the monitor does not yet have
> >>>>> a list of the BBs it owns.
> >>>>
> >>>> Oh yes, it has.  It's just outsources their actual storage to
> >>>> block-backend.c, but that's detail.
> >>>
> >>> In my opinion it's not a reference made by the monitor, then, especially
> >>> because it's done through magic. With this interpretation,
> >>> block-backend.c considers every BB to be monitor-owned (until
> >>> blk_hide...() is called).
> >> 
> >> block-backend.c holds a reference from blk_backends.  By *why* does it
> >> do that?  Let's go through the uses of blk_backends.
> >> 
> >> 0. blk_backends maintenance: blk_new(), blk_delete(),
> >>    blk_hide_on_behalf_of_hmp_drive_del()
> >> 
> >> 1. To permit lookup by name, with blk_by_name().
> >> 
> >>    In my view, block-backend.c holds this reference in trust for those
> >>    parts of QEMU that reference by name rather than by pointer, most
> >>    prominently the monitor.
> >> 
> >>    I can't see the point of backing up the reference by name with a
> >>    reference by pointer in the monitor, like your patch seems to do.
> >>    What's the difference between the two?  Can one ever exist without
> >>    the other?  Why in the monitor, and not in any other part looking up
> >>    by name?
> >> 
> >> 2. To iterate over all named ones, with blk_next().
> >> 
> >>    Since this can only return named BBs, the reference held in trust for
> >>    named lookup suffices.
> >> 
> >> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
> >> 
> >>    Since this must only return named BBs, the reference held in trust
> >>    for named lookup suffices.
> >> 
> >> 4. To do something so unimportant that it's not worth explaining, with
> >>    blk_remove_bs().
> >> 
> >>    I made a point of giving every single external function a carefully
> >>    worded contract, to hopefully shame future contributors into
> >>    following suit.  Didn't work.
> >
> > Side note: I consider it very important and there was no other way to do
> > it before this series, because there is no list of all block backends.
> >
> >>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
> >>> then it's a really bad name and I put all the blame on that.
> >> 
> >> Naming is hard.  Feel free to propose better comments and/or better
> >> names.
> >
> > I did. It's "monitor_block_backends".
> >
> >>>>> So it's not an "emphasize the verb because it may be contrary to your
> >>>>> expectations", but an "emphasize it because it is contrary to what the
> >>>>> current code does" (which is not actually referencing these BBs).
> >>>>
> >>>> I disagree :)
> >>>
> >>> Then I'd say "It's contrary to my interpretation of what the current
> >>> code wants to do." Now it's my personal opinion! ;-)
> >>>
> >>> I wouldn't mind removing the "does" from the commit message (obviously),
> >>> but that is not the only thing there which conflicts with your opinion.
> >>> It clearly states that blk_backends is not the list of monitor-owned
> >>> BlockBackends, whereas you are saying that it very much is.
> >>>
> >>> So...? Rephrase it entirely? State that blk_backends is a bad name and
> >>> this commit is basically duplicating blk_backends as
> >>> monitor_block_backends, which is the correct name, and that a follow-up
> >>> commit will make blk_backends contain what it name implies it does? Or
> >>> just call the commit "Remove magic", because it adds explicit functions
> >>> for saying that a BB is monitor-owned or that it is not?
> >> 
> >> Can you explain the *actual* difference between blk_backends and
> >> monitor_block_backends?  "Actual" in the sense of difference in contents
> >> over time.
> >
> > First difference: The name. That's enough reason for me.
> >
> > Second difference: blk_new() adds any BB to blk_backends automatically.
> > It doesn't do so for monitor_block_backends.
> >
> > Third difference: Often the monitor doesn't take care of signalling that
> > it's releasing its reference, only sometimes (where "sometimes" means
> > every time blk_hide...() is called). blk_delete() will instead
> > automatically remove it from blk_backends, because it's assuming that
> > the last reference had been held by the monitor.
> 
> The reference held in trust for lookup by name exists as long as lookup
> by name is permitted.
> 
> Therefore, it goes away when the BB dies or when it loses its name.
> 
> The only way for a BB to lose its name is (was?) the messed up HMP
> drive_del: it wants to kill the BB right away, but can't, so it needs to
> hide it instead until it can.
> 
> New functionality may lead to a more complex live cycle where BBs may
> acquire and lose their name in new ways.
> 
> Note that I carefully avoid calling the reference the monitor's
> reference.  Because you made me realize it's not the monitor's alone!
> Lookup by name has more customers than just the monitor.

Legitimate ones or bugs to fix?

> > The second and third difference are what I referred to as "magic". You
> > could also call them "convenience", if you prefer, but in any case as we
> > can see by the existence blk_hide...(), removing the monitor reference
> > in blk_delete() appears to be wrong. Both should be separate operations,
> > and this is what this patch does.
> 
> I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del()
> shows that the name going away in blk_delete() is wrong.  It must go
> away there, because the thing it names goes away.

It must go away there _the latest_, but it can (should?) go away earlier.

> Additional operations to add / remove a BBs name may make sense; I'd
> have to see their users.  See "more complex live cycle" above.
> 
> > Assuming that any blk_new() is ultimately done by the monitor, we maybe
> > actually do not need an own monitor_add_blk() function; except that
> > Kevin stated that he does deem it useful when I proposed inlining it
> > (back) into blk_new().
> 
> As Kevin noted, that's not a good assumption.
> 
> > All in all, you have convinced me that the commit message is incorrect.
> > It should state that blk_backends is effectively repurposed to contain
> > the list of all BBs, and that a more explicit monitor_block_backends
> > list will take its place, with explicit operations for the monitor to
> > signal when it takes or releases the reference to a BB.
> 
> A member of monitor_block_backends is always a member of blk_backends.
> Correct?
> 
> Is a member of blk_backends with a name always a member of
> monitor_block_backends?

I would have expected that yes, but apparently you found other places
that use the name, so it might not be as clear as I thought it was.

Kevin

Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4479 bytes --]

On 24.02.2016 10:28, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 23.02.2016 10:48, Markus Armbruster wrote:

[...]

>>> Can you explain the *actual* difference between blk_backends and
>>> monitor_block_backends?  "Actual" in the sense of difference in contents
>>> over time.
>>
>> First difference: The name. That's enough reason for me.
>>
>> Second difference: blk_new() adds any BB to blk_backends automatically.
>> It doesn't do so for monitor_block_backends.
>>
>> Third difference: Often the monitor doesn't take care of signalling that
>> it's releasing its reference, only sometimes (where "sometimes" means
>> every time blk_hide...() is called). blk_delete() will instead
>> automatically remove it from blk_backends, because it's assuming that
>> the last reference had been held by the monitor.
> 
> The reference held in trust for lookup by name exists as long as lookup
> by name is permitted.
> 
> Therefore, it goes away when the BB dies or when it loses its name.

And I'd like it to be more explicit than this. A BB should simply never
have a name anymore when it's deleted. The current life cycle has a
short time where a named BB can have a refcount of 0.

While I know that this time span is considered to be "atomic", it still
seems off.

> The only way for a BB to lose its name is (was?) the messed up HMP
> drive_del: it wants to kill the BB right away, but can't, so it needs to
> hide it instead until it can.
> 
> New functionality may lead to a more complex live cycle where BBs may
> acquire and lose their name in new ways.
> 
> Note that I carefully avoid calling the reference the monitor's
> reference.  Because you made me realize it's not the monitor's alone!
> Lookup by name has more customers than just the monitor.

Maybe, but I'd personally consider this a service offered by the
monitor, and thus a reference by the monitor.

>> The second and third difference are what I referred to as "magic". You
>> could also call them "convenience", if you prefer, but in any case as we
>> can see by the existence blk_hide...(), removing the monitor reference
>> in blk_delete() appears to be wrong. Both should be separate operations,
>> and this is what this patch does.
> 
> I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del()
> shows that the name going away in blk_delete() is wrong.  It must go
> away there, because the thing it names goes away.

Yes, it must, but I believe it shouldn't have a name at that point at all.

> Additional operations to add / remove a BBs name may make sense; I'd
> have to see their users.  See "more complex live cycle" above.

They do make sense, if for nothing else, then because blk_hide...() is
replaced by a function that actually does something that makes sense in
the general life cycle.

>> Assuming that any blk_new() is ultimately done by the monitor, we maybe
>> actually do not need an own monitor_add_blk() function; except that
>> Kevin stated that he does deem it useful when I proposed inlining it
>> (back) into blk_new().
> 
> As Kevin noted, that's not a good assumption.

Which is a reason why we should have said explicit functions.

>> All in all, you have convinced me that the commit message is incorrect.
>> It should state that blk_backends is effectively repurposed to contain
>> the list of all BBs, and that a more explicit monitor_block_backends
>> list will take its place, with explicit operations for the monitor to
>> signal when it takes or releases the reference to a BB.
> 
> A member of monitor_block_backends is always a member of blk_backends.
> Correct?

I sure hope so.

> Is a member of blk_backends with a name always a member of
> monitor_block_backends?

No. Right now, after blk_new() it isn't; but Kevin proposed moving the
name setting to monitor_add_blk(), then it will be.

Apart from that, a BB that's passed to blk_delete() is currently
generally still a member of blk_backends (with a name). As said above, I
don't think it should be, and consequently it will not be a member of
monitor_block_backends.

So I guess you could say that I believe that being a member of
blk_backends hand having a name before this patch should be equivalent
to being a member of monitor_block_backends after this patch. It isn't,
because blk_backends contained some BBs which shouldn't be there, in my
opinion.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v3 08/14] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()

[Max Reitz]

This function first removed the BlockBackend from the blk_backends list
and cleared its name so it would no longer be found by blk_name(); since
blk_next() now iterates through monitor_block_backends (which the BB is
removed from in hmp_drive_del()), this is no longer necessary.

Second, bdrv_make_anon() was called on the BDS. This was intended for
cases where the BDS was owned by that BB alone; in which case the BDS
will no longer exist at this point thanks to the blk_remove_bs() in
hmp_drive_del().

Therefore, this function does nothing useful anymore. Remove it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 25 ++-----------------------
 blockdev.c                     |  7 ++-----
 include/sysemu/block-backend.h |  2 --
 3 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 430d7d5..a10fe44 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -69,7 +69,7 @@ static const AIOCBInfo block_backend_aiocb_info = {
 
 static void drive_info_del(DriveInfo *dinfo);
 
-/* All the BlockBackends (except for hidden ones) */
+/* All the BlockBackends */
 static QTAILQ_HEAD(, BlockBackend) blk_backends =
     QTAILQ_HEAD_INITIALIZER(blk_backends);
 
@@ -181,10 +181,7 @@ static void blk_delete(BlockBackend *blk)
         g_free(blk->root_state.throttle_group);
         throttle_group_unref(blk->root_state.throttle_state);
     }
-    /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
-    if (blk->name[0]) {
-        QTAILQ_REMOVE(&blk_backends, blk, link);
-    }
+    QTAILQ_REMOVE(&blk_backends, blk, link);
     g_free(blk->name);
     drive_info_del(blk->legacy_dinfo);
     block_acct_cleanup(&blk->stats);
@@ -400,24 +397,6 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 }
 
 /*
- * Hide @blk.
- * @blk must not have been hidden already.
- * Make attached BlockDriverState, if any, anonymous.
- * Once hidden, @blk is invisible to all functions that don't receive
- * it as argument.  For example, blk_by_name() won't return it.
- * Strictly for use by do_drive_del().
- * TODO get rid of it!
- */
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
-{
-    QTAILQ_REMOVE(&blk_backends, blk, link);
-    blk->name[0] = 0;
-    if (blk->bs) {
-        bdrv_make_anon(blk->bs);
-    }
-}
-
-/*
  * Disassociates the currently associated BlockDriverState from @blk.
  */
 void blk_remove_bs(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index ba1f648..6b929a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2818,13 +2818,10 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
     monitor_remove_blk(blk);
 
-    /* if we have a device attached to this BlockDriverState
-     * then we need to make the drive anonymous until the device
-     * can be removed.  If this is a drive with no device backing
-     * then we can just get rid of the block driver state right here.
+    /* If this BlockBackend has a device attached to it, its refcount will be
+     * decremented when the device is removed; otherwise we have to do so here.
      */
     if (blk_get_attached_dev(blk)) {
-        blk_hide_on_behalf_of_hmp_drive_del(blk);
         /* Further I/O must not pause the guest */
         blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
                          BLOCKDEV_ON_ERROR_REPORT);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 078690a..3a47982 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -80,8 +80,6 @@ BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
-
 void blk_iostatus_enable(BlockBackend *blk);
 bool blk_iostatus_is_enabled(const BlockBackend *blk);
 BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 08/14] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> This function first removed the BlockBackend from the blk_backends list
> and cleared its name so it would no longer be found by blk_name(); since
> blk_next() now iterates through monitor_block_backends (which the BB is
> removed from in hmp_drive_del()), this is no longer necessary.

Not clearing the name any more means that you can't create a new
BlockBackend with the same ID until the hidden BB finally goes away.

As it happens, you already can't do that with drive_add, because we also
keep the ID reserved in the QemuOpts, so it's already broken today.
However, before this patch, you can still use blockdev-add to create a
new BB with the ID of a BB that drive_del removed; after it, you get an
error:

{"error": {"class": "GenericError", "desc": "Device with id 'disk'
already exists"}}

In order to fix this, I suggested on IRC that we view the BB name as
belonging to the monitor reference logically, i.e. we add a name with
monitor_add_blk() (which would get a new ID parameter) and clear it
again in monitor_remove_blk().

> Second, bdrv_make_anon() was called on the BDS. This was intended for
> cases where the BDS was owned by that BB alone; in which case the BDS
> will no longer exist at this point thanks to the blk_remove_bs() in
> hmp_drive_del().
> 
> Therefore, this function does nothing useful anymore. Remove it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Kevin

Re: [Qemu-devel] [PATCH v3 08/14] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On 17.02.2016 15:18, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> This function first removed the BlockBackend from the blk_backends list
>> and cleared its name so it would no longer be found by blk_name(); since
>> blk_next() now iterates through monitor_block_backends (which the BB is
>> removed from in hmp_drive_del()), this is no longer necessary.
> 
> Not clearing the name any more means that you can't create a new
> BlockBackend with the same ID until the hidden BB finally goes away.
> 
> As it happens, you already can't do that with drive_add, because we also
> keep the ID reserved in the QemuOpts, so it's already broken today.
> However, before this patch, you can still use blockdev-add to create a
> new BB with the ID of a BB that drive_del removed; after it, you get an
> error:
> 
> {"error": {"class": "GenericError", "desc": "Device with id 'disk'
> already exists"}}
> 
> In order to fix this, I suggested on IRC that we view the BB name as
> belonging to the monitor reference logically, i.e. we add a name with
> monitor_add_blk() (which would get a new ID parameter) and clear it
> again in monitor_remove_blk().

Moving the @name parameter from blk_new() to monitor_add_blk() seems
reasonable but would necessitate pulling in two patches from the
follow-up series to this (there are places which try to emit the BB's
name while its BDS is created, which these two patches change so they no
longer do this).

Alternatively, as I said, we might want to call monitor_add_blk() in
blk_new() if the name is non-NULL (and allow NULL names).

Max

>> Second, bdrv_make_anon() was called on the BDS. This was intended for
>> cases where the BDS was owned by that BB alone; in which case the BDS
>> will no longer exist at this point thanks to the blk_remove_bs() in
>> hmp_drive_del().
>>
>> Therefore, this function does nothing useful anymore. Remove it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 08/14] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

Am 17.02.2016 um 16:47 hat Max Reitz geschrieben:
> On 17.02.2016 15:18, Kevin Wolf wrote:
> > Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> >> This function first removed the BlockBackend from the blk_backends list
> >> and cleared its name so it would no longer be found by blk_name(); since
> >> blk_next() now iterates through monitor_block_backends (which the BB is
> >> removed from in hmp_drive_del()), this is no longer necessary.
> > 
> > Not clearing the name any more means that you can't create a new
> > BlockBackend with the same ID until the hidden BB finally goes away.
> > 
> > As it happens, you already can't do that with drive_add, because we also
> > keep the ID reserved in the QemuOpts, so it's already broken today.
> > However, before this patch, you can still use blockdev-add to create a
> > new BB with the ID of a BB that drive_del removed; after it, you get an
> > error:
> > 
> > {"error": {"class": "GenericError", "desc": "Device with id 'disk'
> > already exists"}}
> > 
> > In order to fix this, I suggested on IRC that we view the BB name as
> > belonging to the monitor reference logically, i.e. we add a name with
> > monitor_add_blk() (which would get a new ID parameter) and clear it
> > again in monitor_remove_blk().
> 
> Moving the @name parameter from blk_new() to monitor_add_blk() seems
> reasonable but would necessitate pulling in two patches from the
> follow-up series to this (there are places which try to emit the BB's
> name while its BDS is created, which these two patches change so they no
> longer do this).
> 
> Alternatively, as I said, we might want to call monitor_add_blk() in
> blk_new() if the name is non-NULL (and allow NULL names).

But if you allow NULL names, you need to fix those places, too. If the
two patches aren't too bad, let's include them here.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB

[Max Reitz]

Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                                       | 20 ------------
 block/block-backend.c                         | 44 +++++++++++++++++++++++----
 block/io.c                                    | 20 ------------
 include/block/block.h                         |  2 --
 stubs/Makefile.objs                           |  2 +-
 stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
 6 files changed, 41 insertions(+), 51 deletions(-)
 rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)

diff --git a/block.c b/block.c
index a119840..5eac9a3 100644
--- a/block.c
+++ b/block.c
@@ -2531,26 +2531,6 @@ ro_cleanup:
     return ret;
 }
 
-int bdrv_commit_all(void)
-{
-    BlockDriverState *bs;
-
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        if (bs->drv && bs->backing) {
-            int ret = bdrv_commit(bs);
-            if (ret < 0) {
-                aio_context_release(aio_context);
-                return ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-    return 0;
-}
-
 /*
  * Return values:
  * 0        - success
diff --git a/block/block-backend.c b/block/block-backend.c
index a10fe44..a918c35 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -908,11 +908,6 @@ int blk_flush(BlockBackend *blk)
     return bdrv_flush(blk->bs);
 }
 
-int blk_flush_all(void)
-{
-    return bdrv_flush_all();
-}
-
 void blk_drain(BlockBackend *blk)
 {
     if (blk->bs) {
@@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 
 int blk_commit_all(void)
 {
-    return bdrv_commit_all();
+    BlockBackend *blk = NULL;
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {
+            int ret = bdrv_commit(blk->bs);
+            if (ret < 0) {
+                aio_context_release(aio_context);
+                return ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+    return 0;
+}
+
+int blk_flush_all(void)
+{
+    BlockBackend *blk = NULL;
+    int result = 0;
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+        int ret;
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk)) {
+            ret = blk_flush(blk);
+            if (ret < 0 && !result) {
+                result = ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+
+    return result;
 }
diff --git a/block/io.c b/block/io.c
index a69bfc4..5f9b6d6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1445,26 +1445,6 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                              BDRV_REQ_ZERO_WRITE | flags);
 }
 
-int bdrv_flush_all(void)
-{
-    BlockDriverState *bs = NULL;
-    int result = 0;
-
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-        int ret;
-
-        aio_context_acquire(aio_context);
-        ret = bdrv_flush(bs);
-        if (ret < 0 && !result) {
-            result = ret;
-        }
-        aio_context_release(aio_context);
-    }
-
-    return result;
-}
-
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index 1c4f4d8..db9e0f5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -273,7 +273,6 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
-int bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
@@ -374,7 +373,6 @@ int bdrv_inactivate_all(void);
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
-int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void bdrv_drain_all(void);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index e922de9..9d9f1d0 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
-stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
diff --git a/stubs/bdrv-commit-all.c b/stubs/blk-commit-all.c
similarity index 53%
rename from stubs/bdrv-commit-all.c
rename to stubs/blk-commit-all.c
index bf84a1d..c82fb7f 100644
--- a/stubs/bdrv-commit-all.c
+++ b/stubs/blk-commit-all.c
@@ -1,8 +1,8 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
-#include "block/block.h"
+#include "sysemu/block-backend.h"
 
-int bdrv_commit_all(void)
+int blk_commit_all(void)
 {
     return 0;
 }
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                                       | 20 ------------
>  block/block-backend.c                         | 44 +++++++++++++++++++++++----
>  block/io.c                                    | 20 ------------
>  include/block/block.h                         |  2 --
>  stubs/Makefile.objs                           |  2 +-
>  stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
>  6 files changed, 41 insertions(+), 51 deletions(-)
>  rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)
> 
> diff --git a/block.c b/block.c
> index a119840..5eac9a3 100644
> --- a/block.c
> +++ b/block.c
> @@ -2531,26 +2531,6 @@ ro_cleanup:
>      return ret;
>  }
>  
> -int bdrv_commit_all(void)
> -{
> -    BlockDriverState *bs;
> -
> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -        AioContext *aio_context = bdrv_get_aio_context(bs);
> -
> -        aio_context_acquire(aio_context);
> -        if (bs->drv && bs->backing) {
> -            int ret = bdrv_commit(bs);
> -            if (ret < 0) {
> -                aio_context_release(aio_context);
> -                return ret;
> -            }
> -        }
> -        aio_context_release(aio_context);
> -    }
> -    return 0;
> -}
> -
>  /*
>   * Return values:
>   * 0        - success
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a10fe44..a918c35 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -908,11 +908,6 @@ int blk_flush(BlockBackend *blk)
>      return bdrv_flush(blk->bs);
>  }
>  
> -int blk_flush_all(void)
> -{
> -    return bdrv_flush_all();
> -}
> -
>  void blk_drain(BlockBackend *blk)
>  {
>      if (blk->bs) {
> @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
>  
>  int blk_commit_all(void)
>  {
> -    return bdrv_commit_all();
> +    BlockBackend *blk = NULL;
> +
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        AioContext *aio_context = blk_get_aio_context(blk);
> +
> +        aio_context_acquire(aio_context);
> +        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {

blk->bs->drv is already implied by blk_is_available(), so it's
unnecessary, even though it doesn't actively hurt.

Also, using blk_is_available() instead of blk_is_inserted() as in
blk_flush_all() means that a drive with an open tray, but inserted
medium isn't committed. I assume you did this on purpose because you're
using two different functions in this patch, but isn't this a change in
behaviour? If so, and we really want it, it should at least be mentioned
in the commit message.

> +            int ret = bdrv_commit(blk->bs);
> +            if (ret < 0) {
> +                aio_context_release(aio_context);
> +                return ret;
> +            }
> +        }
> +        aio_context_release(aio_context);
> +    }
> +    return 0;
> +}
> +
> +int blk_flush_all(void)
> +{
> +    BlockBackend *blk = NULL;
> +    int result = 0;
> +
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        AioContext *aio_context = blk_get_aio_context(blk);
> +        int ret;
> +
> +        aio_context_acquire(aio_context);
> +        if (blk_is_inserted(blk)) {
> +            ret = blk_flush(blk);
> +            if (ret < 0 && !result) {
> +                result = ret;
> +            }
> +        }
> +        aio_context_release(aio_context);
> +    }
> +
> +    return result;
>  }

Kevin

Re: [Qemu-devel] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On 17.02.2016 16:51, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                                       | 20 ------------
>>  block/block-backend.c                         | 44 +++++++++++++++++++++++----
>>  block/io.c                                    | 20 ------------
>>  include/block/block.h                         |  2 --
>>  stubs/Makefile.objs                           |  2 +-
>>  stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
>>  6 files changed, 41 insertions(+), 51 deletions(-)
>>  rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)
>>

[...]

>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index a10fe44..a918c35 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c

[...]

>> @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
>>  
>>  int blk_commit_all(void)
>>  {
>> -    return bdrv_commit_all();
>> +    BlockBackend *blk = NULL;
>> +
>> +    while ((blk = blk_all_next(blk)) != NULL) {
>> +        AioContext *aio_context = blk_get_aio_context(blk);
>> +
>> +        aio_context_acquire(aio_context);
>> +        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {
> 
> blk->bs->drv is already implied by blk_is_available(), so it's
> unnecessary, even though it doesn't actively hurt.
> 
> Also, using blk_is_available() instead of blk_is_inserted() as in
> blk_flush_all() means that a drive with an open tray, but inserted
> medium isn't committed. I assume you did this on purpose because you're
> using two different functions in this patch, but isn't this a change in
> behaviour? If so, and we really want it, it should at least be mentioned
> in the commit message.

Drives with open trays are strange.

I found it reasonable that every medium in the system should be flushed
on blk_flush_all(), because flushing data should only bring the on-disk
state to a state it is supposed to be in anyway.

On the other hand, the commit operation actively changes data.
Therefore, I decided to treat it like a guest write operation. However,
considering that blk_commit_all() is basically only available through
HMP and is thus more of a legacy operation, I don't think its behavior
should be changed here. I'm therefore going to change this to
blk_is_inserted(), thanks.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v3 10/14] block: Add bdrv_next_monitor_owned()

[Max Reitz]

Add a function for iterating over all monitor-owned BlockDriverStates so
the generic block layer can do so.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c                      | 7 +++++++
 include/block/block.h           | 1 +
 stubs/Makefile.objs             | 1 +
 stubs/bdrv-next-monitor-owned.c | 6 ++++++
 4 files changed, 15 insertions(+)
 create mode 100644 stubs/bdrv-next-monitor-owned.c

diff --git a/blockdev.c b/blockdev.c
index 6b929a9..6370490 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -722,6 +722,13 @@ void blockdev_close_all_bdrv_states(void)
     }
 }
 
+/* Iterates over the list of monitor-owned BlockDriverStates */
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs)
+{
+    return bs ? QTAILQ_NEXT(bs, monitor_list)
+              : QTAILQ_FIRST(&monitor_bdrv_states);
+}
+
 static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
                             Error **errp)
 {
diff --git a/include/block/block.h b/include/block/block.h
index db9e0f5..bd27e6c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -413,6 +413,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
 bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next_node(BlockDriverState *bs);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
 int bdrv_set_key(BlockDriverState *bs, const char *key);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9d9f1d0..b6d1e65 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,4 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
+stub-obj-y += bdrv-next-monitor-owned.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
diff --git a/stubs/bdrv-next-monitor-owned.c b/stubs/bdrv-next-monitor-owned.c
new file mode 100644
index 0000000..09028db
--- /dev/null
+++ b/stubs/bdrv-next-monitor-owned.c
@@ -0,0 +1,6 @@
+#include "block/block.h"
+
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs)
+{
+    return NULL;
+}
-- 
2.7.1

[Qemu-devel] [PATCH v3 11/14] block: Add blk_next_root_bs()

[Max Reitz]

This function iterates over all BDSs attached to a BB. We are going to
need it when rewriting bdrv_next() so it no longer uses bdrv_states.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 24 ++++++++++++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index a918c35..d1621ec 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -269,6 +269,30 @@ BlockBackend *blk_next(BlockBackend *blk)
 }
 
 /*
+ * Iterates over all BlockDriverStates which are attached to a BlockBackend.
+ * This function is for use by bdrv_next().
+ *
+ * @bs must be NULL or a BDS that is attached to a BB.
+ */
+BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
+{
+    BlockBackend *blk;
+
+    if (bs) {
+        assert(bs->blk);
+        blk = bs->blk;
+    } else {
+        blk = NULL;
+    }
+
+    do {
+        blk = blk_all_next(blk);
+    } while (blk && !blk->bs);
+
+    return blk ? blk->bs : NULL;
+}
+
+/*
  * Add a BlockBackend into the list of backends referenced by the monitor.
  * Strictly for use by blockdev.c.
  */
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3a47982..5ac9bd2 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -73,6 +73,7 @@ const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
+BlockDriverState *blk_next_root_bs(BlockDriverState *bs);
 void monitor_add_blk(BlockBackend *blk);
 void monitor_remove_blk(BlockBackend *blk);
 
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 11/14] block: Add blk_next_root_bs()

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> This function iterates over all BDSs attached to a BB. We are going to
> need it when rewriting bdrv_next() so it no longer uses bdrv_states.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/block-backend.c          | 24 ++++++++++++++++++++++++
>  include/sysemu/block-backend.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a918c35..d1621ec 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -269,6 +269,30 @@ BlockBackend *blk_next(BlockBackend *blk)
>  }
>  
>  /*
> + * Iterates over all BlockDriverStates which are attached to a BlockBackend.
> + * This function is for use by bdrv_next().
> + *
> + * @bs must be NULL or a BDS that is attached to a BB.
> + */
> +BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
> +{
> +    BlockBackend *blk;
> +
> +    if (bs) {
> +        assert(bs->blk);
> +        blk = bs->blk;

I'm trying to remove bs->blk, and you're adding new instances. :-/

This isn't going to be an easy one to remove. But of course, it wouldn't
be easy for you to avoid it either, so unless you have a good idea,
let's apply this and we'll figure out how to get rid of it again.

> +    } else {
> +        blk = NULL;
> +    }
> +
> +    do {
> +        blk = blk_all_next(blk);
> +    } while (blk && !blk->bs);
> +
> +    return blk ? blk->bs : NULL;
> +}

Kevin

[Qemu-devel] [PATCH v3 12/14] block: Rewrite bdrv_next()

[Max Reitz]

Instead of using the bdrv_states list, iterate over all the
BlockDriverStates attached to BlockBackends, and over all the
monitor-owned BDSs afterwards (except for those attached to a BB).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 5eac9a3..f3597ac 100644
--- a/block.c
+++ b/block.c
@@ -2979,12 +2979,23 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
     return QTAILQ_NEXT(bs, node_list);
 }
 
+/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
+ * the monitor or attached to a BlockBackend */
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
-    if (!bs) {
-        return QTAILQ_FIRST(&bdrv_states);
+    if (!bs || bs->blk) {
+        bs = blk_next_root_bs(bs);
+        if (bs) {
+            return bs;
+        }
     }
-    return QTAILQ_NEXT(bs, device_list);
+
+    /* Ignore all BDSs that are attached to a BlockBackend here; they have been
+     * handled by the above block already */
+    do {
+        bs = bdrv_next_monitor_owned(bs);
+    } while (bs && bs->blk);
+    return bs;
 }
 
 const char *bdrv_get_node_name(const BlockDriverState *bs)
-- 
2.7.1

[Qemu-devel] [PATCH v3 13/14] block: Use bdrv_next() instead of bdrv_states

[Max Reitz]

There is no point in manually iterating through the bdrv_states list
when there is bdrv_next().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index f3597ac..69fc696 100644
--- a/block.c
+++ b/block.c
@@ -3303,10 +3303,10 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 
 void bdrv_invalidate_cache_all(Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     Error *local_err = NULL;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
@@ -3336,10 +3336,10 @@ static int bdrv_inactivate(BlockDriverState *bs)
 
 int bdrv_inactivate_all(void)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     int ret;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
@@ -4185,10 +4185,10 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
  */
 bool bdrv_is_first_non_filter(BlockDriverState *candidate)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
 
     /* walk down the bs forest recursively */
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         bool perm;
 
         /* try to recurse in this top level bs */
-- 
2.7.1

[Qemu-devel] [PATCH v3 14/14] block: Remove bdrv_states list

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 31 +++----------------------------
 blockdev.c                |  7 -------
 include/block/block.h     |  1 -
 include/block/block_int.h |  4 ----
 4 files changed, 3 insertions(+), 40 deletions(-)

diff --git a/block.c b/block.c
index 69fc696..95a7b61 100644
--- a/block.c
+++ b/block.c
@@ -72,8 +72,6 @@ struct BdrvDirtyBitmap {
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
-
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
@@ -246,10 +244,7 @@ void bdrv_register(BlockDriver *bdrv)
 
 BlockDriverState *bdrv_new_root(void)
 {
-    BlockDriverState *bs = bdrv_new();
-
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
-    return bs;
+    return bdrv_new();
 }
 
 BlockDriverState *bdrv_new(void)
@@ -2240,26 +2235,10 @@ void bdrv_close_all(void)
     }
 }
 
-/* Note that bs->device_list.tqe_prev is initially null,
- * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
- * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
- * resetting it to null on remove.  */
-void bdrv_device_remove(BlockDriverState *bs)
-{
-    QTAILQ_REMOVE(&bdrv_states, bs, device_list);
-    bs->device_list.tqe_prev = NULL;
-}
-
-/* make a BlockDriverState anonymous by removing from bdrv_state and
- * graph_bdrv_state list.
-   Also, NULL terminate the device_name to prevent double remove */
+/* make a BlockDriverState anonymous by removing from graph_bdrv_state list.
+ * Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
 {
-    /* Take care to remove bs from bdrv_states only when it's actually
-     * in it. */
-    if (bs->device_list.tqe_prev) {
-        bdrv_device_remove(bs);
-    }
     if (bs->node_name[0] != '\0') {
         QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
     }
@@ -2296,10 +2275,6 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
-            QTAILQ_INSERT_BEFORE(from, to, device_list);
-        }
-        bdrv_device_remove(from);
     }
 }
 
diff --git a/blockdev.c b/blockdev.c
index 6370490..b8ebdda 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2417,11 +2417,6 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
         goto out;
     }
 
-    /* This follows the convention established by bdrv_make_anon() */
-    if (bs->device_list.tqe_prev) {
-        bdrv_device_remove(bs);
-    }
-
     blk_remove_bs(blk);
 
     if (!blk_dev_has_tray(blk)) {
@@ -2469,8 +2464,6 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
 
     blk_insert_bs(blk, bs);
 
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
-
     if (!blk_dev_has_tray(blk)) {
         /* For tray-less devices, blockdev-close-tray is a no-op (or may not be
          * called at all); therefore, the medium needs to be pushed into the
diff --git a/include/block/block.h b/include/block/block.h
index bd27e6c..6a9cfc6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -199,7 +199,6 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 BlockDriverState *bdrv_new_root(void);
 BlockDriverState *bdrv_new(void);
-void bdrv_device_remove(BlockDriverState *bs);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_replace_in_backing_chain(BlockDriverState *old,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ef823a..fdcacab 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -442,8 +442,6 @@ struct BlockDriverState {
     char node_name[32];
     /* element of the list of named nodes building the graph */
     QTAILQ_ENTRY(BlockDriverState) node_list;
-    /* element of the list of "drives" the guest sees */
-    QTAILQ_ENTRY(BlockDriverState) device_list;
     /* element of the list of all BlockDriverStates (all_bdrv_states) */
     QTAILQ_ENTRY(BlockDriverState) bs_list;
     /* element of the list of monitor-owned BDS */
@@ -501,8 +499,6 @@ extern BlockDriver bdrv_file;
 extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
-extern QTAILQ_HEAD(BdrvStates, BlockDriverState) bdrv_states;
-
 /**
  * bdrv_setup_io_funcs:
  *
-- 
2.7.1

Re: [Qemu-devel] [PATCH v3 00/14] blockdev: Further BlockBackend work

[Kevin Wolf]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> At this point, the main merit of this series is to remove
> blk_hide_on_behalf_of_hmp_drive_del() and the bdrv_states list. A
> follow-up to it will make bdrv_open() return a BDS pointer.

The patches that I didn't comment on (i.e. 2-3, 5-6, 10, 12-14) are:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>