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

[Fam Zheng]

On 2013年12月14日 02:26, Kevin Wolf wrote:
> 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 <address@hidden>
> > ---
> >   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);

Dropping "p" here (points to sn1 in your example) recursively results in ...

> 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

... sn2(1) left alone. It is also reference by s->target and is 
bdrv_unref'ed right before block_job_completed(). So...

> Drop the commit job's reference:
>
>                               +------ NBD server
>                               v
> sn2 (1) <-- sn1 (1)     base (3) <-- guest
>                               ^
>                               +------ something else

you mean sn2 (1) <-- sn1 (1) is leaked but I double checked it is not. 
(Did you overlooked the bdrv_unref(p)?)

Fam