[Qemu-devel] [PATCH 0/2] Handle interferences between multiple children drivers and stream and commit

[Qemu-devel] [PATCH 0/2] Handle interferences between multiple children drivers and stream and commit

[Benoît Canet]

We do not want to try to stream or commit with a base argument through
a multiple children driver.

Handle this case.

Benoît Canet (2):
  block: Introduce a BlockDriver field to flag drivers supporting
    multiple children
  block: commit and stream return an error when a subtree is found

 block.c                   | 17 +++++++++++++++--
 block/blkverify.c         |  1 +
 block/quorum.c            |  1 +
 block/vmdk.c              |  1 +
 blockdev.c                | 18 ++++++++++++------
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  6 ++++++
 7 files changed, 38 insertions(+), 9 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] block: Introduce a BlockDriver field to flag drivers supporting multiple children

[Benoît Canet]

The recursive op blocker patch to come will take an optional base argument
which will not have any meaning when an intermediary BDS driver of the graph
support multiple children.

Flag such drivers to be able to handle this case.

CC: Jeff Cody <jcody@redhat.com>
Signed-off-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block/blkverify.c         | 1 +
 block/quorum.c            | 1 +
 block/vmdk.c              | 1 +
 include/block/block_int.h | 6 ++++++
 4 files changed, 9 insertions(+)

diff --git a/block/blkverify.c b/block/blkverify.c
index 163064c..13f8359 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -368,6 +368,7 @@ static BlockDriver bdrv_blkverify = {
     .bdrv_detach_aio_context          = blkverify_detach_aio_context,
 
     .is_filter                        = true,
+    .supports_multiple_children       = true,
     .bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
 };
 
diff --git a/block/quorum.c b/block/quorum.c
index 093382e..a4d11eb 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1087,6 +1087,7 @@ static BlockDriver bdrv_quorum = {
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
 
     .is_filter                          = true,
+    .supports_multiple_children         = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 };
 
diff --git a/block/vmdk.c b/block/vmdk.c
index a1cb911..933a57d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2244,6 +2244,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_attach_aio_context      = vmdk_attach_aio_context,
 
     .supports_backing             = true,
+    .supports_multiple_children   = true,
     .create_opts                  = &vmdk_create_opts,
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a61215..b653e48 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -104,6 +104,12 @@ struct BlockDriver {
     /* Set if a driver can support backing files */
     bool supports_backing;
 
+    /* Set to true if the BlockDriver supports multiple children.
+     * (quorum, blkverify, vmdk)
+     * This field being true will disable some block layer operations.
+     */
+    bool supports_multiple_children;
+
     /* For handling image reopen for split or non-split files */
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue, Error **errp);
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] block: commit and stream return an error when a subtree is found

[Benoît Canet]

In bdrv_find_backing_image when looking for the base BDS of a backing chain and
a BDS having a multiple children block driver is found return NULL and set an
error message.

This will hopefully remove any unwanted interference between stream, commit
and drivers like quorum.

CC: Jeff Cody <jcody@redhat.com>
Signed-off-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block.c               | 17 +++++++++++++++--
 blockdev.c            | 18 ++++++++++++------
 include/block/block.h |  3 ++-
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index d06dd51..ea86252 100644
--- a/block.c
+++ b/block.c
@@ -4339,9 +4339,13 @@ int bdrv_is_snapshot(BlockDriverState *bs)
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
- * the CWD rather than the chain. */
+ * the CWD rather than the chain.
+ * If a multiple children BDS driver is found down the backing chain path then
+ * return NULL and set an error message.
+ */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
-        const char *backing_file)
+                                          const char *backing_file,
+                                          Error **errp)
 {
     char *filename_full = NULL;
     char *backing_file_full = NULL;
@@ -4362,6 +4366,15 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 
     for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
 
+        /* If a BDS having a multiple children block driver is found then the
+         * function should fail.
+         */
+        if (curr_bs->drv && curr_bs->drv->supports_multiple_children) {
+            error_setg(errp, "multiple children block driver found down the "
+                             "backing chain");
+            break;
+        }
+
         /* If either of the filename paths is actually a protocol, then
          * compare unmodified paths; otherwise make paths relative */
         if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
diff --git a/blockdev.c b/blockdev.c
index e919566..4c5c28e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1894,9 +1894,11 @@ void qmp_block_stream(const char *device,
     }
 
     if (has_base) {
-        base_bs = bdrv_find_backing_image(bs, base);
+        base_bs = bdrv_find_backing_image(bs, base, errp);
         if (base_bs == NULL) {
-            error_set(errp, QERR_BASE_NOT_FOUND, base);
+            if (!*errp) {
+                error_set(errp, QERR_BASE_NOT_FOUND, base);
+            }
             return;
         }
         base_name = base;
@@ -1965,23 +1967,27 @@ void qmp_block_commit(const char *device,
 
     if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
-            top_bs = bdrv_find_backing_image(bs, top);
+            top_bs = bdrv_find_backing_image(bs, top, errp);
         }
     }
 
     if (top_bs == NULL) {
-        error_setg(errp, "Top image file %s not found", top ? top : "NULL");
+        if (!*errp) {
+            error_setg(errp, "Top image file %s not found", top ? top : "NULL");
+        }
         return;
     }
 
     if (has_base && base) {
-        base_bs = bdrv_find_backing_image(top_bs, base);
+        base_bs = bdrv_find_backing_image(top_bs, base, errp);
     } else {
         base_bs = bdrv_find_base(top_bs);
     }
 
     if (base_bs == NULL) {
-        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
+        if (!*errp) {
+            error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
+        }
         return;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index 8f4ad16..65cde65 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -272,7 +272,8 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
 int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors, BdrvRequestFlags flags);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
-    const char *backing_file);
+                                          const char *backing_file,
+                                          Error **errp);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] Handle interferences between multiple children drivers and stream and commit

[Jeff Cody]

On Mon, Sep 15, 2014 at 04:21:18PM +0200, Benoît Canet wrote:
> We do not want to try to stream or commit with a base argument through
> a multiple children driver.
> 
> Handle this case.
> 
> Benoît Canet (2):
>   block: Introduce a BlockDriver field to flag drivers supporting
>     multiple children
>   block: commit and stream return an error when a subtree is found
> 
>  block.c                   | 17 +++++++++++++++--
>  block/blkverify.c         |  1 +
>  block/quorum.c            |  1 +
>  block/vmdk.c              |  1 +
>  blockdev.c                | 18 ++++++++++++------
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  6 ++++++
>  7 files changed, 38 insertions(+), 9 deletions(-)
> 
> -- 
> 2.1.0
>


I had some specific comments on the implementation, but once I stepped
back some I am not sure this series is needed.

When we talked on irc, you mentioned it was needed because your
recursive blocker code would assert in quorum/blkverify/vmdk - but I
think that might mean the recursive code needs refactoring.

Here is why I am not sure this is needed: we should be able to commit
through/to an image that has multiple child nodes, if we can treat
that image as an actual BDS.

I view these as a black box, like so:

               (imageE)
             ------------
             | [childA] |
             | [childB] |
[base] <---- |          | <----- [imageF] <--- [active]
             | [childC] |
 ^           | [childD] |
 |           ------------
 |
 |
(Perhaps base isn't support by the format, in which case it will always
be NULL anyway)


ImageE may have multiple children (and perhaps each of those children
are an entire chain themselves), but ultimately imageE is a 'BDS'
entry.

Perhaps the format, like quorum, will not have a backing_hd pointer.
But in that case, backing_hd will be NULL.

Thinking about your recursive blocker code, why assert when the 'base'
termination argument is NULL?  If the multi-child format does not
support a backing_hd, then bdrv_find_base_image() will just find it as
the end.

The .bdrv_op_recursive_block() in that case could still navigate down
each private child node, because if the original 'base' blocker
applied to the multi-child parent (imageE), then that same blocker
should apply to each private child (and their children), regardless of
'base' - because they are essentially part of the pseudo-atomic entity
of (imageE).

Does this make sense, or am I missing a piece?

Thanks,
Jeff

Re: [Qemu-devel] [PATCH 0/2] Handle interferences between multiple children drivers and stream and commit

[Benoît Canet]

On Fri, Sep 19, 2014 at 12:35:36AM -0400, Jeff Cody wrote:

> On Mon, Sep 15, 2014 at 04:21:18PM +0200, Benoît Canet wrote:
> > We do not want to try to stream or commit with a base argument through
> > a multiple children driver.
> > 
> > Handle this case.
> > 
> > Benoît Canet (2):
> >   block: Introduce a BlockDriver field to flag drivers supporting
> >     multiple children
> >   block: commit and stream return an error when a subtree is found
> > 
> >  block.c                   | 17 +++++++++++++++--
> >  block/blkverify.c         |  1 +
> >  block/quorum.c            |  1 +
> >  block/vmdk.c              |  1 +
> >  blockdev.c                | 18 ++++++++++++------
> >  include/block/block.h     |  3 ++-
> >  include/block/block_int.h |  6 ++++++
> >  7 files changed, 38 insertions(+), 9 deletions(-)
> > 
> > -- 
> > 2.1.0
> >
> 
> 
> I had some specific comments on the implementation, but once I stepped
> back some I am not sure this series is needed.
> 
> When we talked on irc, you mentioned it was needed because your
> recursive blocker code would assert in quorum/blkverify/vmdk - but I
> think that might mean the recursive code needs refactoring.
> 
> Here is why I am not sure this is needed: we should be able to commit
> through/to an image that has multiple child nodes, if we can treat
> that image as an actual BDS.
> 
> I view these as a black box, like so:
> 
>                (imageE)
>              ------------
>              | [childA] |
>              | [childB] |
> [base] <---- |          | <----- [imageF] <--- [active]
>              | [childC] |
>  ^           | [childD] |
>  |           ------------
>  |
>  |
> (Perhaps base isn't support by the format, in which case it will always
> be NULL anyway)
> 
> 
> ImageE may have multiple children (and perhaps each of those children
> are an entire chain themselves), but ultimately imageE is a 'BDS'
> entry.
> 
> Perhaps the format, like quorum, will not have a backing_hd pointer.
> But in that case, backing_hd will be NULL.
> 
> Thinking about your recursive blocker code, why assert when the 'base'
> termination argument is NULL?  If the multi-child format does not
> support a backing_hd, then bdrv_find_base_image() will just find it as
> the end.
> 
> The .bdrv_op_recursive_block() in that case could still navigate down
> each private child node, because if the original 'base' blocker
> applied to the multi-child parent (imageE), then that same blocker
> should apply to each private child (and their children), regardless of
> 'base' - because they are essentially part of the pseudo-atomic entity
> of (imageE).
> 
> Does this make sense, or am I missing a piece?

Thanks for the reply Jeff.

I think I'll drop the entry I CCed you, remove the assert then work to post
the new recursive blocker code.

Best regards

Benoît

> 
> Thanks,
> Jeff
>