Re: [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL

[Max Reitz]

On 30.01.2016 06:17, Jeff Cody wrote:
> In change_parent_backing_link(), we only inserted the new
> BlockDriverState entry into the device_list if the tqe_prev pointer was
> NULL.   However, we must also allow insertion when the BDS pointed
> to by the tqe_prev pointer is NULL as well.
> 
> This fixes a bug with external snapshots, and live active layer commits.
> 
> After a live snapshot occurs, the active layer and the base layer both
> have a non-NULL tqe_prev field in the device_list, although the base
> node's tqe_prev field points to a NULL entry.
> 
> Once the active commit is finished, bdrv_replace_in_backing_chain() is
> called to set the base node as the new active node, and remove the
> node that was the prior active layer from the device_list.
> 
> If we only check against the tqe_prev pointer field and not the entity
> it is pointing to, then we fail to insert base image into the device
> list.  The previous active layer is still removed from the device_list,
> leaving an empty device_list queue.
> 
> With an empty device_list queue, odd behavior occurs - such as not
> allowing any more live snapshots.
> 
> This commit fixes this issue, by checking for a NULL tqe_prev entity
> in the devices_list.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 5709d3d..0b8526b 100644
> --- a/block.c
> +++ b/block.c
> @@ -2272,7 +2272,7 @@ static void change_parent_backing_link(BlockDriverState 
> *from,
>      }
>      if (from->blk) {
>          blk_set_bs(from->blk, to);
> -        if (!to->device_list.tqe_prev) {
> +        if (!to->device_list.tqe_prev || !*to->device_list.tqe_prev) {

I'm not sure this is the right fix; bdrv_make_anon() clearly states that
we do want device_list.tqe_prev to be NULL if and only if the BDS is not
part of the device list. So this should not be happening.

>              QTAILQ_INSERT_BEFORE(from, to, device_list);
>          }
>          QTAILQ_REMOVE(&bdrv_states, from, device_list);

Inserting a from->device_list.tqe_prev = NULL; here makes the iotest
happy and looks like the better fix to me.

Max

>

signature.asc
Description: OpenPGP digital signature