QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths
@ 2019-09-11 15:00 Vladimir Sementsov-Ogievskiy
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-11 15:00 UTC (permalink / raw)
  To: qemu-block; +Cc: fam, kwolf, vsementsov, armbru, qemu-devel, mreitz, den, jsnow

We need to lock qcow2 mutex on accessing in-image metadata, especially
on updating this metadata. Let's implement it.

v2:

fix uninitialized variable in 03, found by Patchew

Vladimir Sementsov-Ogievskiy (3):
  block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
  block/dirty-bitmap: return int from
    bdrv_remove_persistent_dirty_bitmap
  block/qcow2: proper locking on bitmap add/remove paths

 block/qcow2.h                |  14 ++---
 include/block/block_int.h    |  14 ++---
 include/block/dirty-bitmap.h |   5 +-
 block.c                      |  22 -------
 block/dirty-bitmap.c         | 119 +++++++++++++++++++++++++++++++++--
 block/qcow2-bitmap.c         |  38 ++++++-----
 block/qcow2.c                |   5 +-
 blockdev.c                   |  28 +++------
 8 files changed, 164 insertions(+), 81 deletions(-)

-- 
2.18.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
  2019-09-11 15:00 [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
@ 2019-09-11 15:00 ` Vladimir Sementsov-Ogievskiy
  2019-09-13 21:51   ` John Snow
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap Vladimir Sementsov-Ogievskiy
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
  2 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-11 15:00 UTC (permalink / raw)
  To: qemu-block; +Cc: fam, kwolf, vsementsov, armbru, qemu-devel, mreitz, den, jsnow

block/dirty-bitmap.c seems to be more appropriate for it and
bdrv_remove_persistent_dirty_bitmap already in it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c              | 22 ----------------------
 block/dirty-bitmap.c | 22 ++++++++++++++++++++++
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 5944124845..bea03cfcc9 100644
--- a/block.c
+++ b/block.c
@@ -6555,25 +6555,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
 
     parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp)
-{
-    BlockDriver *drv = bs->drv;
-
-    if (!drv) {
-        error_setg_errno(errp, ENOMEDIUM,
-                         "Can't store persistent bitmaps to %s",
-                         bdrv_get_device_or_node_name(bs));
-        return false;
-    }
-
-    if (!drv->bdrv_can_store_new_dirty_bitmap) {
-        error_setg_errno(errp, ENOTSUP,
-                         "Can't store persistent bitmaps to %s",
-                         bdrv_get_device_or_node_name(bs));
-        return false;
-    }
-
-    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..8f42015db9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -464,6 +464,28 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
     }
 }
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                     uint32_t granularity, Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!drv) {
+        error_setg_errno(errp, ENOMEDIUM,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return false;
+    }
+
+    if (!drv->bdrv_can_store_new_dirty_bitmap) {
+        error_setg_errno(errp, ENOTSUP,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return false;
+    }
+
+    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     bdrv_dirty_bitmap_lock(bitmap);
-- 
2.18.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
  2019-09-11 15:00 [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
@ 2019-09-11 15:00 ` Vladimir Sementsov-Ogievskiy
  2019-09-13 22:01   ` John Snow
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
  2 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-11 15:00 UTC (permalink / raw)
  To: qemu-block; +Cc: fam, kwolf, vsementsov, armbru, qemu-devel, mreitz, den, jsnow

It's more comfortable to not deal with local_err.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h                |  5 ++---
 include/block/block_int.h    |  6 +++---
 include/block/dirty-bitmap.h |  5 ++---
 block/dirty-bitmap.c         |  9 +++++----
 block/qcow2-bitmap.c         | 20 +++++++++++---------
 blockdev.c                   |  7 +++----
 6 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..99ee88f802 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                       const char *name,
                                       uint32_t granularity,
                                       Error **errp);
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-                                          const char *name,
-                                          Error **errp);
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                         Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0422acdf1c..503ac9e3cd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -556,9 +556,9 @@ struct BlockDriver {
                                             const char *name,
                                             uint32_t granularity,
                                             Error **errp);
-    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
-                                                const char *name,
-                                                Error **errp);
+    int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+                                               const char *name,
+                                               Error **errp);
 
     /**
      * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b46..07503b03b5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
                             Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-                                         const char *name,
-                                         Error **errp);
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8f42015db9..a52b83b619 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-                                         const char *name,
-                                         Error **errp)
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        Error **errp)
 {
     if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-        bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+        return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
     }
+
+    return -ENOTSUP;
 }
 
 bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..1aaedb3b55 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
     return NULL;
 }
 
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-                                          const char *name,
-                                          Error **errp)
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                         Error **errp)
 {
-    int ret;
+    int ret = 0;
     BDRVQcow2State *s = bs->opaque;
     Qcow2Bitmap *bm;
     Qcow2BitmapList *bm_list;
@@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
     if (s->nb_bitmaps == 0) {
         /* Absence of the bitmap is not an error: see explanation above
          * bdrv_remove_persistent_dirty_bitmap() definition. */
-        return;
+        return 0;
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
     if (bm_list == NULL) {
-        return;
+        return -EIO;
     }
 
     bm = find_bitmap_by_name(bm_list, name);
     if (bm == NULL) {
-        goto fail;
+        ret = -EINVAL;
+        goto out;
     }
 
     QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
@@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
     ret = update_ext_header_and_dir(bs, bm_list);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed to update bitmap extension");
-        goto fail;
+        goto out;
     }
 
     free_bitmap_clusters(bs, &bm->table);
 
-fail:
+out:
     bitmap_free(bm);
     bitmap_list_free(bm_list);
+
+    return ret;
 }
 
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
diff --git a/blockdev.c b/blockdev.c
index fbef6845c8..0813adfb2b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+        int ret;
         AioContext *aio_context = bdrv_get_aio_context(bs);
-        Error *local_err = NULL;
 
         aio_context_acquire(aio_context);
-        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
+        ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
         aio_context_release(aio_context);
 
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
+        if (ret < 0) {
             return NULL;
         }
     }
-- 
2.18.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths
  2019-09-11 15:00 [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2019-09-11 15:00 ` Vladimir Sementsov-Ogievskiy
  2019-09-13 22:26   ` John Snow
  2 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-11 15:00 UTC (permalink / raw)
  To: qemu-block; +Cc: fam, kwolf, vsementsov, armbru, qemu-devel, mreitz, den, jsnow

qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
aio context since 0a6c86d024c52b. But this is not enough: we also must
lock qcow2 mutex when access in-image metadata. Especially it concerns
freeing qcow2 clusters.

To achieve this, move qcow2_can_store_new_dirty_bitmap and
qcow2_remove_persistent_dirty_bitmap to coroutine context.

Since we work in coroutines in correct aio context, we don't need
context acquiring in blockdev.c anymore, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h             |  11 ++--
 include/block/block_int.h |  10 ++--
 block/dirty-bitmap.c      | 102 +++++++++++++++++++++++++++++++++++---
 block/qcow2-bitmap.c      |  24 ++++++---
 block/qcow2.c             |   5 +-
 blockdev.c                |  27 +++-------
 6 files changed, 131 insertions(+), 48 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 99ee88f802..27e20c9b4a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -743,12 +743,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-                                      const char *name,
-                                      uint32_t granularity,
-                                      Error **errp);
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+                                         const char *name,
+                                         uint32_t granularity,
                                          Error **errp);
+int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                            const char *name,
+                                            Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 503ac9e3cd..1e54486ad1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -552,13 +552,13 @@ struct BlockDriver {
      * field of BlockDirtyBitmap's in case of success.
      */
     int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
-    bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
-                                            const char *name,
-                                            uint32_t granularity,
-                                            Error **errp);
-    int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+    bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
                                                const char *name,
+                                               uint32_t granularity,
                                                Error **errp);
+    int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+                                                  const char *name,
+                                                  Error **errp);
 
     /**
      * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index a52b83b619..f50c682308 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
+#include "qemu/main-loop.h"
 
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
@@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                        Error **errp)
+static int coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                       Error **errp)
 {
-    if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-        return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+    if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
+        return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
     }
 
     return -ENOTSUP;
 }
 
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp)
+typedef struct BdrvRemovePersistentDirtyBitmapCo {
+    BlockDriverState *bs;
+    const char *name;
+    Error **errp;
+    int ret;
+} BdrvRemovePersistentDirtyBitmapCo;
+
+static void coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
+{
+    BdrvRemovePersistentDirtyBitmapCo *s = opaque;
+
+    s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
+    aio_wait_kick();
+}
+
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        Error **errp)
+{
+    if (qemu_in_coroutine()) {
+        return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
+    } else {
+        Coroutine *co;
+        BdrvRemovePersistentDirtyBitmapCo s = {
+            .bs = bs,
+            .name = name,
+            .errp = errp,
+            .ret = -EINPROGRESS,
+        };
+
+        co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
+                                   &s);
+        bdrv_coroutine_enter(bs, co);
+        BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
+
+        return s.ret;
+    }
+}
+
+static bool coroutine_fn
+bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                   uint32_t granularity, Error **errp)
 {
     BlockDriver *drv = bs->drv;
 
@@ -477,14 +519,58 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
         return false;
     }
 
-    if (!drv->bdrv_can_store_new_dirty_bitmap) {
+    if (!drv->bdrv_co_can_store_new_dirty_bitmap) {
         error_setg_errno(errp, ENOTSUP,
                          "Can't store persistent bitmaps to %s",
                          bdrv_get_device_or_node_name(bs));
         return false;
     }
 
-    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+    return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
+typedef struct BdrvCanStoreNewDirtyBitmapCo {
+    BlockDriverState *bs;
+    const char *name;
+    uint32_t granularity;
+    Error **errp;
+    bool ret;
+
+    bool in_progress;
+} BdrvCanStoreNewDirtyBitmapCo;
+
+static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
+{
+    BdrvCanStoreNewDirtyBitmapCo *s = opaque;
+
+    s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
+                                                s->errp);
+    s->in_progress = false;
+    aio_wait_kick();
+}
+
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                     uint32_t granularity, Error **errp)
+{
+    if (qemu_in_coroutine()) {
+        return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+    } else {
+        Coroutine *co;
+        BdrvCanStoreNewDirtyBitmapCo s = {
+            .bs = bs,
+            .name = name,
+            .granularity = granularity,
+            .errp = errp,
+            .in_progress = true,
+        };
+
+        co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
+                                   &s);
+        bdrv_coroutine_enter(bs, co);
+        BDRV_POLL_WHILE(bs, s.in_progress);
+
+        return s.ret;
+    }
 }
 
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 1aaedb3b55..c684e4c8bc 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,12 +1404,13 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
     return NULL;
 }
 
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                         Error **errp)
+int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                                         const char *name,
+                                                         Error **errp)
 {
     int ret = 0;
     BDRVQcow2State *s = bs->opaque;
-    Qcow2Bitmap *bm;
+    Qcow2Bitmap *bm = NULL;
     Qcow2BitmapList *bm_list;
 
     if (s->nb_bitmaps == 0) {
@@ -1418,10 +1419,13 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
         return 0;
     }
 
+    qemu_co_mutex_lock(&s->lock);
+
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
     if (bm_list == NULL) {
-        return -EIO;
+        ret = -EIO;
+        goto out;
     }
 
     bm = find_bitmap_by_name(bm_list, name);
@@ -1441,6 +1445,8 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
     free_bitmap_clusters(bs, &bm->table);
 
 out:
+    qemu_co_mutex_unlock(&s->lock);
+
     bitmap_free(bm);
     bitmap_list_free(bm_list);
 
@@ -1615,10 +1621,10 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
     return 0;
 }
 
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-                                      const char *name,
-                                      uint32_t granularity,
-                                      Error **errp)
+bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+                                                      const char *name,
+                                                      uint32_t granularity,
+                                                      Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     bool found;
@@ -1655,8 +1661,10 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
+    qemu_co_mutex_lock(&s->lock);
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
+    qemu_co_mutex_unlock(&s->lock);
     if (bm_list == NULL) {
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 0882ff6e92..488738f499 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5258,8 +5258,9 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
     .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
-    .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
-    .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
+    .bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
+    .bdrv_co_remove_persistent_dirty_bitmap =
+            qcow2_co_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/blockdev.c b/blockdev.c
index 0813adfb2b..228ce94a88 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2898,16 +2898,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         disabled = false;
     }
 
-    if (persistent) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-        bool ok;
-
-        aio_context_acquire(aio_context);
-        ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-        aio_context_release(aio_context);
-        if (!ok) {
-            return;
-        }
+    if (persistent &&
+        !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+    {
+        return;
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
@@ -2939,17 +2933,10 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
         return NULL;
     }
 
-    if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-        int ret;
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
-        aio_context_release(aio_context);
-
-        if (ret < 0) {
+    if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
+        bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
+    {
             return NULL;
-        }
     }
 
     if (release) {
-- 
2.18.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
@ 2019-09-13 21:51   ` John Snow
  0 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2019-09-13 21:51 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block
  Cc: fam, kwolf, qemu-devel, armbru, mreitz, den



On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> block/dirty-bitmap.c seems to be more appropriate for it and
> bdrv_remove_persistent_dirty_bitmap already in it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

No disagreements here. I think we put it in block.c initially because
it's a bdrv callback/dispatch function, but dirty-block.c is just a
logical subdivision of block.c, so it's fine.

Reviewed-by: John Snow <jsnow@redhat.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2019-09-13 22:01   ` John Snow
  2019-09-16  8:22     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 8+ messages in thread
From: John Snow @ 2019-09-13 22:01 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block
  Cc: fam, kwolf, qemu-devel, armbru, mreitz, den



On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's more comfortable to not deal with local_err.
> 

I agree.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h                |  5 ++---
>  include/block/block_int.h    |  6 +++---
>  include/block/dirty-bitmap.h |  5 ++---
>  block/dirty-bitmap.c         |  9 +++++----
>  block/qcow2-bitmap.c         | 20 +++++++++++---------
>  blockdev.c                   |  7 +++----
>  6 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 998bcdaef1..99ee88f802 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                        const char *name,
>                                        uint32_t granularity,
>                                        Error **errp);
> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                          const char *name,
> -                                          Error **errp);
> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                         Error **errp);
>  
>  ssize_t coroutine_fn
>  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 0422acdf1c..503ac9e3cd 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -556,9 +556,9 @@ struct BlockDriver {
>                                              const char *name,
>                                              uint32_t granularity,
>                                              Error **errp);
> -    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> -                                                const char *name,
> -                                                Error **errp);
> +    int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> +                                               const char *name,
> +                                               Error **errp);
>  
>      /**
>       * Register/unregister a buffer for I/O. For example, when the driver is
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 4b4b731b46..07503b03b5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>                              Error **errp);
>  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>  void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                         const char *name,
> -                                         Error **errp);
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                        Error **errp);
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8f42015db9..a52b83b619 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>   * not fail.
>   * This function doesn't release corresponding BdrvDirtyBitmap.
>   */
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                         const char *name,
> -                                         Error **errp)
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                        Error **errp)
>  {
>      if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
> -        bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
> +        return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>      }
> +

But is it a problem if we return an error code without setting errp now?
If this is for the sake of not having to deal with local_err, we should
make sure that a non-zero return means that errp is set. Right?

> +    return -ENOTSUP;
>  }
>  
>  bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b2487101ed..1aaedb3b55 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>      return NULL;
>  }
>  
> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> -                                          const char *name,
> -                                          Error **errp)
> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                         Error **errp)
>  {
> -    int ret;
> +    int ret = 0;

I was going to say I'd rather not initialize this, but is this related
to ubsan linting?

>      BDRVQcow2State *s = bs->opaque;
>      Qcow2Bitmap *bm;
>      Qcow2BitmapList *bm_list;
> @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>      if (s->nb_bitmaps == 0) {
>          /* Absence of the bitmap is not an error: see explanation above
>           * bdrv_remove_persistent_dirty_bitmap() definition. */
> -        return;
> +        return 0;
>      }
>  
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                 s->bitmap_directory_size, errp);
>      if (bm_list == NULL) {
> -        return;
> +        return -EIO;
>      }
>  
>      bm = find_bitmap_by_name(bm_list, name);
>      if (bm == NULL) {
> -        goto fail;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
>      QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
> @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>      ret = update_ext_header_and_dir(bs, bm_list);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Failed to update bitmap extension");
> -        goto fail;
> +        goto out;
>      }
>  
>      free_bitmap_clusters(bs, &bm->table);
>  
> -fail:
> +out:
>      bitmap_free(bm);
>      bitmap_list_free(bm_list);
> +
> +    return ret;
>  }
>  
>  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> diff --git a/blockdev.c b/blockdev.c
> index fbef6845c8..0813adfb2b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>      }
>  
>      if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> +        int ret;
>          AioContext *aio_context = bdrv_get_aio_context(bs);
> -        Error *local_err = NULL;
>  
>          aio_context_acquire(aio_context);
> -        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
> +        ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>          aio_context_release(aio_context);
>  
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> +        if (ret < 0) {
>              return NULL;
>          }
>      }
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths
  2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
@ 2019-09-13 22:26   ` John Snow
  0 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2019-09-13 22:26 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block
  Cc: fam, kwolf, qemu-devel, armbru, mreitz, den



On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
> aio context since 0a6c86d024c52b. But this is not enough: we also must
> lock qcow2 mutex when access in-image metadata. Especially it concerns
> freeing qcow2 clusters.
> 
> To achieve this, move qcow2_can_store_new_dirty_bitmap and
> qcow2_remove_persistent_dirty_bitmap to coroutine context.
> 
> Since we work in coroutines in correct aio context, we don't need
> context acquiring in blockdev.c anymore, drop it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

It makes sense to me given the context, but I'll want Kevin or Max to
eye it over for locking correctness, admittedly.

Here's my good faith RB:

Reviewed-by: John Snow <jsnow@redhat.com>


(I'm not so sure about the aio_wait_kick. Why is that needed here? ...

oh, 4720cbeea1f42fd905fc69338fd42b191e58b412 explains why.
I suppose any synchronous caller of a co_ function needs to make sure
that the entry shim kicks on the way out. This seems very easy to get
wrong, doesn't it?)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
  2019-09-13 22:01   ` John Snow
@ 2019-09-16  8:22     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-16  8:22 UTC (permalink / raw)
  To: John Snow, qemu-block; +Cc: fam, kwolf, Denis Lunev, qemu-devel, armbru, mreitz

14.09.2019 1:01, John Snow wrote:
> 
> 
> On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's more comfortable to not deal with local_err.
>>
> 
> I agree.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.h                |  5 ++---
>>   include/block/block_int.h    |  6 +++---
>>   include/block/dirty-bitmap.h |  5 ++---
>>   block/dirty-bitmap.c         |  9 +++++----
>>   block/qcow2-bitmap.c         | 20 +++++++++++---------
>>   blockdev.c                   |  7 +++----
>>   6 files changed, 26 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 998bcdaef1..99ee88f802 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>                                         const char *name,
>>                                         uint32_t granularity,
>>                                         Error **errp);
>> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> -                                          const char *name,
>> -                                          Error **errp);
>> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>> +                                         Error **errp);
>>   
>>   ssize_t coroutine_fn
>>   qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 0422acdf1c..503ac9e3cd 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -556,9 +556,9 @@ struct BlockDriver {
>>                                               const char *name,
>>                                               uint32_t granularity,
>>                                               Error **errp);
>> -    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> -                                                const char *name,
>> -                                                Error **errp);
>> +    int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> +                                               const char *name,
>> +                                               Error **errp);
>>   
>>       /**
>>        * Register/unregister a buffer for I/O. For example, when the driver is
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 4b4b731b46..07503b03b5 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>>                               Error **errp);
>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> -                                         const char *name,
>> -                                         Error **errp);
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>> +                                        Error **errp);
>>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>>   void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>>   void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8f42015db9..a52b83b619 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>>    * not fail.
>>    * This function doesn't release corresponding BdrvDirtyBitmap.
>>    */
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> -                                         const char *name,
>> -                                         Error **errp)
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>> +                                        Error **errp)
>>   {
>>       if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
>> -        bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>> +        return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>>       }
>> +
> 
> But is it a problem if we return an error code without setting errp now?
> If this is for the sake of not having to deal with local_err, we should
> make sure that a non-zero return means that errp is set. Right?

Oops, of course, I should set errp here

> 
>> +    return -ENOTSUP;
>>   }
>>   
>>   bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index b2487101ed..1aaedb3b55 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>>       return NULL;
>>   }
>>   
>> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> -                                          const char *name,
>> -                                          Error **errp)
>> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>> +                                         Error **errp)
>>   {
>> -    int ret;
>> +    int ret = 0;
> 
> I was going to say I'd rather not initialize this, but is this related
> to ubsan linting?

Hmm, don't remember. For some reason I thought  it's needed..
Will drop and check what compiler says.

> 
>>       BDRVQcow2State *s = bs->opaque;
>>       Qcow2Bitmap *bm;
>>       Qcow2BitmapList *bm_list;
>> @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>       if (s->nb_bitmaps == 0) {
>>           /* Absence of the bitmap is not an error: see explanation above
>>            * bdrv_remove_persistent_dirty_bitmap() definition. */
>> -        return;
>> +        return 0;
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>                                  s->bitmap_directory_size, errp);
>>       if (bm_list == NULL) {
>> -        return;
>> +        return -EIO;
>>       }
>>   
>>       bm = find_bitmap_by_name(bm_list, name);
>>       if (bm == NULL) {
>> -        goto fail;
>> +        ret = -EINVAL;
>> +        goto out;
>>       }
>>   
>>       QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
>> @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>       ret = update_ext_header_and_dir(bs, bm_list);
>>       if (ret < 0) {
>>           error_setg_errno(errp, -ret, "Failed to update bitmap extension");
>> -        goto fail;
>> +        goto out;
>>       }
>>   
>>       free_bitmap_clusters(bs, &bm->table);
>>   
>> -fail:
>> +out:
>>       bitmap_free(bm);
>>       bitmap_list_free(bm_list);
>> +
>> +    return ret;
>>   }
>>   
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> diff --git a/blockdev.c b/blockdev.c
>> index fbef6845c8..0813adfb2b 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>>       }
>>   
>>       if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> +        int ret;
>>           AioContext *aio_context = bdrv_get_aio_context(bs);
>> -        Error *local_err = NULL;
>>   
>>           aio_context_acquire(aio_context);
>> -        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>> +        ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>>           aio_context_release(aio_context);
>>   
>> -        if (local_err != NULL) {
>> -            error_propagate(errp, local_err);
>> +        if (ret < 0) {
>>               return NULL;
>>           }
>>       }
>>


-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 15:00 [Qemu-devel] [PATCH v2 0/3] proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 1/3] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c Vladimir Sementsov-Ogievskiy
2019-09-13 21:51   ` John Snow
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap Vladimir Sementsov-Ogievskiy
2019-09-13 22:01   ` John Snow
2019-09-16  8:22     ` Vladimir Sementsov-Ogievskiy
2019-09-11 15:00 ` [Qemu-devel] [PATCH v2 3/3] block/qcow2: proper locking on bitmap add/remove paths Vladimir Sementsov-Ogievskiy
2019-09-13 22:26   ` John Snow

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox