[v2,03/21] mirror: Error out when a BDS would get two BBs

bdrv_replace_in_backing_chain() asserts that not both old and new
BlockDdriverState have a BlockBackend attached to them because both
would have to end up pointing to the new BDS and we don't support more
than one BB per BDS yet.

Before we can safely allow references to existing nodes as backing
files, we need to make sure that even if a backing file has a BB on it,
this doesn't crash qemu.

There are probably also some cases with the 'replaces' option set where
drive-mirror could fail this assertion today. They are fixed with this
error check as well.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

On 23.11.2015 16:59, Kevin Wolf wrote:
> bdrv_replace_in_backing_chain() asserts that not both old and new
> BlockDdriverState have a BlockBackend attached to them because both
> would have to end up pointing to the new BDS and we don't support more
> than one BB per BDS yet.
> 
> Before we can safely allow references to existing nodes as backing
> files, we need to make sure that even if a backing file has a BB on it,
> this doesn't crash qemu.
> 
> There are probably also some cases with the 'replaces' option set where
> drive-mirror could fail this assertion today. They are fixed with this
> error check as well.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 52c9abf..0620068 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -18,6 +18,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/error-report.h"
>  
>  #define SLICE_TIME    100000000ULL /* ns */
>  #define MAX_IN_FLIGHT 16
> @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
>          if (s->to_replace) {
>              to_replace = s->to_replace;
>          }
> +
> +        /* This was checked in mirror_start_job(), but meanwhile one of the
> +         * nodes could have been newly attached to a BlockBackend. */
> +        if (to_replace->blk && s->target->blk) {
> +            error_report("block job: Can't create node with two BlockBackends");
> +            data->ret = -EINVAL;
> +            goto out;
> +        }
> +
>          if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
>              bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
>          }
>          bdrv_replace_in_backing_chain(to_replace, s->target);
>      }
> +
> +out:
>      if (s->to_replace) {
>          bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
>          error_free(s->replace_blocker);
> @@ -701,6 +713,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>                               bool is_none_mode, BlockDriverState *base)
>  {
>      MirrorBlockJob *s;
> +    BlockDriverState *replaced_bs;
>  
>      if (granularity == 0) {
>          granularity = bdrv_get_default_bitmap_granularity(target);
> @@ -724,6 +737,21 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>          buf_size = DEFAULT_MIRROR_BUF_SIZE;
>      }
>  
> +    /* We can't support this case as long as the block layer can't handle
> +     * multple BlockBackends per BlockDriverState. */

*multiple

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +    if (replaces) {
> +        replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
> +        if (replaced_bs == NULL) {
> +            return;
> +        }
> +    } else {
> +        replaced_bs = bs;
> +    }
> +    if (replaced_bs->blk && target->blk) {
> +        error_setg(errp, "Can't create node with two BlockBackends");
> +        return;
> +    }
> +
>      s = block_job_create(driver, bs, speed, cb, opaque, errp);
>      if (!s) {
>          return;
>

On Mon 23 Nov 2015 04:59:42 PM CET, Kevin Wolf wrote:

> @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
>          if (s->to_replace) {
>              to_replace = s->to_replace;
>          }
> +
> +        /* This was checked in mirror_start_job(), but meanwhile one of the
> +         * nodes could have been newly attached to a BlockBackend. */
> +        if (to_replace->blk && s->target->blk) {
> +            error_report("block job: Can't create node with two BlockBackends");
> +            data->ret = -EINVAL;
> +            goto out;
> +        }

Does it make sense to even allow attaching a BDS to a Block Backend
during this block job? Is there any use case for that?

Berto

Am 30.11.2015 um 15:51 hat Alberto Garcia geschrieben:
> On Mon 23 Nov 2015 04:59:42 PM CET, Kevin Wolf wrote:
> 
> > @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
> >          if (s->to_replace) {
> >              to_replace = s->to_replace;
> >          }
> > +
> > +        /* This was checked in mirror_start_job(), but meanwhile one of the
> > +         * nodes could have been newly attached to a BlockBackend. */
> > +        if (to_replace->blk && s->target->blk) {
> > +            error_report("block job: Can't create node with two BlockBackends");
> > +            data->ret = -EINVAL;
> > +            goto out;
> > +        }
> 
> Does it make sense to even allow attaching a BDS to a Block Backend
> during this block job? Is there any use case for that?

Well, is there a good reason for forbidding it? I can imagine that
someone could have good reasons to start an NBD server on any node,
including nodes that are going to be replaced at some point.

The only reason for not allowing this is that a BDS can only have a
single BB, which is a limitation of the implementation rather than a
fundamental problem.

Kevin