Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Kevin Wolf <kwolf@redhat.com>]

Am 26.11.2013 um 06:45 hat Fam Zheng geschrieben:
> If active is top, it will be mirrored to base, (with block/mirror.c
> code), then the image is switched when user completes the block job.
> 
> QMP documentation is updated.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c   | 11 +++++++++++
>  blockdev.c       |  9 +++++++--
>  qapi-schema.json |  5 +++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 9be741a..86ffac8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -478,6 +478,13 @@ immediate_exit:
>              bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
>          }
>          bdrv_swap(s->target, s->common.bs);
> +        if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
> +            /* drop the bs loop chain formed by the swap: break the loop then
> +             * trigger the unref from the top one */
> +            BlockDriverState *p = s->base->backing_hd;
> +            s->base->backing_hd = NULL;
> +            bdrv_unref(p);

I'm not sure about the refcounts...

Let's assume we have the following backing file chain, refcount in
brackets:

                             +------ NBD server
                             v
base (1) <-- sn1 (1) <-- sn2 (3) <-- guest
                             ^
                             +------ something else ;-)

> +        }
>      }
>      bdrv_unref(s->target);
>      block_job_completed(&s->common, ret);
> @@ -619,6 +626,10 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>                           BlockDriverCompletionFunc *cb,
>                           void *opaque, Error **errp)
>  {
> +    if (bdrv_reopen(base, bs->open_flags, errp)) {
> +        return;
> +    }
> +    bdrv_ref(base);

So when we start, the refcount changes as follows:

  +--- commit job            +------ NBD server
  v                          v
base (2) <-- sn1 (1) <-- sn2 (3) <-- guest
                             ^
                             +------ something else


Once all the data is copied over, we get o the code above, and first do
a bdrv_swap, which results in the following:

  +--- commit job            +------ NBD server
  v                          v
sn2 (2) <-- sn1 (1)     base (3) <-- guest
  |            ^             ^
  +------------+             +------ something else


Break the loop (but no refcount update!):

  +--- commit job            +------ NBD server
  v                          v
sn2 (2) <-- sn1 (1)     base (3) <-- guest
                             ^
                             +------ something else


Drop the commit job's reference:

                             +------ NBD server
                             v
sn2 (1) <-- sn1 (1)     base (3) <-- guest
                             ^
                             +------ something else


Okay, so writing this down made me actually understand what's wrong with
the refcount logic. You can't set s->base->backing_hd = NULL without
updating the reference count of the backing file.

Easy enough to fix. :-)

Kevin