[Qemu-devel] [PULL v2 0/3] Bitmaps patches

[Qemu-devel] [PULL v2 0/3] Bitmaps patches

[John Snow]

The following changes since commit 2e68b8620637a4ee8c79b5724144b726af1e261b:

  Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.0-20190219' into staging (2019-02-18 16:20:13 +0000)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request

for you to fetch changes up to 0a6c86d024c52b1e66d4f7ec01a3bb8ea2600145:

  blockdev: acquire aio_context for bitmap add/remove (2019-02-19 17:49:43 -0500)

----------------------------------------------------------------
Pull request

----------------------------------------------------------------

Eric Blake (1):
  dirty-bitmap: Expose persistent flag to 'query-block'

John Snow (2):
  block/dirty-bitmap: Documentation and Comment fixups
  blockdev: acquire aio_context for bitmap add/remove

 block/dirty-bitmap.c       | 21 +++++++++++++++------
 blockdev.c                 | 26 ++++++++++++++++++++------
 qapi/block-core.json       | 37 ++++++++++++++++++++++++++++---------
 tests/qemu-iotests/124     |  1 +
 tests/qemu-iotests/236.out | 14 ++++++++++++++
 5 files changed, 78 insertions(+), 21 deletions(-)

-- 
2.17.2

[Qemu-devel] [PULL v2 1/3] dirty-bitmap: Expose persistent flag to 'query-block'

[John Snow]

From: Eric Blake <eblake@redhat.com>

Since qemu currently doesn't flush persistent bitmaps to disk until
shutdown (which might be MUCH later), it's useful if 'query-block'
at least shows WHICH bitmaps will (eventually) make it to persistent
storage.  Update affected iotests.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190204210512.27458-1-eblake@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c       |  1 +
 qapi/block-core.json       |  5 ++++-
 tests/qemu-iotests/124     |  1 +
 tests/qemu-iotests/236.out | 14 ++++++++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 00ea36f554..e46f72b346 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -440,6 +440,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
+        info->persistent = bm->persistent;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ee1ab7a8a2..b8c8e04d02 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -450,11 +450,14 @@
 #
 # @status: current status of the dirty bitmap (since 2.4)
 #
+# @persistent: true if the bitmap will eventually be flushed to persistent
+#              storage (since 4.0)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'status': 'DirtyBitmapStatus'} }
+           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
 
 ##
 # @Qcow2BitmapInfoFlags:
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 9f189e3b54..5aa1bf1bd6 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -350,6 +350,7 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
         self.assert_qmp(result, 'return[0]/dirty-bitmaps[0]/count', 458752)
         self.assert_qmp(result, 'return[0]/dirty-bitmaps[0]/granularity', 65536)
         self.assert_qmp(result, 'return[0]/dirty-bitmaps[0]/status', 'active')
+        self.assert_qmp(result, 'return[0]/dirty-bitmaps[0]/persistent', False)
 
         # Prepare a cluster_size=128k backup target without a backing file.
         (target, _) = bitmap0.new_target()
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index bb2d71ea5e..5006f7bca1 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -25,12 +25,14 @@ write -P0xcd 0x3ff0000 64k
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
+        "persistent": false,
         "status": "active"
       },
       {
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
+        "persistent": false,
         "status": "active"
       }
     ]
@@ -85,12 +87,14 @@ write -P0xcd 0x3ff0000 64k
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
+        "persistent": false,
         "status": "active"
       },
       {
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
+        "persistent": false,
         "status": "active"
       }
     ]
@@ -183,18 +187,21 @@ write -P0xea 0x3fe0000 64k
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
+        "persistent": false,
         "status": "disabled"
       }
     ]
@@ -247,18 +254,21 @@ write -P0xea 0x3fe0000 64k
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
+        "persistent": false,
         "status": "disabled"
       }
     ]
@@ -304,24 +314,28 @@ write -P0xea 0x3fe0000 64k
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapD",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
+        "persistent": false,
         "status": "disabled"
       },
       {
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
+        "persistent": false,
         "status": "disabled"
       }
     ]
-- 
2.17.2

[Qemu-devel] [PULL v2 2/3] block/dirty-bitmap: Documentation and Comment fixups

[John Snow]

The meaning of the states has changed subtly over time,
this should bring the understanding more in-line with the
current, actual usages.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190202011048.12343-1-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 20 ++++++++++++++------
 qapi/block-core.json | 32 ++++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index e46f72b346..c6d4acebfa 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -29,12 +29,20 @@
 #include "block/blockjob.h"
 
 /**
- * A BdrvDirtyBitmap can be in three possible states:
- * (1) successor is NULL and disabled is false: full r/w mode
- * (2) successor is NULL and disabled is true: read only mode ("disabled")
- * (3) successor is set: frozen mode.
- *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
- *     or enabled. A frozen bitmap can only abdicate() or reclaim().
+ * A BdrvDirtyBitmap can be in four possible user-visible states:
+ * (1) Active:   successor is NULL, and disabled is false: full r/w mode
+ * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
+ *               guest writes are dropped, but monitor writes are possible,
+ *               through commands like merge and clear.
+ * (3) Frozen:   successor is not NULL.
+ *               A frozen bitmap cannot be renamed, deleted, cleared, set,
+ *               enabled, merged to, etc. A frozen bitmap can only abdicate()
+ *               or reclaim().
+ *               In this state, the anonymous successor bitmap may be either
+ *               Active and recording writes from the guest (e.g. backup jobs),
+ *               but it can be Disabled and not recording writes.
+ * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
+ *               in any way from the monitor.
  */
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b8c8e04d02..2b8afbb924 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -420,17 +420,27 @@
 #
 # An enumeration of possible states that a dirty bitmap can report to the user.
 #
-# @frozen: The bitmap is currently in-use by a backup operation or block job,
-#          and is immutable.
+# @frozen: The bitmap is currently in-use by some operation and is immutable.
+#          If the bitmap was @active prior to the operation, new writes by the
+#          guest are being recorded in a temporary buffer, and will not be lost.
+#          Generally, bitmaps are cleared on successful use in an operation and
+#          the temporary buffer is committed into the bitmap. On failure, the
+#          temporary buffer is merged back into the bitmap without first
+#          clearing it.
+#          Please refer to the documentation for each bitmap-using operation,
+#          See also @blockdev-backup, @drive-backup.
 #
-# @disabled: The bitmap is currently in-use by an internal operation and is
-#            read-only. It can still be deleted.
+# @disabled: The bitmap is not currently recording new writes by the guest.
+#            This is requested explicitly via @block-dirty-bitmap-disable.
+#            It can still be cleared, deleted, or used for backup operations.
 #
 # @active: The bitmap is actively monitoring for new writes, and can be cleared,
 #          deleted, or used for backup operations.
 #
-# @locked: The bitmap is currently in-use by some operation and can not be
-#          cleared, deleted, or used for backup operations. (Since 2.12)
+# @locked: The bitmap is currently in-use by some operation and is immutable.
+#          If the bitmap was @active prior to the operation, it is still
+#          recording new writes. If the bitmap was @disabled, it is not
+#          recording new writes. (Since 2.12)
 #
 # Since: 2.4
 ##
@@ -2094,9 +2104,15 @@
 # @block-dirty-bitmap-merge:
 #
 # Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
-# The @bitmaps dirty bitmaps are unchanged.
+# Dirty bitmaps in @bitmaps will be unchanged, except if it also appears
+# as the @target bitmap. Any bits already set in @target will still be
+# set after the merge, i.e., this operation does not clear the target.
 # On error, @target is unchanged.
 #
+# The resulting bitmap will count as dirty any clusters that were dirty in any
+# of the source bitmaps. This can be used to achieve backup checkpoints, or in
+# simpler usages, to copy bitmaps.
+#
 # Returns: nothing on success
 #          If @node is not a valid block device, DeviceNotFound
 #          If any bitmap in @bitmaps or @target is not found, GenericError
@@ -2131,7 +2147,7 @@
 ##
 # @x-debug-block-dirty-bitmap-sha256:
 #
-# Get bitmap SHA256
+# Get bitmap SHA256.
 #
 # Returns: BlockDirtyBitmapSha256 on success
 #          If @node is not a valid block device, DeviceNotFound
-- 
2.17.2

[Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[John Snow]

When bitmaps are persistent, they may incur a disk read or write when bitmaps
are added or removed. For configurations like virtio-dataplane, failing to
acquire this lock will abort QEMU when disk IO occurs.

We used to acquire aio_context as part of the bitmap lookup, so re-introduce
the lock for just the cases that have an IO penalty. Commit 2119882c removed
these locks, and I failed to notice this when we committed fd5ae4cc, so this
has been broken since persistent bitmaps were introduced.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
Reported-By: Aihua Liang <aliang@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20190218233154.19303-1-jsnow@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fb18e9c975..8714ad2702 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2820,6 +2820,7 @@ 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");
@@ -2854,15 +2855,17 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         disabled = false;
     }
 
-    if (persistent &&
-        !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
-    {
-        return;
+    if (persistent) {
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
+        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
+            goto out;
+        }
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
     if (bitmap == NULL) {
-        return;
+        goto out;
     }
 
     if (disabled) {
@@ -2870,6 +2873,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+ out:
+    if (aio_context) {
+        aio_context_release(aio_context);
+    }
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
@@ -2878,6 +2885,7 @@ 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) {
@@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
     }
 
     if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
         bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
-            return;
+            goto out;
         }
     }
 
     bdrv_release_dirty_bitmap(bs, bitmap);
+ out:
+    if (aio_context) {
+        aio_context_release(aio_context);
+    }
 }
 
 /**
-- 
2.17.2

Re: [Qemu-devel] [PULL v2 0/3] Bitmaps patches

[Peter Maydell]

On Wed, 20 Feb 2019 at 18:01, John Snow <jsnow@redhat.com> wrote:
>
> The following changes since commit 2e68b8620637a4ee8c79b5724144b726af1e261b:
>
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.0-20190219' into staging (2019-02-18 16:20:13 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to 0a6c86d024c52b1e66d4f7ec01a3bb8ea2600145:
>
>   blockdev: acquire aio_context for bitmap add/remove (2019-02-19 17:49:43 -0500)
>
> ----------------------------------------------------------------
> Pull request
>
> ----------------------------------------------------------------

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[Vladimir Sementsov-Ogievskiy]

Hi!

20.02.2019 21:01, John Snow wrote:
> When bitmaps are persistent, they may incur a disk read or write when bitmaps
> are added or removed. For configurations like virtio-dataplane, failing to
> acquire this lock will abort QEMU when disk IO occurs.
> 
> We used to acquire aio_context as part of the bitmap lookup, so re-introduce
> the lock for just the cases that have an IO penalty. Commit 2119882c removed
> these locks, and I failed to notice this when we committed fd5ae4cc, so this
> has been broken since persistent bitmaps were introduced.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
> Reported-By: Aihua Liang <aliang@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-id: 20190218233154.19303-1-jsnow@redhat.com
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

[..]

>   void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> @@ -2878,6 +2885,7 @@ 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) {
> @@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>       }
>   
>       if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
> +        aio_context = bdrv_get_aio_context(bs);
> +        aio_context_acquire(aio_context);
>           bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>           if (local_err != NULL) {
>               error_propagate(errp, local_err);
> -            return;
> +            goto out;
>           }
>       }
>   
>       bdrv_release_dirty_bitmap(bs, bitmap);
> + out:
> +    if (aio_context) {
> +        aio_context_release(aio_context);
> +    }
>   }
>   
>   /**
> 

A bit late, but I have a question:

Why did you include bdrv_release_dirty_bitmap call into context-acquired section? As I can
understand from commit message, it's not actually needed?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[John Snow]

On 5/31/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> 20.02.2019 21:01, John Snow wrote:
>> When bitmaps are persistent, they may incur a disk read or write when bitmaps
>> are added or removed. For configurations like virtio-dataplane, failing to
>> acquire this lock will abort QEMU when disk IO occurs.
>>
>> We used to acquire aio_context as part of the bitmap lookup, so re-introduce
>> the lock for just the cases that have an IO penalty. Commit 2119882c removed
>> these locks, and I failed to notice this when we committed fd5ae4cc, so this
>> has been broken since persistent bitmaps were introduced.
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
>> Reported-By: Aihua Liang <aliang@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
> [..]
> 
>>   void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> @@ -2878,6 +2885,7 @@ 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) {
>> @@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>       }
>>   
>>       if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
>> +        aio_context = bdrv_get_aio_context(bs);
>> +        aio_context_acquire(aio_context);
>>           bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>           if (local_err != NULL) {
>>               error_propagate(errp, local_err);
>> -            return;
>> +            goto out;
>>           }
>>       }
>>   
>>       bdrv_release_dirty_bitmap(bs, bitmap);
>> + out:
>> +    if (aio_context) {
>> +        aio_context_release(aio_context);
>> +    }
>>   }
>>   
>>   /**
>>
> 
> A bit late, but I have a question:
> 
> Why did you include bdrv_release_dirty_bitmap call into context-acquired section? As I can
> understand from commit message, it's not actually needed?
> 

No reason beyond habit.

Re: [Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[Vladimir Sementsov-Ogievskiy]

31.05.2019 21:16, John Snow wrote:
> 
> 
> On 5/31/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi!
>>
>> 20.02.2019 21:01, John Snow wrote:
>>> When bitmaps are persistent, they may incur a disk read or write when bitmaps
>>> are added or removed. For configurations like virtio-dataplane, failing to
>>> acquire this lock will abort QEMU when disk IO occurs.
>>>
>>> We used to acquire aio_context as part of the bitmap lookup, so re-introduce
>>> the lock for just the cases that have an IO penalty. Commit 2119882c removed
>>> these locks, and I failed to notice this when we committed fd5ae4cc, so this
>>> has been broken since persistent bitmaps were introduced.
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
>>> Reported-By: Aihua Liang <aliang@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>
>> [..]
>>
>>>    void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>> @@ -2878,6 +2885,7 @@ 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) {
>>> @@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>>        }
>>>    
>>>        if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>> +        aio_context = bdrv_get_aio_context(bs);
>>> +        aio_context_acquire(aio_context);
>>>            bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>>            if (local_err != NULL) {
>>>                error_propagate(errp, local_err);
>>> -            return;
>>> +            goto out;
>>>            }
>>>        }
>>>    
>>>        bdrv_release_dirty_bitmap(bs, bitmap);
>>> + out:
>>> +    if (aio_context) {
>>> +        aio_context_release(aio_context);
>>> +    }
>>>    }
>>>    
>>>    /**
>>>
>>
>> A bit late, but I have a question:
>>
>> Why did you include bdrv_release_dirty_bitmap call into context-acquired section? As I can
>> understand from commit message, it's not actually needed?
>>
> 
> No reason beyond habit.
> 

Ok thanks. I'm now working (ok worked, I'd better go home now) on transaction action for bitmap-remove.
It occurs, that we need it to have an ability to _move_, not _copy_ bitmaps in transaction (we of course
can copy in transaction and then remove after transaction, but it doesn't work if bitmap is persistent and
after transaction node is RO). So, I have to refactor this code anyway, and therefore I've asked for this
thing which I want to refactor too.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PULL v2 3/3] blockdev: acquire aio_context for bitmap add/remove

[John Snow]

On 5/31/19 3:01 PM, Vladimir Sementsov-Ogievskiy wrote:
> 31.05.2019 21:16, John Snow wrote:
>>
>>
>> On 5/31/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi!
>>>
>>> 20.02.2019 21:01, John Snow wrote:
>>>> When bitmaps are persistent, they may incur a disk read or write when bitmaps
>>>> are added or removed. For configurations like virtio-dataplane, failing to
>>>> acquire this lock will abort QEMU when disk IO occurs.
>>>>
>>>> We used to acquire aio_context as part of the bitmap lookup, so re-introduce
>>>> the lock for just the cases that have an IO penalty. Commit 2119882c removed
>>>> these locks, and I failed to notice this when we committed fd5ae4cc, so this
>>>> has been broken since persistent bitmaps were introduced.
>>>>
>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1672010
>>>> Reported-By: Aihua Liang <aliang@redhat.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Message-id: 20190218233154.19303-1-jsnow@redhat.com
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>
>>> [..]
>>>
>>>>    void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>>> @@ -2878,6 +2885,7 @@ 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) {
>>>> @@ -2892,14 +2900,20 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>>>        }
>>>>    
>>>>        if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
>>>> +        aio_context = bdrv_get_aio_context(bs);
>>>> +        aio_context_acquire(aio_context);
>>>>            bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>>>>            if (local_err != NULL) {
>>>>                error_propagate(errp, local_err);
>>>> -            return;
>>>> +            goto out;
>>>>            }
>>>>        }
>>>>    
>>>>        bdrv_release_dirty_bitmap(bs, bitmap);
>>>> + out:
>>>> +    if (aio_context) {
>>>> +        aio_context_release(aio_context);
>>>> +    }
>>>>    }
>>>>    
>>>>    /**
>>>>
>>>
>>> A bit late, but I have a question:
>>>
>>> Why did you include bdrv_release_dirty_bitmap call into context-acquired section? As I can
>>> understand from commit message, it's not actually needed?
>>>
>>
>> No reason beyond habit.
>>
> 
> Ok thanks. I'm now working (ok worked, I'd better go home now) on transaction action for bitmap-remove.
> It occurs, that we need it to have an ability to _move_, not _copy_ bitmaps in transaction (we of course
> can copy in transaction and then remove after transaction, but it doesn't work if bitmap is persistent and
> after transaction node is RO). So, I have to refactor this code anyway, and therefore I've asked for this
> thing which I want to refactor too.
> 

Safe travels home!

(Why transactions for remove? Just convenience for batching actions?
I'll find out on Monday.)

Have a good weekend!

--js