[Qemu-block] [PATCH v6 20/21] iotests: add incremental backup failure recovery test

[John Snow]

Test the failure case for incremental backups.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/124 | 57 ++
 tests/qemu-iotests/124.out |  4 ++--
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 5c3b434..95f6de5 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -240,6 +240,63 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 self.check_backups()
 
 
+def test_incremental_failure(self):
+'''Test: Verify backups made after a failure are correct.
+
+Simulate a failure during an incremental backup block job,
+emulate additional writes, then create another incremental backup
+afterwards and verify that the backup created is correct.
+'''
+
+# Create a blkdebug interface to this img as 'drive1',
+# but don't actually create a new image.
+drive1 = self.add_node('drive1', self.drives[0]['fmt'],
+   path=self.drives[0]['file'],
+   backup=self.drives[0]['backup'])
+result = self.vm.qmp('blockdev-add', options={
+'id': drive1['id'],
+'driver': drive1['fmt'],
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': drive1['file']
+},
+'set-state': [{
+'event': 'flush_to_disk',
+'state': 1,
+'new_state': 2
+}],
+'inject-error': [{
+'event': 'read_aio',
+'errno': 5,
+'state': 2,
+'immediately': False,
+'once': True
+}],
+}
+})
+self.assert_qmp(result, 'return', {})
+
+self.create_anchor_backup(self.drives[0])
+self.add_bitmap('bitmap0', drive1)
+# Note: at this point, during a normal execution,
+# Assume that the VM resumes and begins issuing IO requests here.
+
+self.hmp_io_writes(drive1['id'], (('0xab', 0, 512),
+  ('0xfe', '16M', '256k'),
+  ('0x64', '32736k', '64k')))
+
+result = self.create_incremental(validate=False)
+self.assertFalse(result)
+self.hmp_io_writes(drive1['id'], (('0x9a', 0, 512),
+  ('0x55', '8M', '352k'),
+  ('0x78', '15872k', '1M')))
+self.create_incremental()
+self.vm.shutdown()
+self.check_backups()
+
+
 def test_sync_dirty_bitmap_missing(self):
 self.assert_no_active_block_jobs()
 self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 8d7e996..89968f3 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-...
+
 --
-Ran 3 tests
+Ran 4 tests
 
 OK
-- 
2.1.0

[Qemu-block] [PATCH v2.5 00/10] block: incremental backup transactions

[John Snow]

I am not prepared to send a v3 on this, primarily because I am still 
waffling on whether or not to do the code motion patch that is present 
in patch #8 of v2 of this series.


However, for the purposes of testing, reviewers may find it convenient 
to have a new version of this series that applies cleanly in concert 
with v6 of the transactionless series, so I am presenting an informal 
"version 2.5" of this series.


Patches are available on github: 
https://github.com/jnsnow/qemu/commits/incremental-transactions


And here is the v2.5 patchset diff:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, 
respectively


001/10:[0002] [FC] 'qapi: Add transaction support to block-dirty-bitmap 
operations'

002/10:[0021] [FC] 'iotests: add transactional incremental backup test'
003/10:[] [--] 'block: rename BlkTransactionState and BdrvActionOps'
004/10:[] [--] 'block: re-add BlkTransactionState'
005/10:[0010] [FC] 'block: add transactional callbacks feature'
006/10:[] [--] 'block: add refcount to Job object'
007/10:[] [-C] 'block: add delayed bitmap successor cleanup'
008/10:[0051] [FC] 'qmp: Add an implementation wrapper for qmp_drive_backup'
009/10:[0001] [FC] 'block: drive_backup transaction callback support'
010/10:[0033] [FC] 'iotests: 124 - transactional failure test'

01: Removed "Not yet implemented" line from bitmaps.md
02: Rebase fallout
05: Removed superfluous deletion loop pointed out by Max
08: Fallout from removing the code motion patch,
new forward declaration.
09: Fallout from removing the code motion patch,
new forward declaration.
10: Fallout from the rebase.


There are some more changes that need to happen to #10, primarily a bit 
more refactoring after the changes made to how images are checked.


On 03/27/2015 03:19 PM, John Snow wrote:

requires: 1426879023-18151-1-git-send-email-js...@redhat.com
   "[PATCH v4 00/20] block: transactionless incremental backup series"

This series adds support for incremental backup primitives in QMP
transactions. It requires my transactionless incremental backup series,
currently at v4.

Patch 1 adds basic support for add and clear transactions.
Patch 2 tests this basic support.
Patches 3-4 refactor transactions a little bit, to add clarity.
Patch 5 adds the framework for error scenarios where only
 some jobs that were launched by a transaction complete successfully,
 and we need to perform context-sensitive cleanup after the transaction
 itself has already completed.
Patches 6-7 add necessary bookkeeping information to backup job
 data structures to take advantage of this new feature.
Patch 8 just moves code.
Patch 9 modifies qmp_drive_backup to support the new feature.
Patch 10 implements the new feature for drive_backup transaction actions.
Patch 11 tests the new feature.

Lingering questions:
  - Is it worth it to add a post-transaction completion event to QMP that
signifies all jobs launched by the transaction have completed? This
would be of primary interest to libvirt, in particular, but only as
a convenience feature.

Thank you,
--John Snow

v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/11:[0009] [FC] 'qapi: Add transaction support to block-dirty-bitmap 
operations'
002/11:[0036] [FC] 'iotests: add transactional incremental backup test'
003/11:[down] 'blockdev: rename BlkTransactionState and BdrvActionOps'
004/11:[down] 'block: re-add BlkTransactionState'
005/11:[0274] [FC] 'block: add transactional callbacks feature'
006/11:[0004] [FC] 'block: add refcount to Job object'
007/11:[0048] [FC] 'block: add delayed bitmap successor cleanup'
008/11:[down] 'block: move transactions beneath qmp interfaces'
009/11:[0084] [FC] 'qmp: Add an implementation wrapper for qmp_drive_backup'
010/11:[0050] [FC] 'block: drive_backup transaction callback support'
011/11:[0004] [FC] 'iotests: 124 - transactional failure test'

  01: Fixed indentation.
  Fixed QMP commands to behave with new bitmap_lookup from
transactionless-v4.
  2.3 --> 2.4.
  02: Folded in improvements to qemu-iotest 124 from transactional-v1.
  03: NEW
  04: NEW
  05: A lot:
  Don't delete the comma in the transaction actions config
  use g_new0 instead of g_malloc0
  Phrasing: "retcode" --> "Return code"
  Use GCC attributes to mark functions as unused until future patches.
  Added some data structure documentation.
  Many structure and function renames, hopefully to improve readability.
  Use just one list for all Actions instead of two separate lists.
  Remove ActionState from the list upon deletion/decref
  And many other small tweaks.
  0

[Qemu-block] [PATCH v6 19/21] iotests: add simple incremental backup case

[John Snow]

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/124 | 174 +++--
 tests/qemu-iotests/124.out |   4 +-
 2 files changed, 172 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 85675ec..5c3b434 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -29,6 +29,51 @@ def io_write_patterns(img, patterns):
 iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
 
 
+def try_remove(img):
+try:
+os.remove(img)
+except OSError:
+pass
+
+
+class Bitmap:
+def __init__(self, name, drive):
+self.name = name
+self.drive = drive
+self.num = 0
+self.backups = list()
+
+def base_target(self):
+return (self.drive['backup'], None)
+
+def new_target(self, num=None):
+if num is None:
+num = self.num
+self.num = num + 1
+base = os.path.join(iotests.test_dir,
+"%s.%s." % (self.drive['id'], self.name))
+suff = "%i.%s" % (num, self.drive['fmt'])
+target = base + "inc" + suff
+reference = base + "ref" + suff
+self.backups.append((target, reference))
+return (target, reference)
+
+def last_target(self):
+if self.backups:
+return self.backups[-1]
+return self.base_target()
+
+def del_target(self):
+for image in self.backups.pop():
+try_remove(image)
+self.num -= 1
+
+def cleanup(self):
+for backup in self.backups:
+for image in backup:
+try_remove(image)
+
+
 class TestIncrementalBackup(iotests.QMPTestCase):
 def setUp(self):
 self.bitmaps = list()
@@ -73,6 +118,128 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 iotests.qemu_img('create', '-f', fmt, img, size)
 self.files.append(img)
 
+
+def do_qmp_backup(self, error='Input/output error', **kwargs):
+res = self.vm.qmp('drive-backup', **kwargs)
+self.assert_qmp(res, 'return', {})
+
+event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
+   match={'data': {'device': 
kwargs['device']}})
+self.assertIsNotNone(event)
+
+try:
+failure = self.dictpath(event, 'data/error')
+except AssertionError:
+# Backup succeeded.
+self.assert_qmp(event, 'data/offset', event['data']['len'])
+return True
+else:
+# Backup failed.
+self.assert_qmp(event, 'data/error', error)
+return False
+
+
+def create_anchor_backup(self, drive=None):
+if drive is None:
+drive = self.drives[-1]
+res = self.do_qmp_backup(device=drive['id'], sync='full',
+ format=drive['fmt'], target=drive['backup'])
+self.assertTrue(res)
+self.files.append(drive['backup'])
+return drive['backup']
+
+
+def make_reference_backup(self, bitmap=None):
+if bitmap is None:
+bitmap = self.bitmaps[-1]
+_, reference = bitmap.last_target()
+res = self.do_qmp_backup(device=bitmap.drive['id'], sync='full',
+ format=bitmap.drive['fmt'], target=reference)
+self.assertTrue(res)
+
+
+def add_bitmap(self, name, drive):
+bitmap = Bitmap(name, drive)
+self.bitmaps.append(bitmap)
+result = self.vm.qmp('block-dirty-bitmap-add', node=drive['id'],
+ name=bitmap.name)
+self.assert_qmp(result, 'return', {})
+return bitmap
+
+
+def prepare_backup(self, bitmap=None, parent=None):
+if bitmap is None:
+bitmap = self.bitmaps[-1]
+if parent is None:
+parent, _ = bitmap.last_target()
+
+target, _ = bitmap.new_target()
+self.img_create(target, bitmap.drive['fmt'], parent=parent)
+return target
+
+
+def create_incremental(self, bitmap=None, parent=None,
+   parentFormat=None, validate=True):
+if bitmap is None:
+bitmap = self.bitmaps[-1]
+if parent is None:
+parent, _ = bitmap.last_target()
+
+target = self.prepare_backup(bitmap, parent)
+res = self.do_qmp_backup(device=bitmap.drive['id'],
+ sync='dirty-bitmap', bitmap=bitmap.name,
+ format=bitmap.drive['fmt'], target=target,
+ mode='existing')
+if not res:
+bitmap.del_target();
+self.assertFalse(validate)
+else:
+self.make_reference_backup(bitmap)
+return res
+
+
+def check_backups(self):
+for bitmap in self.bitmaps:
+for incremental, reference in bitmap.backups:
+self.assertTrue(iotests.compare_images(incremental, refe

[Qemu-block] [PATCH v6 18/21] iotests: add QMP event waiting queue

[John Snow]

A filter is added to allow callers to request very specific
events to be pulled from the event queue, while leaving undesired
events still in the stream.

This allows us to poll for completion data for multiple asynchronous
events in any arbitrary order.

A new timeout context is added to the qmp pull_event method's
wait parameter to allow tests to fail if they do not complete
within some expected period of time.

Also fixed is a bug in qmp.pull_event where we try to retrieve an event
from an empty list if we attempt to retrieve an event with wait=False
but no events have occurred.

Signed-off-by: John Snow 
---
 scripts/qmp/qmp.py| 95 +--
 tests/qemu-iotests/iotests.py | 38 +
 2 files changed, 103 insertions(+), 30 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 20b6ec7..1d38e3e 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -21,6 +21,9 @@ class QMPConnectError(QMPError):
 class QMPCapabilitiesError(QMPError):
 pass
 
+class QMPTimeoutError(QMPError):
+pass
+
 class QEMUMonitorProtocol:
 def __init__(self, address, server=False):
 """
@@ -72,6 +75,44 @@ class QEMUMonitorProtocol:
 
 error = socket.error
 
+def __get_events(self, wait=False):
+"""
+Check for new events in the stream and cache them in __events.
+
+@param wait (bool): block until an event is available.
+@param wait (float): If wait is a float, treat it as a timeout value.
+
+@raise QMPTimeoutError: If a timeout float is provided and the timeout
+period elapses.
+@raise QMPConnectError: If wait is True but no events could be 
retrieved
+or if some other error occurred.
+"""
+
+# Check for new events regardless and pull them into the cache:
+self.__sock.setblocking(0)
+try:
+self.__json_read()
+except socket.error, err:
+if err[0] == errno.EAGAIN:
+# No data available
+pass
+self.__sock.setblocking(1)
+
+# Wait for new events, if needed.
+# if wait is 0.0, this means "no wait" and is also implicitly false.
+if not self.__events and wait:
+if isinstance(wait, float):
+self.__sock.settimeout(wait)
+try:
+ret = self.__json_read(only_event=True)
+except socket.timeout:
+raise QMPTimeoutError("Timeout waiting for event")
+except:
+raise QMPConnectError("Error while reading from socket")
+if ret is None:
+raise QMPConnectError("Error while reading from socket")
+self.__sock.settimeout(None)
+
 def connect(self, negotiate=True):
 """
 Connect to the QMP Monitor and perform capabilities negotiation.
@@ -140,43 +181,37 @@ class QEMUMonitorProtocol:
 """
 Get and delete the first available QMP event.
 
-@param wait: block until an event is available (bool)
+@param wait (bool): block until an event is available.
+@param wait (float): If wait is a float, treat it as a timeout value.
+
+@raise QMPTimeoutError: If a timeout float is provided and the timeout
+period elapses.
+@raise QMPConnectError: If wait is True but no events could be 
retrieved
+or if some other error occurred.
+
+@return The first available QMP event, or None.
 """
-self.__sock.setblocking(0)
-try:
-self.__json_read()
-except socket.error, err:
-if err[0] == errno.EAGAIN:
-# No data available
-pass
-self.__sock.setblocking(1)
-if not self.__events and wait:
-self.__json_read(only_event=True)
-event = self.__events[0]
-del self.__events[0]
-return event
+self.__get_events(wait)
+
+if self.__events:
+return self.__events.pop(0)
+return None
 
 def get_events(self, wait=False):
 """
 Get a list of available QMP events.
 
-@param wait: block until an event is available (bool)
+@param wait (bool): block until an event is available.
+@param wait (float): If wait is a float, treat it as a timeout value.
+
+@raise QMPTimeoutError: If a timeout float is provided and the timeout
+period elapses.
+@raise QMPConnectError: If wait is True but no events could be 
retrieved
+or if some other error occurred.
+
+@return The list of available QMP events.
 """
-self.__sock.setblocking(0)
-try:
-self.__json_read()
-except socket.error, err:
-if err[0] == errno.EAGAIN:
-# N

[Qemu-block] [PATCH v6 16/21] hbitmap: truncate tests

[John Snow]

The general approach is to set bits close to the boundaries of
where we are truncating and ensure that everything appears to
have gone OK.

We test growing and shrinking by different amounts:
- Less than the granularity
- Less than the granularity, but across a boundary
- Less than sizeof(unsigned long)
- Less than sizeof(unsigned long), but across a ulong boundary
- More than sizeof(unsigned long)

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 tests/test-hbitmap.c | 255 +++
 1 file changed, 255 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 8c902f2..9f41b5f 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -11,6 +11,8 @@
 
 #include 
 #include 
+#include 
+#include 
 #include "qemu/hbitmap.h"
 
 #define LOG_BITS_PER_LONG  (BITS_PER_LONG == 32 ? 5 : 6)
@@ -23,6 +25,7 @@ typedef struct TestHBitmapData {
 HBitmap   *hb;
 unsigned long *bits;
 size_t size;
+size_t old_size;
 intgranularity;
 } TestHBitmapData;
 
@@ -91,6 +94,44 @@ static void hbitmap_test_init(TestHBitmapData *data,
 }
 }
 
+static inline size_t hbitmap_test_array_size(size_t bits)
+{
+size_t n = (bits + BITS_PER_LONG - 1) / BITS_PER_LONG;
+return n ? n : 1;
+}
+
+static void hbitmap_test_truncate_impl(TestHBitmapData *data,
+   size_t size)
+{
+size_t n;
+size_t m;
+data->old_size = data->size;
+data->size = size;
+
+if (data->size == data->old_size) {
+return;
+}
+
+n = hbitmap_test_array_size(size);
+m = hbitmap_test_array_size(data->old_size);
+data->bits = g_realloc(data->bits, sizeof(unsigned long) * n);
+if (n > m) {
+memset(&data->bits[m], 0x00, sizeof(unsigned long) * (n - m));
+}
+
+/* If we shrink to an uneven multiple of sizeof(unsigned long),
+ * scrub the leftover memory. */
+if (data->size < data->old_size) {
+m = size % (sizeof(unsigned long) * 8);
+if (m) {
+unsigned long mask = (1ULL << m) - 1;
+data->bits[n-1] &= mask;
+}
+}
+
+hbitmap_truncate(data->hb, size);
+}
+
 static void hbitmap_test_teardown(TestHBitmapData *data,
   const void *unused)
 {
@@ -369,6 +410,198 @@ static void test_hbitmap_iter_granularity(TestHBitmapData 
*data,
 g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
 }
 
+static void hbitmap_test_set_boundary_bits(TestHBitmapData *data, ssize_t diff)
+{
+size_t size = data->size;
+
+/* First bit */
+hbitmap_test_set(data, 0, 1);
+if (diff < 0) {
+/* Last bit in new, shortened map */
+hbitmap_test_set(data, size + diff - 1, 1);
+
+/* First bit to be truncated away */
+hbitmap_test_set(data, size + diff, 1);
+}
+/* Last bit */
+hbitmap_test_set(data, size - 1, 1);
+if (data->granularity == 0) {
+hbitmap_test_check_get(data);
+}
+}
+
+static void hbitmap_test_check_boundary_bits(TestHBitmapData *data)
+{
+size_t size = MIN(data->size, data->old_size);
+
+if (data->granularity == 0) {
+hbitmap_test_check_get(data);
+hbitmap_test_check(data, 0);
+} else {
+/* If a granularity was set, note that every distinct
+ * (bit >> granularity) value that was set will increase
+ * the bit pop count by 2^granularity, not just 1.
+ *
+ * The hbitmap_test_check facility does not currently tolerate
+ * non-zero granularities, so test the boundaries and the population
+ * count manually.
+ */
+g_assert(hbitmap_get(data->hb, 0));
+g_assert(hbitmap_get(data->hb, size - 1));
+g_assert_cmpint(2 << data->granularity, ==, hbitmap_count(data->hb));
+}
+}
+
+/* Generic truncate test. */
+static void hbitmap_test_truncate(TestHBitmapData *data,
+  size_t size,
+  ssize_t diff,
+  int granularity)
+{
+hbitmap_test_init(data, size, granularity);
+hbitmap_test_set_boundary_bits(data, diff);
+hbitmap_test_truncate_impl(data, size + diff);
+hbitmap_test_check_boundary_bits(data);
+}
+
+static void test_hbitmap_truncate_nop(TestHBitmapData *data,
+  const void *unused)
+{
+hbitmap_test_truncate(data, L2, 0, 0);
+}
+
+/**
+ * Grow by an amount smaller than the granularity, without crossing
+ * a granularity alignment boundary. Effectively a NOP.
+ */
+static void test_hbitmap_truncate_grow_negligible(TestHBitmapData *data,
+  const void *unused)
+{
+size_t size = L2 - 1;
+size_t diff = 1;
+int granularity = 1;
+
+hbitmap_test_truncate(data, size, diff, granularity);
+}
+
+/**
+ * Shrink by an amount smaller than the granularity, without crossi

[Qemu-block] [PATCH v6 14/21] block: Ensure consistent bitmap function prototypes

[John Snow]

We often don't need the BlockDriverState for functions
that operate on bitmaps. Remove it.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 block.c   | 13 ++---
 block/backup.c|  2 +-
 block/mirror.c| 26 ++
 blockdev.c|  2 +-
 include/block/block.h | 11 +--
 migration/block.c |  7 +++
 6 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index 30c8568..735acff 100644
--- a/block.c
+++ b/block.c
@@ -5460,7 +5460,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
*bs, const char *name)
 return NULL;
 }
 
-void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
@@ -5629,7 +5629,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
 BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
 BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-info->count = bdrv_get_dirty_count(bs, bm);
+info->count = bdrv_get_dirty_count(bm);
 info->granularity = bdrv_dirty_bitmap_granularity(bm);
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
@@ -5676,20 +5676,19 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap 
*bitmap)
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
 
-void bdrv_dirty_iter_init(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
 hbitmap_iter_init(hbi, bitmap->bitmap, 0);
 }
 
-void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
-void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
@@ -5735,7 +5734,7 @@ void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
 hbitmap_iter_init(hbi, hbi->hb, offset);
 }
 
-int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->bitmap);
 }
diff --git a/block/backup.c b/block/backup.c
index e77f7e8..a297df6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -284,7 +284,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 
 granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
 clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
-bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
 
 /* Find the next dirty sector(s) */
 while ((sector = hbitmap_iter_next(&hbi)) != -1) {
diff --git a/block/mirror.c b/block/mirror.c
index f89eccf..dcd6f65 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -125,11 +125,9 @@ static void mirror_write_complete(void *opaque, int ret)
 MirrorOp *op = opaque;
 MirrorBlockJob *s = op->s;
 if (ret < 0) {
-BlockDriverState *source = s->common.bs;
 BlockErrorAction action;
 
-bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
-  op->nb_sectors);
+bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
 action = mirror_error_action(s, false, -ret);
 if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
 s->ret = ret;
@@ -143,11 +141,9 @@ static void mirror_read_complete(void *opaque, int ret)
 MirrorOp *op = opaque;
 MirrorBlockJob *s = op->s;
 if (ret < 0) {
-BlockDriverState *source = s->common.bs;
 BlockErrorAction action;
 
-bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
-  op->nb_sectors);
+bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
 action = mirror_error_action(s, true, -ret);
 if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
 s->ret = ret;
@@ -170,10 +166,9 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 
 s->sector_num = hbitmap_iter_next(&s->hbi);
 if (s->sector_num < 0) {
-bdrv_dirty_iter_init(source, s->dirty_bitmap, &s->hbi);
+bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
 s->sector_num = hbitmap_iter_next(&s->hbi);
-trace_mirror_restart_iter(s,
-  bdrv_get_dirty_count(source, 
s->dirty_bit

[Qemu-block] [PATCH v6 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[John Snow]

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
---
 block.c   |   9 +++
 block/backup.c| 155 +++---
 block/mirror.c|   4 ++
 blockdev.c|  18 +-
 hmp.c |   3 +-
 include/block/block.h |   1 +
 include/block/block_int.h |   2 +
 qapi/block-core.json  |  14 +++--
 qmp-commands.hx   |   8 ++-
 9 files changed, 181 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 8f08b6e..185cd7f 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
int64_t cur_sector,
 }
 }
 
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+assert(hbi->hb);
+hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
 int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..e77f7e8 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
 BlockJob common;
 BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
 MirrorSyncMode sync_mode;
 RateLimit limit;
 BlockdevOnError on_source_error;
@@ -242,6 +244,91 @@ static void backup_complete(BlockJob *job, void *opaque)
 g_free(data);
 }
 
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+  job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+bool error_is_read;
+int ret = 0;
+int clusters_per_iter;
+uint32_t granularity;
+int64_t sector;
+int64_t cluster;
+int64_t end;
+int64_t last_cluster = -1;
+BlockDriverState *bs = job->common.bs;
+HBitmapIter hbi;
+
+granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
+bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+/* Find the next dirty sector(s) */
+while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped */
+if (cluster != last_cluster + 1) {
+job->common.offset += ((cluster - last_cluster - 1) *
+   BACKUP_CLUSTER_SIZE);
+}
+
+for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
+do {
+if (yield_and_check(job)) {
+return ret;
+}
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER, 
&error_is_read);
+if ((ret < 0) &&
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+}
+} while (ret < 0);
+}
+
+/* If the bitmap granularity is smaller than the backup granularity,
+ * we need to advance the iterator pointer to the next cluster. */
+if (granularity < BACKUP_CLUSTER_SIZE) {
+bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+}
+
+last_cluster = cluster - 1;
+}
+
+/* Play some final catchup with the progress meter */
+end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
+if (last_cluster + 1 < end) {
+job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
+}
+
+return ret;
+}
+
 static void coroutine_fn backup_run(void *opaque)
 {
 BackupBlockJob *job = opaque;
@@ -259,8 +346,7 @@ static void coroutine_fn backup_run(void *opaque)
 qemu_co_rwlock_init(&job->flush_rwlock);
 
 start = 0;
-end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE,
-   BACKUP_SECTORS_PER_CLUSTER);
+end = DIV_ROUND_UP(job->common.len, BACKUP_CL

[Qemu-block] [PATCH v6 21/21] iotests: add incremental backup granularity tests

[John Snow]

Test what happens if you fiddle with the granularity.

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/124 | 58 +-
 tests/qemu-iotests/124.out |  4 ++--
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 95f6de5..3ee78cd 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -158,11 +158,11 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 self.assertTrue(res)
 
 
-def add_bitmap(self, name, drive):
+def add_bitmap(self, name, drive, **kwargs):
 bitmap = Bitmap(name, drive)
 self.bitmaps.append(bitmap)
 result = self.vm.qmp('block-dirty-bitmap-add', node=drive['id'],
- name=bitmap.name)
+ name=bitmap.name, **kwargs)
 self.assert_qmp(result, 'return', {})
 return bitmap
 
@@ -212,16 +212,9 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 self.vm.hmp_qemu_io(drive, 'flush')
 
 
-def test_incremental_simple(self):
-'''
-Test: Create and verify three incremental backups.
-
-Create a bitmap and a full backup before VM execution begins,
-then create a series of three incremental backups "during execution,"
-i.e.; after IO requests begin modifying the drive.
-'''
+def do_incremental_simple(self, **kwargs):
 self.create_anchor_backup()
-self.add_bitmap('bitmap0', self.drives[0])
+self.add_bitmap('bitmap0', self.drives[0], **kwargs)
 
 # Sanity: Create a "hollow" incremental backup
 self.create_incremental()
@@ -240,6 +233,37 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 self.check_backups()
 
 
+def test_incremental_simple(self):
+'''
+Test: Create and verify three incremental backups.
+
+Create a bitmap and a full backup before VM execution begins,
+then create a series of three incremental backups "during execution,"
+i.e.; after IO requests begin modifying the drive.
+'''
+return self.do_incremental_simple()
+
+
+def test_small_granularity(self):
+'''
+Test: Create and verify backups made with a small granularity bitmap.
+
+Perform the same test as test_incremental_simple, but with a 
granularity
+of only 32KiB instead of the present default of 64KiB.
+'''
+return self.do_incremental_simple(granularity=32768)
+
+
+def test_large_granularity(self):
+'''
+Test: Create and verify backups made with a large granularity bitmap.
+
+Perform the same test as test_incremental_simple, but with a 
granularity
+of 128KiB instead of the present default of 64KiB.
+'''
+return self.do_incremental_simple(granularity=131072)
+
+
 def test_incremental_failure(self):
 '''Test: Verify backups made after a failure are correct.
 
@@ -315,6 +339,18 @@ class TestIncrementalBackup(iotests.QMPTestCase):
 self.assert_qmp(result, 'error/class', 'GenericError')
 
 
+def test_sync_dirty_bitmap_bad_granularity(self):
+'''
+Test: Test what happens if we provide an improper granularity.
+
+The granularity must always be a power of 2.
+'''
+self.assert_no_active_block_jobs()
+self.assertRaises(AssertionError, self.add_bitmap,
+  'bitmap0', self.drives[0],
+  granularity=64000)
+
+
 def tearDown(self):
 self.vm.shutdown()
 for bitmap in self.bitmaps:
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 89968f3..2f7d390 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-
+...
 --
-Ran 4 tests
+Ran 7 tests
 
 OK
-- 
2.1.0

[Qemu-block] [PATCH v6 13/21] block: add BdrvDirtyBitmap documentation

[John Snow]

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 block.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index ca73a6a..30c8568 100644
--- a/block.c
+++ b/block.c
@@ -60,11 +60,11 @@
  * or enabled. A frozen bitmap can only abdicate() or reclaim().
  */
 struct BdrvDirtyBitmap {
-HBitmap *bitmap;
-BdrvDirtyBitmap *successor;
-int64_t size;
-char *name;
-bool disabled;
+HBitmap *bitmap;/* Dirty sector bitmap implementation */
+BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+char *name; /* Optional non-empty unique ID */
+int64_t size;   /* Size of the bitmap (Number of sectors) */
+bool disabled;  /* Bitmap is read-only */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
-- 
2.1.0

[Qemu-block] [PATCH v6 09/21] block: Add bitmap successors

[John Snow]

A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
be created just prior to a sensitive operation (e.g. Incremental Backup)
that can either succeed or fail, but during the course of which we still
want a bitmap tracking writes.

On creating a successor, we "freeze" the parent bitmap which prevents
its deletion, enabling, anonymization, or creating a bitmap with the
same name.

On success, the parent bitmap can "abdicate" responsibility to the
successor, which will inherit its name. The successor will have been
tracking writes during the course of the backup operation. The parent
will be safely deleted.

On failure, we can "reclaim" the successor from the parent, unifying
them such that the resulting bitmap describes all writes occurring since
the last successful backup, for instance. Reclamation will thaw the
parent, but not explicitly re-enable it.

BdrvDirtyBitmap operations that target a single bitmap are protected
by assertions that the bitmap is not frozen and/or disabled.

BdrvDirtyBitmap operations that target a group of bitmaps, such as
bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
conditional instead.

Internal functions that enable/disable dirty bitmaps have assertions
added to them to prevent modifying frozen bitmaps.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
---
 block.c   | 104 +-
 blockdev.c|   7 
 include/block/block.h |  10 +
 qapi/block-core.json  |   1 +
 4 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index db742a9..8f08b6e 100644
--- a/block.c
+++ b/block.c
@@ -51,8 +51,17 @@
 #include 
 #endif
 
+/**
+ * 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().
+ */
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
+BdrvDirtyBitmap *successor;
 char *name;
 bool disabled;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5452,6 +5461,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
*bs, const char *name)
 
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
+assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
 }
@@ -5487,9 +5497,98 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 return bitmap;
 }
 
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->successor;
+}
+
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
-return !bitmap->disabled;
+return !(bitmap->disabled || bitmap->successor);
+}
+
+/**
+ * Create a successor bitmap destined to replace this bitmap after an 
operation.
+ * Requires that the bitmap is not frozen and has no successor.
+ */
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+   BdrvDirtyBitmap *bitmap, Error **errp)
+{
+uint64_t granularity;
+BdrvDirtyBitmap *child;
+
+if (bdrv_dirty_bitmap_frozen(bitmap)) {
+error_setg(errp, "Cannot create a successor for a bitmap that is "
+   "currently frozen");
+return -1;
+}
+assert(!bitmap->successor);
+
+/* Create an anonymous successor */
+granularity = bdrv_dirty_bitmap_granularity(bitmap);
+child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+if (!child) {
+return -1;
+}
+
+/* Successor will be on or off based on our current state. */
+child->disabled = bitmap->disabled;
+
+/* Install the successor and freeze the parent */
+bitmap->successor = child;
+return 0;
+}
+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * delete the old bitmap, and return a handle to the new bitmap.
+ */
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap,
+Error **errp)
+{
+char *name;
+BdrvDirtyBitmap *successor = bitmap->successor;
+
+if (successor == NULL) {
+error_setg(errp, "Cannot relinquish control if "
+   "there's no successor present");
+return NULL;
+}
+
+name = bitmap->name;
+bitmap->name = NULL;
+successor->name = name;
+bitmap->successor = NULL;
+bdrv_release_dirty_bitmap(bs, bitmap);
+
+return successor;
+}
+
+/**
+ * In cases of failure where we can no longer safely delete the parent,
+ * we may wish to re-join the parent and child/successor.
+ * The merged parent will be un-frozen, but not explicitly re-enabled.
+ */
+BdrvDirtyBitmap *bdrv_reclaim_dirty_

[Qemu-block] [PATCH v6 17/21] iotests: add invalid input incremental backup tests

[John Snow]

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 tests/qemu-iotests/124 | 104 +
 tests/qemu-iotests/124.out |   5 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 110 insertions(+)
 create mode 100644 tests/qemu-iotests/124
 create mode 100644 tests/qemu-iotests/124.out

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
new file mode 100644
index 000..85675ec
--- /dev/null
+++ b/tests/qemu-iotests/124
@@ -0,0 +1,104 @@
+#!/usr/bin/env python
+#
+# Tests for incremental drive-backup
+#
+# Copyright (C) 2015 John Snow for Red Hat, Inc.
+#
+# Based on 056.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import iotests
+
+
+def io_write_patterns(img, patterns):
+for pattern in patterns:
+iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
+
+
+class TestIncrementalBackup(iotests.QMPTestCase):
+def setUp(self):
+self.bitmaps = list()
+self.files = list()
+self.drives = list()
+self.vm = iotests.VM()
+self.err_img = os.path.join(iotests.test_dir, 'err.%s' % 
iotests.imgfmt)
+
+# Create a base image with a distinctive patterning
+drive0 = self.add_node('drive0')
+self.img_create(drive0['file'], drive0['fmt'])
+self.vm.add_drive(drive0['file'])
+io_write_patterns(drive0['file'], (('0x41', 0, 512),
+   ('0xd5', '1M', '32k'),
+   ('0xdc', '32M', '124k')))
+self.vm.launch()
+
+
+def add_node(self, node_id, fmt=iotests.imgfmt, path=None, backup=None):
+if path is None:
+path = os.path.join(iotests.test_dir, '%s.%s' % (node_id, fmt))
+if backup is None:
+backup = os.path.join(iotests.test_dir,
+  '%s.full.backup.%s' % (node_id, fmt))
+
+self.drives.append({
+'id': node_id,
+'file': path,
+'backup': backup,
+'fmt': fmt })
+return self.drives[-1]
+
+
+def img_create(self, img, fmt=iotests.imgfmt, size='64M',
+   parent=None, parentFormat=None):
+if parent:
+if parentFormat is None:
+parentFormat = fmt
+iotests.qemu_img('create', '-f', fmt, img, size,
+ '-b', parent, '-F', parentFormat)
+else:
+iotests.qemu_img('create', '-f', fmt, img, size)
+self.files.append(img)
+
+def test_sync_dirty_bitmap_missing(self):
+self.assert_no_active_block_jobs()
+self.files.append(self.err_img)
+result = self.vm.qmp('drive-backup', device=self.drives[0]['id'],
+ sync='dirty-bitmap', format=self.drives[0]['fmt'],
+ target=self.err_img)
+self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+def test_sync_dirty_bitmap_not_found(self):
+self.assert_no_active_block_jobs()
+self.files.append(self.err_img)
+result = self.vm.qmp('drive-backup', device=self.drives[0]['id'],
+ sync='dirty-bitmap', bitmap='unknown',
+ format=self.drives[0]['fmt'], target=self.err_img)
+self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+def tearDown(self):
+self.vm.shutdown()
+for filename in self.files:
+try:
+os.remove(filename)
+except OSError:
+pass
+
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
new file mode 100644
index 000..fbc63e6
--- /dev/null
+++ b/tests/qemu-iotests/124.out
@@ -0,0 +1,5 @@
+..
+--
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bcf2578..f9830d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -123,5 +123,6 @@
 116 rw auto quick
 121 rw auto
 123 rw auto quick
+124 rw auto backing
 128 rw auto quick
 130 rw auto quick
-- 
2.1.0

[Qemu-block] [PATCH v6 07/21] hbitmap: add hbitmap_merge

[John Snow]

We add a bitmap merge operation to assist in error cases
where we wish to combine two bitmaps together.

This is algorithmically O(bits) provided HBITMAP_LEVELS remains
constant. For a full bitmap on a 64bit machine:
sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits

We may be able to improve running speed for particularly sparse
bitmaps by using iterators, but the running time for dense maps
will be worse.

We present the simpler solution first, and we can refine it later
if needed.

Signed-off-by: John Snow 
---
 include/qemu/hbitmap.h | 13 +
 util/hbitmap.c | 33 +
 2 files changed, 46 insertions(+)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..6cb2d0e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,19 @@ struct HBitmapIter {
 HBitmap *hbitmap_alloc(uint64_t size, int granularity);
 
 /**
+ * hbitmap_merge:
+ * @a: The bitmap to store the result in.
+ * @b: The bitmap to merge into @a.
+ * @return true if the merge was successful,
+ * false if it was not attempted.
+ *
+ * Merge two bitmaps together.
+ * A := A (BITOR) B.
+ * B is left unmodified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b);
+
+/**
  * hbitmap_empty:
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 5b78613..150d6e9 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -399,3 +399,36 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
 hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
 return hb;
 }
+
+/**
+ * Given HBitmaps A and B, let A := A (BITOR) B.
+ * Bitmap B will not be modified.
+ *
+ * @return true if the merge was successful,
+ * false if it was not attempted.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b)
+{
+int i;
+uint64_t j;
+
+if ((a->size != b->size) || (a->granularity != b->granularity)) {
+return false;
+}
+
+if (hbitmap_count(b) == 0) {
+return true;
+}
+
+/* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
+ * It may be possible to improve running times for sparsely populated maps
+ * by using hbitmap_iter_next, but this is suboptimal for dense maps.
+ */
+for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
+for (j = 0; j < a->sizes[i]; j++) {
+a->levels[i][j] |= b->levels[i][j];
+}
+}
+
+return true;
+}
-- 
2.1.0

[Qemu-block] [PATCH v6 08/21] block: Add bitmap disabled status

[John Snow]

Add a status indicating the enabled/disabled state of the bitmap.
A bitmap is by default enabled, but you can lock the bitmap into
a read-only state by setting disabled = true.

A previous version of this patch added a QMP interface for changing
the state of the bitmap, but it has since been removed for now until
a use case emerges where this state must be revealed to the user.

The disabled state WILL be used internally for bitmap migration and
bitmap persistence.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 block.c   | 25 +
 include/block/block.h |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/block.c b/block.c
index 41c5a67..db742a9 100644
--- a/block.c
+++ b/block.c
@@ -54,6 +54,7 @@
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
 char *name;
+bool disabled;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5481,10 +5482,16 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
 bitmap->name = g_strdup(name);
+bitmap->disabled = false;
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
 return bitmap;
 }
 
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
+{
+return !bitmap->disabled;
+}
+
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
 BdrvDirtyBitmap *bm, *next;
@@ -5499,6 +5506,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 }
 }
 
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+bitmap->disabled = true;
+}
+
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+bitmap->disabled = false;
+}
+
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 {
 BdrvDirtyBitmap *bm;
@@ -5563,12 +5580,14 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
 void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
 {
+assert(bdrv_dirty_bitmap_enabled(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
 void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int nr_sectors)
 {
+assert(bdrv_dirty_bitmap_enabled(bitmap));
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
@@ -5577,6 +5596,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 {
 BdrvDirtyBitmap *bitmap;
 QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+continue;
+}
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 }
@@ -5586,6 +5608,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
int64_t cur_sector,
 {
 BdrvDirtyBitmap *bitmap;
 QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+continue;
+}
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 493b7c5..029a8a7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -457,9 +457,12 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
*bs,
 const char *name);
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
 uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
sector);
 void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-- 
2.1.0

[Qemu-block] [PATCH v6 11/21] qmp: add block-dirty-bitmap-clear

[John Snow]

Add bdrv_clear_dirty_bitmap and a matching QMP command,
qmp_block_dirty_bitmap_clear that enables a user to reset
the bitmap attached to a drive.

This allows us to reset a bitmap in the event of a full
drive backup.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
---
 block.c   |  8 
 blockdev.c| 34 ++
 include/block/block.h |  1 +
 qapi/block-core.json  | 14 ++
 qmp-commands.hx   | 29 +
 5 files changed, 86 insertions(+)

diff --git a/block.c b/block.c
index 185cd7f..679991b 100644
--- a/block.c
+++ b/block.c
@@ -62,6 +62,7 @@
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
 BdrvDirtyBitmap *successor;
+int64_t size;
 char *name;
 bool disabled;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5491,6 +5492,7 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
+bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
 bitmap->disabled = false;
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
@@ -5693,6 +5695,12 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+assert(bdrv_dirty_bitmap_enabled(bitmap));
+hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
 static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors)
 {
diff --git a/blockdev.c b/blockdev.c
index 90ba5b6..df96959 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2078,6 +2078,40 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
 aio_context_release(aio_context);
 }
 
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
+  Error **errp)
+{
+AioContext *aio_context;
+BdrvDirtyBitmap *bitmap;
+BlockDriverState *bs;
+
+bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp);
+if (!bitmap || !bs) {
+return;
+}
+
+if (bdrv_dirty_bitmap_frozen(bitmap)) {
+error_setg(errp,
+   "Bitmap '%s' is currently frozen and cannot be modified",
+   name);
+goto out;
+} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+error_setg(errp,
+   "Bitmap '%s' is currently disabled and cannot be cleared",
+   name);
+goto out;
+}
+
+bdrv_clear_dirty_bitmap(bitmap);
+
+ out:
+aio_context_release(aio_context);
+}
+
 int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index 80ac2cc..0961b1e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -478,6 +478,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int nr_sectors);
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_init(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
 void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a4e2897..fc9ca04 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1022,6 +1022,20 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-clear
+#
+# Clear (reset) a dirty bitmap on the device
+#
+# Returns: nothing on success
+#  If @node is not a valid block device, DeviceNotFound
+#  If @name is not found, GenericError with an explanation
+#
+# Since 2.4
+##
+{ 'command': 'block-dirty-bitmap-clear',
+  'data': 'BlockDirtyBitmap' }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index eb54dcd..e7db3a3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1328,6 +1328,35 @@ Example:
 EQMP
 
 {
+.name   = "block-dirty-bitmap-clear",
+.args_type  = "node:B,name:s",
+.mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_clear,
+},
+
+SQMP
+
+block-dirty-bitmap-clear
+
+Since 2.4
+
+Reset the dirty bitmap associated with a node so that an incremental backup
+from this point in time forward will only backup clusters modified after this
+clear operation.
+
+Arguments:
+
+- "node": device/node on which

[Qemu-block] [PATCH v6 15/21] block: Resize bitmaps on bdrv_truncate

[John Snow]

Signed-off-by: John Snow 
---
 block.c| 18 ++
 include/qemu/hbitmap.h | 10 ++
 util/hbitmap.c | 48 
 3 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 735acff..b29aafe 100644
--- a/block.c
+++ b/block.c
@@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
int nr_sectors);
 static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
  int nr_sectors);
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -3583,6 +3584,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 ret = drv->bdrv_truncate(bs, offset);
 if (ret == 0) {
 ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+bdrv_dirty_bitmap_truncate(bs);
 if (bs->blk) {
 blk_dev_resize_cb(bs->blk);
 }
@@ -5593,6 +5595,22 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 return parent;
 }
 
+/**
+ * Truncates _all_ bitmaps attached to a BDS.
+ */
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bitmap;
+uint64_t size = bdrv_nb_sectors(bs);
+
+QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (bdrv_dirty_bitmap_frozen(bitmap)) {
+continue;
+}
+hbitmap_truncate(bitmap->bitmap, size);
+}
+}
+
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
 BdrvDirtyBitmap *bm, *next;
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6cb2d0e..f0a85f8 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,16 @@ struct HBitmapIter {
 HBitmap *hbitmap_alloc(uint64_t size, int granularity);
 
 /**
+ * hbitmap_truncate:
+ * @hb: The bitmap to change the size of.
+ * @size: The number of elements to change the bitmap to accommodate.
+ *
+ * truncate or grow an existing bitmap to accommodate a new number of elements.
+ * This may invalidate existing HBitmapIterators.
+ */
+void hbitmap_truncate(HBitmap *hb, uint64_t size);
+
+/**
  * hbitmap_merge:
  * @a: The bitmap to store the result in.
  * @b: The bitmap to merge into @a.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 150d6e9..a10c7ae 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -400,6 +400,54 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
 return hb;
 }
 
+void hbitmap_truncate(HBitmap *hb, uint64_t size)
+{
+bool shrink;
+unsigned i;
+uint64_t num_elements = size;
+uint64_t old;
+
+/* Size comes in as logical elements, adjust for granularity. */
+size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
+assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
+shrink = size < hb->size;
+
+/* bit sizes are identical; nothing to do. */
+if (size == hb->size) {
+return;
+}
+
+/* If we're losing bits, let's clear those bits before we invalidate all of
+ * our invariants. This helps keep the bitcount consistent, and will 
prevent
+ * us from carrying around garbage bits beyond the end of the map.
+ */
+if (shrink) {
+/* Don't clear partial granularity groups;
+ * start at the first full one. */
+uint64_t start = QEMU_ALIGN_UP(num_elements, 1 << hb->granularity);
+uint64_t fix_count = (hb->size << hb->granularity) - start;
+
+assert(fix_count);
+hbitmap_reset(hb, start, fix_count);
+}
+
+hb->size = size;
+for (i = HBITMAP_LEVELS; i-- > 0; ) {
+size = MAX(BITS_TO_LONGS(size), 1);
+if (hb->sizes[i] == size) {
+break;
+}
+old = hb->sizes[i];
+hb->sizes[i] = size;
+hb->levels[i] = g_realloc(hb->levels[i], size * sizeof(unsigned long));
+if (!shrink) {
+memset(&hb->levels[i][old], 0x00,
+   (size - old) * sizeof(*hb->levels[i]));
+}
+}
+}
+
+
 /**
  * Given HBitmaps A and B, let A := A (BITOR) B.
  * Bitmap B will not be modified.
-- 
2.1.0

[Qemu-block] [PATCH v6 12/21] qmp: Add dirty bitmap status field in query-block

[John Snow]

Add the "frozen" status booleans, to inform clients
when a bitmap is occupied doing a task.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
---
 block.c  | 1 +
 qapi/block-core.json | 5 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 679991b..ca73a6a 100644
--- a/block.c
+++ b/block.c
@@ -5633,6 +5633,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 info->granularity = bdrv_dirty_bitmap_granularity(bm);
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
+info->frozen = bdrv_dirty_bitmap_frozen(bm);
 entry->value = info;
 *plist = entry;
 plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fc9ca04..cae995b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -336,10 +336,13 @@
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
+# @frozen: whether the dirty bitmap is frozen (Since 2.4)
+#
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+   'frozen': 'bool'} }
 
 ##
 # @BlockInfo:
-- 
2.1.0

[Qemu-block] [PATCH v6 02/21] qapi: Add optional field "name" to block dirty bitmap

[John Snow]

From: Fam Zheng 

This field will be set for user created dirty bitmap. Also pass in an
error pointer to bdrv_create_dirty_bitmap, so when a name is already
taken on this BDS, it can report an error message. This is not global
check, two BDSes can have dirty bitmap with a common name.

Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
be used later when other QMP commands want to reference dirty bitmap by
name.

Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
---
 block.c   | 32 +++-
 block/mirror.c|  2 +-
 include/block/block.h |  7 ++-
 migration/block.c |  2 +-
 qapi/block-core.json  |  4 +++-
 5 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index f2f8ae7..0da35ae 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
 
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
+char *name;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5435,7 +5436,28 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov)
 return true;
 }
 
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
granularity,
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+BdrvDirtyBitmap *bm;
+
+assert(name);
+QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+if (bm->name && !strcmp(name, bm->name)) {
+return bm;
+}
+}
+return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+g_free(bitmap->name);
+bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+  int granularity,
+  const char *name,
   Error **errp)
 {
 int64_t bitmap_size;
@@ -5443,6 +5465,10 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
 
 assert((granularity & (granularity - 1)) == 0);
 
+if (name && bdrv_find_dirty_bitmap(bs, name)) {
+error_setg(errp, "Bitmap already exists: %s", name);
+return NULL;
+}
 granularity >>= BDRV_SECTOR_BITS;
 assert(granularity);
 bitmap_size = bdrv_nb_sectors(bs);
@@ -5453,6 +5479,7 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+bitmap->name = g_strdup(name);
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
 return bitmap;
 }
@@ -5464,6 +5491,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 if (bm == bitmap) {
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
+g_free(bitmap->name);
 g_free(bitmap);
 return;
 }
@@ -5482,6 +5510,8 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 info->count = bdrv_get_dirty_count(bs, bm);
 info->granularity =
 ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+info->has_name = !!bm->name;
+info->name = g_strdup(bm->name);
 entry->value = info;
 *plist = entry;
 plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index 4056164..f073ad7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -703,7 +703,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 s->granularity = granularity;
 s->buf_size = MAX(buf_size, granularity);
 
-s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp);
+s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
 if (!s->dirty_bitmap) {
 return;
 }
diff --git a/include/block/block.h b/include/block/block.h
index 4c57d63..05e32f9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -449,8 +449,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov);
 
 struct HBitmapIter;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
granularity,
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+  int granularity,
+  const char *name,
   Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+const char *name);
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdr

[Qemu-block] [PATCH v6 06/21] hbitmap: cache array lengths

[John Snow]

As a convenience: between incremental backups, bitmap migrations
and bitmap persistence we seem to need to recalculate these a lot.

Because the lengths are a little bit-twiddly, let's just solidly
cache them and be done with it.

Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: John Snow 
---
 util/hbitmap.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index ab13971..5b78613 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -90,6 +90,9 @@ struct HBitmap {
  * bitmap will still allocate HBITMAP_LEVELS arrays.
  */
 unsigned long *levels[HBITMAP_LEVELS];
+
+/* The length of each levels[] array. */
+uint64_t sizes[HBITMAP_LEVELS];
 };
 
 /* Advance hbi to the next nonzero word and return it.  hbi->pos
@@ -384,6 +387,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
 hb->granularity = granularity;
 for (i = HBITMAP_LEVELS; i-- > 0; ) {
 size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+hb->sizes[i] = size;
 hb->levels[i] = g_new0(unsigned long, size);
 }
 
-- 
2.1.0

[Qemu-block] [PATCH v6 03/21] qmp: Ensure consistent granularity type

[John Snow]

We treat this field with a variety of different types everywhere
in the code. Now it's just uint32_t.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
 block.c   | 11 ++-
 block/mirror.c|  4 ++--
 include/block/block.h |  2 +-
 include/block/block_int.h |  2 +-
 qapi/block-core.json  |  2 +-
 5 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 0da35ae..2b609e7 100644
--- a/block.c
+++ b/block.c
@@ -5456,12 +5456,13 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
-  int granularity,
+  uint32_t granularity,
   const char *name,
   Error **errp)
 {
 int64_t bitmap_size;
 BdrvDirtyBitmap *bitmap;
+uint32_t sector_granularity;
 
 assert((granularity & (granularity - 1)) == 0);
 
@@ -5469,8 +5470,8 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 error_setg(errp, "Bitmap already exists: %s", name);
 return NULL;
 }
-granularity >>= BDRV_SECTOR_BITS;
-assert(granularity);
+sector_granularity = granularity >> BDRV_SECTOR_BITS;
+assert(sector_granularity);
 bitmap_size = bdrv_nb_sectors(bs);
 if (bitmap_size < 0) {
 error_setg_errno(errp, -bitmap_size, "could not get length of device");
@@ -5478,7 +5479,7 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 return NULL;
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
-bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
 bitmap->name = g_strdup(name);
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
 return bitmap;
@@ -5509,7 +5510,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
 info->count = bdrv_get_dirty_count(bs, bm);
 info->granularity =
-((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
 entry->value = info;
diff --git a/block/mirror.c b/block/mirror.c
index f073ad7..f8aac33 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,7 @@ static const BlockJobDriver commit_active_job_driver = {
 
 static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
  const char *replaces,
- int64_t speed, int64_t granularity,
+ int64_t speed, uint32_t granularity,
  int64_t buf_size,
  BlockdevOnError on_source_error,
  BlockdevOnError on_target_error,
@@ -717,7 +717,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
   const char *replaces,
-  int64_t speed, int64_t granularity, int64_t buf_size,
+  int64_t speed, uint32_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
   BlockCompletionFunc *cb,
diff --git a/include/block/block.h b/include/block/block.h
index 05e32f9..5235086 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -450,7 +450,7 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov);
 struct HBitmapIter;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
-  int granularity,
+  uint32_t granularity,
   const char *name,
   Error **errp);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dccb092..fb9e100 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -590,7 +590,7 @@ void commit_active_start(BlockDriverState *bs, 
BlockDriverState *base,
  */
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
   const char *replaces,
-  int64_t speed, int64_t granularity, int64_t buf_size,
+  int64_t speed, uint32_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
 

[Qemu-block] [PATCH v6 00/21] block: transactionless incremental backup series

[John Snow]

It's spring! The winter snow has thawed and so here is a new
patch series to enter your life and warm your heart.

This patchset enables the in-memory part of the incremental backup
feature, without transactional support.

Support for transactions was separated into a separate series which
is also now available on-list. Getting this portion of the series
committed will help stabilize work on bitmap persistence and bitmap
migration.

Thanks to Fam Zheng for the original versions of this patchset,
And thanks to Max and Eric for reviewing 2,396 versions of it since.

===
v6:
===

01: s/underlaying/underlying/
Removed a reference to 'disabled' bitmaps.
Touching up inconsistent list indentation.
Added FreeBSD Documentation License, primarily to be difficult
07: More in-line documentation for hbitmap_merge, for return value.
Fix size cache index to be uint64_t.
09: Grammar fixes from Eric Blake, kept R-Bs.
10: Moved yield into the do{}while(). Now we check to see if we should
   yield/cancel after each unsuccessful sector we transfer.
Some documentation additions for Eric Blake.
15: corrected 'num_elements' to 'start'
18: Refactored qmp.py event functions,
Added in more explicit exception classes.
No changes to iotests.py, just qmp.py.

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/21:[0057] [FC] 'docs: incremental backup documentation'
002/21:[] [--] 'qapi: Add optional field "name" to block dirty bitmap'
003/21:[] [--] 'qmp: Ensure consistent granularity type'
004/21:[] [--] 'qmp: Add block-dirty-bitmap-add and 
block-dirty-bitmap-remove'
005/21:[] [--] 'block: Introduce bdrv_dirty_bitmap_granularity()'
006/21:[] [--] 'hbitmap: cache array lengths'
007/21:[0008] [FC] 'hbitmap: add hbitmap_merge'
008/21:[] [--] 'block: Add bitmap disabled status'
009/21:[0008] [FC] 'block: Add bitmap successors'
010/21:[0013] [FC] 'qmp: Add support of "dirty-bitmap" sync mode for 
drive-backup'
011/21:[] [--] 'qmp: add block-dirty-bitmap-clear'
012/21:[] [--] 'qmp: Add dirty bitmap status field in query-block'
013/21:[] [--] 'block: add BdrvDirtyBitmap documentation'
014/21:[] [--] 'block: Ensure consistent bitmap function prototypes'
015/21:[0002] [FC] 'block: Resize bitmaps on bdrv_truncate'
016/21:[] [--] 'hbitmap: truncate tests'
017/21:[] [-C] 'iotests: add invalid input incremental backup tests'
018/21:[0095] [FC] 'iotests: add QMP event waiting queue'
019/21:[] [--] 'iotests: add simple incremental backup case'
020/21:[] [--] 'iotests: add incremental backup failure recovery test'
021/21:[] [--] 'iotests: add incremental backup granularity tests'

===
v5:
===

10: Code has been moved into backup_run_incremental()
'polyrhythm' check is removed,
clusters_per_iter variable is introduced instead.
If the bitmap granularity is larger than the backup granularity,
loop over the backup_do_cow call multiple times.
If the bitmap granularity is smaller, skip the iterator ahead as
we had been doing previously.
14: Only whitespace changes caused by patch 10.
15: Changed my approach for clearing out data for the hbitmap
truncate shrink case, as suggested by Stefan
18: Added a proper timeout mechanism to qmp.pull_event():
  wait=False or wait=0.0 implies non-blocking.
  wait=True implies blocking.
  wait=60.0 implies a 60 second timeout.
  VM.event_wait() now uses a 60 second timeout by default.
19: Many things:
The big picture is to add a set of full backups alongside the
incremental backups created during the test to be able to test
the validity of each incremental at the conclusion of the test
when we can shut the VM down.

To do this, there are two basic changes:
(1) Keep a list of pairs of backup filenames (incremental, reference);
create a full reference backup for every incremental created.
(2) Refactor the backup helper functions a bit.
20: Naming fallout from 19
Added calls to vm.shutdown() and check_backups().
21: NEW, adds granularity tests that cover the changes in patch 10.

===
v4:
===

04: Some in-line documentation for block_dirty_bitmap_lookup
Changed behavior with respect to aio_context
  (always acquire, release if pbs == null, give to user otherwise)
10: Removed vestigial (currently nop) bdrv_enable_dirty_bitmap call
Kept R-B.
16: Added some comments to test_check_boundary_bits.
Kept R-B.
17: Folded in refactor from "incremental transactions v1" (Poor Kitty)
18: Pulled forward from "incremental transactions v1"
Kept R-B from that series.
19: Folded in refactor from "incremental transactions v1"
Added offset assertions into wait_incremental
20: Removed error tolerance from wait_until_completed, as
these patches no longer

[Qemu-block] [PATCH v6 04/21] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[John Snow]

The new command pair is added to manage a user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.

The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.

This patch also introduces the 'block_dirty_bitmap_lookup' helper,
which takes a device name and a dirty bitmap name and validates the
lookup, returning NULL and setting errp if there is a problem with
either field. This helper will be re-used in future patches in this
series.

The types added to block-core.json will be re-used in future patches
in this series, see:
'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
---
 block.c   |  20 +
 block/mirror.c|  10 +
 blockdev.c| 117 ++
 include/block/block.h |   1 +
 qapi/block-core.json  |  55 
 qmp-commands.hx   |  56 
 6 files changed, 250 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 2b609e7..ebea54d 100644
--- a/block.c
+++ b/block.c
@@ -5530,6 +5530,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap, int64_t sector
 }
 }
 
+/**
+ * Chooses a default granularity based on the existing cluster size,
+ * but clamped between [4K, 64K]. Defaults to 64K in the case that there
+ * is no cluster size information available.
+ */
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+{
+BlockDriverInfo bdi;
+uint32_t granularity;
+
+if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
+granularity = MAX(4096, bdi.cluster_size);
+granularity = MIN(65536, granularity);
+} else {
+granularity = 65536;
+}
+
+return granularity;
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/block/mirror.c b/block/mirror.c
index f8aac33..1cb700e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -668,15 +668,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 MirrorBlockJob *s;
 
 if (granularity == 0) {
-/* Choose the default granularity based on the target file's cluster
- * size, clamped between 4k and 64k.  */
-BlockDriverInfo bdi;
-if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
-granularity = MAX(4096, bdi.cluster_size);
-granularity = MIN(65536, granularity);
-} else {
-granularity = 65536;
-}
+granularity = bdrv_get_default_bitmap_granularity(target);
 }
 
 assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index fbb3a79..7d71b81 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1164,6 +1164,68 @@ out_aio_context:
 return NULL;
 }
 
+/**
+ * block_dirty_bitmap_lookup:
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names.
+ *
+ * @node: The name of the BDS node to search for bitmaps
+ * @name: The name of the bitmap to search for
+ * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
+ * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
+ * @errp: Output pointer for error information. Can be NULL.
+ *
+ * @return: A bitmap object on success, or NULL on failure.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+  const char *name,
+  BlockDriverState **pbs,
+  AioContext **paio,
+  Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
+AioContext *aio_context;
+
+if (!node) {
+error_setg(errp, "Node cannot be NULL");
+return NULL;
+}
+if (!name) {
+error_setg(errp, "Bitmap name cannot be NULL");
+return NULL;
+}
+bs = bdrv_lookup_bs(node, node, NULL);
+if (!bs) {
+error_setg(errp, "Node '%s' not found", node);
+return NULL;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
+bitmap = bdrv_find_dirty_bitmap(bs, name);
+if (!bitmap) {
+error_setg(errp, "Dirty bitmap '%s' not found", name);
+goto fail;
+}
+
+if (pbs) {
+*pbs = bs;
+}
+if (paio) {
+*paio = aio_context;
+ 

[Qemu-block] [PATCH v6 01/21] docs: incremental backup documentation

[John Snow]

Signed-off-by: John Snow 
---
 docs/bitmaps.md | 352 
 1 file changed, 352 insertions(+)
 create mode 100644 docs/bitmaps.md

diff --git a/docs/bitmaps.md b/docs/bitmaps.md
new file mode 100644
index 000..f066b48
--- /dev/null
+++ b/docs/bitmaps.md
@@ -0,0 +1,352 @@
+
+
+# Dirty Bitmaps and Incremental Backup
+
+* Dirty Bitmaps are objects that track which data needs to be backed up for the
+  next incremental backup.
+
+* Dirty bitmaps can be created at any time and attached to any node
+  (not just complete drives.)
+
+## Dirty Bitmap Names
+
+* A dirty bitmap's name is unique to the node, but bitmaps attached to 
different
+  nodes can share the same name.
+
+## Bitmap Modes
+
+* A Bitmap can be "frozen," which means that it is currently in-use by a backup
+  operation and cannot be deleted, renamed, written to, reset,
+  etc.
+
+## Basic QMP Usage
+
+### Supported Commands ###
+
+* block-dirty-bitmap-add
+* block-dirty-bitmap-remove
+* block-dirty-bitmap-clear
+
+### Creation
+
+* To create a new bitmap, enabled, on the drive with id=drive0:
+
+```json
+{ "execute": "block-dirty-bitmap-add",
+  "arguments": {
+"node": "drive0",
+"name": "bitmap0"
+  }
+}
+```
+
+* This bitmap will have a default granularity that matches the cluster size of
+  its associated drive, if available, clamped to between [4KiB, 64KiB].
+  The current default for qcow2 is 64KiB.
+
+* To create a new bitmap that tracks changes in 32KiB segments:
+
+```json
+{ "execute": "block-dirty-bitmap-add",
+  "arguments": {
+"node": "drive0",
+"name": "bitmap0",
+"granularity": 32768
+  }
+}
+```
+
+### Deletion
+
+* Bitmaps that are frozen cannot be deleted.
+
+* Deleting the bitmap does not impact any other bitmaps attached to the same
+  node, nor does it affect any backups already created from this node.
+
+* Because bitmaps are only unique to the node to which they are attached,
+  you must specify the node/drive name here, too.
+
+```json
+{ "execute": "block-dirty-bitmap-remove",
+  "arguments": {
+"node": "drive0",
+"name": "bitmap0"
+  }
+}
+```
+
+### Resetting
+
+* Resetting a bitmap will clear all information it holds.
+
+* An incremental backup created from an empty bitmap will copy no data,
+  as if nothing has changed.
+
+```json
+{ "execute": "block-dirty-bitmap-clear",
+  "arguments": {
+"node": "drive0",
+"name": "bitmap0"
+  }
+}
+```
+
+## Transactions (Not yet implemented)
+
+* Transactional commands are forthcoming in a future version,
+  and are not yet available for use. This section serves as
+  documentation of intent for their design and usage.
+
+### Justification
+
+Bitmaps can be safely modified when the VM is paused or halted by using
+the basic QMP commands. For instance, you might perform the following actions:
+
+1. Boot the VM in a paused state.
+2. Create a full drive backup of drive0.
+3. Create a new bitmap attached to drive0.
+4. Resume execution of the VM.
+5. Incremental backups are ready to be created.
+
+At this point, the bitmap and drive backup would be correctly in sync,
+and incremental backups made from this point forward would be correctly aligned
+to the full drive backup.
+
+This is not particularly useful if we decide we want to start incremental
+backups after the VM has been running for a while, for which we will need to
+perform actions such as the following:
+
+1. Boot the VM and begin execution.
+2. Using a single transaction, perform the following operations:
+* Create bitmap0.
+* Create a full drive backup of drive0.
+3. Incremental backups are now ready to be created.
+
+### Supported Bitmap Transactions
+
+* block-dirty-bitmap-add
+* block-dirty-bitmap-clear
+
+The usages are identical to their respective QMP commands, but see below
+for examples.
+
+### Example: New Incremental Backup
+
+As outlined in the justification, perhaps we want to create a new incremental
+backup chain attached to a drive.
+
+```json
+{ "execute": "transaction",
+  "arguments": {
+"actions": [
+  {"type": "block-dirty-bitmap-add",
+   "data": {"node": "drive0", "name": "bitmap0"} },
+  {"type": "drive-backup",
+   "data": {"device": "drive0", "target": "/path/to/full_backup.img",
+"sync": "full", "format": "qcow2"} }
+]
+  }
+}
+```
+
+### Example: New Incremental Backup Anchor Point
+
+Maybe we just want to create a new full backup with an existing bitmap and
+want to reset the bitmap to track the new chain.
+
+```json
+{ "execute": "transaction",
+  "arguments": {
+"actions": [
+  {"type": "block-dirty-bitmap-clear",
+   "data": {"node": "drive0", "name": "bitmap0"} },
+  {"type": "drive-backup",
+   "data": {"device": "drive0", "target": "/path/to/new_full_backup.img",
+"sync": "full", "format": "qcow2"} }
+]
+  }
+}
+```
+
+## Incremental Backups
+
+The star of the show.
+
+**Nota Bene!** Only incremental backups of en

[Qemu-block] [PATCH v6 05/21] block: Introduce bdrv_dirty_bitmap_granularity()

[John Snow]

This returns the granularity (in bytes) of dirty bitmap,
which matches the QMP interface and the existing query
interface.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
---
 block.c   | 8 ++--
 include/block/block.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ebea54d..41c5a67 100644
--- a/block.c
+++ b/block.c
@@ -5509,8 +5509,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
 BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
 info->count = bdrv_get_dirty_count(bs, bm);
-info->granularity =
-((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+info->granularity = bdrv_dirty_bitmap_granularity(bm);
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
 entry->value = info;
@@ -5550,6 +5549,11 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 return granularity;
 }
 
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+{
+return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 0f014a3..493b7c5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -459,6 +459,7 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
sector);
 void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-- 
2.1.0

Re: [Qemu-block] [Qemu-devel] [PATCH v2 05/11] block: add transactional callbacks feature

[John Snow]

On 04/17/2015 11:41 AM, Max Reitz wrote:

On 27.03.2015 20:19, John Snow wrote:

The goal here is to add a new method to transactions that allows
developers to specify a callback that will get invoked only once
all jobs spawned by a transaction are completed, allowing developers
the chance to perform actions conditionally pending complete success,
partial failure, or complete failure.

In order to register the new callback to be invoked, a user must request
a callback pointer and closure by calling new_action_cb_wrapper, which
creates a wrapper around an opaque pointer and callback that would have
originally been passed to e.g. backup_start().

The function will return a function pointer and a new opaque pointer to
be passed instead. The transaction system will effectively intercept the
original callbacks and perform book-keeping on the transaction after it
has delivered the original enveloped callback.

This means that Transaction Action callback methods will be called after
all callbacks triggered by all Actions in the Transactional group have
been received.

This feature has no knowledge of any jobs spawned by Actions that do not
inform the system via new_action_cb_wrapper().

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an
example
for how to hook up a post-transaction callback to the Drive Backup
action.


Note 1: Defining a callback method alone is not sufficient to have the
new
 method invoked. You must call new_action_cb_wrapper() AND
ensure the
 callback it returns is the one used as the callback for the job
 launched by the action.

Note 2: You can use this feature for any system that registers
completions of
 an asynchronous task via a callback of the form
 (void *opaque, int ret), not just block job callbacks.

Signed-off-by: John Snow 
---
  blockdev.c | 191
+++--
  1 file changed, 187 insertions(+), 4 deletions(-)


I like it much better than v1! :-)



I do, too.


Some minor comments below. (But just as in v1, I need to look into the
following patches to know exactly how some of the functions introduced
here are used)


diff --git a/blockdev.c b/blockdev.c
index f806d40..d404251 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1239,6 +1239,8 @@ typedef struct BlkActionState BlkActionState;
   * @abort: Abort the changes on fail, can be NULL.
   * @clean: Clean up resources after all transaction actions have called
   * commit() or abort(). Can be NULL.
+ * @cb: Executed after all jobs launched by actions in the
transaction finish,
+ *  but only if requested by new_action_cb_wrapper() prior to
clean().
   *
   * Only prepare() may fail. In a single transaction, only one of
commit() or
   * abort() will be called. clean() will always be called if it is
present.
@@ -1249,6 +1251,7 @@ typedef struct BlkActionOps {
  void (*commit)(BlkActionState *common);
  void (*abort)(BlkActionState *common);
  void (*clean)(BlkActionState *common);
+void (*cb)(BlkActionState *common);
  } BlkActionOps;
  /**
@@ -1257,19 +1260,46 @@ typedef struct BlkActionOps {
   * by a transaction group.
   *
   * @jobs: A reference count that tracks how many jobs still need to
complete.
+ * @status: A cumulative return code for all actions that have reported
+ *  a return code via callback in the transaction.
   * @actions: A list of all Actions in the Transaction.
+ *   However, once the transaction has completed, it will be
only a list
+ *   of transactions that have registered a post-transaction
callback.
   */
  typedef struct BlkTransactionState {
  int jobs;
+int status;
  QTAILQ_HEAD(actions, BlkActionState) actions;
  } BlkTransactionState;
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/**
+ * BlkActionCallbackData:
+ * Necessary state for intercepting and
+ * re-delivering a callback triggered by an Action.
+ *
+ * @opaque: The data to be given to the encapsulated callback when
+ *  a job launched by an Action completes.
+ * @ret: The status code that was delivered to the encapsulated
callback.
+ * @callback: The encapsulated callback to invoke upon completion of
+ *the Job launched by the Action.
+ */
+typedef struct BlkActionCallbackData {
+void *opaque;
+int ret;
+CallbackFn *callback;
+} BlkActionCallbackData;
+
  /**
   * BlkActionState:
   * Describes one Action's state within a Transaction.
   *
   * @action: QAPI-defined enum identifying which Action to perform.
   * @ops: Table of ActionOps this Action can perform.
+ * @transaction: A pointer back to the Transaction this Action
belongs to.
+ * @cb_data: Information on this Action's encapsulated callback, if any.
+ * @refcount: reference count, allowing access to this state beyond
clean().
   * @entry: List membership for all Actions in this Transaction.
   *
   * This struct

Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
> 
> Signed-off-by: Fam Zheng 
> Signed-off-by: John Snow 
> ---
>  block.c   |   9 +++
>  block/backup.c| 156 
> +++---
>  block/mirror.c|   4 ++
>  blockdev.c|  18 +-
>  hmp.c |   3 +-
>  include/block/block.h |   1 +
>  include/block/block_int.h |   2 +
>  qapi/block-core.json  |  13 ++--
>  qmp-commands.hx   |   7 ++-
>  9 files changed, 180 insertions(+), 33 deletions(-)
> 

Just reviewing the interface...

> +++ b/qapi/block-core.json
> @@ -512,10 +512,12 @@
>  #
>  # @none: only copy data written from now on
>  #
> +# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
> +#
>  # Since: 1.3
>  ##
>  { 'enum': 'MirrorSyncMode',
> -  'data': ['top', 'full', 'none'] }
> +  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
>  
>  ##
>  # @BlockJobType:
> @@ -690,14 +692,17 @@
>  #  probe if @mode is 'existing', else the format of the source
>  #
>  # @sync: what parts of the disk image should be copied to the destination
> -#(all the disk, only the sectors allocated in the topmost image, or
> -#only new I/O).
> +#(all the disk, only the sectors allocated in the topmost image, 
> from a
> +#dirty bitmap, or only new I/O).
>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #'absolute-paths'.
>  #
>  # @speed: #optional the maximum speed, in bytes per second
>  #
> +# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
> +#  (Since 2.4)
> +#
>  # @on-source-error: #optional the action to take on an error on the source,
>  #   default 'report'.  'stop' and 'enospc' can only be used
>  #   if the block device supports io-status (see BlockInfo).
> @@ -715,7 +720,7 @@
>  { 'type': 'DriveBackup',
>'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>  'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -'*speed': 'int',
> +'*speed': 'int', '*bitmap': 'str',

Looks okay.  Is it an error if bitmap is supplied, but mode is not
dirty-bitmap?  Likewise, if mode is dirty-bitmap but bitmap is not supplied?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[John Snow]

On 04/17/2015 06:51 PM, Eric Blake wrote:

On 04/08/2015 04:19 PM, John Snow wrote:

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
---
  block.c   |   9 +++
  block/backup.c| 156 +++---
  block/mirror.c|   4 ++
  blockdev.c|  18 +-
  hmp.c |   3 +-
  include/block/block.h |   1 +
  include/block/block_int.h |   2 +
  qapi/block-core.json  |  13 ++--
  qmp-commands.hx   |   7 ++-
  9 files changed, 180 insertions(+), 33 deletions(-)



Just reviewing the interface...


+++ b/qapi/block-core.json
@@ -512,10 +512,12 @@
  #
  # @none: only copy data written from now on
  #
+# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
+#
  # Since: 1.3
  ##
  { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }

  ##
  # @BlockJobType:
@@ -690,14 +692,17 @@
  #  probe if @mode is 'existing', else the format of the source
  #
  # @sync: what parts of the disk image should be copied to the destination
-#(all the disk, only the sectors allocated in the topmost image, or
-#only new I/O).
+#(all the disk, only the sectors allocated in the topmost image, from a
+#dirty bitmap, or only new I/O).
  #
  # @mode: #optional whether and how QEMU should create a new image, default is
  #'absolute-paths'.
  #
  # @speed: #optional the maximum speed, in bytes per second
  #
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+#  (Since 2.4)
+#
  # @on-source-error: #optional the action to take on an error on the source,
  #   default 'report'.  'stop' and 'enospc' can only be used
  #   if the block device supports io-status (see BlockInfo).
@@ -715,7 +720,7 @@
  { 'type': 'DriveBackup',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
  'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-'*speed': 'int',
+'*speed': 'int', '*bitmap': 'str',


Looks okay.  Is it an error if bitmap is supplied, but mode is not
dirty-bitmap?  Likewise, if mode is dirty-bitmap but bitmap is not supplied?



Yes:

+if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+if (!sync_bitmap) {
+error_setg(errp, "must provide a valid bitmap name for "
+ "\"dirty-bitmap\" sync mode");
+return;
+}
+
[...]
+} else if (sync_bitmap) {
+error_setg(errp,
+   "a sync_bitmap was provided to backup_run, "
+   "but received an incompatible sync_mode (%s)",
+   MirrorSyncMode_lookup[sync_mode]);
+return;
+}
+

Re: [Qemu-block] [PATCH v5 11/21] qmp: add block-dirty-bitmap-clear

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> Add bdrv_clear_dirty_bitmap and a matching QMP command,
> qmp_block_dirty_bitmap_clear that enables a user to reset
> the bitmap attached to a drive.
> 
> This allows us to reset a bitmap in the event of a full
> drive backup.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> Reviewed-by: Stefan Hajnoczi 
> ---

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Eric Blake]

On 04/17/2015 05:02 PM, John Snow wrote:

>>>   { 'type': 'DriveBackup',
>>> 'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>>>   'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>>> -'*speed': 'int',
>>> +'*speed': 'int', '*bitmap': 'str',
>>
>> Looks okay.  Is it an error if bitmap is supplied, but mode is not
>> dirty-bitmap?  Likewise, if mode is dirty-bitmap but bitmap is not
>> supplied?
>>
> 
> Yes:

Then we _could_ do this as a flat union (here, using a shorthand syntax
of anonymous inline types, although qapi does not yet support it):

{ 'type': 'DriveBackupBase',
  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
'*speed': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
{ 'union': 'DriveBackup', 'base': 'DriveBackupBase',
  'discriminator': 'sync',
  'data': { 'top': {}, 'full': {}, 'none': {},
'dirty-bitmap': { 'bitmap': 'str' } } }

which would enforce that 'bitmap' be present exactly when
'sync':'dirty-bitmap'.  But that's probably overkill; I don't expect you
to do it (especially since qapi shorthand for anonymous inline types is
not there yet).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 09/21] block: Add bitmap successors

[John Snow]

On 04/17/2015 06:43 PM, Eric Blake wrote:

On 04/08/2015 04:19 PM, John Snow wrote:

A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
be created just prior to a sensitive operation (e.g. Incremental Backup)
that can either succeed or fail, but during the course of which we still
want a bitmap tracking writes.

On creating a successor, we "freeze" the parent bitmap which prevents
its deletion, enabling, anonymization, or creating a bitmap with the
same name.

On success, the parent bitmap can "abdicate" responsibility to the
successor, which will inherit its name. The successor will have been
tracking writes during the course of the backup operation. The parent
will be safely deleted.

On failure, we can "reclaim" the successor from the parent, unifying
them such that the resulting bitmap describes all writes occurring since
the last successful backup, for instance. Reclamation will thaw the
parent, but not explicitly re-enable it.

BdrvDirtyBitmap operations that target a single bitmap are protected
by assertions that the bitmap is not frozen and/or disabled.

BdrvDirtyBitmap operations that target a group of bitmaps, such as
bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
conditional instead.

QMP transactions that enable/disable bitmaps have extra error checking
surrounding them that prevent modifying bitmaps that are frozen.


Is this last paragraph stale now?



Yes, thank you. Those transactions no longer exist.

The implementations, however, have *assertions* that protect frozen 
bitmaps and still exist. Bitmaps can still be enabled/disabled as an 
internal concept, and it is still invalid to try to enable/disable one 
that is frozen.




Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---
  block.c   | 104 +-
  blockdev.c|   7 
  include/block/block.h |  10 +
  qapi/block-core.json  |   1 +
  4 files changed, 121 insertions(+), 1 deletion(-)




+/**
+ * A BdrvDirtyBitmap can be in three possible states:
+ * (1) successor is false and disabled is false: full r/w mode
+ * (2) successor is false 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().
+ */
  struct BdrvDirtyBitmap {
  HBitmap *bitmap;
+BdrvDirtyBitmap *successor;


'successor is false' reads awkwardly given that it is not a bool; maybe
'successor is NULL' is better?



That is better.



+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->successor;


Good thing C99 requires conversion of pointer to bool to work :)



Is there a case to be made for explicit NULL checks?




+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * Delete the old bitmap, and return a handle to the new bitmap.


Sentences with capitals after comma, Even with a line break, Look weird.
(s/Delete/delete/)



You Are Right, Sorry About That.




+
+/**
+ * In cases of failure where we can no longer safely delete the parent,
+ * We may wish to re-join the parent and child/successor.


and again (s/We/we/)



It is an /extra/ royal 'we.'


Grammar fixes are minor, so:
Reviewed-by: Eric Blake 

Re: [Qemu-block] [PATCH v5 12/21] qmp: Add dirty bitmap status field in query-block

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> Add the "frozen" status booleans, to inform clients
> when a bitmap is occupied doing a task.
> 
> Signed-off-by: Fam Zheng 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> Reviewed-by: Stefan Hajnoczi 
> ---
>  block.c  | 1 +
>  qapi/block-core.json | 5 -
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 09/21] block: Add bitmap successors

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
> be created just prior to a sensitive operation (e.g. Incremental Backup)
> that can either succeed or fail, but during the course of which we still
> want a bitmap tracking writes.
> 
> On creating a successor, we "freeze" the parent bitmap which prevents
> its deletion, enabling, anonymization, or creating a bitmap with the
> same name.
> 
> On success, the parent bitmap can "abdicate" responsibility to the
> successor, which will inherit its name. The successor will have been
> tracking writes during the course of the backup operation. The parent
> will be safely deleted.
> 
> On failure, we can "reclaim" the successor from the parent, unifying
> them such that the resulting bitmap describes all writes occurring since
> the last successful backup, for instance. Reclamation will thaw the
> parent, but not explicitly re-enable it.
> 
> BdrvDirtyBitmap operations that target a single bitmap are protected
> by assertions that the bitmap is not frozen and/or disabled.
> 
> BdrvDirtyBitmap operations that target a group of bitmaps, such as
> bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
> conditional instead.
> 
> QMP transactions that enable/disable bitmaps have extra error checking
> surrounding them that prevent modifying bitmaps that are frozen.

Is this last paragraph stale now?

> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> Reviewed-by: Stefan Hajnoczi 
> ---
>  block.c   | 104 
> +-
>  blockdev.c|   7 
>  include/block/block.h |  10 +
>  qapi/block-core.json  |   1 +
>  4 files changed, 121 insertions(+), 1 deletion(-)
> 

> +/**
> + * A BdrvDirtyBitmap can be in three possible states:
> + * (1) successor is false and disabled is false: full r/w mode
> + * (2) successor is false 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().
> + */
>  struct BdrvDirtyBitmap {
>  HBitmap *bitmap;
> +BdrvDirtyBitmap *successor;

'successor is false' reads awkwardly given that it is not a bool; maybe
'successor is NULL' is better?

>  
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +{
> +return bitmap->successor;

Good thing C99 requires conversion of pointer to bool to work :)


> +
> +/**
> + * For a bitmap with a successor, yield our name to the successor,
> + * Delete the old bitmap, and return a handle to the new bitmap.

Sentences with capitals after comma, Even with a line break, Look weird.
(s/Delete/delete/)


> +
> +/**
> + * In cases of failure where we can no longer safely delete the parent,
> + * We may wish to re-join the parent and child/successor.

and again (s/We/we/)

Grammar fixes are minor, so:
Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v5 07/21] hbitmap: add hbitmap_merge

[John Snow]

On 04/17/2015 11:23 AM, Eric Blake wrote:

On 04/08/2015 04:19 PM, John Snow wrote:

We add a bitmap merge operation to assist in error cases
where we wish to combine two bitmaps together.

This is algorithmically O(bits) provided HBITMAP_LEVELS remains
constant. For a full bitmap on a 64bit machine:
sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits

We may be able to improve running speed for particularly sparse
bitmaps by using iterators, but the running time for dense maps
will be worse.

We present the simpler solution first, and we can refine it later
if needed.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
---



  /**
+ * hbitmap_merge:
+ * @a: The bitmap to store the result in.
+ * @b: The bitmap to merge into @a.
+ *
+ * Merge two bitmaps together.
+ * A := A (BITOR) B.
+ * B is left unmodified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b);


No mention of what the return value means.



+bool hbitmap_merge(HBitmap *a, const HBitmap *b)
+{
+int i, j;
+
+if ((a->size != b->size) || (a->granularity != b->granularity)) {
+return false;
+}
+
+if (hbitmap_count(b) == 0) {
+return true;
+}
+
+/* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
+ * It may be possible to improve running times for sparsely populated maps
+ * by using hbitmap_iter_next, but this is suboptimal for dense maps.
+ */
+for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
+for (j = 0; j < a->sizes[i]; j++) {


j is 'int', but a->sizes[i] is uint64_t.  If we can ever have a bitmap
large enough that its size exceeds 2G at the largest level, then we have
an inf-loop due to overflow problem.

I'd feel safer if you make j be uint64_t here; but it might also be
possible to fix 6/21 to store sizes in a smaller data type along with
appropriate assertions that our bitmaps are never big enough to need a
level with more than 2G size.



Unfortunately, our bitmaps may indeed be colossal. uint32_t would 
suffice for 32bit, but 64bit bitmaps tolerate up to 2^41 bits, which is 
still a ULL size of 2^35, so just a little bit too big for us.


We will never hit this in practice (for this usage, anyway) but I'd 
rather not nerf the base class unnecessarily, so I'll just expand out 
the index here.


Thanks for the good catch.

--js

Re: [Qemu-block] [Qemu-devel] [PATCH v5 18/21] iotests: add QMP event waiting queue

[John Snow]

On 04/17/2015 09:33 AM, Max Reitz wrote:

On 09.04.2015 00:20, John Snow wrote:

A filter is added to allow callers to request very specific
events to be pulled from the event queue, while leaving undesired
events still in the stream.

This allows to poll for completion data for multiple asynchronous
events in any arbitrary order.

A new timeout context is added to the qmp pull_event method's
wait parameter to allow tests to fail if they do not complete
within some expected period of time.

Signed-off-by: John Snow 
---
  scripts/qmp/qmp.py|  6 +-
  tests/qemu-iotests/iotests.py | 38
++
  2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 20b6ec7..7f9ac1b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -140,7 +140,8 @@ class QEMUMonitorProtocol:
  """
  Get and delete the first available QMP event.
-@param wait: block until an event is available (bool)
+@param wait: block until an event is available. If a float is
provided,
+ use this as a timeout value. (bool, float)


Ouch... I guess it works, though...



It does read awkwardly in the declaration, but I also thought that:

wait=False
wait=True
wait=60.0
wait=0.0

all read fairly clearly and had very clear implications, so from a usage 
standpoint it seemed very nice and convenient.


Making a second parameter gets strange:

wait=False, timeout=50.0

^ What does this mean?

wait=True, timeout=0.0

^ Does this mean to wait forever, or to not wait at all?

So in this case I stand by what looks like hackishness as a nice, 
semantically appropriate interface.



  """
  self.__sock.setblocking(0)
  try:
@@ -151,7 +152,10 @@ class QEMUMonitorProtocol:
  pass
  self.__sock.setblocking(1)
  if not self.__events and wait:
+if isinstance(wait, float):
+self.__sock.settimeout(wait)
  self.__json_read(only_event=True)
+self.__sock.settimeout(None)


If a timeout occurs, __json_read will do nothing and return (I guess...).



It will actually throw an exception.


  event = self.__events[0]


This will therefore probably return None...



So we'll never make it this far.


  del self.__events[0]


And I don't know what this will do. Will it be a no-op?



Better than a no-op!


  return event
diff --git a/tests/qemu-iotests/iotests.py
b/tests/qemu-iotests/iotests.py
index 1402854..e93e623 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -78,6 +78,23 @@ def create_image(name, size):
  i = i + 512
  file.close()
+# Test if 'match' is a recursive subset of 'event'
+def event_match(event, match=None):
+if match is None:
+return True
+
+for key in match:
+if key in event:
+if isinstance(event[key], dict):
+if not event_match(event[key], match[key]):
+return False
+elif event[key] != match[key]:
+return False
+else:
+return False
+
+return True
+
  class VM(object):
  '''A QEMU VM'''
@@ -92,6 +109,7 @@ class VM(object):
   '-machine', 'accel=qtest',
   '-display', 'none', '-vga', 'none']
  self._num_drives = 0
+self._events = []
  # This can be used to add an unused monitor instance.
  def add_monitor_telnet(self, ip, port):
@@ -202,14 +220,34 @@ class VM(object):
  def get_qmp_event(self, wait=False):
  '''Poll for one queued QMP events and return it'''
+if len(self._events) > 0:
+return self._events.pop(0)
  return self._qmp.pull_event(wait=wait)
  def get_qmp_events(self, wait=False):
  '''Poll for queued QMP events and return a list of dicts'''
  events = self._qmp.get_events(wait=wait)
+events.extend(self._events)
+del self._events[:]
  self._qmp.clear_events()
  return events
+def event_wait(self, name='BLOCK_JOB_COMPLETED', timeout=60.0,
match=None):
+# Search cached events
+for event in self._events:
+if (event['event'] == name) and event_match(event, match):
+self._events.remove(event)
+return event
+
+# Poll for new events
+while True:
+event = self._qmp.pull_event(wait=timeout)


So if a timeout occurred, event will probably be None.


+if (event['event'] == name) and event_match(event, match):


And this (indexing event == None) will probably generate an exception. I
guess it's one way to fail the test, but certainly not the best...

Max



In practice, the timeout throws a socket.timeout exception I don't 
bother to catch, so execution of the test will fail at this point, which 
is acceptable.


Here's what happens in the next patc

Re: [Qemu-block] [PATCH v2 11/11] iotests: 124 - transactional failure test

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

Use a transaction to request an incremental backup across two drives.
Coerce one of the jobs to fail, and then re-run the transaction.

Verify that no bitmap data was lost due to the partial transaction
failure.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 119 +
  tests/qemu-iotests/124.out |   4 +-
  2 files changed, 121 insertions(+), 2 deletions(-)


Just as patch 2, this will need fixup for v5 of the transaction-less series.

(the changes from v1 look good, though)

Max

Re: [Qemu-block] [Qemu-devel] [PATCH v5 19/21] iotests: add simple incremental backup case

[John Snow]

On 04/17/2015 10:33 AM, Max Reitz wrote:

On 09.04.2015 00:20, John Snow wrote:

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 174
+++--
  tests/qemu-iotests/124.out |   4 +-
  2 files changed, 172 insertions(+), 6 deletions(-)


I was happier with the previous simpler approach, but I guess I'll have
to get my happiness somewhere else today.

Reviewed-by: Max Reitz 



I was much happier with that approach as well. I would actually still 
prefer to roll back the tests to the earlier version if at all possible 
and use the simple "check as we go" mechanism.

Re: [Qemu-block] [PATCH v2 10/11] block: drive_backup transaction callback support

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

This patch actually implements the transactional callback system
 for the drive_backup transaction.


This line is probably not intended to be indented (I always wanted to 
make that pun).



(1) We manually pick up a reference to the bitmap if present to allow
 its cleanup to be delayed until after all drive_backup jobs launched
 by the transaction have fully completed.

(2) We create a functional closure that envelops the original drive_backup
 callback, to be able to intercept the completion status and return code
 for the job.

(3) We add the drive_backup_cb method for the drive_backup action, which
 unpacks the completion information and invokes the final cleanup.

(4) backup_transaction_complete will perform the final cleanup on the
 backup job.

(5) In the case of transaction cancellation, drive_backup_cb is still
 responsible for cleaning up the mess we may have already made.

Signed-off-by: John Snow 
---
  block/backup.c|  9 
  blockdev.c| 52 ---
  include/block/block_int.h |  8 
  3 files changed, 66 insertions(+), 3 deletions(-)


With that fixed:

Reviewed-by: Max Reitz 

Re: [Qemu-block] [Qemu-devel] [PATCH v5 15/21] block: Resize bitmaps on bdrv_truncate

[John Snow]

On 04/17/2015 09:25 AM, Max Reitz wrote:

On 09.04.2015 00:19, John Snow wrote:

Signed-off-by: John Snow 
---
  block.c| 18 ++
  include/qemu/hbitmap.h | 10 ++
  util/hbitmap.c | 48

  3 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 16209a2..42839a0 100644
--- a/block.c
+++ b/block.c
@@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs,
int64_t cur_sector,
 int nr_sectors);
  static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
   int nr_sectors);
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
  /* If non-zero, use only whitelisted block drivers */
  static int use_bdrv_whitelist;
@@ -3583,6 +3584,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
offset)
  ret = drv->bdrv_truncate(bs, offset);
  if (ret == 0) {
  ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+bdrv_dirty_bitmap_truncate(bs);
  if (bs->blk) {
  blk_dev_resize_cb(bs->blk);
  }
@@ -5593,6 +5595,22 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
  return parent;
  }
+/**
+ * Truncates _all_ bitmaps attached to a BDS.
+ */
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bitmap;
+uint64_t size = bdrv_nb_sectors(bs);
+
+QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (bdrv_dirty_bitmap_frozen(bitmap)) {
+continue;
+}
+hbitmap_truncate(bitmap->bitmap, size);
+}
+}
+
  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
*bitmap)
  {
  BdrvDirtyBitmap *bm, *next;
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index c19c1cb..a75157e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,16 @@ struct HBitmapIter {
  HBitmap *hbitmap_alloc(uint64_t size, int granularity);
  /**
+ * hbitmap_truncate:
+ * @hb: The bitmap to change the size of.
+ * @size: The number of elements to change the bitmap to accommodate.
+ *
+ * truncate or grow an existing bitmap to accommodate a new number of
elements.
+ * This may invalidate existing HBitmapIterators.
+ */
+void hbitmap_truncate(HBitmap *hb, uint64_t size);
+
+/**
   * hbitmap_merge:
   * @a: The bitmap to store the result in.
   * @b: The bitmap to merge into @a.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index ba11fd3..1ad3bf3 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -400,6 +400,54 @@ HBitmap *hbitmap_alloc(uint64_t size, int
granularity)
  return hb;
  }
+void hbitmap_truncate(HBitmap *hb, uint64_t size)
+{
+bool shrink;
+unsigned i;
+uint64_t num_elements = size;
+uint64_t old;
+
+/* Size comes in as logical elements, adjust for granularity. */
+size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
+assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
+shrink = size < hb->size;
+
+/* bit sizes are identical; nothing to do. */
+if (size == hb->size) {
+return;
+}
+
+/* If we're losing bits, let's clear those bits before we
invalidate all of
+ * our invariants. This helps keep the bitcount consistent, and
will prevent
+ * us from carrying around garbage bits beyond the end of the map.
+ */
+if (shrink) {
+/* Don't clear partial granularity groups;
+ * start at the first full one. */
+uint64_t start = QEMU_ALIGN_UP(num_elements, 1 <<
hb->granularity);
+uint64_t fix_count = (hb->size << hb->granularity) -
num_elements;


Shouldn't this be s/num_elements/start/?

Max



Hmm... Yes, though in practice it doesn't appear to have mattered. Odd.

The effect here is that we will extend the fix count beyond the end of 
the bitmap by an amount less than the granularity.


hbitmap_reset doesn't seem to make any particular effort to ensure the 
virtual bit ranges you give it are valid in any way.


And I suppose due to the extra bits that hbitmap allocates for its own 
use meant that we never ran off the end of the array by a single bit or 
anything.


Well, fixed, regardless. Thanks.


+
+assert(fix_count);
+hbitmap_reset(hb, start, fix_count);
+}
+
+hb->size = size;
+for (i = HBITMAP_LEVELS; i-- > 0; ) {
+size = MAX(BITS_TO_LONGS(size), 1);
+if (hb->sizes[i] == size) {
+break;
+}
+old = hb->sizes[i];
+hb->sizes[i] = size;
+hb->levels[i] = g_realloc(hb->levels[i], size *
sizeof(unsigned long));
+if (!shrink) {
+memset(&hb->levels[i][old], 0x00,
+   (size - old) * sizeof(*hb->levels[i]));
+}
+}
+}
+
+
  /**
   * Given HBitmaps A and B, let A := A (BITOR) B.
   * Bitmap B will not be modified.

Re: [Qemu-block] [PATCH v2 08/11] block: move transactions beneath qmp interfaces

[Eric Blake]

On 04/17/2015 10:01 AM, Max Reitz wrote:
> On 27.03.2015 20:20, John Snow wrote:
>> In general, since transactions may reference QMP function helpers,
>> it would be nice for them to sit beneath them.
>>
>> This will avoid the need for forward declaring any QMP interfaces,
>> which would be aggravating to update in so many places.
>>
>> Signed-off-by: John Snow 
>> ---
>>   blockdev.c | 2581
>> ++--
>>   1 file changed, 1292 insertions(+), 1289 deletions(-)
> 
> I'm not so sure about this patch. I mean... 2581 changes? :-/
> 
> If you (or someone else) can convince me that forward declarations are
> really worse than this (is it more aggravating than having a patch with
> this diffcount?), well...

I like avoiding forward declarations of static functions, where it makes
sense, but I'm not going to insist on reordering code if it makes things
ugly.

> 
> But even then, I'd strongly vote for removing dropping all functional
> changes beside the code movement from this patch. Because I'm seeing this:
> 
> 2096c2096,2097
> < error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> "power of 2");
> ---
>> error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>>   "granularity", "power of 2");

Indeed - you WANT code motion patches to be as easy as possible to
review.  From http://wiki.qemu.org/Contribute/SubmitAPatch

"Ideally, a code motion patch can be reviewed by doing git format-patch
--stdout -1 > patch; diff -u <(sed -n 's/^-//p' patch) <(sed -n
's/^\+//p' patch), to focus on the few changes that weren't wholesale
code motion."

> Probably sensible changes, but if you really, really, really want to go
> for this code movement, please apply them in an own patch before this
> one so that this one really is nothing but code movement.
> 
> Max
> 
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 08/11] block: move transactions beneath qmp interfaces

[John Snow]

On 04/17/2015 12:01 PM, Max Reitz wrote:

On 27.03.2015 20:20, John Snow wrote:

In general, since transactions may reference QMP function helpers,
it would be nice for them to sit beneath them.

This will avoid the need for forward declaring any QMP interfaces,
which would be aggravating to update in so many places.

Signed-off-by: John Snow 
---
  blockdev.c | 2581
++--
  1 file changed, 1292 insertions(+), 1289 deletions(-)


I'm not so sure about this patch. I mean... 2581 changes? :-/

If you (or someone else) can convince me that forward declarations are
really worse than this (is it more aggravating than having a patch with
this diffcount?), well...

But even then, I'd strongly vote for removing dropping all functional
changes beside the code movement from this patch. Because I'm seeing this:

2096c2096,2097
< error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
"power of 2");
---
 > error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 >   "granularity", "power of 2");
2517c2518,2519
< /* New and old BlockDriverState structs for atomic group operations */
---
 >
/**/

 > /* Transactions*/
3439a3442,3443
 >
 >
/**/


Probably sensible changes, but if you really, really, really want to go
for this code movement, please apply them in an own patch before this
one so that this one really is nothing but code movement.

Max



OK. I fixed the line length because checkpatch yelled at me.

I can add a micropatch that adds my organizational flair and fixes line 
lengths in a teensy patch that precedes this one.


Now: is the code movement necessary? well... Maybe, maybe not. I'd 
actually like to just break out transactions into their own file 
entirely, but that has its own challenges (like our heavy usage of 
internal block goodies) ...


My only real justification is in the cover letter: forward declarations 
of QMP functions is a burden.


Then again, so is basically erasing transaction development history...

:(

Re: [Qemu-block] [PATCH v5 01/21] docs: incremental backup documentation

[Eric Blake]

On 04/17/2015 09:50 AM, John Snow wrote:
> 
> 
> On 04/17/2015 11:06 AM, Eric Blake wrote:
>> On 04/08/2015 04:19 PM, John Snow wrote:
>>> Reviewed-by: Max Reitz 
>>> Signed-off-by: John Snow 
>>> ---
>>>   docs/bitmaps.md | 311
>>> 
>>>   1 file changed, 311 insertions(+)
>>>   create mode 100644 docs/bitmaps.md
>>>
>>> diff --git a/docs/bitmaps.md b/docs/bitmaps.md
>>> new file mode 100644
>>> index 000..ad8c33b
>>> --- /dev/null
>>> +++ b/docs/bitmaps.md
>>> @@ -0,0 +1,311 @@
>>> +# Dirty Bitmaps and Incremental Backup
>>> +
>>
>> Still might be nice to list explicit copyright/license instead of
>> relying on implicit top-level GPLv2+, but I won't insist.
>>
> 
> I think I would rather not clutter up the document itself, if that
> remains suitable. I don't mind those declarations in source code, but
> for a document like this, it seems weird to have it in the preamble.
> 
> I can attach a license to the footer, if that's suitable?

A footer is fine by me (I don't care where it lives in the document,
only that it can be found).  As this is a markup document, you should
also consider whether a copyright should be passed on through to the
rendered document, or whether it is fine for just the markup source as a
comment that gets stripped during rendering (I would probably include it
in the rendered document, but am not strongly opposed if you don't agree).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[John Snow]

On 04/17/2015 09:17 AM, Max Reitz wrote:

On 09.04.2015 00:19, John Snow wrote:

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
---
  block.c   |   9 +++
  block/backup.c| 156
+++---
  block/mirror.c|   4 ++
  blockdev.c|  18 +-
  hmp.c |   3 +-
  include/block/block.h |   1 +
  include/block/block_int.h |   2 +
  qapi/block-core.json  |  13 ++--
  qmp-commands.hx   |   7 ++-
  9 files changed, 180 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 9d30379..2367311 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState
*bs, int64_t cur_sector,
  }
  }
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+assert(hbi->hb);
+hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
  int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap
*bitmap)
  {
  return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..8513917 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
  typedef struct BackupBlockJob {
  BlockJob common;
  BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
  MirrorSyncMode sync_mode;
  RateLimit limit;
  BlockdevOnError on_source_error;
@@ -242,6 +244,92 @@ static void backup_complete(BlockJob *job, void
*opaque)
  g_free(data);
  }
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+
job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+bool error_is_read;
+int ret = 0;
+int clusters_per_iter;
+uint32_t granularity;
+int64_t sector;
+int64_t cluster;
+int64_t end;
+int64_t last_cluster = -1;
+BlockDriverState *bs = job->common.bs;
+HBitmapIter hbi;
+
+granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);


DIV_ROUND_UP(granularity, BACKUP_CLUSTER_SIZE) would've worked, too
(instead of the MAX()), but since both are powers of two, this is
equivalent.



But this way we get to put your name in the source code.


+bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+/* Find the next dirty sector(s) */
+while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped */
+if (cluster != last_cluster + 1) {
+job->common.offset += ((cluster - last_cluster - 1) *
+   BACKUP_CLUSTER_SIZE);
+}
+
+for (end = cluster + clusters_per_iter; cluster < end;
cluster++) {
+if (yield_and_check(job)) {
+return ret;
+}
+
+do {
+ret = backup_do_cow(bs, cluster *
BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
&error_is_read);
+if ((ret < 0) &&
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+}


Now that I'm reading this code again... The other backup implementation
handles retries differently; it redoes the whole loop, with the
effective difference being that it calls yield_and_check() between every
retry. Would it make sense to move the yield_and_check() call into this
loop?



Yes, I should be mindful of the case where we might have to copy many 
clusters per dirty bit. I don't think we lose anything by inserting it 
at the top of the do{}while(), but we will potentially exit the loop 
quicker on cancellation cases.



+} while (ret < 0);
+}
+
+/* If the bitmap granularity is smaller than the backup
granularity,
+ * we need to advance the iterator pointer to the next
cluster. */
+if (granularity < BACKUP_CLUSTER_SIZE) {


Actu

Re: [Qemu-block] [PATCH v2 09/11] qmp: Add an implementation wrapper for qmp_drive_backup

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

We'd like to be able to specify the callback given to backup_start
manually in the case of transactions, so split apart qmp_drive_backup
into an implementation and a wrapper.

Switch drive_backup_prepare to use the new wrapper, but don't overload
the callback and closure yet.

Signed-off-by: John Snow 
---
  blockdev.c | 65 --
  1 file changed, 46 insertions(+), 19 deletions(-)


Reviewed-by: Max Reitz 

The problem being that it relies on patch 8. I don't think being able to 
avoid a single forward declaration warrants moving half (or more...) of 
blockdev.c around, but I'm open to be convinced otherwise.


Max

Re: [Qemu-block] [PATCH v2 08/11] block: move transactions beneath qmp interfaces

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

In general, since transactions may reference QMP function helpers,
it would be nice for them to sit beneath them.

This will avoid the need for forward declaring any QMP interfaces,
which would be aggravating to update in so many places.

Signed-off-by: John Snow 
---
  blockdev.c | 2581 ++--
  1 file changed, 1292 insertions(+), 1289 deletions(-)


I'm not so sure about this patch. I mean... 2581 changes? :-/

If you (or someone else) can convince me that forward declarations are 
really worse than this (is it more aggravating than having a patch with 
this diffcount?), well...


But even then, I'd strongly vote for removing dropping all functional 
changes beside the code movement from this patch. Because I'm seeing this:


2096c2096,2097
< error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity", 
"power of 2");

---
> error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>   "granularity", "power of 2");
2517c2518,2519
< /* New and old BlockDriverState structs for atomic group operations */
---
> 
/**/

> /* Transactions*/
3439a3442,3443
>
> 
/**/


Probably sensible changes, but if you really, really, really want to go 
for this code movement, please apply them in an own patch before this 
one so that this one really is nothing but code movement.


Max

Re: [Qemu-block] [PATCH v5 01/21] docs: incremental backup documentation

[John Snow]

On 04/17/2015 11:06 AM, Eric Blake wrote:

On 04/08/2015 04:19 PM, John Snow wrote:

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
  docs/bitmaps.md | 311 
  1 file changed, 311 insertions(+)
  create mode 100644 docs/bitmaps.md

diff --git a/docs/bitmaps.md b/docs/bitmaps.md
new file mode 100644
index 000..ad8c33b
--- /dev/null
+++ b/docs/bitmaps.md
@@ -0,0 +1,311 @@
+# Dirty Bitmaps and Incremental Backup
+


Still might be nice to list explicit copyright/license instead of
relying on implicit top-level GPLv2+, but I won't insist.



I think I would rather not clutter up the document itself, if that 
remains suitable. I don't mind those declarations in source code, but 
for a document like this, it seems weird to have it in the preamble.


I can attach a license to the footer, if that's suitable?




+### Deletion
+
+* Can be performed on a disabled bitmap, but not a frozen one.


Do you still have a notion of disabled bitmaps?  Earlier, in '## Bitmap
Modes', you only document 'frozen' (as opposed to the default unnamed
state).



We do internally. It's not likely to come up from a user's perspective, 
but we do intend to disable the bitmap during e.g. migration, boot, etc.


I did pull the "disabled" bit out because it's not a necessary detail yet.

I'll tidy this up and reintroduce the language alongside the patch that 
may expose the user to witnessing a disabled bitmap.





+
+## Transactions (Not yet implemented)


I'm assuming that "[PATCH v2 00/11] block: incremental backup
transactions" is incomplete, because it forgot to clean this up as part
of adding transaction support.



Fixed in my local copy, yes.




+
+5. Retry the command after fixing the underlaying problem,


s/underlaying/underlying/



:(

Re: [Qemu-block] [PATCH v2 07/11] block: add delayed bitmap successor cleanup

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap
according to both the individual job's success and the success of all
jobs in the transaction group.

The code for cleaning up a bitmap is also moved from backup_run to
backup_complete.

Signed-off-by: John Snow 
---
  block.c   | 65 ++-
  block/backup.c| 20 ++--
  include/block/block.h | 10 
  3 files changed, 70 insertions(+), 25 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v2 05/11] block: add transactional callbacks feature

[Max Reitz]

On 27.03.2015 20:19, John Snow wrote:

The goal here is to add a new method to transactions that allows
developers to specify a callback that will get invoked only once
all jobs spawned by a transaction are completed, allowing developers
the chance to perform actions conditionally pending complete success,
partial failure, or complete failure.

In order to register the new callback to be invoked, a user must request
a callback pointer and closure by calling new_action_cb_wrapper, which
creates a wrapper around an opaque pointer and callback that would have
originally been passed to e.g. backup_start().

The function will return a function pointer and a new opaque pointer to
be passed instead. The transaction system will effectively intercept the
original callbacks and perform book-keeping on the transaction after it
has delivered the original enveloped callback.

This means that Transaction Action callback methods will be called after
all callbacks triggered by all Actions in the Transactional group have
been received.

This feature has no knowledge of any jobs spawned by Actions that do not
inform the system via new_action_cb_wrapper().

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example
for how to hook up a post-transaction callback to the Drive Backup action.


Note 1: Defining a callback method alone is not sufficient to have the new
 method invoked. You must call new_action_cb_wrapper() AND ensure the
 callback it returns is the one used as the callback for the job
 launched by the action.

Note 2: You can use this feature for any system that registers completions of
 an asynchronous task via a callback of the form
 (void *opaque, int ret), not just block job callbacks.

Signed-off-by: John Snow 
---
  blockdev.c | 191 +++--
  1 file changed, 187 insertions(+), 4 deletions(-)


I like it much better than v1! :-)

Some minor comments below. (But just as in v1, I need to look into the 
following patches to know exactly how some of the functions introduced 
here are used)



diff --git a/blockdev.c b/blockdev.c
index f806d40..d404251 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1239,6 +1239,8 @@ typedef struct BlkActionState BlkActionState;
   * @abort: Abort the changes on fail, can be NULL.
   * @clean: Clean up resources after all transaction actions have called
   * commit() or abort(). Can be NULL.
+ * @cb: Executed after all jobs launched by actions in the transaction finish,
+ *  but only if requested by new_action_cb_wrapper() prior to clean().
   *
   * Only prepare() may fail. In a single transaction, only one of commit() or
   * abort() will be called. clean() will always be called if it is present.
@@ -1249,6 +1251,7 @@ typedef struct BlkActionOps {
  void (*commit)(BlkActionState *common);
  void (*abort)(BlkActionState *common);
  void (*clean)(BlkActionState *common);
+void (*cb)(BlkActionState *common);
  } BlkActionOps;
  
  /**

@@ -1257,19 +1260,46 @@ typedef struct BlkActionOps {
   * by a transaction group.
   *
   * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @status: A cumulative return code for all actions that have reported
+ *  a return code via callback in the transaction.
   * @actions: A list of all Actions in the Transaction.
+ *   However, once the transaction has completed, it will be only a 
list
+ *   of transactions that have registered a post-transaction callback.
   */
  typedef struct BlkTransactionState {
  int jobs;
+int status;
  QTAILQ_HEAD(actions, BlkActionState) actions;
  } BlkTransactionState;
  
+typedef void (CallbackFn)(void *opaque, int ret);

+
+/**
+ * BlkActionCallbackData:
+ * Necessary state for intercepting and
+ * re-delivering a callback triggered by an Action.
+ *
+ * @opaque: The data to be given to the encapsulated callback when
+ *  a job launched by an Action completes.
+ * @ret: The status code that was delivered to the encapsulated callback.
+ * @callback: The encapsulated callback to invoke upon completion of
+ *the Job launched by the Action.
+ */
+typedef struct BlkActionCallbackData {
+void *opaque;
+int ret;
+CallbackFn *callback;
+} BlkActionCallbackData;
+
  /**
   * BlkActionState:
   * Describes one Action's state within a Transaction.
   *
   * @action: QAPI-defined enum identifying which Action to perform.
   * @ops: Table of ActionOps this Action can perform.
+ * @transaction: A pointer back to the Transaction this Action belongs to.
+ * @cb_data: Information on this Action's encapsulated callback, if any.
+ * @refcount: reference count, allowing access to this state beyond clean().
   * @entry: List membership for all Actions in this Transaction.
   *
   * This structure must be arranged as first member in a subcl

Re: [Qemu-block] [PATCH v2 06/11] block: add refcount to Job object

[Max Reitz]

On 27.03.2015 20:20, John Snow wrote:

If we want to get at the job after the life of the job,
we'll need a refcount for this object.

This may occur for example if we wish to inspect the actions
taken by a particular job after a transactional group of jobs
runs, and further actions are required.

Signed-off-by: John Snow 
---
  blockjob.c   | 18 --
  include/block/blockjob.h | 21 +
  2 files changed, 37 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v5 07/21] hbitmap: add hbitmap_merge

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> We add a bitmap merge operation to assist in error cases
> where we wish to combine two bitmaps together.
> 
> This is algorithmically O(bits) provided HBITMAP_LEVELS remains
> constant. For a full bitmap on a 64bit machine:
> sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits
> 
> We may be able to improve running speed for particularly sparse
> bitmaps by using iterators, but the running time for dense maps
> will be worse.
> 
> We present the simpler solution first, and we can refine it later
> if needed.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> Reviewed-by: Stefan Hajnoczi 
> ---

>  /**
> + * hbitmap_merge:
> + * @a: The bitmap to store the result in.
> + * @b: The bitmap to merge into @a.
> + *
> + * Merge two bitmaps together.
> + * A := A (BITOR) B.
> + * B is left unmodified.
> + */
> +bool hbitmap_merge(HBitmap *a, const HBitmap *b);

No mention of what the return value means.


> +bool hbitmap_merge(HBitmap *a, const HBitmap *b)
> +{
> +int i, j;
> +
> +if ((a->size != b->size) || (a->granularity != b->granularity)) {
> +return false;
> +}
> +
> +if (hbitmap_count(b) == 0) {
> +return true;
> +}
> +
> +/* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are 
> constant.
> + * It may be possible to improve running times for sparsely populated 
> maps
> + * by using hbitmap_iter_next, but this is suboptimal for dense maps.
> + */
> +for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
> +for (j = 0; j < a->sizes[i]; j++) {

j is 'int', but a->sizes[i] is uint64_t.  If we can ever have a bitmap
large enough that its size exceeds 2G at the largest level, then we have
an inf-loop due to overflow problem.

I'd feel safer if you make j be uint64_t here; but it might also be
possible to fix 6/21 to store sizes in a smaller data type along with
appropriate assertions that our bitmaps are never big enough to need a
level with more than 2G size.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 06/21] hbitmap: cache array lengths

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> As a convenience: between incremental backups, bitmap migrations
> and bitmap persistence we seem to need to recalculate these a lot.
> 
> Because the lengths are a little bit-twiddly, let's just solidly
> cache them and be done with it.
> 
> Reviewed-by: Max Reitz 
> Signed-off-by: John Snow 
> ---
>  util/hbitmap.c | 4 
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 04/11] block: re-add BlkTransactionState

[Max Reitz]

On 27.03.2015 20:19, John Snow wrote:

Now that the structure formerly known as BlkTransactionState has been
renamed to something sensible (BlkActionState), re-introduce an actual
BlkTransactionState that actually manages state for the entire Transaction.

In the process, convert the old QSIMPLEQ list of actions into a QTAILQ,
to let us more efficiently delete items in arbitrary order, which will
be more important in the future when some actions will expire at the end
of the transaction, but others may persist until all callbacks triggered
by the transaction are recollected.

Signed-off-by: John Snow 
---
  blockdev.c | 66 +++---
  1 file changed, 59 insertions(+), 7 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v5 01/21] docs: incremental backup documentation

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> Reviewed-by: Max Reitz 
> Signed-off-by: John Snow 
> ---
>  docs/bitmaps.md | 311 
> 
>  1 file changed, 311 insertions(+)
>  create mode 100644 docs/bitmaps.md
> 
> diff --git a/docs/bitmaps.md b/docs/bitmaps.md
> new file mode 100644
> index 000..ad8c33b
> --- /dev/null
> +++ b/docs/bitmaps.md
> @@ -0,0 +1,311 @@
> +# Dirty Bitmaps and Incremental Backup
> +

Still might be nice to list explicit copyright/license instead of
relying on implicit top-level GPLv2+, but I won't insist.


> +### Deletion
> +
> +* Can be performed on a disabled bitmap, but not a frozen one.

Do you still have a notion of disabled bitmaps?  Earlier, in '## Bitmap
Modes', you only document 'frozen' (as opposed to the default unnamed
state).


> +
> +## Transactions (Not yet implemented)

I'm assuming that "[PATCH v2 00/11] block: incremental backup
transactions" is incomplete, because it forgot to clean this up as part
of adding transaction support.


> +
> +5. Retry the command after fixing the underlaying problem,

s/underlaying/underlying/

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 04/21] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Eric Blake]

On 04/08/2015 04:19 PM, John Snow wrote:
> The new command pair is added to manage a user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
> 
> The granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.
> 
> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> which takes a device name and a dirty bitmap name and validates the
> lookup, returning NULL and setting errp if there is a problem with
> either field. This helper will be re-used in future patches in this
> series.
> 
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> Reviewed-by: Stefan Hajnoczi 
> ---
>  block.c   |  20 +
>  block/mirror.c|  10 +
>  blockdev.c| 117 
> ++
>  include/block/block.h |   1 +
>  qapi/block-core.json  |  55 
>  qmp-commands.hx   |  56 
>  6 files changed, 250 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 03/11] block: rename BlkTransactionState and BdrvActionOps

[Max Reitz]

On 27.03.2015 20:19, John Snow wrote:

These structures are misnomers, somewhat.

(1) BlockTransactionState is not state for a transaction,
 but is rather state for a single transaction action.
 Rename it "BlkActionState" to be more accurate.

(2) The BdrvActionOps describes operations for the BlkActionState,
 above. This name might imply a 'BdrvAction' or a 'BdrvActionState',
 which there isn't.
 Rename this to 'BlkActionOps' to match 'BlkActionState'.

Lastly, update the surrounding in-line documentation and comments
to reflect the current nature of how Transactions operate.

This patch changes only comments and names, and should not affect
behavior in any way.

Signed-off-by: John Snow 
---
  blockdev.c | 114 ++---
  1 file changed, 64 insertions(+), 50 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v2 01/11] qapi: Add transaction support to block-dirty-bitmap operations

[Eric Blake]

On 03/27/2015 01:19 PM, John Snow wrote:
> This adds two qmp commands to transactions.
> 
> block-dirty-bitmap-add allows you to create a bitmap simultaneously
> alongside a new full backup to accomplish a clean synchronization
> point.
> 
> block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
> it were new, which can also be used alongside a full backup to
> accomplish a clean synchronization point.
> 
> Signed-off-by: Fam Zheng 
> Signed-off-by: John Snow 
> ---
>  blockdev.c   | 100 
> +++
>  qapi-schema.json |   6 +++-
>  2 files changed, 105 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 02/11] iotests: add transactional incremental backup test

[Max Reitz]

On 27.03.2015 20:19, John Snow wrote:

Test simple usage cases for using transactions to create
and synchronize incremental backups.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 51 ++
  tests/qemu-iotests/124.out |  4 ++--
  2 files changed, 53 insertions(+), 2 deletions(-)


Saying the obvious: This patch will need fixup for v5 of the 
transaction-less series.


Max

Re: [Qemu-block] [PATCH v5 21/21] iotests: add incremental backup granularity tests

[Max Reitz]

On 09.04.2015 00:20, John Snow wrote:

Test what happens if you fiddle with the granularity.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 58 +-
  tests/qemu-iotests/124.out |  4 ++--
  2 files changed, 49 insertions(+), 13 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v5 20/21] iotests: add incremental backup failure recovery test

[Max Reitz]

On 09.04.2015 00:20, John Snow wrote:

Test the failure case for incremental backups.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 57 ++
  tests/qemu-iotests/124.out |  4 ++--
  2 files changed, 59 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v2 01/11] qapi: Add transaction support to block-dirty-bitmap operations

[Max Reitz]

On 27.03.2015 20:19, John Snow wrote:

This adds two qmp commands to transactions.

block-dirty-bitmap-add allows you to create a bitmap simultaneously
alongside a new full backup to accomplish a clean synchronization
point.

block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
it were new, which can also be used alongside a full backup to
accomplish a clean synchronization point.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
---
  blockdev.c   | 100 +++
  qapi-schema.json |   6 +++-
  2 files changed, 105 insertions(+), 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v5 19/21] iotests: add simple incremental backup case

[Max Reitz]

On 09.04.2015 00:20, John Snow wrote:

Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 174 +++--
  tests/qemu-iotests/124.out |   4 +-
  2 files changed, 172 insertions(+), 6 deletions(-)


I was happier with the previous simpler approach, but I guess I'll have 
to get my happiness somewhere else today.


Reviewed-by: Max Reitz 

Re: [Qemu-block] [PATCH v5 18/21] iotests: add QMP event waiting queue

[Max Reitz]

On 09.04.2015 00:20, John Snow wrote:

A filter is added to allow callers to request very specific
events to be pulled from the event queue, while leaving undesired
events still in the stream.

This allows to poll for completion data for multiple asynchronous
events in any arbitrary order.

A new timeout context is added to the qmp pull_event method's
wait parameter to allow tests to fail if they do not complete
within some expected period of time.

Signed-off-by: John Snow 
---
  scripts/qmp/qmp.py|  6 +-
  tests/qemu-iotests/iotests.py | 38 ++
  2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 20b6ec7..7f9ac1b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -140,7 +140,8 @@ class QEMUMonitorProtocol:
  """
  Get and delete the first available QMP event.
  
-@param wait: block until an event is available (bool)

+@param wait: block until an event is available. If a float is provided,
+ use this as a timeout value. (bool, float)


Ouch... I guess it works, though...


  """
  self.__sock.setblocking(0)
  try:
@@ -151,7 +152,10 @@ class QEMUMonitorProtocol:
  pass
  self.__sock.setblocking(1)
  if not self.__events and wait:
+if isinstance(wait, float):
+self.__sock.settimeout(wait)
  self.__json_read(only_event=True)
+self.__sock.settimeout(None)


If a timeout occurs, __json_read will do nothing and return (I guess...).


  event = self.__events[0]


This will therefore probably return None...


  del self.__events[0]


And I don't know what this will do. Will it be a no-op?


  return event
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1402854..e93e623 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -78,6 +78,23 @@ def create_image(name, size):
  i = i + 512
  file.close()
  
+# Test if 'match' is a recursive subset of 'event'

+def event_match(event, match=None):
+if match is None:
+return True
+
+for key in match:
+if key in event:
+if isinstance(event[key], dict):
+if not event_match(event[key], match[key]):
+return False
+elif event[key] != match[key]:
+return False
+else:
+return False
+
+return True
+
  class VM(object):
  '''A QEMU VM'''
  
@@ -92,6 +109,7 @@ class VM(object):

   '-machine', 'accel=qtest',
   '-display', 'none', '-vga', 'none']
  self._num_drives = 0
+self._events = []
  
  # This can be used to add an unused monitor instance.

  def add_monitor_telnet(self, ip, port):
@@ -202,14 +220,34 @@ class VM(object):
  
  def get_qmp_event(self, wait=False):

  '''Poll for one queued QMP events and return it'''
+if len(self._events) > 0:
+return self._events.pop(0)
  return self._qmp.pull_event(wait=wait)
  
  def get_qmp_events(self, wait=False):

  '''Poll for queued QMP events and return a list of dicts'''
  events = self._qmp.get_events(wait=wait)
+events.extend(self._events)
+del self._events[:]
  self._qmp.clear_events()
  return events
  
+def event_wait(self, name='BLOCK_JOB_COMPLETED', timeout=60.0, match=None):

+# Search cached events
+for event in self._events:
+if (event['event'] == name) and event_match(event, match):
+self._events.remove(event)
+return event
+
+# Poll for new events
+while True:
+event = self._qmp.pull_event(wait=timeout)


So if a timeout occurred, event will probably be None.


+if (event['event'] == name) and event_match(event, match):


And this (indexing event == None) will probably generate an exception. I 
guess it's one way to fail the test, but certainly not the best...


Max


+return event
+self._events.append(event)
+
+return None
+
  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
  
  class QMPTestCase(unittest.TestCase):

Re: [Qemu-block] [PATCH v5 15/21] block: Resize bitmaps on bdrv_truncate

[Max Reitz]

On 09.04.2015 00:19, John Snow wrote:

Signed-off-by: John Snow 
---
  block.c| 18 ++
  include/qemu/hbitmap.h | 10 ++
  util/hbitmap.c | 48 
  3 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 16209a2..42839a0 100644
--- a/block.c
+++ b/block.c
@@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 int nr_sectors);
  static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
   int nr_sectors);
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
  /* If non-zero, use only whitelisted block drivers */
  static int use_bdrv_whitelist;
  
@@ -3583,6 +3584,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)

  ret = drv->bdrv_truncate(bs, offset);
  if (ret == 0) {
  ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+bdrv_dirty_bitmap_truncate(bs);
  if (bs->blk) {
  blk_dev_resize_cb(bs->blk);
  }
@@ -5593,6 +5595,22 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
  return parent;
  }
  
+/**

+ * Truncates _all_ bitmaps attached to a BDS.
+ */
+static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bitmap;
+uint64_t size = bdrv_nb_sectors(bs);
+
+QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (bdrv_dirty_bitmap_frozen(bitmap)) {
+continue;
+}
+hbitmap_truncate(bitmap->bitmap, size);
+}
+}
+
  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {
  BdrvDirtyBitmap *bm, *next;
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index c19c1cb..a75157e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,16 @@ struct HBitmapIter {
  HBitmap *hbitmap_alloc(uint64_t size, int granularity);
  
  /**

+ * hbitmap_truncate:
+ * @hb: The bitmap to change the size of.
+ * @size: The number of elements to change the bitmap to accommodate.
+ *
+ * truncate or grow an existing bitmap to accommodate a new number of elements.
+ * This may invalidate existing HBitmapIterators.
+ */
+void hbitmap_truncate(HBitmap *hb, uint64_t size);
+
+/**
   * hbitmap_merge:
   * @a: The bitmap to store the result in.
   * @b: The bitmap to merge into @a.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index ba11fd3..1ad3bf3 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -400,6 +400,54 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
  return hb;
  }
  
+void hbitmap_truncate(HBitmap *hb, uint64_t size)

+{
+bool shrink;
+unsigned i;
+uint64_t num_elements = size;
+uint64_t old;
+
+/* Size comes in as logical elements, adjust for granularity. */
+size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
+assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
+shrink = size < hb->size;
+
+/* bit sizes are identical; nothing to do. */
+if (size == hb->size) {
+return;
+}
+
+/* If we're losing bits, let's clear those bits before we invalidate all of
+ * our invariants. This helps keep the bitcount consistent, and will 
prevent
+ * us from carrying around garbage bits beyond the end of the map.
+ */
+if (shrink) {
+/* Don't clear partial granularity groups;
+ * start at the first full one. */
+uint64_t start = QEMU_ALIGN_UP(num_elements, 1 << hb->granularity);
+uint64_t fix_count = (hb->size << hb->granularity) - num_elements;


Shouldn't this be s/num_elements/start/?

Max


+
+assert(fix_count);
+hbitmap_reset(hb, start, fix_count);
+}
+
+hb->size = size;
+for (i = HBITMAP_LEVELS; i-- > 0; ) {
+size = MAX(BITS_TO_LONGS(size), 1);
+if (hb->sizes[i] == size) {
+break;
+}
+old = hb->sizes[i];
+hb->sizes[i] = size;
+hb->levels[i] = g_realloc(hb->levels[i], size * sizeof(unsigned long));
+if (!shrink) {
+memset(&hb->levels[i][old], 0x00,
+   (size - old) * sizeof(*hb->levels[i]));
+}
+}
+}
+
+
  /**
   * Given HBitmaps A and B, let A := A (BITOR) B.
   * Bitmap B will not be modified.

Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Max Reitz]

On 09.04.2015 00:19, John Snow wrote:

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

Signed-off-by: Fam Zheng 
Signed-off-by: John Snow 
---
  block.c   |   9 +++
  block/backup.c| 156 +++---
  block/mirror.c|   4 ++
  blockdev.c|  18 +-
  hmp.c |   3 +-
  include/block/block.h |   1 +
  include/block/block_int.h |   2 +
  qapi/block-core.json  |  13 ++--
  qmp-commands.hx   |   7 ++-
  9 files changed, 180 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 9d30379..2367311 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
int64_t cur_sector,
  }
  }
  
+/**

+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+assert(hbi->hb);
+hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
  int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {
  return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..8513917 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
  typedef struct BackupBlockJob {
  BlockJob common;
  BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
  MirrorSyncMode sync_mode;
  RateLimit limit;
  BlockdevOnError on_source_error;
@@ -242,6 +244,92 @@ static void backup_complete(BlockJob *job, void *opaque)
  g_free(data);
  }
  
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)

+{
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+  job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+bool error_is_read;
+int ret = 0;
+int clusters_per_iter;
+uint32_t granularity;
+int64_t sector;
+int64_t cluster;
+int64_t end;
+int64_t last_cluster = -1;
+BlockDriverState *bs = job->common.bs;
+HBitmapIter hbi;
+
+granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);


DIV_ROUND_UP(granularity, BACKUP_CLUSTER_SIZE) would've worked, too 
(instead of the MAX()), but since both are powers of two, this is 
equivalent.



+bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+/* Find the next dirty sector(s) */
+while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped */
+if (cluster != last_cluster + 1) {
+job->common.offset += ((cluster - last_cluster - 1) *
+   BACKUP_CLUSTER_SIZE);
+}
+
+for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
+if (yield_and_check(job)) {
+return ret;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER, 
&error_is_read);
+if ((ret < 0) &&
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+}


Now that I'm reading this code again... The other backup implementation 
handles retries differently; it redoes the whole loop, with the 
effective difference being that it calls yield_and_check() between every 
retry. Would it make sense to move the yield_and_check() call into this 
loop?



+} while (ret < 0);
+}
+
+/* If the bitmap granularity is smaller than the backup granularity,
+ * we need to advance the iterator pointer to the next cluster. */
+if (granularity < BACKUP_CLUSTER_SIZE) {


Actually, whenever BACKUP_CLUSTER_SIZE isn't a factor of granularity. 
Both are powers of two, though, so that's the case iff granularity < 
BACKUP_CLUSTER_SIZE. (thus, the condition is correct)



+bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+

Re: [Qemu-block] [PATCH 4/6] block: Support streaming to an intermediate layer

[Max Reitz]

On 16.04.2015 16:30, Alberto Garcia wrote:

On Wed 15 Apr 2015 06:09:18 PM CEST, Max Reitz  wrote:


+orig_bs_flags = bdrv_get_flags(bs);
+if (!(orig_bs_flags & BDRV_O_RDWR)) {

I feel like we don't want to do this if we're not streaming to an
intermediate layer but to the top layer (because that means there is
some reason for the BDS to be read-only beyond it just being a backing
BDS).

Looks like we don't actually need to do anything, bdrv_reopen_prepare()
already takes care of checking the BDRV_O_ALLOW_RDWR flag, which is not
set if the image was opened in read-only mode.


Oh, that's nice. :-)

Max

Re: [Qemu-block] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Alberto Garcia]

On Fri 17 Apr 2015 01:52:43 PM CEST, Alberto Garcia  wrote:

> Anyone calling bdrv_block_device_info() directly will get a null image
> field. As a consequence of this, the HMP command 'info block -n -v'
> crashes QEMU.
>
> This patch moves the code that fills in that field from
> bdrv_query_info() to bdrv_block_device_info().

And in case this change is too big/risky for 2.3, there's also the
simple workaround for the crash:

--- a/hmp.c
+++ b/hmp.c
@@ -391,7 +391,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
 inserted->iops_size);
 }
 
-if (verbose) {
+if (verbose && inserted->image) {
 monitor_printf(mon, "\nImages:\n");
 image_info = inserted->image;
 while (1) {

Berto

[Qemu-block] [PATCH] qmp: fill in the image field in BlockDeviceInfo

[Alberto Garcia]

The image field in BlockDeviceInfo is supposed to contain an ImageInfo
object. However that is being filled in by bdrv_query_info(), not by
bdrv_block_device_info(), which is where BlockDeviceInfo is actually
created.

Anyone calling bdrv_block_device_info() directly will get a null image
field. As a consequence of this, the HMP command 'info block -n -v'
crashes QEMU.

This patch moves the code that fills in that field from
bdrv_query_info() to bdrv_block_device_info().

Signed-off-by: Alberto Garcia 
---
 block.c   |  9 +++--
 block/qapi.c  | 46 +-
 blockdev.c|  2 +-
 include/block/block.h |  2 +-
 include/block/qapi.h  |  2 +-
 5 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index f2f8ae7..9db2ad9 100644
--- a/block.c
+++ b/block.c
@@ -3870,15 +3870,20 @@ BlockDriverState *bdrv_find_node(const char *node_name)
 }
 
 /* Put this QMP function here so it can access the static graph_bdrv_states. */
-BlockDeviceInfoList *bdrv_named_nodes_list(void)
+BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
 {
 BlockDeviceInfoList *list, *entry;
 BlockDriverState *bs;
 
 list = NULL;
 QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+BlockDeviceInfo *info = bdrv_block_device_info(bs, errp);
+if (!info) {
+qapi_free_BlockDeviceInfoList(list);
+return NULL;
+}
 entry = g_malloc0(sizeof(*entry));
-entry->value = bdrv_block_device_info(bs);
+entry->value = info;
 entry->next = list;
 list = entry;
 }
diff --git a/block/qapi.c b/block/qapi.c
index 8a19aed..063dd1b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -31,8 +31,10 @@
 #include "qapi/qmp/types.h"
 #include "sysemu/block-backend.h"
 
-BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
+BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
 {
+ImageInfo **p_image_info;
+BlockDriverState *bs0;
 BlockDeviceInfo *info = g_malloc0(sizeof(*info));
 
 info->file   = g_strdup(bs->filename);
@@ -92,6 +94,25 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
 
 info->write_threshold = bdrv_write_threshold_get(bs);
 
+bs0 = bs;
+p_image_info = &info->image;
+while (1) {
+Error *local_err = NULL;
+bdrv_query_image_info(bs0, p_image_info, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+qapi_free_BlockDeviceInfo(info);
+return NULL;
+}
+if (bs0->drv && bs0->backing_hd) {
+bs0 = bs0->backing_hd;
+(*p_image_info)->has_backing_image = true;
+p_image_info = &((*p_image_info)->backing_image);
+} else {
+break;
+}
+}
+
 return info;
 }
 
@@ -264,9 +285,6 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
 {
 BlockInfo *info = g_malloc0(sizeof(*info));
 BlockDriverState *bs = blk_bs(blk);
-BlockDriverState *bs0;
-ImageInfo **p_image_info;
-Error *local_err = NULL;
 info->device = g_strdup(blk_name(blk));
 info->type = g_strdup("unknown");
 info->locked = blk_dev_is_medium_locked(blk);
@@ -289,23 +307,9 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
 
 if (bs->drv) {
 info->has_inserted = true;
-info->inserted = bdrv_block_device_info(bs);
-
-bs0 = bs;
-p_image_info = &info->inserted->image;
-while (1) {
-bdrv_query_image_info(bs0, p_image_info, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto err;
-}
-if (bs0->drv && bs0->backing_hd) {
-bs0 = bs0->backing_hd;
-(*p_image_info)->has_backing_image = true;
-p_image_info = &((*p_image_info)->backing_image);
-} else {
-break;
-}
+info->inserted = bdrv_block_device_info(bs, errp);
+if (info->inserted == NULL) {
+goto err;
 }
 }
 
diff --git a/blockdev.c b/blockdev.c
index fbb3a79..65e9bb6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2391,7 +2391,7 @@ out:
 
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
-return bdrv_named_nodes_list();
+return bdrv_named_nodes_list(errp);
 }
 
 void qmp_blockdev_backup(const char *device, const char *target,
diff --git a/include/block/block.h b/include/block/block.h
index 4c57d63..40131b2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -382,7 +382,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find_node(const char *node_name);
-BlockDeviceInfoList *bdrv_named_nodes_list(void);
+Bloc

Re: [Qemu-block] [Qemu-devel] [PATCH 4/6] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 16.04.2015 um 14:34 hat Alberto Garcia geschrieben:
> On Thu 16 Apr 2015 02:27:39 PM CEST, Eric Blake wrote:
> 
>  +orig_bs_flags = bdrv_get_flags(bs);
>  +if (!(orig_bs_flags & BDRV_O_RDWR)) {
> >>>
> >>> I feel like we don't want to do this if we're not streaming to an
> >>> intermediate layer but to the top layer (because that means there is
> >>> some reason for the BDS to be read-only beyond it just being a
> >>> backing BDS).
> >> 
> >> I didn't think about this... that I can fix easily, but I wonder
> >> what's the scenario where the top layer is read-only.
> >
> > Suppose I tell qemu to open the chain 'base <- mid <- top' read-only,
> > and then later decide to stream base into mid or top. It should be
> > fine from the guest's perspective to keep the guest in read-only mode,
> > while still using streaming to reduce the chain.
> 
> Ok, I'll add the check for the top image then.
> 
> >> I actually tried with scenarios such as A>B>C>D>E, streaming from B
> >> to E and from A to C simultaneously, and it seems to work (at least I
> >> didn't see any obvious problems), but I don't think we want to
> >> support that because a) I don't see the use case and b) we're likely
> >> opening a can of worms.
> >
> > Streaming doesn't corrupt any part of the chain, so I think you are
> > safe doing overlapping streams like that.  However, I also agree that
> > it is not worth supporting, and that we are safer not allowing
> > overlapping streams.
> 
> I'm thinking about cases where streaming from B to E finishes (and all
> the intermediate nodes are removed from the chain) while A to C is still
> ongoing. At the very least it doesn't seem to make sense to allow that
> kind of things.

Let's stay conservative with what we allow as long as we don't have the
new op blockers yet. As soon as we have them, it might be a lot easier
and less error prone to accurately describe the real requirements.

Kevin

Re: [Qemu-block] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Peter Lieven]

Am 16.04.2015 um 15:20 schrieb Paolo Bonzini:
>
> On 16/04/2015 14:58, Peter Lieven wrote:
>>> On 16/04/2015 14:18, Peter Lieven wrote:
 We need this to support SCSI_STATUS_TASK_SET_FULL.
>>> Any reason apart from the missing constant?
>> No, but I wanted to avoid starting checking for constants that were
>> added shortly after this.
>> You can't check with #ifdef for a constant in an enum.
> But you can #define it if libiscsi version is <1.10.

There is no macro to check for that. I took the easy way
hardcoding the value. Its an official standard so there is
no chance it will change.

Peter

Re: [Qemu-block] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Peter Lieven]

Am 16.04.2015 um 15:17 schrieb Paolo Bonzini:
>
> On 16/04/2015 15:02, Peter Lieven wrote:
>>> Also, I think it is iscsi_co_generic_cb that should set
>>> force_next_flush, so that it is only set on failure.  Not really for the
>>> optimization value, but because it's clearer.
>> I don't get what you mean with it should only "set on failure".
>> My approach would be to add a flag to the iTask that the command
>> requires to set force_flush after successful completion. Is it that
>> what you mean?
> Set on success.  Lack of sleep.

I have send a v2 following your suggestions.

Peter