Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot

[Max Reitz]

On 07.04.2016 13:29, Kevin Wolf wrote:
> Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
>> If bdrv_open_inherit() creates a snapshot BDS and *pbs is NULL, that
>> snapshot BDS should be returned instead of the BDS under it.
>>
>> To this end, bdrv_append_temp_snapshot() now returns the snapshot BDS
>> instead of just appending it on top of the snapshotted BDS. Also, it
>> calls bdrv_ref() before bdrv_append() (which bdrv_open_inherit() has to
>> undo if not returning the overlay).
>>
>> Signed-off-by: Max Reitz <address@hidden>
> 
> This is a tricky patch, but after all it looks correct to me. I think we
> could improve a bit on the documentation, though:
> 
> 1. The commit message suggests that by returning the wrong BDS we may
>    have an observable bug. It would be good to add details on why this
>    used to be harmless (IIUC, all users of BDRV_O_SNAPSHOT go through
>    blk_new_open(), and there first setting *pbs (which is blk->root->bs)
>    and then doing bdrv_append() does the right thing)

To be honest, I'd rather not wrap my head around why it worked, but I'll
try to.

> 2. The refcounting stuff isn't obvious either:
> 
>> @@ -1481,12 +1482,16 @@ static int 
>> bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
>>          goto out;
>>      }
>>  
>> +    bdrv_ref(bs_snapshot);
>>      bdrv_append(bs_snapshot, bs);
> 
> This is because bdrv_append() drops the reference, but we want to return
> a strong reference.

Well, it's mostly because now we do return a (strong) reference. Before,
bs_snapshot simply was not returned at all.

I'll add a comment, though.

>>      /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
>>       * temporary snapshot afterwards. */
>>      if (snapshot_flags) {
>> -        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, 
>> snapshot_options,
>> -                                        &local_err);
>> +        BlockDriverState *snapshot_bs;
>> +        snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
>> +                                                snapshot_options, 
>> &local_err);
>>          snapshot_options = NULL;
>>          if (local_err) {
>> +            ret = -EINVAL;
>>              goto close_and_fail;
>>          }
>> +        if (!*pbs) {
>> +            /* The reference is now held by the overlay BDS */
>> +            bdrv_unref(bs);
> 
> We still hold a strong reference to the newly created bs that we wanted
> to return, but now we're returning a different BDS, so we must drop the
> reference. (The overlay BDS doesn't hold "the" same reference as the
> comment suggests, but an additional one.)
> 
>> +            bs = snapshot_bs;
>> +        } else {
>> +            /* It is still referenced in the same way that *pbs was 
>> referenced,
>> +             * however that may be */
>> +            bdrv_unref(snapshot_bs);
> 
> In this case we don't in fact return the reference for bs_snapshot, so
> drop it.
> 
> So I think what I would like here is comments that explain where the
> ownership of the individual strong references goes, not who else may or
> may not hold additional references to a BDS.

Well, the ownership goes away. ;-)

I've fattened the comments so they explain exactly why the refcount is
decremented, respectively, and why this will most likely not result in
the deletion of the BDS.

Max

signature.asc
Description: OpenPGP digital signature