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

[Kevin Wolf]

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)

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.

>      /* 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.

Kevin