Re: [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal

[Max Reitz]

On 02.02.2016 02:33, Jeff Cody wrote:
> This fixes a regression introduced with commit 3f09bfbc7.  Multiple
> bugs arise in conjunction with live snapshots and mirroring operations
> (which include active layer commit).
> 
> 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.  This non-NULL tqe_prev
> field occurs after the bdrv_append() in the external snapshot calls
> change_parent_backing_link().
> 
> In change_parent_backing_link(), when the previous active layer is
> removed from device_list, the device_list.tqe_prev pointer is not
> set to NULL.
> 
> The operating scheme in the block layer is to indicate that a BDS belongs
> in the bdrv_states device_list iff the device_list.tqe_prev pointer
> is non-NULL.
> 
> This patch does two things:
> 
> 1.) Introduces a new block layer helper bdrv_device_remove() to remove a
> BDS from the device_list, and
> 2.) uses that new API, which also fixes the regression once used in
> change_parent_backing_link().
> 
> Signed-off-by: Jeff Cody 
> ---
>  block.c   | 24 ++--
>  blockdev.c|  3 +--
>  include/block/block.h |  1 +
>  3 files changed, 16 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal

[Jeff Cody]

This fixes a regression introduced with commit 3f09bfbc7.  Multiple
bugs arise in conjunction with live snapshots and mirroring operations
(which include active layer commit).

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.  This non-NULL tqe_prev
field occurs after the bdrv_append() in the external snapshot calls
change_parent_backing_link().

In change_parent_backing_link(), when the previous active layer is
removed from device_list, the device_list.tqe_prev pointer is not
set to NULL.

The operating scheme in the block layer is to indicate that a BDS belongs
in the bdrv_states device_list iff the device_list.tqe_prev pointer
is non-NULL.

This patch does two things:

1.) Introduces a new block layer helper bdrv_device_remove() to remove a
BDS from the device_list, and
2.) uses that new API, which also fixes the regression once used in
change_parent_backing_link().

Signed-off-by: Jeff Cody 
---
 block.c   | 24 ++--
 blockdev.c|  3 +--
 include/block/block.h |  1 +
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 5709d3d..08cc130 100644
--- a/block.c
+++ b/block.c
@@ -2220,21 +2220,25 @@ void bdrv_close_all(void)
 }
 }
 
+/* Note that bs->device_list.tqe_prev is initially null,
+ * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
+ * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
+ * resetting it to null on remove.  */
+void bdrv_device_remove(BlockDriverState *bs)
+{
+QTAILQ_REMOVE(_states, bs, device_list);
+bs->device_list.tqe_prev = NULL;
+}
+
 /* make a BlockDriverState anonymous by removing from bdrv_state and
  * graph_bdrv_state list.
Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
 {
-/*
- * Take care to remove bs from bdrv_states only when it's actually
- * in it.  Note that bs->device_list.tqe_prev is initially null,
- * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
- * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
- * resetting it to null on remove.
- */
+/* Take care to remove bs from bdrv_states only when it's actually
+ * in it. */
 if (bs->device_list.tqe_prev) {
-QTAILQ_REMOVE(_states, bs, device_list);
-bs->device_list.tqe_prev = NULL;
+bdrv_device_remove(bs);
 }
 if (bs->node_name[0] != '\0') {
 QTAILQ_REMOVE(_bdrv_states, bs, node_list);
@@ -2275,7 +2279,7 @@ static void change_parent_backing_link(BlockDriverState 
*from,
 if (!to->device_list.tqe_prev) {
 QTAILQ_INSERT_BEFORE(from, to, device_list);
 }
-QTAILQ_REMOVE(_states, from, device_list);
+bdrv_device_remove(from);
 }
 }
 
diff --git a/blockdev.c b/blockdev.c
index 07cfe25..5f8e1b6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2382,8 +2382,7 @@ void qmp_x_blockdev_remove_medium(const char *device, 
Error **errp)
 
 /* This follows the convention established by bdrv_make_anon() */
 if (bs->device_list.tqe_prev) {
-QTAILQ_REMOVE(_states, bs, device_list);
-bs->device_list.tqe_prev = NULL;
+bdrv_device_remove(bs);
 }
 
 blk_remove_bs(blk);
diff --git a/include/block/block.h b/include/block/block.h
index 25f36dc..8c53b93 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -198,6 +198,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 BlockDriverState *bdrv_new_root(void);
 BlockDriverState *bdrv_new(void);
+void bdrv_device_remove(BlockDriverState *bs);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_replace_in_backing_chain(BlockDriverState *old,
-- 
1.9.3