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 <jc...@redhat.com>
> > ---
> > 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
