[Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is block-dirty-bitmap-remove transaction action.

It is used to do transactional movement of the bitmap (which is
possible in conjunction with merge command). Transactional bitmap
movement is needed in scenarios with external snapshot, when we don't
want to leave copy of the bitmap in the base image.

Implementation itself in 03, in short:

.prepare: make bitmap unnamed and non-persistent, delete stored version
          of the bitmap from the image

.commit: release bitmap

.abort: restore bitmap name and persistence. We don't restore bitmap
        version in the image. It's not critical, we have in-RAM version,
        it will be stored on shutdown

Vladimir Sementsov-Ogievskiy (4):
  blockdev: reduce aio_context locked sections in bitmap add/remove
  block/dirty-bitmap: add hide/unhide API
  qapi: implement block-dirty-bitmap-remove transaction action
  iotests: test bitmap moving inside 254

 qapi/transaction.json        |   2 +
 include/block/dirty-bitmap.h |   2 +
 block/dirty-bitmap.c         |  26 +++++++++
 blockdev.c                   | 100 +++++++++++++++++++++++++++--------
 tests/qemu-iotests/254       |  30 ++++++++++-
 tests/qemu-iotests/254.out   |  82 ++++++++++++++++++++++++++++
 6 files changed, 219 insertions(+), 23 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/4] blockdev: reduce aio_context locked sections in bitmap add/remove

[Vladimir Sementsov-Ogievskiy]

Commit 0a6c86d024c52 returned these locks back to add/remove
functionality, to protect from intersection of persistent bitmap
related IO with other IO. But other bitmap-related functions called
here are unrelated to the problem, and there are no needs to keep these
calls inside critical sections.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 17c2d801d7..5b3eef0d3e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2812,7 +2812,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
-    AioContext *aio_context = NULL;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2848,16 +2847,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     if (persistent) {
-        aio_context = bdrv_get_aio_context(bs);
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+        bool ok;
+
         aio_context_acquire(aio_context);
-        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
-            goto out;
+        ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+        aio_context_release(aio_context);
+        if (!ok) {
+            return;
         }
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
     if (bitmap == NULL) {
-        goto out;
+        return;
     }
 
     if (disabled) {
@@ -2865,10 +2868,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
- out:
-    if (aio_context) {
-        aio_context_release(aio_context);
-    }
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
@@ -2876,8 +2875,6 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
-    Error *local_err = NULL;
-    AioContext *aio_context = NULL;
 
     bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
     if (!bitmap || !bs) {
@@ -2890,20 +2887,19 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-        aio_context = bdrv_get_aio_context(bs);
+        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);
+        aio_context_release(aio_context);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
-            goto out;
+            return;
         }
     }
 
     bdrv_release_dirty_bitmap(bs, bitmap);
- out:
-    if (aio_context) {
-        aio_context_release(aio_context);
-    }
 }
 
 /**
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/4] blockdev: reduce aio_context locked sections in bitmap add/remove

[John Snow]

On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> Commit 0a6c86d024c52 returned these locks back to add/remove
> functionality, to protect from intersection of persistent bitmap
> related IO with other IO. But other bitmap-related functions called
> here are unrelated to the problem, and there are no needs to keep these
> calls inside critical sections.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Fine, of course. I'll probably rebase my series on top of this if the
rest of the patches look good.

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

[Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

Add functionality to make bitmap temporary anonymous. It will be used
to implement bitmap remove transaction action. We need hide bitmap
persistence too, as there are should not be unnamed persistent bitmaps.

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

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8044ace63e..542e437123 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
                                                   BdrvDirtyBitmap *bitmap,
                                                   Error **errp);
+void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 49646a30e6..592964635e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
     bool busy;                  /* Bitmap is busy, it can't be used via QMP */
     BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
     char *name;                 /* Optional non-empty unique ID */
+    char *hidden_name;          /* Backup of @name for removal transaction
+                                   action. Used for hide/unhide API. */
+    bool hidden_persistent;     /* Backup of @persistent for removal transaction
+                                   action. */
     int64_t size;               /* Size of the bitmap, in bytes */
     bool disabled;              /* Bitmap is disabled. It ignores all writes to
                                    the device */
@@ -849,3 +853,25 @@ out:
         qemu_mutex_unlock(src->mutex);
     }
 }
+
+void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
+{
+    qemu_mutex_lock(bitmap->mutex);
+    assert(!bitmap->hidden_name);
+    bitmap->hidden_name = bitmap->name;
+    bitmap->hidden_persistent = bitmap->persistent;
+    bitmap->name = NULL;
+    bitmap->persistent = false;
+    qemu_mutex_unlock(bitmap->mutex);
+}
+
+void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
+{
+    qemu_mutex_lock(bitmap->mutex);
+    assert(!bitmap->name);
+    bitmap->name = bitmap->hidden_name;
+    bitmap->persistent = bitmap->hidden_persistent;
+    bitmap->hidden_name = NULL;
+    bitmap->hidden_persistent = false;
+    qemu_mutex_unlock(bitmap->mutex);
+}
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[John Snow]

On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add functionality to make bitmap temporary anonymous. It will be used
> to implement bitmap remove transaction action. We need hide bitmap
> persistence too, as there are should not be unnamed persistent bitmaps.
> 

Ah, so this effectively ... "hides" a bitmap from any further
transaction actions. It also "hides" it from getting flushed to disk...
sort of?

The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
skip this bitmap because it's anonymous/not persistent.

There's a second loop where we iterate bm_list, and we'll skip storing
this bitmap because that entry won't have an in-memory bitmap associated
with it in bm_list.

...But then we'll call update_ext_header_and_dir with the stale entries
in bm_list?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8044ace63e..542e437123 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>  BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>                                                    BdrvDirtyBitmap *bitmap,
>                                                    Error **errp);
> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>  
>  #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 49646a30e6..592964635e 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>      bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>      BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>      char *name;                 /* Optional non-empty unique ID */
> +    char *hidden_name;          /* Backup of @name for removal transaction
> +                                   action. Used for hide/unhide API. */
> +    bool hidden_persistent;     /* Backup of @persistent for removal transaction
> +                                   action. */
>      int64_t size;               /* Size of the bitmap, in bytes */
>      bool disabled;              /* Bitmap is disabled. It ignores all writes to
>                                     the device */
> @@ -849,3 +853,25 @@ out:
>          qemu_mutex_unlock(src->mutex);
>      }
>  }
> +
> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    assert(!bitmap->hidden_name);
> +    bitmap->hidden_name = bitmap->name;
> +    bitmap->hidden_persistent = bitmap->persistent;
> +    bitmap->name = NULL;
> +    bitmap->persistent = false;
> +    qemu_mutex_unlock(bitmap->mutex);
> +}
> +
> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    assert(!bitmap->name);
> +    bitmap->name = bitmap->hidden_name;
> +    bitmap->persistent = bitmap->hidden_persistent;
> +    bitmap->hidden_name = NULL;
> +    bitmap->hidden_persistent = false;
> +    qemu_mutex_unlock(bitmap->mutex);
> +}
> 

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

08.06.2019 1:39, John Snow wrote:
> 
> 
> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add functionality to make bitmap temporary anonymous. It will be used
>> to implement bitmap remove transaction action. We need hide bitmap
>> persistence too, as there are should not be unnamed persistent bitmaps.
>>
> 
> Ah, so this effectively ... "hides" a bitmap from any further
> transaction actions. It also "hides" it from getting flushed to disk...
> sort of?
> 
> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
> skip this bitmap because it's anonymous/not persistent.
> 
> There's a second loop where we iterate bm_list, and we'll skip storing
> this bitmap because that entry won't have an in-memory bitmap associated
> with it in bm_list.
> 
> ...But then we'll call update_ext_header_and_dir with the stale entries
> in bm_list?

Hidden is a temprory state, so, we should not go to close() in this state.
It's a state inside a transaction.

Stale entries in list will be marked IN_USE anyway, so there is no damage anyway.

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 8044ace63e..542e437123 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>                                                     BdrvDirtyBitmap *bitmap,
>>                                                     Error **errp);
>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>>   
>>   #endif
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 49646a30e6..592964635e 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>>       bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>>       BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>>       char *name;                 /* Optional non-empty unique ID */
>> +    char *hidden_name;          /* Backup of @name for removal transaction
>> +                                   action. Used for hide/unhide API. */
>> +    bool hidden_persistent;     /* Backup of @persistent for removal transaction
>> +                                   action. */
>>       int64_t size;               /* Size of the bitmap, in bytes */
>>       bool disabled;              /* Bitmap is disabled. It ignores all writes to
>>                                      the device */
>> @@ -849,3 +853,25 @@ out:
>>           qemu_mutex_unlock(src->mutex);
>>       }
>>   }
>> +
>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    assert(!bitmap->hidden_name);
>> +    bitmap->hidden_name = bitmap->name;
>> +    bitmap->hidden_persistent = bitmap->persistent;
>> +    bitmap->name = NULL;
>> +    bitmap->persistent = false;
>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
>> +
>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    assert(!bitmap->name);
>> +    bitmap->name = bitmap->hidden_name;
>> +    bitmap->persistent = bitmap->hidden_persistent;
>> +    bitmap->hidden_name = NULL;
>> +    bitmap->hidden_persistent = false;
>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2019 1:39, John Snow wrote:
>>
>>
>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add functionality to make bitmap temporary anonymous. It will be used
>>> to implement bitmap remove transaction action. We need hide bitmap
>>> persistence too, as there are should not be unnamed persistent bitmaps.
>>>
>>
>> Ah, so this effectively ... "hides" a bitmap from any further
>> transaction actions. It also "hides" it from getting flushed to disk...
>> sort of?
>>
>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
>> skip this bitmap because it's anonymous/not persistent.
>>
>> There's a second loop where we iterate bm_list, and we'll skip storing
>> this bitmap because that entry won't have an in-memory bitmap associated
>> with it in bm_list.
>>
>> ...But then we'll call update_ext_header_and_dir with the stale entries
>> in bm_list?
> 
> Hidden is a temprory state, so, we should not go to close() in this state.
> It's a state inside a transaction.
> 
> Stale entries in list will be marked IN_USE anyway, so there is no damage anyway.

I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you
are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design:
creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow.

> 
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/dirty-bitmap.h |  2 ++
>>>   block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index 8044ace63e..542e437123 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>>>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>>                                                     BdrvDirtyBitmap *bitmap,
>>>                                                     Error **errp);
>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>>>   #endif
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 49646a30e6..592964635e 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>>>       bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>>>       BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>>>       char *name;                 /* Optional non-empty unique ID */
>>> +    char *hidden_name;          /* Backup of @name for removal transaction
>>> +                                   action. Used for hide/unhide API. */
>>> +    bool hidden_persistent;     /* Backup of @persistent for removal transaction
>>> +                                   action. */
>>>       int64_t size;               /* Size of the bitmap, in bytes */
>>>       bool disabled;              /* Bitmap is disabled. It ignores all writes to
>>>                                      the device */
>>> @@ -849,3 +853,25 @@ out:
>>>           qemu_mutex_unlock(src->mutex);
>>>       }
>>>   }
>>> +
>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    assert(!bitmap->hidden_name);
>>> +    bitmap->hidden_name = bitmap->name;
>>> +    bitmap->hidden_persistent = bitmap->persistent;
>>> +    bitmap->name = NULL;
>>> +    bitmap->persistent = false;
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>> +
>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    assert(!bitmap->name);
>>> +    bitmap->name = bitmap->hidden_name;
>>> +    bitmap->persistent = bitmap->hidden_persistent;
>>> +    bitmap->hidden_name = NULL;
>>> +    bitmap->hidden_persistent = false;
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

10.06.2019 12:42, Vladimir Sementsov-Ogievskiy wrote:
> 10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote:
>> 08.06.2019 1:39, John Snow wrote:
>>>
>>>
>>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add functionality to make bitmap temporary anonymous. It will be used
>>>> to implement bitmap remove transaction action. We need hide bitmap
>>>> persistence too, as there are should not be unnamed persistent bitmaps.
>>>>
>>>
>>> Ah, so this effectively ... "hides" a bitmap from any further
>>> transaction actions. It also "hides" it from getting flushed to disk...
>>> sort of?
>>>
>>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
>>> skip this bitmap because it's anonymous/not persistent.
>>>
>>> There's a second loop where we iterate bm_list, and we'll skip storing
>>> this bitmap because that entry won't have an in-memory bitmap associated
>>> with it in bm_list.
>>>
>>> ...But then we'll call update_ext_header_and_dir with the stale entries
>>> in bm_list?
>>
>> Hidden is a temprory state, so, we should not go to close() in this state.
>> It's a state inside a transaction.
>>
>> Stale entries in list will be marked IN_USE anyway, so there is no damage anyway.
> 
> I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you
> are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design:
> creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow.

Ahaha, and I'm again wrong, as my series do right thing: remove bitmap from the image
in .prepare, so there would not be stale entries, all is OK.

> 
>>
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   include/block/dirty-bitmap.h |  2 ++
>>>>   block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index 8044ace63e..542e437123 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>>>>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>>>                                                     BdrvDirtyBitmap *bitmap,
>>>>                                                     Error **errp);
>>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
>>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>>>>   #endif
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 49646a30e6..592964635e 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>>>>       bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>>>>       BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>>>>       char *name;                 /* Optional non-empty unique ID */
>>>> +    char *hidden_name;          /* Backup of @name for removal transaction
>>>> +                                   action. Used for hide/unhide API. */
>>>> +    bool hidden_persistent;     /* Backup of @persistent for removal transaction
>>>> +                                   action. */
>>>>       int64_t size;               /* Size of the bitmap, in bytes */
>>>>       bool disabled;              /* Bitmap is disabled. It ignores all writes to
>>>>                                      the device */
>>>> @@ -849,3 +853,25 @@ out:
>>>>           qemu_mutex_unlock(src->mutex);
>>>>       }
>>>>   }
>>>> +
>>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    assert(!bitmap->hidden_name);
>>>> +    bitmap->hidden_name = bitmap->name;
>>>> +    bitmap->hidden_persistent = bitmap->persistent;
>>>> +    bitmap->name = NULL;
>>>> +    bitmap->persistent = false;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>> +
>>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    assert(!bitmap->name);
>>>> +    bitmap->name = bitmap->hidden_name;
>>>> +    bitmap->persistent = bitmap->hidden_persistent;
>>>> +    bitmap->hidden_name = NULL;
>>>> +    bitmap->hidden_persistent = false;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

10.06.2019 12:42, Vladimir Sementsov-Ogievskiy wrote:
> 10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote:
>> 08.06.2019 1:39, John Snow wrote:
>>>
>>>
>>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add functionality to make bitmap temporary anonymous. It will be used
>>>> to implement bitmap remove transaction action. We need hide bitmap
>>>> persistence too, as there are should not be unnamed persistent bitmaps.
>>>>
>>>
>>> Ah, so this effectively ... "hides" a bitmap from any further
>>> transaction actions. It also "hides" it from getting flushed to disk...
>>> sort of?
>>>
>>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
>>> skip this bitmap because it's anonymous/not persistent.
>>>
>>> There's a second loop where we iterate bm_list, and we'll skip storing
>>> this bitmap because that entry won't have an in-memory bitmap associated
>>> with it in bm_list.
>>>
>>> ...But then we'll call update_ext_header_and_dir with the stale entries
>>> in bm_list?
>>
>> Hidden is a temprory state, so, we should not go to close() in this state.
>> It's a state inside a transaction.
>>
>> Stale entries in list will be marked IN_USE anyway, so there is no damage anyway.
> 
> I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, seems you
> are right, we are storing stale IN_USE bitmaps. No serious damage, but bad design:
> creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow.

Ahaha, and I'm wrong again, as my series do right thing: remove bitmap from the image
in .prepare, so there will be no stale entries..

> 
>>
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   include/block/dirty-bitmap.h |  2 ++
>>>>   block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index 8044ace63e..542e437123 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>>>>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>>>                                                     BdrvDirtyBitmap *bitmap,
>>>>                                                     Error **errp);
>>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
>>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>>>>   #endif
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 49646a30e6..592964635e 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>>>>       bool busy;                  /* Bitmap is busy, it can't be used via QMP */
>>>>       BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>>>>       char *name;                 /* Optional non-empty unique ID */
>>>> +    char *hidden_name;          /* Backup of @name for removal transaction
>>>> +                                   action. Used for hide/unhide API. */
>>>> +    bool hidden_persistent;     /* Backup of @persistent for removal transaction
>>>> +                                   action. */
>>>>       int64_t size;               /* Size of the bitmap, in bytes */
>>>>       bool disabled;              /* Bitmap is disabled. It ignores all writes to
>>>>                                      the device */
>>>> @@ -849,3 +853,25 @@ out:
>>>>           qemu_mutex_unlock(src->mutex);
>>>>       }
>>>>   }
>>>> +
>>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    assert(!bitmap->hidden_name);
>>>> +    bitmap->hidden_name = bitmap->name;
>>>> +    bitmap->hidden_persistent = bitmap->persistent;
>>>> +    bitmap->name = NULL;
>>>> +    bitmap->persistent = false;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>> +
>>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    assert(!bitmap->name);
>>>> +    bitmap->name = bitmap->hidden_name;
>>>> +    bitmap->persistent = bitmap->hidden_persistent;
>>>> +    bitmap->hidden_name = NULL;
>>>> +    bitmap->hidden_persistent = false;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

It is used to do transactional movement of the bitmap (which is
possible in conjunction with merge command). Transactional bitmap
movement is needed in scenarios with external snapshot, when we don't
want to leave copy of the bitmap in the base image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/transaction.json |  2 ++
 blockdev.c            | 74 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index 95edb78227..da95b804aa 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -45,6 +45,7 @@
 #
 # - @abort: since 1.6
 # - @block-dirty-bitmap-add: since 2.5
+# - @block-dirty-bitmap-remove: since 4.1
 # - @block-dirty-bitmap-clear: since 2.5
 # - @block-dirty-bitmap-enable: since 4.0
 # - @block-dirty-bitmap-disable: since 4.0
@@ -61,6 +62,7 @@
   'data': {
        'abort': 'Abort',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+       'block-dirty-bitmap-remove': 'BlockDirtyBitmap',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
        'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
        'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
diff --git a/blockdev.c b/blockdev.c
index 5b3eef0d3e..0d9aa7f0a1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2135,6 +2135,46 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
                                                 errp);
 }
 
+static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
+        const char *node, const char *name, bool release,
+        BlockDriverState **bitmap_bs, Error **errp);
+
+static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
+                                              Error **errp)
+{
+    BlockDirtyBitmap *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.block_dirty_bitmap_remove.data;
+
+    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
+                                                 false, &state->bs, errp);
+    if (state->bitmap) {
+        bdrv_dirty_bitmap_hide(state->bitmap);
+    }
+}
+
+static void block_dirty_bitmap_remove_abort(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    bdrv_dirty_bitmap_unhide(state->bitmap);
+}
+
+static void block_dirty_bitmap_remove_commit(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2212,6 +2252,12 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_free_backup,
         .abort = block_dirty_bitmap_restore,
     },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_remove_prepare,
+        .commit = block_dirty_bitmap_remove_commit,
+        .abort = block_dirty_bitmap_remove_abort,
+    },
     /* Where are transactions for MIRROR, COMMIT and STREAM?
      * Although these blockjobs use transaction callbacks like the backup job,
      * these jobs do not necessarily adhere to transaction semantics.
@@ -2870,20 +2916,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
 }
 
-void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
-                                   Error **errp)
+static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
+        const char *node, const char *name, bool release,
+        BlockDriverState **bitmap_bs, Error **errp)
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
 
     bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
     if (!bitmap || !bs) {
-        return;
+        return NULL;
     }
 
     if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
                                 errp)) {
-        return;
+        return NULL;
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
@@ -2893,13 +2940,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         aio_context_acquire(aio_context);
         bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
         aio_context_release(aio_context);
+
         if (local_err != NULL) {
             error_propagate(errp, local_err);
-            return;
+            return NULL;
         }
     }
 
-    bdrv_release_dirty_bitmap(bs, bitmap);
+    if (release) {
+        bdrv_release_dirty_bitmap(bs, bitmap);
+    }
+
+    if (bitmap_bs) {
+        *bitmap_bs = bs;
+    }
+
+    return bitmap;
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+                                   Error **errp)
+{
+    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
 }
 
 /**
-- 
2.18.0

Re: [Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bitmap-remove transaction action

[John Snow]

On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/transaction.json |  2 ++
>  blockdev.c            | 74 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 70 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index 95edb78227..da95b804aa 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -45,6 +45,7 @@
>  #
>  # - @abort: since 1.6
>  # - @block-dirty-bitmap-add: since 2.5
> +# - @block-dirty-bitmap-remove: since 4.1
>  # - @block-dirty-bitmap-clear: since 2.5
>  # - @block-dirty-bitmap-enable: since 4.0
>  # - @block-dirty-bitmap-disable: since 4.0
> @@ -61,6 +62,7 @@
>    'data': {
>         'abort': 'Abort',
>         'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
> +       'block-dirty-bitmap-remove': 'BlockDirtyBitmap',
>         'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>         'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>         'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> diff --git a/blockdev.c b/blockdev.c
> index 5b3eef0d3e..0d9aa7f0a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2135,6 +2135,46 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>                                                  errp);
>  }
>  
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> +        const char *node, const char *name, bool release,
> +        BlockDriverState **bitmap_bs, Error **errp);
> +
> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
> +                                              Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.block_dirty_bitmap_remove.data;
> +
> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
> +                                                 false, &state->bs, errp);
> +    if (state->bitmap) {
> +        bdrv_dirty_bitmap_hide(state->bitmap);
> +    }
> +}
> +
> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    bdrv_dirty_bitmap_unhide(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_remove_commit(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
> +}
> +
>  static void abort_prepare(BlkActionState *common, Error **errp)
>  {
>      error_setg(errp, "Transaction aborted using Abort action");
> @@ -2212,6 +2252,12 @@ static const BlkActionOps actions[] = {
>          .commit = block_dirty_bitmap_free_backup,
>          .abort = block_dirty_bitmap_restore,
>      },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_remove_prepare,
> +        .commit = block_dirty_bitmap_remove_commit,
> +        .abort = block_dirty_bitmap_remove_abort,
> +    },
>      /* Where are transactions for MIRROR, COMMIT and STREAM?
>       * Although these blockjobs use transaction callbacks like the backup job,
>       * these jobs do not necessarily adhere to transaction semantics.
> @@ -2870,20 +2916,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>  }
>  
> -void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> -                                   Error **errp)
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> +        const char *node, const char *name, bool release,
> +        BlockDriverState **bitmap_bs, Error **errp)

Hm, why does the hide feature need to copy the persistent bit when we're
removing it here anyway?

If release is false, we still call bdrv_remove_persistent_dirty_bitmap,
yeah?

And when we go to undo it, we won't have undone the persistent removal,
right?

>  {
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *bitmap;
>  
>      bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>      if (!bitmap || !bs) {
> -        return;
> +        return NULL;
>      }
>  
>      if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
>                                  errp)) {
> -        return;
> +        return NULL;
>      }
>  
>      if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> @@ -2893,13 +2940,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>          aio_context_acquire(aio_context);
>          bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>          aio_context_release(aio_context);
> +
>          if (local_err != NULL) {
>              error_propagate(errp, local_err);
> -            return;
> +            return NULL;
>          }
>      }
>  
> -    bdrv_release_dirty_bitmap(bs, bitmap);
> +    if (release) {
> +        bdrv_release_dirty_bitmap(bs, bitmap);
> +    }
> +
> +    if (bitmap_bs) {
> +        *bitmap_bs = bs;
> +    }
> +
> +    return bitmap;
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> +                                   Error **errp)
> +{
> +    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
>  }
>  
>  /**
> 

Seems about right otherwise, though!

Re: [Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

08.06.2019 1:57, John Snow wrote:
> 
> 
> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It is used to do transactional movement of the bitmap (which is
>> possible in conjunction with merge command). Transactional bitmap
>> movement is needed in scenarios with external snapshot, when we don't
>> want to leave copy of the bitmap in the base image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/transaction.json |  2 ++
>>   blockdev.c            | 74 +++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 70 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index 95edb78227..da95b804aa 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -45,6 +45,7 @@
>>   #
>>   # - @abort: since 1.6
>>   # - @block-dirty-bitmap-add: since 2.5
>> +# - @block-dirty-bitmap-remove: since 4.1
>>   # - @block-dirty-bitmap-clear: since 2.5
>>   # - @block-dirty-bitmap-enable: since 4.0
>>   # - @block-dirty-bitmap-disable: since 4.0
>> @@ -61,6 +62,7 @@
>>     'data': {
>>          'abort': 'Abort',
>>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>> +       'block-dirty-bitmap-remove': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>          'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>> diff --git a/blockdev.c b/blockdev.c
>> index 5b3eef0d3e..0d9aa7f0a1 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2135,6 +2135,46 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>>                                                   errp);
>>   }
>>   
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>> +        const char *node, const char *name, bool release,
>> +        BlockDriverState **bitmap_bs, Error **errp);
>> +
>> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
>> +                                              Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_remove.data;
>> +
>> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
>> +                                                 false, &state->bs, errp);
>> +    if (state->bitmap) {
>> +        bdrv_dirty_bitmap_hide(state->bitmap);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    bdrv_dirty_bitmap_unhide(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_remove_commit(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2212,6 +2252,12 @@ static const BlkActionOps actions[] = {
>>           .commit = block_dirty_bitmap_free_backup,
>>           .abort = block_dirty_bitmap_restore,
>>       },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_remove_prepare,
>> +        .commit = block_dirty_bitmap_remove_commit,
>> +        .abort = block_dirty_bitmap_remove_abort,
>> +    },
>>       /* Where are transactions for MIRROR, COMMIT and STREAM?
>>        * Although these blockjobs use transaction callbacks like the backup job,
>>        * these jobs do not necessarily adhere to transaction semantics.
>> @@ -2870,20 +2916,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>       bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>>   }
>>   
>> -void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> -                                   Error **errp)
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>> +        const char *node, const char *name, bool release,
>> +        BlockDriverState **bitmap_bs, Error **errp)
> 
> Hm, why does the hide feature need to copy the persistent bit when we're
> removing it here anyway?
> 
> If release is false, we still call bdrv_remove_persistent_dirty_bitmap,
> yeah?
> 
> And when we go to undo it, we won't have undone the persistent removal,
> right?

Hm, good question. I remember there was bad thing on storing persistent bitmap,
as this code is not prepared for unnamed persistent bitmaps. Aha, I was wrong in
my previous answer, it is valid to go to storing during transaction, and this is
exactly reopen to RO. So we must drop persistence.

> 
>>   {
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>>   
>>       bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>>       if (!bitmap || !bs) {
>> -        return;
>> +        return NULL;
>>       }
>>   
>>       if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
>>                                   errp)) {
>> -        return;
>> +        return NULL;
>>       }
>>   
>>       if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> @@ -2893,13 +2940,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>           aio_context_acquire(aio_context);
>>           bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>           aio_context_release(aio_context);
>> +
>>           if (local_err != NULL) {
>>               error_propagate(errp, local_err);
>> -            return;
>> +            return NULL;
>>           }
>>       }
>>   
>> -    bdrv_release_dirty_bitmap(bs, bitmap);
>> +    if (release) {
>> +        bdrv_release_dirty_bitmap(bs, bitmap);
>> +    }
>> +
>> +    if (bitmap_bs) {
>> +        *bitmap_bs = bs;
>> +    }
>> +
>> +    return bitmap;
>> +}
>> +
>> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> +                                   Error **errp)
>> +{
>> +    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
>>   }
>>   
>>   /**
>>
> 
> Seems about right otherwise, though!
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 4/4] iotests: test bitmap moving inside 254

[Vladimir Sementsov-Ogievskiy]

Test persistent bitmap copying with and without removal of original
bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/254     | 30 +++++++++++++-
 tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 33cb80a512..05afc6d6f1 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Test external snapshot with bitmap copying.
+# Test external snapshot with bitmap copying and moving.
 #
 # Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
 #
@@ -30,6 +30,10 @@ vm = iotests.VM().add_drive(disk, opts='node-name=base')
 vm.launch()
 
 vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0')
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap1',
+           persistent=True)
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap2',
+           persistent=True)
 
 vm.hmp_qemu_io('drive0', 'write 0 512K')
 
@@ -37,16 +41,38 @@ vm.qmp_log('transaction', indent=2, actions=[
     {'type': 'blockdev-snapshot-sync',
      'data': {'device': 'drive0', 'snapshot-file': top,
               'snapshot-node-name': 'snap'}},
+
+    # copy non-persistent bitmap0
     {'type': 'block-dirty-bitmap-add',
      'data': {'node': 'snap', 'name': 'bitmap0'}},
     {'type': 'block-dirty-bitmap-merge',
      'data': {'node': 'snap', 'target': 'bitmap0',
-              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}}
+              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}},
+
+    # copy persistent bitmap1, original will be saved to base image
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'snap', 'name': 'bitmap1', 'persistent': True}},
+    {'type': 'block-dirty-bitmap-merge',
+     'data': {'node': 'snap', 'target': 'bitmap1',
+              'bitmaps': [{'node': 'base', 'name': 'bitmap1'}]}},
+
+    # move persistent bitmap1, original will be removed and not saved
+    # to base image
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'snap', 'name': 'bitmap2', 'persistent': True}},
+    {'type': 'block-dirty-bitmap-merge',
+     'data': {'node': 'snap', 'target': 'bitmap2',
+              'bitmaps': [{'node': 'base', 'name': 'bitmap2'}]}},
+    {'type': 'block-dirty-bitmap-remove',
+     'data': {'node': 'base', 'name': 'bitmap2'}}
 ], filters=[iotests.filter_qmp_testfiles])
 
 result = vm.qmp('query-block')['return'][0]
 log("query-block: device = {}, node-name = {}, dirty-bitmaps:".format(
     result['device'], result['inserted']['node-name']))
 log(result['dirty-bitmaps'], indent=2)
+log("\nbitmaps in backing image:")
+log(result['inserted']['image']['backing-image']['format-specific'] \
+    ['data']['bitmaps'], indent=2)
 
 vm.shutdown()
diff --git a/tests/qemu-iotests/254.out b/tests/qemu-iotests/254.out
index d7394cf002..d185c0532f 100644
--- a/tests/qemu-iotests/254.out
+++ b/tests/qemu-iotests/254.out
@@ -1,5 +1,9 @@
 {"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0"}}
 {"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap1", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap2", "node": "drive0", "persistent": true}}
+{"return": {}}
 {
   "execute": "transaction",
   "arguments": {
@@ -31,6 +35,55 @@
           "target": "bitmap0"
         },
         "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap1",
+          "node": "snap",
+          "persistent": true
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "bitmaps": [
+            {
+              "name": "bitmap1",
+              "node": "base"
+            }
+          ],
+          "node": "snap",
+          "target": "bitmap1"
+        },
+        "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap2",
+          "node": "snap",
+          "persistent": true
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "bitmaps": [
+            {
+              "name": "bitmap2",
+              "node": "base"
+            }
+          ],
+          "node": "snap",
+          "target": "bitmap2"
+        },
+        "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap2",
+          "node": "base"
+        },
+        "type": "block-dirty-bitmap-remove"
       }
     ]
   }
@@ -40,6 +93,24 @@
 }
 query-block: device = drive0, node-name = snap, dirty-bitmaps:
 [
+  {
+    "busy": false,
+    "count": 524288,
+    "granularity": 65536,
+    "name": "bitmap2",
+    "persistent": true,
+    "recording": true,
+    "status": "active"
+  },
+  {
+    "busy": false,
+    "count": 524288,
+    "granularity": 65536,
+    "name": "bitmap1",
+    "persistent": true,
+    "recording": true,
+    "status": "active"
+  },
   {
     "busy": false,
     "count": 524288,
@@ -50,3 +121,14 @@ query-block: device = drive0, node-name = snap, dirty-bitmaps:
     "status": "active"
   }
 ]
+
+bitmaps in backing image:
+[
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap1"
+  }
+]
-- 
2.18.0

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[John Snow]

On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is block-dirty-bitmap-remove transaction action.
> 
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 

Oh, interesting. I see why you want this now. OK, let's do it.

> Implementation itself in 03, in short:
> 
> .prepare: make bitmap unnamed and non-persistent, delete stored version
>           of the bitmap from the image
> 
> .commit: release bitmap
> 
> .abort: restore bitmap name and persistence. We don't restore bitmap
>         version in the image. It's not critical, we have in-RAM version,
>         it will be stored on shutdown
> 
> Vladimir Sementsov-Ogievskiy (4):
>   blockdev: reduce aio_context locked sections in bitmap add/remove
>   block/dirty-bitmap: add hide/unhide API
>   qapi: implement block-dirty-bitmap-remove transaction action
>   iotests: test bitmap moving inside 254
> 
>  qapi/transaction.json        |   2 +
>  include/block/dirty-bitmap.h |   2 +
>  block/dirty-bitmap.c         |  26 +++++++++
>  blockdev.c                   | 100 +++++++++++++++++++++++++++--------
>  tests/qemu-iotests/254       |  30 ++++++++++-
>  tests/qemu-iotests/254.out   |  82 ++++++++++++++++++++++++++++
>  6 files changed, 219 insertions(+), 23 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

08.06.2019 1:26, John Snow wrote:
> 
> 
> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is block-dirty-bitmap-remove transaction action.
>>
>> It is used to do transactional movement of the bitmap (which is
>> possible in conjunction with merge command). Transactional bitmap
>> movement is needed in scenarios with external snapshot, when we don't
>> want to leave copy of the bitmap in the base image.
>>
> 
> Oh, interesting. I see why you want this now. OK, let's do it.
> 


Hi John!

Hmm, could you stage it, or should I fix something? Seems I've answered all questions.
We need this for our nearest release and wanting to avoid x-vz- prefixes in the API,
I'd be very grateful if we merge it soon.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Kevin Wolf]

Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.06.2019 1:26, John Snow wrote:
> > 
> > 
> > On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> >> Hi all!
> >>
> >> Here is block-dirty-bitmap-remove transaction action.
> >>
> >> It is used to do transactional movement of the bitmap (which is
> >> possible in conjunction with merge command). Transactional bitmap
> >> movement is needed in scenarios with external snapshot, when we don't
> >> want to leave copy of the bitmap in the base image.
> >>
> > 
> > Oh, interesting. I see why you want this now. OK, let's do it.
> > 
> 
> 
> Hi John!
> 
> Hmm, could you stage it, or should I fix something? Seems I've answered all questions.
> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API,
> I'd be very grateful if we merge it soon.

I hope you won't have to do this, but in any case x-vz- isn't the right
prefix. Please read section '6. Downstream extension of QMP' in
docs/interop/qmp-spec.txt before adding your own extensions.

According to the spec, your prefix would be something like
__com.virtuozzo-...

Kevin

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

17.06.2019 19:03, Kevin Wolf wrote:
> Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 08.06.2019 1:26, John Snow wrote:
>>>
>>>
>>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> Here is block-dirty-bitmap-remove transaction action.
>>>>
>>>> It is used to do transactional movement of the bitmap (which is
>>>> possible in conjunction with merge command). Transactional bitmap
>>>> movement is needed in scenarios with external snapshot, when we don't
>>>> want to leave copy of the bitmap in the base image.
>>>>
>>>
>>> Oh, interesting. I see why you want this now. OK, let's do it.
>>>
>>
>>
>> Hi John!
>>
>> Hmm, could you stage it, or should I fix something? Seems I've answered all questions.
>> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API,
>> I'd be very grateful if we merge it soon.
> 
> I hope you won't have to do this, but in any case x-vz- isn't the right
> prefix. Please read section '6. Downstream extension of QMP' in
> docs/interop/qmp-spec.txt before adding your own extensions.
> 
> According to the spec, your prefix would be something like
> __com.virtuozzo-...
> 

Thanks for pointing to that, I thought about this some time ago when saw Red Hat prefixes..
Still x-vz- is a lot better than nothing and most probably will not intersect with future
things. However, we'll move to correct prefixes of course.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[Kevin Wolf]

Am 18.06.2019 um 09:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.06.2019 19:03, Kevin Wolf wrote:
> > Am 17.06.2019 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 08.06.2019 1:26, John Snow wrote:
> >>>
> >>>
> >>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>>> Hi all!
> >>>>
> >>>> Here is block-dirty-bitmap-remove transaction action.
> >>>>
> >>>> It is used to do transactional movement of the bitmap (which is
> >>>> possible in conjunction with merge command). Transactional bitmap
> >>>> movement is needed in scenarios with external snapshot, when we don't
> >>>> want to leave copy of the bitmap in the base image.
> >>>>
> >>>
> >>> Oh, interesting. I see why you want this now. OK, let's do it.
> >>>
> >>
> >>
> >> Hi John!
> >>
> >> Hmm, could you stage it, or should I fix something? Seems I've answered all questions.
> >> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API,
> >> I'd be very grateful if we merge it soon.
> > 
> > I hope you won't have to do this, but in any case x-vz- isn't the right
> > prefix. Please read section '6. Downstream extension of QMP' in
> > docs/interop/qmp-spec.txt before adding your own extensions.
> > 
> > According to the spec, your prefix would be something like
> > __com.virtuozzo-...
> > 
> 
> Thanks for pointing to that, I thought about this some time ago when saw Red Hat prefixes..
> Still x-vz- is a lot better than nothing and most probably will not intersect with future
> things. However, we'll move to correct prefixes of course.

Yes, I agree that x-vz- is unlikely to cause any trouble in practice,
it's just out-of-spec strictly speaking. So for anything new that you
introduce, it would be better to follow the spec.

Kevin

Re: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

[John Snow]

On 6/17/19 7:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2019 1:26, John Snow wrote:
>>
>>
>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is block-dirty-bitmap-remove transaction action.
>>>
>>> It is used to do transactional movement of the bitmap (which is
>>> possible in conjunction with merge command). Transactional bitmap
>>> movement is needed in scenarios with external snapshot, when we don't
>>> want to leave copy of the bitmap in the base image.
>>>
>>
>> Oh, interesting. I see why you want this now. OK, let's do it.
>>
> 
> 
> Hi John!
> 
> Hmm, could you stage it, or should I fix something? Seems I've answered all questions.
> We need this for our nearest release and wanting to avoid x-vz- prefixes in the API,
> I'd be very grateful if we merge it soon.
> 
> 

Hi Vladimir,

Sorry, I lost track of this thread. (In the future, if you have
something pressing like a release and I don't seem to have noticed an
email, try sending me an email directly without the word "patch" in the
subject it to get my attention, or come ping me on IRC.)


For this series:

I think this is pretty confusing, as evidenced by how we both
misunderstood what it did. So "block_dirty_bitmap_remove" now removes a
bitmap from storage but might not actually release it. That's a little
surprising, but I see why we want it.

So what really happens is:

1. Remove a bitmap from storage, but actually don't release it; then
2. hide the bitmap we're holding onto (holding the persistence and name
data aside)
3. On success, we release the bitmap.

During this process, taking the bitmap's name away and marking it as
non-persistent helps keep it from getting rewritten to disk or from
having anything else interact with it.

On Failure:
1. unhide:
	- Restore the persistent bit
	- Restore the name

So we restore the persistent bit, but we don't actually go back through
the trouble of making sure that there isn't a collision on the name. I
suppose we are guaranteed this won't happen because if a bitmap got
added, it MUST have been added AFTER this action, and if we are
aborting, it MUST have been removed before we get here.

.... phew, I think that this works, but isn't obvious that it does.


However, if we ever use "hide" or "unhide" outside of the remove API,
you might actually run into troubles where we collide on the name when
you "unhide" it, and I don't like that very much.

I feel like a "hidden" bitmap probably should still occupy namespace to
avoid this problem. But then, it isn't truly hidden from API queries and
such, and subsequent commands could interact with it... we could add a
new permissions flag, but that starts to get kind of messy.

Is this a problem of naming? do we need to "hide" bitmaps? Could we get
away with something simpler?

We could rename the "migration" bool to be something generic and then
use that.

This way, it keeps its persistence flag and name, but it won't get
flushed back out to disk or anything at the conclusion of the
transaction. This avoids the need for worrying about name collision on
"unhide", and doesn't need any new fields.

During this time, we can also mark the bitmap as BUSY which will prevent
it from being used for any operations. The ones that we could use during
this critical window are:

- BACKUP
- ADD
- CLEAR
- ENABLE
- DISABLE
- MERGE
- REMOVE

Backup, clear, enable, disable, and remove will all fail a BUSY check.
Merge will fail for either the source or the destination being BUSY.

So I think that it's possible to avoid the need for a hide() API right
now, just by using the migration bool (renamed) and the busy status.

I want to be very aware of your time constraints though, so I prepared a
mock-up on top of your patches;

https://github.com/jnsnow/qemu/commit/9b3434cc86bbd1cbd86f9fc845435d8d6883c205

If this seems agreeable to you I'll re-send and stage right away
tomorrow. If you really DO want hide() semantics we can work on those
instead.

--js