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

[Fam Zheng <famz@redhat.com>]

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 <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);

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