From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36565) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQPn4-0007cN-DM for qemu-devel@nongnu.org; Mon, 01 Feb 2016 20:29:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQPn3-00048U-6S for qemu-devel@nongnu.org; Mon, 01 Feb 2016 20:29:58 -0500 Date: Mon, 1 Feb 2016 20:29:49 -0500 From: Jeff Cody Message-ID: <20160202012949.GA5658@localhost.localdomain> References: <56AFD166.9000904@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56AFD166.9000904@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org On Mon, Feb 01, 2016 at 10:43:02PM +0100, Max Reitz wrote: > 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 > > --- > > 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. > Good point. This also screams for a helper function to remove a BDS from the device_list, to enforce this behavior (we remove a BDS from the device_list is 3 spots, and this time it was missed in one of them). Hopefully that will help prevent future errors. > > 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 > Thanks! -Jeff