[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL
Date: Mon, 1 Feb 2016 22:43:02 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1

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.



Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]