[Qemu-block] [PATCH] blockjob: cancel blockjobs before stopping all iothreads

[sochin.jiang]

From: "sochin.jiang" 

commit 88b062c and commit c9d1a56 introduce BDRV_POLL_WHILE,
checking bs->in_flight in the main thread and exit till
bs->in_flight reaches 0. This leaves a chance that
BDRV_POLL_WHILE will hang until iothread complete the
relative block job when bdrv_drain is called from outside
the I/O thread, say call 'iothread_stop_all' when shutting
down the vm. Cancelling blockjobs seems more reasonable before
iothread_stop_all is called when vm shutdown.

Signed-off-by: sochin.jiang 
---
 block.c  | 9 -
 blockjob.c   | 5 +
 include/block/block.h| 1 +
 include/block/blockjob.h | 7 +++
 vl.c | 1 +
 5 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index fa1d06d..b564c29 100644
--- a/block.c
+++ b/block.c
@@ -3084,9 +3084,16 @@ static void bdrv_close(BlockDriverState *bs)
 bdrv_drained_end(bs);
 }
 
+void bdrv_cancel_all(void)
+{
+if (!block_jobs_is_empty()) {
+block_job_cancel_sync_all();
+}
+}
+
 void bdrv_close_all(void)
 {
-block_job_cancel_sync_all();
+bdrv_cancel_all();
 nbd_export_close_all();
 
 /* Drop references from requests still in flight, such as canceled block
diff --git a/blockjob.c b/blockjob.c
index a0d7e29..c659994 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -539,6 +539,11 @@ void block_job_cancel_sync_all(void)
 }
 }
 
+bool block_jobs_is_empty(void)
+{
+return QLIST_EMPTY(_jobs);
+}
+
 int block_job_complete_sync(BlockJob *job, Error **errp)
 {
 return block_job_finish_sync(job, _job_complete, errp);
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e9..0ebbd50 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -376,6 +376,7 @@ int bdrv_inactivate_all(void);
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
 int bdrv_flush_all(void);
+void bdrv_cancel_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 09c7c69..d9ee58d 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -282,6 +282,13 @@ int block_job_cancel_sync(BlockJob *job);
 void block_job_cancel_sync_all(void);
 
 /**
+ * block_jobs_is_empty:
+ *
+ * Check whether block_jobs is empty.
+ */
+bool block_jobs_is_empty(void);
+
+/**
  * block_job_complete_sync:
  * @job: The job to be completed.
  * @errp: Error object which may be set by block_job_complete(); this is not
diff --git a/vl.c b/vl.c
index 80b86c0..cff6ec7 100644
--- a/vl.c
+++ b/vl.c
@@ -4751,6 +4751,7 @@ int main(int argc, char **argv, char **envp)
 
 main_loop();
 replay_disable_events();
+bdrv_cancel_all();
 iothread_stop_all();
 
 bdrv_close_all();
-- 
1.8.3.1

Re: [Qemu-block] [PATCH 0/2] commit: Fix use after free in completion

[John Snow]

On 06/02/2017 05:12 PM, Kevin Wolf wrote:
> Kevin Wolf (2):
>   commit: Fix use after free in completion
>   qemu-iotests: Test automatic commit job cancel on hot unplug
> 
>  block/commit.c |  7 +++
>  tests/qemu-iotests/040 | 35 +--
>  tests/qemu-iotests/040.out |  4 ++--
>  3 files changed, 42 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH v20 14/30] qcow2: support .bdrv_reopen_bitmaps_rw

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Realize bdrv_reopen_bitmaps_rw interface.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: John Snow 

> ---
>  block/qcow2-bitmap.c | 61 
> 
>  block/qcow2.c|  2 ++
>  block/qcow2.h|  1 +
>  3 files changed, 64 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 2c7b057e21..a21fab8ce8 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -826,3 +826,64 @@ fail:
>  
>  return false;
>  }
> +
> +int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +Qcow2BitmapList *bm_list;
> +Qcow2Bitmap *bm;
> +GSList *ro_dirty_bitmaps = NULL;
> +int ret = 0;
> +
> +if (s->nb_bitmaps == 0) {
> +/* No bitmaps - nothing to do */
> +return 0;
> +}
> +
> +if (!can_write(bs)) {
> +error_setg(errp, "Can't write to the image on reopening bitmaps rw");
> +return -EINVAL;
> +}
> +
> +bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +   s->bitmap_directory_size, errp);
> +if (bm_list == NULL) {
> +return -EINVAL;
> +}
> +
> +QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +if (!(bm->flags & BME_FLAG_IN_USE)) {
> +BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +if (bitmap == NULL) {
> +continue;
> +}
> +
> +if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> +error_setg(errp, "Bitmap %s is not readonly but not marked"
> + "'IN_USE' in the image. Something went 
> wrong,"
> + "all the bitmaps may be corrupted", 
> bm->name);
> +ret = -EINVAL;
> +goto out;
> +}
> +
> +bm->flags |= BME_FLAG_IN_USE;
> +ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +}
> +}
> +
> +if (ro_dirty_bitmaps != NULL) {
> +/* in_use flags must be updated */
> +ret = update_ext_header_and_dir_in_place(bs, bm_list);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret, "Can't update bitmap directory");
> +goto out;
> +}
> +g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
> +}
> +
> +out:
> +g_slist_free(ro_dirty_bitmaps);
> +bitmap_list_free(bm_list);
> +
> +return ret;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a70d284b75..ec00db7e49 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3596,6 +3596,8 @@ BlockDriver bdrv_qcow2 = {
>  
>  .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>  .bdrv_attach_aio_context  = qcow2_attach_aio_context,
> +
> +.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
>  };
>  
>  static void bdrv_qcow2_init(void)
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 67c61de008..3e23bb7361 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -630,5 +630,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>void **refcount_table,
>int64_t *refcount_table_size);
>  bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
> +int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  
>  #endif
>

Re: [Qemu-block] [Qemu-devel] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add format driver handler, which should mark loaded read-only
> bitmaps as 'IN_USE' in the image and unset read_only field in
> corresponding BdrvDirtyBitmap's.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c   | 17 +
>  include/block/block_int.h |  7 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 04af7697dc..161db9e32a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  {
>  BlockDriver *drv;
>  BlockDriverState *bs;
> +bool old_can_write, new_can_write;
>  
>  assert(reopen_state != NULL);
>  bs = reopen_state->bs;
>  drv = bs->drv;
>  assert(drv != NULL);
>  
> +old_can_write =
> +!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
> +
>  /* If there are any driver level actions to take */
>  if (drv->bdrv_reopen_commit) {
>  drv->bdrv_reopen_commit(reopen_state);
> @@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>  
>  bdrv_refresh_limits(bs, NULL);
> +
> +new_can_write =
> +!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
> +if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
> +Error *local_err = NULL;
> +if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
> +/* This is not fatal, bitmaps just left read-only, so all 
> following
> + * writes will fail. User can remove read-only bitmaps to unblock
> + * writes.
> + */
> +error_report_err(local_err);
> +}
> +}
>  }
>  
>  /*
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d3724cce6..1dc6f2e90d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -380,6 +380,13 @@ struct BlockDriver {
>   uint64_t parent_perm, uint64_t parent_shared,
>   uint64_t *nperm, uint64_t *nshared);
>  
> +/**
> + * Bitmaps should be marked as 'IN_USE' in the image on reopening image
> + * as rw. This handler should realize it. It also should unset readonly
> + * field of BlockDirtyBitmap's in case of success.
> + */
> +int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
> +

Hmm, do we need a new top-level hook for this? We already have
.bdrv_reopen_commit and .bdrv_reopen_prepare which inform the
blockdriver that a reopen event is occurring.

Can't we just amend qcow2_reopen_prepare and qcow2_reopen_commit to do
the necessary tasks of either:

(A) Flushing the bitmap in preparation for reopening as RO, or
(B) Writing in_use and removing the RO flag in preparation for reopening
as RW

>  QLIST_ENTRY(BlockDriver) list;
>  };
>  
> 


Well, design issues aside, I will at least confirm that I think this
patch should work as designed, and I will leave the design critiques to
Max and Kevin:

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH v20 12/30] block: refactor bdrv_reopen_commit

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add bs local variable to simplify code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..04af7697dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -2945,9 +2945,11 @@ error:
>  void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  {
>  BlockDriver *drv;
> +BlockDriverState *bs;
>  
>  assert(reopen_state != NULL);
> -drv = reopen_state->bs->drv;
> +bs = reopen_state->bs;
> +drv = bs->drv;
>  assert(drv != NULL);
>  
>  /* If there are any driver level actions to take */
> @@ -2956,13 +2958,13 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  }
>  
>  /* set BDS specific flags now */
> -QDECREF(reopen_state->bs->explicit_options);
> +QDECREF(bs->explicit_options);
>  
> -reopen_state->bs->explicit_options   = reopen_state->explicit_options;
> -reopen_state->bs->open_flags = reopen_state->flags;
> -reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> +bs->explicit_options   = reopen_state->explicit_options;
> +bs->open_flags = reopen_state->flags;
> +bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>  
> -bdrv_refresh_limits(reopen_state->bs, NULL);
> +bdrv_refresh_limits(bs, NULL);
>  }
>  
>  /*
> 

It's not immediately obvious that this is safe (can reopen_commit change
reopen_state->bs ?) -- but it doesn't, so:

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH v20 11/30] qcow2: autoloading dirty bitmaps

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
> are loaded when the image is opened and become BdrvDirtyBitmaps for the
> corresponding drive.
> 
> Extra data in bitmaps is not supported for now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Max Reitz 

Reviewed-by: John Snow 

> ---
>  block/qcow2-bitmap.c | 389 
> +++
>  block/qcow2.c|  17 ++-
>  block/qcow2.h|   2 +
>  3 files changed, 406 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b8e472b3e8..2c7b057e21 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -44,6 +44,8 @@
>  
>  /* Bitmap directory entry flags */
>  #define BME_RESERVED_FLAGS 0xfffcU
> +#define BME_FLAG_IN_USE (1U << 0)
> +#define BME_FLAG_AUTO   (1U << 1)
>  
>  /* bits [1, 8] U [56, 63] are reserved */
>  #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
> @@ -85,6 +87,23 @@ typedef enum BitmapType {
>  BT_DIRTY_TRACKING_BITMAP = 1
>  } BitmapType;
>  
> +static inline bool can_write(BlockDriverState *bs)
> +{
> +return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
> +}
> +
> +static int update_header_sync(BlockDriverState *bs)
> +{
> +int ret;
> +
> +ret = qcow2_update_header(bs);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +return bdrv_flush(bs);
> +}
> +
>  static int check_table_entry(uint64_t entry, int cluster_size)
>  {
>  uint64_t offset;
> @@ -146,6 +165,120 @@ fail:
>  return ret;
>  }
>  
> +/* This function returns the number of disk sectors covered by a single qcow2
> + * cluster of bitmap data. */
> +static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
> +  const BdrvDirtyBitmap 
> *bitmap)
> +{
> +uint32_t sector_granularity =
> +bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> +
> +return (uint64_t)sector_granularity * (s->cluster_size << 3);
> +}
> +
> +/* load_bitmap_data
> + * @bitmap_table entries must satisfy specification constraints.
> + * @bitmap must be cleared */
> +static int load_bitmap_data(BlockDriverState *bs,
> +const uint64_t *bitmap_table,
> +uint32_t bitmap_table_size,
> +BdrvDirtyBitmap *bitmap)
> +{
> +int ret = 0;
> +BDRVQcow2State *s = bs->opaque;
> +uint64_t sector, sbc;
> +uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +uint8_t *buf = NULL;
> +uint64_t i, tab_size =
> +size_to_clusters(s,
> +bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> +if (tab_size != bitmap_table_size || tab_size > BME_MAX_TABLE_SIZE) {
> +return -EINVAL;
> +}
> +
> +buf = g_malloc(s->cluster_size);
> +sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
> +for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
> +uint64_t count = MIN(bm_size - sector, sbc);
> +uint64_t entry = bitmap_table[i];
> +uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +
> +assert(check_table_entry(entry, s->cluster_size) == 0);
> +
> +if (offset == 0) {
> +if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
> +bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, count,
> +   false);
> +} else {
> +/* No need to deserialize zeros because the dirty bitmap is
> + * already cleared */
> +}
> +} else {
> +ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
> +if (ret < 0) {
> +goto finish;
> +}
> +bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, count,
> +   false);
> +}
> +}
> +ret = 0;
> +
> +bdrv_dirty_bitmap_deserialize_finish(bitmap);
> +
> +finish:
> +g_free(buf);
> +
> +return ret;
> +}
> +
> +static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
> +Qcow2Bitmap *bm, Error **errp)
> +{
> +int ret;
> +uint64_t *bitmap_table = NULL;
> +uint32_t granularity;
> +BdrvDirtyBitmap *bitmap = NULL;
> +
> +if (bm->flags & BME_FLAG_IN_USE) {
> +error_setg(errp, "Bitmap '%s' is in use", bm->name);
> +goto fail;
> +}
> +
> +ret = bitmap_table_load(bs, >table, _table);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret,
> + "Could not read bitmap_table table from image for "
> + "bitmap '%s'", bm->name);
> +goto fail;
> +}
> +
> +granularity = 1U << 

[Qemu-block] [PATCH 2/2] qemu-iotests: Test automatic commit job cancel on hot unplug

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/040 | 35 +--
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 5bdaf3d..9d381d9 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -70,7 +70,9 @@ class ImageCommitTestCase(iotests.QMPTestCase):
 self.wait_for_complete()
 
 class TestSingleDrive(ImageCommitTestCase):
-image_len = 1 * 1024 * 1024
+# Need some space after the copied data so that throttling is effective in
+# tests that use it rather than just completing the job immediately
+image_len = 2 * 1024 * 1024
 test_len = 1 * 1024 * 256
 
 def setUp(self):
@@ -79,7 +81,9 @@ class TestSingleDrive(ImageCommitTestCase):
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
 qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
 qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', 
mid_img)
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive(test_img, interface="none")
+self.vm.add_device("virtio-scsi-pci")
+self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
 self.vm.launch()
 
 def tearDown(self):
@@ -131,6 +135,33 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assert_qmp(result, 'error/class', 'GenericError')
 self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % 
mid_img)
 
+# When the job is running on a BB that is automatically deleted on hot
+# unplug, the job is cancelled when the device disappears
+def test_hot_unplug(self):
+if self.image_len == 0:
+return
+
+self.assert_no_active_block_jobs()
+result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
+ base=backing_img, speed=(self.image_len / 4))
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('device_del', id='scsi0')
+self.assert_qmp(result, 'return', {})
+
+cancelled = False
+deleted = False
+while not cancelled or not deleted:
+for event in self.vm.get_qmp_events(wait=True):
+if event['event'] == 'DEVICE_DELETED':
+self.assert_qmp(event, 'data/device', 'scsi0')
+deleted = True
+elif event['event'] == 'BLOCK_JOB_CANCELLED':
+self.assert_qmp(event, 'data/device', 'drive0')
+cancelled = True
+else:
+self.fail("Unexpected event %s" % (event['event']))
+
+self.assert_no_active_block_jobs()
 
 class TestRelativePaths(ImageCommitTestCase):
 image_len = 1 * 1024 * 1024
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 4fd1c2d..6d9bee1 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-.
+...
 --
-Ran 25 tests
+Ran 27 tests
 
 OK
-- 
1.8.3.1

[Qemu-block] [PATCH 0/2] commit: Fix use after free in completion

[Kevin Wolf]

Kevin Wolf (2):
  commit: Fix use after free in completion
  qemu-iotests: Test automatic commit job cancel on hot unplug

 block/commit.c |  7 +++
 tests/qemu-iotests/040 | 35 +--
 tests/qemu-iotests/040.out |  4 ++--
 3 files changed, 42 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-block] [PATCH 1/2] commit: Fix use after free in completion

[Kevin Wolf]

The final bdrv_set_backing_hd() could be working on already freed nodes
because the commit job drops its references (through BlockBackends) to
both overlay_bs and top already a bit earlier.

One way to trigger the bug is hot unplugging a disk for which
blockdev_mark_auto_del() cancels the block job.

Fix this by taking BDS-level references while we're still using the
nodes.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index a3028b2..af6fa68 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
 int ret = data->ret;
 bool remove_commit_top_bs = false;
 
+/* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
+bdrv_ref(top);
+bdrv_ref(overlay_bs);
+
 /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
  * the normal backing chain can be restored. */
 blk_unref(s->base);
@@ -124,6 +128,9 @@ static void commit_complete(BlockJob *job, void *opaque)
 if (remove_commit_top_bs) {
 bdrv_set_backing_hd(overlay_bs, top, _abort);
 }
+
+bdrv_unref(overlay_bs);
+bdrv_unref(top);
 }
 
 static void coroutine_fn commit_run(void *opaque)
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v20 10/30] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/dirty-bitmap.c | 32 
>  block/io.c   |  8 
>  blockdev.c   |  6 ++
>  include/block/dirty-bitmap.h |  4 
>  4 files changed, 50 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index f25428868c..1c9ffb292a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -45,6 +45,12 @@ struct BdrvDirtyBitmap {
>  bool disabled;  /* Bitmap is disabled. It skips all writes to
> the device */
>  int active_iterators;   /* How many iterators are active */
> +bool readonly;  /* Bitmap is read-only and may be changed 
> only
> +   by deserialize* functions. This field 
> blocks

In what way do the deserialize functions change the bitmaps, again?

> +   any changing operations on owning image
> +   (writes and discards), if bitmap is 
> readonly
> +   such operations must fail and not change
> +   image or this bitmap */
>  QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  
> @@ -437,6 +443,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int64_t nr_sectors)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));

I still feel as if bdrv_dirty_bitmap_enabled() can return false if
bdrv_dirty_bitmap_readonly is true, and you wouldn't have to edit these
parts, but I don't care enough to press the issue.

>  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
> @@ -444,12 +451,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>   int64_t cur_sector, int64_t nr_sectors)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));
>  hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>  {
>  assert(bdrv_dirty_bitmap_enabled(bitmap));
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));
>  if (!out) {
>  hbitmap_reset_all(bitmap->bitmap);
>  } else {
> @@ -520,6 +529,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> cur_sector,
>  if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>  continue;
>  }
> +assert(!bdrv_dirty_bitmap_readonly(bitmap));

Highlighting the difference in strictness between "disabled" and "readonly."

>  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  }
> @@ -541,3 +551,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap 
> *bitmap)
>  {
>  return hbitmap_count(bitmap->meta);
>  }
> +
> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
> +{
> +return bitmap->readonly;
> +}
> +
> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value)
> +{
> +bitmap->readonly = value;
> +}
> +
> +bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
> +{
> +BdrvDirtyBitmap *bm;
> +QLIST_FOREACH(bm, >dirty_bitmaps, list) {
> +if (bm->readonly) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> diff --git a/block/io.c b/block/io.c
> index fdd7485c22..0e28a1f595 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
> *child,
>  uint64_t bytes_remaining = bytes;
>  int max_transfer;
>  
> +if (bdrv_has_readonly_bitmaps(bs)) {
> +return -EPERM;
> +}
> +

Should this be a dynamic error, or an assertion? We should probably
endeavor to never actually hit this circumstance (we should not have
readonly bitmaps on a RW node.)

>  assert(is_power_of_2(align));
>  assert((offset & (align - 1)) == 0);
>  assert((bytes & (align - 1)) == 0);
> @@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState 
> *bs, int64_t offset,
>  return -ENOMEDIUM;
>  }
>  
> +if (bdrv_has_readonly_bitmaps(bs)) {
> +return -EPERM;
> +}
> +
>  ret = bdrv_check_byte_request(bs, offset, count);
>  if (ret < 0) {
>  return ret;
> diff --git a/blockdev.c b/blockdev.c
> index c63f4e82c7..2b397abf66 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2033,6 +2033,9 @@ static void 
> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>  } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
>  

Re: [Qemu-block] [Qemu-devel] [PATCH v20 09/30] block/dirty-bitmap: fix comment for BlockDirtyBitmap.disabled field

[John Snow]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/dirty-bitmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 90af37287f..f25428868c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -42,7 +42,8 @@ struct BdrvDirtyBitmap {
>  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 */
> +bool disabled;  /* Bitmap is disabled. It skips all writes to
> +   the device */


Or, "Bitmap is disabled. Writes to the device are ignored." or similar.

It's not very important.

Reviewed-by: John Snow 

>  int active_iterators;   /* How many iterators are active */
>  QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[John Snow]

On 06/02/2017 05:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> 02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote:
>> 02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote:
>>> 02.06.2017 02:25, John Snow wrote:

 On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:
> Hi John!
>
> Look at our discussion about this in v18 thread.
>> Shortly: readonly is not the same as disabled. disabled= bitmap just
> ignores all writes. readonly= writes are not allowed at all.
>
> And I think, I'll try to go through way 2: "dirty" field instead of
> "readonly" (look at v18 discussion), as it a bit more flexible.
>
 Not sure which I prefer...

 Method 1 is attractive in that it is fairly simple, and enforces fairly
 loudly the inability to write to devices with RO bitmaps. It's a
 natural
 extension of your current approach.
>>>
>>> For now I decided to realize this one, I think I'll publish it today.
>>> Also, I'm going to rename s/readonly/in_use - to avoid the confuse
>>> with disabled. So let this field just be mirror of IN_USE in the
>>> image and just say "persistent storage knows, that bitmap is in use
>>> and may be dirty".
> 
> Finally it would be readonly. in_use is bad for created (not loaded)
> bitmaps. I'll add more descriptive comments for disabled and readonly.
> 

Makes sense. It sounds like "readonly" is simply a stricter superset of
"disabled," where "disabled" doesn't care if the bitmap gets out of sync
with the data, but "readonly" attempts to preserve that semantic
relationship.

>>>
>>> Also, optimization with 'dirty' flag may be added later.
>>

Yes, I agree.

>> And, also, I don't want to influence this "first write", on which we
>> will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less
>> performance-demanding place than write.
>>

"And, also," -- I think you've been reading my emails too much, you're
picking up my 'isms ;)

Thanks,
--John

Re: [Qemu-block] [PULL 0/1] Block patches

[Peter Maydell]

On 2 June 2017 at 16:33, Jeff Cody  wrote:
> The following changes since commit d47a851caeda96d5979bf48d4bae6a87784ad91d:
>
>   Merge remote-tracking branch 'remotes/juanquintela/tags/migration/20170601' 
> into staging (2017-06-02 14:07:53 +0100)
>
> are available in the git repository at:
>
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to df3a429ae82c0f45becdfab105617701d75e0f05:
>
>   gluster: add support for PREALLOC_MODE_FALLOC (2017-06-02 10:51:47 -0400)
>
> 
> Gluster patch(es)
> 
>
> Niels de Vos (1):
>   gluster: add support for PREALLOC_MODE_FALLOC
>
>  block/gluster.c | 78 
> ++---
>  configure   |  6 +
>  2 files changed, 47 insertions(+), 37 deletions(-)
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Stefan Hajnoczi]

On Thu, Jun 01, 2017 at 05:59:53PM +0200, Kevin Wolf wrote:
> Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
> > On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
> > >  static void qed_clear_need_check(void *opaque, int ret)
> > >  {
> > >  BDRVQEDState *s = opaque;
> > >  
> > >  if (ret) {
> > > -qed_unplug_allocating_write_reqs(s);
> > > -return;
> > > +goto out;
> > >  }
> > >  
> > >  s->header.features &= ~QED_F_NEED_CHECK;
> > > -qed_write_header(s, qed_flush_after_clear_need_check, s);
> > > +ret = qed_write_header(s);
> > > +(void) ret;
> > > +
> > > +ret = bdrv_flush(s->bs);
> > > +(void) ret;
> > > +
> > > +out:
> > > +qed_unplug_allocating_write_reqs(s);
> > >  }
> > 
> > Should we unplug allocating write reqs before flushing?  The async code
> > kicks off a flush but doesn't wait for it to complete.
> 
> You're right that moving it up would match the old code. Not sure if it
> would make much of a difference, though, isn't the request queue drained
> in the kernel anyway while doing a flush? But if you prefer, I can
> change it.

I prefer keeping the behavior of the existing code where possible.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Stefan Hajnoczi]

On Thu, Jun 01, 2017 at 06:04:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2017 17:59, Kevin Wolf wrote:
> > Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
> >> On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
> >>>  static void qed_clear_need_check(void *opaque, int ret)
> >>>  {
> >>>  BDRVQEDState *s = opaque;
> >>>  
> >>>  if (ret) {
> >>> -qed_unplug_allocating_write_reqs(s);
> >>> -return;
> >>> +goto out;
> >>>  }
> >>>  
> >>>  s->header.features &= ~QED_F_NEED_CHECK;
> >>> -qed_write_header(s, qed_flush_after_clear_need_check, s);
> >>> +ret = qed_write_header(s);
> >>> +(void) ret;
> >>> +
> >>> +ret = bdrv_flush(s->bs);
> >>> +(void) ret;
> >>> +
> >>> +out:
> >>> +qed_unplug_allocating_write_reqs(s);
> >>>  }
> >>
> >> Should we unplug allocating write reqs before flushing?  The async code
> >> kicks off a flush but doesn't wait for it to complete.
> > 
> > You're right that moving it up would match the old code. Not sure if it
> > would make much of a difference, though, isn't the request queue drained
> > in the kernel anyway while doing a flush?
> 
> No, it isn't AFAIK.

I was also under the impression that applications must call fsync()
only after their write() calls have completed.

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PULL 1/1] gluster: add support for PREALLOC_MODE_FALLOC

[Jeff Cody]

From: Niels de Vos 

Add missing support for "preallocation=falloc" to the Gluster block
driver. This change bases its logic on that of block/file-posix.c and
removed the gluster_supports_zerofill() and qemu_gluster_zerofill()
functions in favour of #ifdef checks in an easy to read
switch-statement.

Both glfs_zerofill() and glfs_fallocate() have been introduced with
GlusterFS 3.5.0 (pkg-config glusterfs-api = 6). A #define for the
availability of glfs_fallocate() has been added to ./configure.

Reported-by: Satheesaran Sundaramoorthi 
Signed-off-by: Niels de Vos 
Message-id: 20170528063114.28691-1-nde...@redhat.com
URL: https://bugzilla.redhat.com/1450759
Signed-off-by: Niels de Vos 
Signed-off-by: Jeff Cody 
---
 block/gluster.c | 78 ++---
 configure   |  6 +
 2 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 8ba3bcc..031596a 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -964,29 +964,6 @@ static coroutine_fn int 
qemu_gluster_co_pwrite_zeroes(BlockDriverState *bs,
 qemu_coroutine_yield();
 return acb.ret;
 }
-
-static inline bool gluster_supports_zerofill(void)
-{
-return 1;
-}
-
-static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
-int64_t size)
-{
-return glfs_zerofill(fd, offset, size);
-}
-
-#else
-static inline bool gluster_supports_zerofill(void)
-{
-return 0;
-}
-
-static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
-int64_t size)
-{
-return 0;
-}
 #endif
 
 static int qemu_gluster_create(const char *filename,
@@ -996,9 +973,10 @@ static int qemu_gluster_create(const char *filename,
 struct glfs *glfs;
 struct glfs_fd *fd;
 int ret = 0;
-int prealloc = 0;
+PreallocMode prealloc;
 int64_t total_size = 0;
 char *tmp = NULL;
+Error *local_err = NULL;
 
 gconf = g_new0(BlockdevOptionsGluster, 1);
 gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
@@ -1026,13 +1004,12 @@ static int qemu_gluster_create(const char *filename,
   BDRV_SECTOR_SIZE);
 
 tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
-if (!tmp || !strcmp(tmp, "off")) {
-prealloc = 0;
-} else if (!strcmp(tmp, "full") && gluster_supports_zerofill()) {
-prealloc = 1;
-} else {
-error_setg(errp, "Invalid preallocation mode: '%s'"
- " or GlusterFS doesn't support zerofill API", tmp);
+prealloc = qapi_enum_parse(PreallocMode_lookup, tmp,
+   PREALLOC_MODE__MAX, PREALLOC_MODE_OFF,
+   _err);
+g_free(tmp);
+if (local_err) {
+error_propagate(errp, local_err);
 ret = -EINVAL;
 goto out;
 }
@@ -1041,21 +1018,48 @@ static int qemu_gluster_create(const char *filename,
 O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | 
S_IWUSR);
 if (!fd) {
 ret = -errno;
-} else {
+goto out;
+}
+
+switch (prealloc) {
+#ifdef CONFIG_GLUSTERFS_FALLOCATE
+case PREALLOC_MODE_FALLOC:
+if (glfs_fallocate(fd, 0, 0, total_size)) {
+error_setg(errp, "Could not preallocate data for the new file");
+ret = -errno;
+}
+break;
+#endif /* CONFIG_GLUSTERFS_FALLOCATE */
+#ifdef CONFIG_GLUSTERFS_ZEROFILL
+case PREALLOC_MODE_FULL:
 if (!glfs_ftruncate(fd, total_size)) {
-if (prealloc && qemu_gluster_zerofill(fd, 0, total_size)) {
+if (glfs_zerofill(fd, 0, total_size)) {
+error_setg(errp, "Could not zerofill the new file");
 ret = -errno;
 }
 } else {
+error_setg(errp, "Could not resize file");
 ret = -errno;
 }
+break;
+#endif /* CONFIG_GLUSTERFS_ZEROFILL */
+case PREALLOC_MODE_OFF:
+if (glfs_ftruncate(fd, total_size) != 0) {
+ret = -errno;
+error_setg(errp, "Could not resize file");
+}
+break;
+default:
+ret = -EINVAL;
+error_setg(errp, "Unsupported preallocation mode: %s",
+   PreallocMode_lookup[prealloc]);
+break;
+}
 
-if (glfs_close(fd) != 0) {
-ret = -errno;
-}
+if (glfs_close(fd) != 0) {
+ret = -errno;
 }
 out:
-g_free(tmp);
 qapi_free_BlockdevOptionsGluster(gconf);
 glfs_clear_preopened(glfs);
 return ret;
diff --git a/configure b/configure
index 0586ec9..21944ea 100755
--- a/configure
+++ b/configure
@@ -300,6 +300,7 @@ seccomp=""
 glusterfs=""
 glusterfs_xlator_opt="no"
 glusterfs_discard="no"
+glusterfs_fallocate="no"
 glusterfs_zerofill="no"
 gtk=""
 gtkabi=""
@@ -3583,6 +3584,7 @@ if test 

[Qemu-block] [PULL 0/1] Block patches

[Jeff Cody]

The following changes since commit d47a851caeda96d5979bf48d4bae6a87784ad91d:

  Merge remote-tracking branch 'remotes/juanquintela/tags/migration/20170601' 
into staging (2017-06-02 14:07:53 +0100)

are available in the git repository at:

  git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request

for you to fetch changes up to df3a429ae82c0f45becdfab105617701d75e0f05:

  gluster: add support for PREALLOC_MODE_FALLOC (2017-06-02 10:51:47 -0400)


Gluster patch(es)


Niels de Vos (1):
  gluster: add support for PREALLOC_MODE_FALLOC

 block/gluster.c | 78 ++---
 configure   |  6 +
 2 files changed, 47 insertions(+), 37 deletions(-)

-- 
2.9.3

Re: [Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

30.05.2017 17:53, Eric Blake wrote:

On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:

The function should collect statistics, about allocted/unallocated by
top-level format driver space (in its .file) and allocation status
(allocated/hole/after eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c   | 16 
  include/block/block.h |  3 +++
  include/block/block_int.h |  2 ++
  qapi/block-core.json  | 26 ++
  4 files changed, 47 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/qapi/block-core.json
@@ -139,6 +139,32 @@
 '*format-specific': 'ImageInfoSpecific' } }
  
  ##

+# @BlockFormatAllocInfo:
+#
+# Information about allocations, including metadata. All fields are in bytes.


s/All fields are in bytes./All fields are in bytes and aligned by sector 
(512 bytes)./


- ok? to emphasize that there is nothing about clusters... Or may be 
better to write that they are aligned by byte.



+#
+# @alloc_alloc: allocated by format driver and allocated in underlying file
+#

New structs should favor naming with '-' rather than '_'. So this should
be 'alloc-alloc', if we like the name.

So this statistic represents the portions of the format file that are in
use by the format, and which are backed by data in the underlying protocol.


+# @alloc_hole: allocated by format driver but actually is a hole in
+#  underlying file

The portions of the format file in use by the format, but where the
entire cluster is a hole in the underlying file (note that with cluster
size large enough, you can get a cluster that is part-data/part-hole in
the underlying file, it looks like you are counting those as data).


+#
+# @alloc_overhead: allocated by format driver after end of underlying file

The portions of the format file in use by the format, but where the
entire cluster is beyond the end of the underlying file (the effective
hole).  Do we really need to distinguish between hole within the
underlying file and hole beyond the end of the file? Or can this number
be combined with the previous?


+#
+# @hole_alloc: not allocated by format driver but allocated in underlying file

I'm not sure I like this name.  "Hole" in file-system terminology means
"reads-as-zero" - but here we are talking about portions of the format
layer that are unallocated (defer to backing file, and we can't
guarantee they read as zero unless there is no backing file).

This statistic represents the portion of the underlying file that has
been previously allocated to hold data, but which is now unused by the
format layer (in other words, leaked clusters that can be reclaimed, or
which can be reused instead of growing the underlying the file if new
clusters are allocated).


+#
+# @hole_hole: not allocated by format driver hole in underlying file

This statistic represents fragmentation - portions of the format layer
that are no longer in use, but which are also occupying no space in the
underlying file.  A refragmentation operation (if we ever implemented
one) could remove the address space used by these clusters, but would
not change the disk usage.


+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'alloc_alloc':'uint64',
+   'alloc_hole': 'uint64',
+   'alloc_overhead': 'uint64',
+   'hole_alloc': 'uint64',
+   'hole_hole':  'uint64' } }

The idea seems okay, but the naming needs to be fixed.  Also, I'm not
sure if we need all 5, or if 4 is enough; and I'm not sure if we have
the right names ("how does alloc-hole differ from hole-alloc?"), or if
we can come up with something more descriptive.  Particularly since
"hole-" is not a hole in the filesystem sense, so much as unused
clusters.  But I'm also not coming up with better names to suggest at
the moment.


how about:

used-allocated
used-discarded
used-overrun

unused-allocated
unused-discarded


also, do you mention that your detailed wordings should be included into 
block-core.json or you just clarify things?



--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2 02/15] file-posix: support BDRV_REQ_ALLOCATE

[Anton Nefedov]

On 06/01/2017 10:54 PM, Eric Blake wrote:

On 06/01/2017 02:49 PM, Eric Blake wrote:

On 06/01/2017 10:14 AM, Anton Nefedov wrote:

Current write_zeroes implementation is good enough to satisfy this flag too

Signed-off-by: Anton Nefedov 
---
  block/file-posix.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)


Are we sure that fallocate() is always fast, or are there some file
systems where it is no faster than manually writing zeroes?  I'm worried
that blindly claiming BDRV_REQ_ALLOCATE may fail if we encounter a libc


not so much fail as in "break the guest", but fail as in "take far more
time than we were expecting, pessimising our behavior to worse than if
we had not tried the allocation at all"


or kernel-based fallback that takes a slow patch on our behalf.





I would expect such filesystems to not support fallocate.

Though I must admit I can't see anywhere in the documentation that it 
MUST be strictly faster than writing zeroes; it would look very strange

to me if there were a slowpath fallback somewhere past the libc.

/Anton

Re: [Qemu-block] NVME: is there any plan to support SGL data transfer?

[Kevin Wolf]

Am 02.06.2017 um 03:47 hat Qu Wenruo geschrieben:
> When going through NVMe specification and hw/block/nvme.c,
> I found that it seems that NVMe qemu implementation only support PRP
> for sq entry.
> And NvmeRwCmd doesn't even use union to define DPTR, but just prp1 and prp2.
> 
> Although I am just a newbie, but I'm quite interested in NVMe and
> want to try to implement SGL support for qemu NVMe.
> 
> Is there anyone already doing such work? Or is there any plan on
> implement such feature?

Keith, you can probably answer this?

Kevin

[Qemu-block] NVME: is there any plan to support SGL data transfer?

[Qu Wenruo]

Hi,

When going through NVMe specification and hw/block/nvme.c,
I found that it seems that NVMe qemu implementation only support PRP for 
sq entry.

And NvmeRwCmd doesn't even use union to define DPTR, but just prp1 and prp2.

Although I am just a newbie, but I'm quite interested in NVMe and want 
to try to implement SGL support for qemu NVMe.


Is there anyone already doing such work? Or is there any plan on 
implement such feature?


Thanks,
Qu

Re: [Qemu-block] [PATCH 1/2] qcow2: add reduce image support

[Kevin Wolf]

Am 02.06.2017 um 11:53 hat Pavel Butsykin geschrieben:
> On 01.06.2017 17:41, Kevin Wolf wrote:
> >Am 31.05.2017 um 16:43 hat Pavel Butsykin geschrieben:
> >>This patch adds the reduction of the image file for qcow2. As a result, this
> >>allows us to reduce the virtual image size and free up space on the disk 
> >>without
> >>copying the image. Image can be fragmented and reduction is done by punching
> >>holes in the image file.
> >>
> >>Signed-off-by: Pavel Butsykin 
> >>---
> >>  block/qcow2-cache.c|  8 +
> >>  block/qcow2-cluster.c  | 83 
> >> ++
> >>  block/qcow2-refcount.c | 65 +++
> >>  block/qcow2.c  | 40 ++--
> >>  block/qcow2.h  |  4 +++
> >>  qapi/block-core.json   |  4 ++-
> >>  6 files changed, 193 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> >>index 1d25147392..da55118ca7 100644
> >>--- a/block/qcow2-cache.c
> >>+++ b/block/qcow2-cache.c
> >>@@ -411,3 +411,11 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState 
> >>*bs, Qcow2Cache *c,
> >>  assert(c->entries[i].offset != 0);
> >>  c->entries[i].dirty = true;
> >>  }
> >>+
> >>+void qcow2_cache_entry_mark_clean(BlockDriverState *bs, Qcow2Cache *c,
> >>+ void *table)
> >>+{
> >>+int i = qcow2_cache_get_table_idx(bs, c, table);
> >>+assert(c->entries[i].offset != 0);
> >>+c->entries[i].dirty = false;
> >>+}
> >
> >This is an interesting function. We can use it whenever we're not
> >interested in the content of the table any more. However, we still keep
> >that data in the cache and may even evict other tables before this one.
> >The data in the cache also becomes inconsistent with the data in the
> >file, which should not be a problem in theory (because nobody should be
> >using it), but it surely could be confusing when debugging something in
> >the cache.
> >
> 
> Good idea!
> 
> >We can easily improve this a little: Make it qcow2_cache_discard(), a
> >function that gets a cluster offset, asserts that a table at this
> >offset isn't in use (not cached or ref == 0), and then just directly
> >drops it from the cache. This can be called from update_refcount()
> >whenever a refcount goes to 0, immediately before or after calling
> >update_refcount_discard() - those two are closely related. Then this
> >would automatically also be used for L2 tables.
> >
> 
> Did I understand correctly? Every time we need to check the incoming
> offset to make sure it is offset to L2/refcount table (not to the
> guest data) ?

Yes. Basically, whenever the refcount of a cluster becomes 0 and it is
in a cache, remove it from the cache.

Kevin

Re: [Qemu-block] [PATCH v2 03/15] blkdebug: support BDRV_REQ_ALLOCATE

[Anton Nefedov]

On 06/01/2017 10:50 PM, Eric Blake wrote:

On 06/01/2017 10:14 AM, Anton Nefedov wrote:

Support the flag if the underlying BDS supports it

Signed-off-by: Anton Nefedov 
---
  block/blkdebug.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)


Shouldn't other passthrough drivers (like raw-format.c) make this change
as well?



Right.

Wonder why they even enumerate those instead of just

bs->supported_zero_flags = bs->file->bs->supported_zero_flags;



diff --git a/block/blkdebug.c b/block/blkdebug.c
index a5196e8..8b1401b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -415,7 +415,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
  
  bs->supported_write_flags = BDRV_REQ_FUA &

  bs->file->bs->supported_write_flags;
-bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
+bs->supported_zero_flags =
+(BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_ALLOCATE) &
  bs->file->bs->supported_zero_flags;
  ret = -EINVAL;
  





/Anton

[Qemu-block] [PATCH v20 15/30] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Mirror AUTO flag from Qcow2 bitmap in BdrvDirtyBitmap. This will be
needed in future, to save this flag back to Qcow2 for persistent
bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 15 +++
 block/qcow2-bitmap.c |  2 ++
 include/block/dirty-bitmap.h |  3 +++
 3 files changed, 20 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 1c9ffb292a..8a380308c3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -51,6 +51,8 @@ struct BdrvDirtyBitmap {
(writes and discards), if bitmap is readonly
such operations must fail and not change
image or this bitmap */
+bool autoload;  /* For persistent bitmaps: bitmap must be
+   autoloaded on image opening */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -77,6 +79,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->autoload = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -245,6 +248,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->autoload = bitmap->autoload;
+bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
 
 return successor;
@@ -573,3 +578,13 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
 
 return false;
 }
+
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
+{
+bitmap->autoload = autoload;
+}
+
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->autoload;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a21fab8ce8..ee6d8f75a9 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -793,6 +793,8 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 if (bitmap == NULL) {
 goto fail;
 }
+
+bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
 g_slist_append(created_dirty_bitmaps, bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index aa6d47ee00..9416f9233f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -79,4 +79,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap 
*bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 09/30] block/dirty-bitmap: fix comment for BlockDirtyBitmap.disabled field

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..f25428868c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -42,7 +42,8 @@ struct BdrvDirtyBitmap {
 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 */
+bool disabled;  /* Bitmap is disabled. It skips all writes to
+   the device */
 int active_iterators;   /* How many iterators are active */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
-- 
2.11.1

[Qemu-block] [PATCH v20 17/30] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
by format driver in .bdrv_close and .bdrv_inactivate. No format driver
supports it for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/dirty-bitmap.c | 26 ++
 block/qcow2-bitmap.c |  1 +
 include/block/dirty-bitmap.h |  5 +
 3 files changed, 32 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8a380308c3..d14adff88d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -53,6 +53,7 @@ struct BdrvDirtyBitmap {
image or this bitmap */
 bool autoload;  /* For persistent bitmaps: bitmap must be
autoloaded on image opening */
+bool persistent;/* bitmap must be saved to owner disk image */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -79,6 +80,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->persistent = false;
 bitmap->autoload = false;
 }
 
@@ -248,6 +250,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->persistent = bitmap->persistent;
+bitmap->persistent = false;
 successor->autoload = bitmap->autoload;
 bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
@@ -588,3 +592,25 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap)
 {
 return bitmap->autoload;
 }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool 
persistent)
+{
+bitmap->persistent = persistent;
+}
+
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->persistent;
+}
+
+bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->persistent && !bm->readonly) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ee6d8f75a9..52e4616b8c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -794,6 +794,7 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 goto fail;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
 bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9416f9233f..721f025850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -82,4 +82,9 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+bool persistent);
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 18/30] block/dirty-bitmap: add bdrv_dirty_bitmap_next()

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 7 +++
 include/block/dirty-bitmap.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d14adff88d..ca60602b48 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -614,3 +614,10 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs)
 
 return false;
 }
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap)
+{
+return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
+QLIST_NEXT(bitmap, list);
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 721f025850..a3ac9c3640 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -87,4 +87,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap,
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 20/30] qcow2: store bitmaps on reopening image as read-only

[Vladimir Sementsov-Ogievskiy]

Store bitmaps and mark them read-only on reopening image as read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 22 ++
 block/qcow2.c|  5 +
 block/qcow2.h|  1 +
 3 files changed, 28 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 5f53486b22..7912a82c8c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1365,3 +1365,25 @@ fail:
 
 bitmap_list_free(bm_list);
 }
+
+int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
+{
+BdrvDirtyBitmap *bitmap;
+Error *local_err = NULL;
+
+qcow2_store_persistent_dirty_bitmaps(bs, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
+ bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
+{
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_dirty_bitmap_set_readonly(bitmap, true);
+}
+}
+
+return 0;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index ebe23ba4a5..76603903ff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1382,6 +1382,11 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
 
 /* We need to write out any unwritten data if we reopen read-only. */
 if ((state->flags & BDRV_O_RDWR) == 0) {
+ret = qcow2_reopen_bitmaps_ro(state->bs, errp);
+if (ret < 0) {
+goto fail;
+}
+
 ret = bdrv_flush(state->bs);
 if (ret < 0) {
 goto fail;
diff --git a/block/qcow2.h b/block/qcow2.h
index 0594551237..7d0a20c053 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -632,5 +632,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 10/30] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 32 
 block/io.c   |  8 
 blockdev.c   |  6 ++
 include/block/dirty-bitmap.h |  4 
 4 files changed, 50 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index f25428868c..1c9ffb292a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -45,6 +45,12 @@ struct BdrvDirtyBitmap {
 bool disabled;  /* Bitmap is disabled. It skips all writes to
the device */
 int active_iterators;   /* How many iterators are active */
+bool readonly;  /* Bitmap is read-only and may be changed only
+   by deserialize* functions. This field blocks
+   any changing operations on owning image
+   (writes and discards), if bitmap is readonly
+   such operations must fail and not change
+   image or this bitmap */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -437,6 +443,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
@@ -444,12 +451,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int64_t nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 if (!out) {
 hbitmap_reset_all(bitmap->bitmap);
 } else {
@@ -520,6 +529,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 if (!bdrv_dirty_bitmap_enabled(bitmap)) {
 continue;
 }
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 }
@@ -541,3 +551,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->meta);
 }
+
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->readonly;
+}
+
+void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value)
+{
+bitmap->readonly = value;
+}
+
+bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->readonly) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/io.c b/block/io.c
index fdd7485c22..0e28a1f595 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 uint64_t bytes_remaining = bytes;
 int max_transfer;
 
+if (bdrv_has_readonly_bitmaps(bs)) {
+return -EPERM;
+}
+
 assert(is_power_of_2(align));
 assert((offset & (align - 1)) == 0);
 assert((bytes & (align - 1)) == 0);
@@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, 
int64_t offset,
 return -ENOMEDIUM;
 }
 
+if (bdrv_has_readonly_bitmaps(bs)) {
+return -EPERM;
+}
+
 ret = bdrv_check_byte_request(bs, offset, count);
 if (ret < 0) {
 return ret;
diff --git a/blockdev.c b/blockdev.c
index c63f4e82c7..2b397abf66 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2033,6 +2033,9 @@ static void 
block_dirty_bitmap_clear_prepare(BlkActionState *common,
 } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
 error_setg(errp, "Cannot clear a disabled bitmap");
 return;
+} else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
+error_setg(errp, "Cannot clear a readonly bitmap");
+return;
 }
 
 bdrv_clear_dirty_bitmap(state->bitmap, >backup);
@@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char *node, const 
char *name,
"Bitmap '%s' is currently disabled and cannot be cleared",
name);
 goto out;
+} else if (bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", 
name);
+goto out;
 }
 
 bdrv_clear_dirty_bitmap(bitmap, NULL);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729ac2..aa6d47ee00 100644
--- a/include/block/dirty-bitmap.h
+++ 

[Qemu-block] [PATCH v20 28/30] qcow2: add .bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_remove_persistent_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-bitmap.c | 41 +
 block/qcow2.c|  1 +
 block/qcow2.h|  3 +++
 3 files changed, 45 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f45324e584..8448bec46d 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1236,6 +1236,47 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
 return NULL;
 }
 
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+Qcow2Bitmap *bm;
+Qcow2BitmapList *bm_list;
+
+if (s->nb_bitmaps == 0) {
+/* Absence of the bitmap is not an error: see explanation above
+ * bdrv_remove_persistent_dirty_bitmap() definition. */
+return;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return;
+}
+
+bm = find_bitmap_by_name(bm_list, name);
+if (bm == NULL) {
+goto fail;
+}
+
+QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
+
+ret = update_ext_header_and_dir(bs, bm_list);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to update bitmap extension");
+goto fail;
+}
+
+free_bitmap_clusters(bs, >table);
+
+fail:
+bitmap_free(bm);
+bitmap_list_free(bm_list);
+}
+
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
diff --git a/block/qcow2.c b/block/qcow2.c
index 77260df512..59da4d0afe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3613,6 +3613,7 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
 .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
+.bdrv_remove_persistent_dirty_bitmap = 
qcow2_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 8b2f66f8b6..ffb951df52 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -637,5 +637,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
   uint32_t granularity,
   Error **errp);
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 01/30] specs/qcow2: fix bitmap granularity qemu-specific note

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0e91..dda53dd2a3 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -472,8 +472,7 @@ Structure of a bitmap directory entry:
  17:granularity_bits
 Granularity bits. Valid values: 0 - 63.
 
-Note: Qemu currently doesn't support granularity_bits
-greater than 31.
+Note: Qemu currently supports only values 9 - 31.
 
 Granularity is calculated as
 granularity = 1 << granularity_bits
-- 
2.11.1

[Qemu-block] [PATCH v20 05/30] block: fix bdrv_dirty_bitmap_granularity signature

[Vladimir Sementsov-Ogievskiy]

Make getter signature const-correct. This allows other functions with
const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 block/dirty-bitmap.c | 2 +-
 include/block/dirty-bitmap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c8d3..186941cfc3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14ba03..7cbe623ba7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -29,7 +29,7 @@ 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);
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-- 
2.11.1

[Qemu-block] [PATCH v20 30/30] block: release persistent bitmaps on inactivate

[Vladimir Sementsov-Ogievskiy]

We should release them here to reload on invalidate cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c  |  4 
 block/dirty-bitmap.c | 29 +++--
 include/block/dirty-bitmap.h |  1 +
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a1e67759b9..1043e64a19 100644
--- a/block.c
+++ b/block.c
@@ -4110,6 +4110,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 }
 }
 
+/* At this point persistent bitmaps should be already stored by the format
+ * driver */
+bdrv_release_persistent_dirty_bitmaps(bs);
+
 return 0;
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6b43ad04fc..45b18dd3f3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -301,13 +301,18 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 }
 }
 
-static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap,
-  bool only_named)
+static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
+{
+return !!bdrv_dirty_bitmap_name(bitmap);
+}
+
+static void bdrv_do_release_matching_dirty_bitmap(
+BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+bool (*cond)(BdrvDirtyBitmap *bitmap))
 {
 BdrvDirtyBitmap *bm, *next;
 QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
-if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
+if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
 assert(!bm->active_iterators);
 assert(!bdrv_dirty_bitmap_frozen(bm));
 assert(!bm->meta);
@@ -328,7 +333,7 @@ static void 
bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
 
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
-bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false);
+bdrv_do_release_matching_dirty_bitmap(bs, bitmap, NULL);
 }
 
 /**
@@ -338,7 +343,19 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
-bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
+bdrv_do_release_matching_dirty_bitmap(bs, NULL, 
bdrv_dirty_bitmap_has_name);
+}
+
+/**
+ * Release all persistent dirty bitmaps attached to a BDS (for use in
+ * bdrv_inactivate_recurse()).
+ * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
+ */
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
+{
+bdrv_do_release_matching_dirty_bitmap(bs, NULL,
+  bdrv_dirty_bitmap_get_persistance);
 }
 
 /**
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 5fac2d8411..6fec93bdeb 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
  const char *name,
  Error **errp);
-- 
2.11.1

[Qemu-block] [PATCH v20 26/30] iotests: test qcow2 persistent dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/165 | 105 +
 tests/qemu-iotests/165.out |   5 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 111 insertions(+)
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
new file mode 100755
index 00..74d7b79a0b
--- /dev/null
+++ b/tests/qemu-iotests/165
@@ -0,0 +1,105 @@
+#!/usr/bin/env python
+#
+# Tests for persistent dirty bitmaps.
+#
+# Copyright: Vladimir Sementsov-Ogievskiy 2015-2017
+#
+# 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 re
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+disk_size = 0x4000 # 1G
+
+# regions for qemu_io: (start, count) in bytes
+regions1 = ((0,0x10),
+(0x20, 0x10))
+
+regions2 = ((0x1000, 0x2),
+(0x3fff, 0x1))
+
+class TestPersistentDirtyBitmap(iotests.QMPTestCase):
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+
+def tearDown(self):
+os.remove(disk)
+
+def mkVm(self):
+return iotests.VM().add_drive(disk)
+
+def mkVmRo(self):
+return iotests.VM().add_drive(disk, opts='readonly=on')
+
+def getSha256(self):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+return result['return']['sha256']
+
+def checkBitmap(self, sha256):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+self.assert_qmp(result, 'return/sha256', sha256);
+
+def writeRegions(self, regions):
+for r in regions:
+self.vm.hmp_qemu_io('drive0',
+'write %d %d' % r)
+
+def qmpAddBitmap(self):
+self.vm.qmp('block-dirty-bitmap-add', node='drive0',
+name='bitmap0', persistent=True, autoload=True)
+
+def test_persistent(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+self.writeRegions(regions1)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+
+self.vm = self.mkVmRo()
+self.vm.launch()
+self.vm.shutdown()
+
+#catch 'Persistent bitmaps are lost' possible error
+log = self.vm.get_log()
+log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+if log:
+print log
+
+self.vm = self.mkVm()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+self.writeRegions(regions2)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+
+self.vm.shutdown()
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/165.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5c8ea0f95c..1a446489c2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -163,6 +163,7 @@
 159 rw auto quick
 160 rw auto quick
 162 auto quick
+165 rw auto quick
 170 rw auto quick
 171 rw auto quick
 172 auto
-- 
2.11.1

[Qemu-block] [PATCH v20 11/30] qcow2: autoloading dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 389 +++
 block/qcow2.c|  17 ++-
 block/qcow2.h|   2 +
 3 files changed, 406 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b8e472b3e8..2c7b057e21 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -44,6 +44,8 @@
 
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
+#define BME_FLAG_IN_USE (1U << 0)
+#define BME_FLAG_AUTO   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
@@ -85,6 +87,23 @@ typedef enum BitmapType {
 BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
 
+static inline bool can_write(BlockDriverState *bs)
+{
+return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+}
+
+static int update_header_sync(BlockDriverState *bs)
+{
+int ret;
+
+ret = qcow2_update_header(bs);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_flush(bs);
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -146,6 +165,120 @@ fail:
 return ret;
 }
 
+/* This function returns the number of disk sectors covered by a single qcow2
+ * cluster of bitmap data. */
+static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
+  const BdrvDirtyBitmap 
*bitmap)
+{
+uint32_t sector_granularity =
+bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+return (uint64_t)sector_granularity * (s->cluster_size << 3);
+}
+
+/* load_bitmap_data
+ * @bitmap_table entries must satisfy specification constraints.
+ * @bitmap must be cleared */
+static int load_bitmap_data(BlockDriverState *bs,
+const uint64_t *bitmap_table,
+uint32_t bitmap_table_size,
+BdrvDirtyBitmap *bitmap)
+{
+int ret = 0;
+BDRVQcow2State *s = bs->opaque;
+uint64_t sector, sbc;
+uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint8_t *buf = NULL;
+uint64_t i, tab_size =
+size_to_clusters(s,
+bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+if (tab_size != bitmap_table_size || tab_size > BME_MAX_TABLE_SIZE) {
+return -EINVAL;
+}
+
+buf = g_malloc(s->cluster_size);
+sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
+for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
+uint64_t count = MIN(bm_size - sector, sbc);
+uint64_t entry = bitmap_table[i];
+uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+
+assert(check_table_entry(entry, s->cluster_size) == 0);
+
+if (offset == 0) {
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, count,
+   false);
+} else {
+/* No need to deserialize zeros because the dirty bitmap is
+ * already cleared */
+}
+} else {
+ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+if (ret < 0) {
+goto finish;
+}
+bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, count,
+   false);
+}
+}
+ret = 0;
+
+bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+g_free(buf);
+
+return ret;
+}
+
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
+Qcow2Bitmap *bm, Error **errp)
+{
+int ret;
+uint64_t *bitmap_table = NULL;
+uint32_t granularity;
+BdrvDirtyBitmap *bitmap = NULL;
+
+if (bm->flags & BME_FLAG_IN_USE) {
+error_setg(errp, "Bitmap '%s' is in use", bm->name);
+goto fail;
+}
+
+ret = bitmap_table_load(bs, >table, _table);
+if (ret < 0) {
+error_setg_errno(errp, -ret,
+ "Could not read bitmap_table table from image for "
+ "bitmap '%s'", bm->name);
+goto fail;
+}
+
+granularity = 1U << bm->granularity_bits;
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
+ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
+ bm->name);
+goto fail;
+}
+
+g_free(bitmap_table);

[Qemu-block] [PATCH v20 19/30] qcow2: add persistent dirty bitmaps support

[Vladimir Sementsov-Ogievskiy]

Store persistent dirty bitmaps in qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 475 +++
 block/qcow2.c|   9 +
 block/qcow2.h|   1 +
 3 files changed, 485 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 52e4616b8c..5f53486b22 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -27,6 +27,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -42,6 +43,10 @@
 #define BME_MIN_GRANULARITY_BITS 9
 #define BME_MAX_NAME_SIZE 1023
 
+#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
+#error In the code bitmap table physical size assumed to fit into int
+#endif
+
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
 #define BME_FLAG_IN_USE (1U << 0)
@@ -72,6 +77,8 @@ typedef struct Qcow2BitmapTable {
 uint32_t size; /* number of 64bit entries */
 QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
 } Qcow2BitmapTable;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
+Qcow2BitmapTableList;
 
 typedef struct Qcow2Bitmap {
 Qcow2BitmapTable table;
@@ -79,6 +86,8 @@ typedef struct Qcow2Bitmap {
 uint8_t granularity_bits;
 char *name;
 
+BdrvDirtyBitmap *dirty_bitmap;
+
 QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
 } Qcow2Bitmap;
 typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
@@ -104,6 +113,15 @@ static int update_header_sync(BlockDriverState *bs)
 return bdrv_flush(bs);
 }
 
+static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
+{
+size_t i;
+
+for (i = 0; i < size; ++i) {
+cpu_to_be64s(_table[i]);
+}
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -127,6 +145,70 @@ static int check_table_entry(uint64_t entry, int 
cluster_size)
 return 0;
 }
 
+static int check_constraints_on_bitmap(BlockDriverState *bs,
+   const char *name,
+   uint32_t granularity,
+   Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+int granularity_bits = ctz32(granularity);
+int64_t len = bdrv_getlength(bs);
+
+assert(granularity > 0);
+assert((granularity & (granularity - 1)) == 0);
+
+if (len < 0) {
+error_setg_errno(errp, -len, "Failed to get size of '%s'",
+ bdrv_get_device_or_node_name(bs));
+return len;
+}
+
+if (granularity_bits > BME_MAX_GRANULARITY_BITS) {
+error_setg(errp, "Granularity exceeds maximum (%llu bytes)",
+   1ULL << BME_MAX_GRANULARITY_BITS);
+return -EINVAL;
+}
+if (granularity_bits < BME_MIN_GRANULARITY_BITS) {
+error_setg(errp, "Granularity is under minimum (%llu bytes)",
+   1ULL << BME_MIN_GRANULARITY_BITS);
+return -EINVAL;
+}
+
+if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
+(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
+   granularity_bits))
+{
+error_setg(errp, "Too much space will be occupied by the bitmap. "
+   "Use larger granularity");
+return -EINVAL;
+}
+
+if (strlen(name) > BME_MAX_NAME_SIZE) {
+error_setg(errp, "Name length exceeds maximum (%u characters)",
+   BME_MAX_NAME_SIZE);
+return -EINVAL;
+}
+
+return 0;
+}
+
+static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+   uint32_t bitmap_table_size)
+{
+BDRVQcow2State *s = bs->opaque;
+int i;
+
+for (i = 0; i < bitmap_table_size; ++i) {
+uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
+if (!addr) {
+continue;
+}
+
+qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
+bitmap_table[i] = 0;
+}
+}
+
 static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
  uint64_t **bitmap_table)
 {
@@ -165,6 +247,28 @@ fail:
 return ret;
 }
 
+static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
+{
+int ret;
+uint64_t *bitmap_table;
+
+ret = bitmap_table_load(bs, tb, _table);
+if (ret < 0) {
+assert(bitmap_table == NULL);
+return ret;
+}
+
+clear_bitmap_table(bs, bitmap_table, tb->size);
+qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
+QCOW2_DISCARD_OTHER);
+g_free(bitmap_table);
+
+tb->offset = 0;
+tb->size = 0;
+
+return 0;
+}
+
 /* This function returns the number of disk sectors covered by a single qcow2
  * cluster of bitmap data. */
 static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State 

[Qemu-block] [PATCH v20 08/30] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

Add bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints. Also, calculate refcounts for qcow2 bitmaps, to not break
qemu-img check.

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/Makefile.objs|   2 +-
 block/qcow2-bitmap.c   | 439 +
 block/qcow2-refcount.c |   6 +
 block/qcow2.c  | 124 +-
 block/qcow2.h  |  27 +++
 5 files changed, 592 insertions(+), 6 deletions(-)
 create mode 100644 block/qcow2-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index ea955302c8..9efc6c49ea 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o 
dmg.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o qcow2-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 00..b8e472b3e8
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,439 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2017 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
+/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
+ * _internal_ constants. Please do not use this _internal_ abbreviation for
+ * other needs and/or outside of this file. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x800
+#define BME_MAX_PHYS_SIZE 0x2000 /* restrict BdrvDirtyBitmap size in RAM */
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xfffcU
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
+#define BME_TABLE_ENTRY_OFFSET_MASK 0x00fffe00ULL
+#define BME_TABLE_ENTRY_FLAG_ALL_ONES (1ULL << 0)
+
+typedef struct QEMU_PACKED Qcow2BitmapDirEntry {
+/* header is 8 byte aligned */
+uint64_t bitmap_table_offset;
+
+uint32_t bitmap_table_size;
+uint32_t flags;
+
+uint8_t type;
+uint8_t granularity_bits;
+uint16_t name_size;
+uint32_t extra_data_size;
+/* extra data follows  */
+/* name follows  */
+} Qcow2BitmapDirEntry;
+
+typedef struct Qcow2BitmapTable {
+uint64_t offset;
+uint32_t size; /* number of 64bit entries */
+QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
+} Qcow2BitmapTable;
+
+typedef struct Qcow2Bitmap {
+Qcow2BitmapTable table;
+uint32_t flags;
+uint8_t granularity_bits;
+char *name;
+
+QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
+} Qcow2Bitmap;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
+
+typedef enum BitmapType {
+BT_DIRTY_TRACKING_BITMAP = 1
+} BitmapType;
+
+static int check_table_entry(uint64_t entry, int cluster_size)
+{
+uint64_t offset;
+
+if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
+return -EINVAL;
+}
+
+offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+if (offset != 0) {
+/* if offset specified, bit 0 is reserved */
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+return -EINVAL;
+}
+
+

[Qemu-block] [PATCH v20 27/30] block/dirty-bitmap: add bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Interface for removing persistent bitmap from its storage.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 18 ++
 include/block/block_int.h|  3 +++
 include/block/dirty-bitmap.h |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 2ae64fc9e8..6b43ad04fc 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -334,12 +334,30 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 /**
  * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()).
  * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
 bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
 }
 
+/**
+ * Remove persistent dirty bitmap from the storage if it exists.
+ * Absence of bitmap is not an error, because we have the following scenario:
+ * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
+ * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
+ * not fail.
+ * This function doesn't release corresponding BdrvDirtyBitmap.
+ */
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp)
+{
+if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
+bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+}
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
diff --git a/include/block/block_int.h b/include/block/block_int.h
index adda0894c7..953107f095 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -390,6 +390,9 @@ struct BlockDriver {
 const char *name,
 uint32_t granularity,
 Error **errp);
+void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+Error **errp);
 
 QLIST_ENTRY(BlockDriver) list;
 };
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 00b298330c..5fac2d8411 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,9 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
-- 
2.11.1

[Qemu-block] [PATCH v20 24/30] qmp: add autoload parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Optional. Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 --
 qapi/block-core.json |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 12ee9a3eda..d04432172a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1983,6 +1983,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
+   action->has_autoload, action->autoload,
_err);
 
 if (!local_err) {
@@ -2732,6 +2733,7 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
 bool has_persistent, bool persistent,
+bool has_autoload, bool autoload,
 Error **errp)
 {
 AioContext *aio_context;
@@ -2765,6 +2767,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 if (!has_persistent) {
 persistent = false;
 }
+if (!has_autoload) {
+autoload = false;
+}
+
+if (has_autoload && !persistent) {
+error_setg(errp, "Autoload flag must be used only for persistent "
+ "bitmaps");
+goto out;
+}
 
 if (persistent &&
 !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
@@ -2773,10 +2784,13 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 }
 
 bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
-if (bitmap != NULL) {
-bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+if (bitmap == NULL) {
+goto out;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
+
  out:
 aio_context_release(aio_context);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5d9ed0e208..9134a317e5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1565,11 +1565,15 @@
 #  Qcow2 disks support persistent bitmaps. Default is false for
 #  block-dirty-bitmap-add. (Since: 2.10)
 #
+# @autoload: the bitmap will be automatically loaded when the image it is 
stored
+#in is opened. This flag may only be specified for persistent
+#bitmaps. Default is false for block-dirty-bitmap-add. (Since: 
2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool' } }
+'*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

[Qemu-block] [PATCH v20 29/30] qmp: block-dirty-bitmap-remove: remove persistent

[Vladimir Sementsov-Ogievskiy]

Remove persistent bitmap from the storage on block-dirty-bitmap-remove.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 10 ++
 qapi/block-core.json |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index e3dbef7427..7ec75524a8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2801,6 +2801,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
 AioContext *aio_context;
 BlockDriverState *bs;
 BdrvDirtyBitmap *bitmap;
+Error *local_err = NULL;
 
 bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
 if (!bitmap || !bs) {
@@ -2813,6 +2814,15 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
name);
 goto out;
 }
+
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_remove_persistent_dirty_bitmap(bs, name, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto out;
+}
+}
+
 bdrv_dirty_bitmap_make_anon(bitmap);
 bdrv_release_dirty_bitmap(bs, bitmap);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3dd9f92e5a..37f35a1811 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1600,7 +1600,8 @@
 # @block-dirty-bitmap-remove:
 #
 # Stop write tracking and remove the dirty bitmap that was created
-# with block-dirty-bitmap-add.
+# with block-dirty-bitmap-add. If the bitmap is persistent, remove it from its
+# storage too.
 #
 # Returns: nothing on success
 #  If @node is not a valid block device or node, DeviceNotFound
-- 
2.11.1

[Qemu-block] [PATCH v20 23/30] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 +-
 qapi/block-core.json |  8 +++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2b397abf66..12ee9a3eda 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1982,6 +1982,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 /* AIO context taken and released within qmp_block_dirty_bitmap_add */
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
+   action->has_persistent, action->persistent,
_err);
 
 if (!local_err) {
@@ -2730,10 +2731,12 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
+bool has_persistent, bool persistent,
 Error **errp)
 {
 AioContext *aio_context;
 BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
 
 if (!name || name[0] == '\0') {
 error_setg(errp, "Bitmap name cannot be empty");
@@ -2759,7 +2762,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 granularity = bdrv_get_default_bitmap_granularity(bs);
 }
 
-bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (!has_persistent) {
+persistent = false;
+}
+
+if (persistent &&
+!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+{
+goto out;
+}
+
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (bitmap != NULL) {
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+}
 
  out:
 aio_context_release(aio_context);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..5d9ed0e208 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1560,10 +1560,16 @@
 # @granularity: the bitmap granularity, default is 64k for
 #   block-dirty-bitmap-add
 #
+# @persistent: the bitmap is persistent, i.e. it will be saved to the
+#  corresponding block device image file on its close. For now only
+#  Qcow2 disks support persistent bitmaps. Default is false for
+#  block-dirty-bitmap-add. (Since: 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+'*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

[Qemu-block] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface

[Vladimir Sementsov-Ogievskiy]

Add format driver handler, which should mark loaded read-only
bitmaps as 'IN_USE' in the image and unset read_only field in
corresponding BdrvDirtyBitmap's.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c   | 17 +
 include/block/block_int.h |  7 +++
 2 files changed, 24 insertions(+)

diff --git a/block.c b/block.c
index 04af7697dc..161db9e32a 100644
--- a/block.c
+++ b/block.c
@@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 {
 BlockDriver *drv;
 BlockDriverState *bs;
+bool old_can_write, new_can_write;
 
 assert(reopen_state != NULL);
 bs = reopen_state->bs;
 drv = bs->drv;
 assert(drv != NULL);
 
+old_can_write =
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+
 /* If there are any driver level actions to take */
 if (drv->bdrv_reopen_commit) {
 drv->bdrv_reopen_commit(reopen_state);
@@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
 bdrv_refresh_limits(bs, NULL);
+
+new_can_write =
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
+Error *local_err = NULL;
+if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
+/* This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes.
+ */
+error_report_err(local_err);
+}
+}
 }
 
 /*
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..1dc6f2e90d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,13 @@ struct BlockDriver {
  uint64_t parent_perm, uint64_t parent_shared,
  uint64_t *nperm, uint64_t *nshared);
 
+/**
+ * Bitmaps should be marked as 'IN_USE' in the image on reopening image
+ * as rw. This handler should realize it. It also should unset readonly
+ * field of BlockDirtyBitmap's in case of success.
+ */
+int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
+
 QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.11.1

[Qemu-block] [PATCH v20 21/30] block: add bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

This will be needed to check some restrictions before making bitmap
persistent in qmp-block-dirty-bitmap-add (this functionality will be
added by future patch)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block.c   | 22 ++
 include/block/block.h |  3 +++
 include/block/block_int.h |  4 
 3 files changed, 29 insertions(+)

diff --git a/block.c b/block.c
index b35387241a..a1e67759b9 100644
--- a/block.c
+++ b/block.c
@@ -4908,3 +4908,25 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
 parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
+
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+if (!drv->bdrv_can_store_new_dirty_bitmap) {
+error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..ee320db909 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -629,4 +629,7 @@ void bdrv_add_child(BlockDriverState *parent, 
BlockDriverState *child,
 Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1dc6f2e90d..adda0894c7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -386,6 +386,10 @@ struct BlockDriver {
  * field of BlockDirtyBitmap's in case of success.
  */
 int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
+bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+uint32_t granularity,
+Error **errp);
 
 QLIST_ENTRY(BlockDriver) list;
 };
-- 
2.11.1

[Qemu-block] [PATCH v20 16/30] block: bdrv_close: release bitmaps after drv->bdrv_close

[Vladimir Sementsov-Ogievskiy]

Release bitmaps after 'if (bs->drv) { ... }' block. This will allow
format driver to save persistent bitmaps, which will appear in following
commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 161db9e32a..b35387241a 100644
--- a/block.c
+++ b/block.c
@@ -3015,9 +3015,6 @@ static void bdrv_close(BlockDriverState *bs)
 bdrv_flush(bs);
 bdrv_drain(bs); /* in case flush left pending I/O */
 
-bdrv_release_named_dirty_bitmaps(bs);
-assert(QLIST_EMPTY(>dirty_bitmaps));
-
 if (bs->drv) {
 BdrvChild *child, *next;
 
@@ -3056,6 +3053,9 @@ static void bdrv_close(BlockDriverState *bs)
 bs->full_open_options = NULL;
 }
 
+bdrv_release_named_dirty_bitmaps(bs);
+assert(QLIST_EMPTY(>dirty_bitmaps));
+
 QLIST_FOREACH_SAFE(ban, >aio_notifiers, list, ban_next) {
 g_free(ban);
 }
-- 
2.11.1

[Qemu-block] [PATCH v20 00/30] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all!

There is a new update of qcow2-bitmap series - v20.

web: 
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v20
git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v20)

v20:

handle reopening images ro and rw.

On reopening ro: store bitmaps (storing sets 'IN_USE'=0 in the image)
and mark them readonly (set readonly flag in BlockDirtyBitmap)

After reopening rw: mark bitmaps IN_USE in the image
and unset readonly flag in BlockDirtyBitmap

09: new
10: improve comment
add parameter 'value' to bdrv_dirty_bitmap_set_readonly
11: use new parameter of bdrv_dirty_bitmap_set_readonly
12-14, 20: new

v19:

rebased on master

05: move 'sign-off' over 'reviewed-by's
08: error_report -> error_setg in qcow2_truncate (because of rebase)
09: return EPERM in bdrv_aligned_pwritev and bdrv_co_pdiscard if there
are readonly bitmaps. EPERM is chosen because it is already used for
readonly image in bdrv_co_pdiscard.
Also handle readonly bitmap in block_dirty_bitmap_clear_prepare and
qmp_block_dirty_bitmap_clear
Max's r-b is not added
10: fix grammar in comment
add Max's r-b
12, 13, 15, 21: add Max's r-b
24: fix grammar in comment
25: fix grammar and wording in comment
also, I see contextual changes in inactiavate mechanism. Hope, they do not
affect these series.

v18:

rebased on master (sorry for v17)

08: contextual: qcow2_do_open is changed instead of qcow2_open
rename s/need_update_header/update_header/ in qcow2_do_open, to not do it 
in 10
save r-b's by Max and John
09: new patch
10: load_bitmap_data: do not clear bitmap parameter - it should be already 
cleared
(it actually created before single load_bitmap_data() call)
if some bitmaps are loaded, but we can't write the image (it is readonly
or inactive), so we can't mark bitmaps "in use" in the image, mark
corresponding BdrvDirtyBitmap read-only.
change error_setg to error_setg_errno for "Can't update bitmap directory"
no needs to rename s/need_update_header/update_header/ here, as it done in 
08
13: function bdrv_has_persistent_bitmaps becomes 
bdrv_has_changed_persistent_bitmaps,
to handle readonly field.
14: declaration moved to the bottom of .h, save r-b's
15: firstly check bdrv_has_changed_persistent_bitmaps and only then fail on 
!can_write, and then QSIMPLEQ_INIT(_tables)
skip readonly bitmaps in saving loop
18: remove '#optional', 2.9 -> 2.10, save r-b's
19: remove '#optional', 2.9 -> 2.10, save r-b's
20: 2.9 -> 2.10, save r-b's
21: add check of read-only image open, drop r-b's
24: add comment to qapi/block-core.json, that block-dirty-bitmap-add removes 
bitmap
from storage. r-b's by Max and John saved


v17:
08: add r-b's by Max and John
09: clear unknown autoclear features from BDRVQcow2State before calling
qcow2_load_autoloading_dirty_bitmaps(), and also do not extra update
header if it is updated by qcow2_load_autoloading_dirty_bitmaps().
11: new patch, splitted out from 16
12: rewrite commit message
14: changing bdrv_close moved to separate new patch 11
s/1/1ULL/ ; s/%u/%llu/ for two errors
16: s/No/Not/
add Max's r-b
24: new patch


v16:

mostly by Kevin's comments:
07: just moved here, as preparation for merging refcounts to 08
08: move "qcow2-bitmap: refcounts" staff to this patch, to not break qcow2-img 
check
drop r-b's
move necessary supporting static functions to this patch too (to not break 
compilation)
fprintf -> error_report
other small changes with error messages and comments
code style
for qcow2-bitmap.c:
  'include "exec/log.h"' was dropped
  s/1/(1U << 0) for BME_FLAG_IN_USE
  add BME_TABLE_ENTRY_FLAG_ALL_ONES to replace magic 1
  don't check 'cluster_size <= 0' in check_table_entry
old "[PATCH v15 08/25] block: introduce auto-loading bitmaps" was dropped
09: was "[PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps"
drop r-b's
some staff was moved to 08
update_header_sync - error on bdrv_flush fail
rename disk_sectors_in_bitmap_cluster to sectors_covered_by_bitmap_cluster
 and adjust comment.
 so, variable for storing this function result: s/dsc/sbc/
s/1/BME_TABLE_ENTRY_FLAG_ALL_ONES/
also, as Qcow2BitmapTable already introduced in 08,
   s/table_offset/table.offset/ and s/table_size/table.size, etc..
update_ext_header_and_dir_in_place: add comments, add additional
  update_header_sync, to reduce indeterminacy in case of error.
call qcow2_load_autoloading_dirty_bitmaps directly from qcow2_open
no .bdrv_load_autoloading_dirty_bitmaps
11: drop bdrv_store_persistent_dirty_bitmaps at all.
drop r-b's
13: rename patch, rewrite commit msg
drop r-b's
move bdrv_release_named_dirty_bitmaps in bdrv_close() after 
drv->bdrv_close()
Qcow2BitmapTable is already introduced, so no
  s/table_offset/table.offset/ and s/table_size/table.size, etc.. here
old 25 with 

[Qemu-block] [PATCH v20 25/30] qmp: add x-debug-block-dirty-bitmap-sha256

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  5 +
 blockdev.c   | 29 +
 include/block/dirty-bitmap.h |  2 ++
 include/qemu/hbitmap.h   |  8 
 qapi/block-core.json | 27 +++
 tests/Makefile.include   |  2 +-
 util/hbitmap.c   | 11 +++
 7 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index ca60602b48..2ae64fc9e8 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -621,3 +621,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState 
*bs,
 return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
 QLIST_NEXT(bitmap, list);
 }
+
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
+{
+return hbitmap_sha256(bitmap->bitmap, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index d04432172a..e3dbef7427 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2857,6 +2857,35 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
const char *name,
 aio_context_release(aio_context);
 }
 
+BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
+  const char *name,
+  Error **errp)
+{
+AioContext *aio_context;
+BdrvDirtyBitmap *bitmap;
+BlockDriverState *bs;
+BlockDirtyBitmapSha256 *ret = NULL;
+char *sha256;
+
+bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
+if (!bitmap || !bs) {
+return NULL;
+}
+
+sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
+if (sha256 == NULL) {
+goto out;
+}
+
+ret = g_new(BlockDirtyBitmapSha256, 1);
+ret->sha256 = sha256;
+
+out:
+aio_context_release(aio_context);
+
+return ret;
+}
+
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a3ac9c3640..00b298330c 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -90,4 +90,6 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 BdrvDirtyBitmap *bitmap);
 
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index b52304ac29..d3a74a21fc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -253,6 +253,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, 
uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_sha256:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns SHA256 hash of the last level.
+ */
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9134a317e5..3dd9f92e5a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1643,6 +1643,33 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @BlockDirtyBitmapSha256:
+#
+# SHA256 hash of dirty bitmap data
+#
+# @sha256: ASCII representation of SHA256 bitmap hash
+#
+# Since: 2.10
+##
+  { 'struct': 'BlockDirtyBitmapSha256',
+'data': {'sha256': 'str'} }
+
+##
+# @x-debug-block-dirty-bitmap-sha256:
+#
+# Get bitmap SHA256
+#
+# Returns: BlockDirtyBitmapSha256 on success
+#  If @node is not a valid block device, DeviceNotFound
+#  If @name is not found or if hashing has failed, GenericError with an
+#  explanation
+#
+# Since: 2.10
+##
+  { 'command': 'x-debug-block-dirty-bitmap-sha256',
+'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+
+##
 # @blockdev-mirror:
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 75893838e5..38dbd2d9c8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -552,7 +552,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o 
$(test-block-obj-y) $(test-u
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
-tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
+tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) 
$(test-crypto-obj-y)
 tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o 
migration/page_cache.o $(test-util-obj-y)
 tests/test-cutils$(EXESUF): 

[Qemu-block] [PATCH v20 02/30] specs/qcow2: do not use wording 'bitmap header'

[Vladimir Sementsov-Ogievskiy]

A bitmap directory entry is sometimes called a 'bitmap header'. This
patch leaves only one name - 'bitmap directory entry'. The name 'bitmap
header' creates misunderstandings with 'qcow2 header' and 'qcow2 bitmap
header extension' (which is extension of qcow2 header)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index dda53dd2a3..8874e8c774 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -201,7 +201,7 @@ The fields of the bitmaps extension are:
 
   8 - 15:  bitmap_directory_size
Size of the bitmap directory in bytes. It is the cumulative
-   size of all (nb_bitmaps) bitmap headers.
+   size of all (nb_bitmaps) bitmap directory entries.
 
  16 - 23:  bitmap_directory_offset
Offset into the image file at which the bitmap directory
@@ -426,8 +426,7 @@ Each bitmap saved in the image is described in a bitmap 
directory entry. The
 bitmap directory is a contiguous area in the image file, whose starting offset
 and length are given by the header extension fields bitmap_directory_offset and
 bitmap_directory_size. The entries of the bitmap directory have variable
-length, depending on the lengths of the bitmap name and extra data. These
-entries are also called bitmap headers.
+length, depending on the lengths of the bitmap name and extra data.
 
 Structure of a bitmap directory entry:
 
-- 
2.11.1

[Qemu-block] [PATCH v20 22/30] qcow2: add .bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_can_store_new_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 51 +++
 block/qcow2.c|  1 +
 block/qcow2.h|  4 
 3 files changed, 56 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 7912a82c8c..f45324e584 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1387,3 +1387,54 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 
 return 0;
 }
+
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+bool found;
+Qcow2BitmapList *bm_list;
+
+if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
+goto fail;
+}
+
+if (s->nb_bitmaps == 0) {
+return true;
+}
+
+if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+error_setg(errp,
+   "Maximum number of persistent bitmaps is already reached");
+goto fail;
+}
+
+if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
+QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
+{
+error_setg(errp, "Not enough space in the bitmap directory");
+goto fail;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+goto fail;
+}
+
+found = find_bitmap_by_name(bm_list, name);
+bitmap_list_free(bm_list);
+if (found) {
+error_setg(errp, "Bitmap with the same name is already stored");
+goto fail;
+}
+
+return true;
+
+fail:
+error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
+  name, bdrv_get_device_or_node_name(bs));
+return false;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 76603903ff..77260df512 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3612,6 +3612,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
 .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
+.bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 7d0a20c053..8b2f66f8b6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -633,5 +633,9 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v20 07/30] qcow2-refcount: rename inc_refcounts() and make it public

[Vladimir Sementsov-Ogievskiy]

This is needed for the following patch, which will introduce refcounts
checking for qcow2 bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-refcount.c | 53 ++
 block/qcow2.h  |  4 
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..d7066c875b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1323,11 +1323,10 @@ static int realloc_refcount_array(BDRVQcow2State *s, 
void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
- BdrvCheckResult *res,
- void **refcount_table,
- int64_t *refcount_table_size,
- int64_t offset, int64_t size)
+int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
+ void **refcount_table,
+ int64_t *refcount_table_size,
+ int64_t offset, int64_t size)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t start, last, cluster_offset, k, refcount;
@@ -1420,8 +1419,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 nb_csectors = ((l2_entry >> s->csize_shift) &
s->csize_mask) + 1;
 l2_entry &= s->cluster_offset_mask;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_entry & ~511, nb_csectors * 512);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_entry & ~511, nb_csectors * 512);
 if (ret < 0) {
 goto fail;
 }
@@ -1454,8 +1454,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 /* Mark cluster as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1508,8 +1509,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
 l1_size2 = l1_size * sizeof(uint64_t);
 
 /* Mark L1 table as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l1_table_offset, l1_size2);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
refcount_table_size,
+   l1_table_offset, l1_size2);
 if (ret < 0) {
 goto fail;
 }
@@ -1538,8 +1539,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
 if (l2_offset) {
 /* Mark L2 table as used */
 l2_offset &= L1E_OFFSET_MASK;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1757,14 +1759,15 @@ static int check_refblocks(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 res->corruptions_fixed++;
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }
 /* No need to check whether the refcount is now greater than 1:
  * This area was just allocated and zeroed, so it can only be
- * exactly 1 after inc_refcounts() */
+ * exactly 1 after qcow2_inc_refcounts_imrt() */
 continue;
 
 resize_fail:
@@ -1779,8 +1782,8 @@ resize_fail:
 }
 
 if (offset != 0) {
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }

[Qemu-block] [PATCH v20 03/30] hbitmap: improve dirty iter

[Vladimir Sementsov-Ogievskiy]

Make dirty iter resistant to resetting bits in corresponding HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 include/qemu/hbitmap.h | 26 --
 util/hbitmap.c | 23 ++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 9239fe515e..6b04391266 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -256,10 +256,9 @@ void hbitmap_free(HBitmap *hb);
  * the lowest-numbered bit that is set in @hb, starting at @first.
  *
  * Concurrent setting of bits is acceptable, and will at worst cause the
- * iteration to miss some of those bits.  Resetting bits before the current
- * position of the iterator is also okay.  However, concurrent resetting of
- * bits can lead to unexpected behavior if the iterator has not yet reached
- * those bits.
+ * iteration to miss some of those bits.
+ *
+ * The concurrent resetting of bits is OK.
  */
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
 
@@ -298,24 +297,7 @@ void hbitmap_free_meta(HBitmap *hb);
  * Return the next bit that is set in @hbi's associated HBitmap,
  * or -1 if all remaining bits are zero.
  */
-static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
-{
-unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-int64_t item;
-
-if (cur == 0) {
-cur = hbitmap_iter_skip_words(hbi);
-if (cur == 0) {
-return -1;
-}
-}
-
-/* The next call will resume work from the next bit.  */
-hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
-item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
-
-return item << hbi->granularity;
-}
+int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
 /**
  * hbitmap_iter_next_word:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 35088e19c4..0b38817505 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 
 unsigned long cur;
 do {
-cur = hbi->cur[--i];
+i--;
 pos >>= BITS_PER_LEVEL;
+cur = hbi->cur[i] & hb->levels[i][pos];
 } while (cur == 0);
 
 /* Check for end of iteration.  We always use fewer than BITS_PER_LONG
@@ -139,6 +140,26 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 return cur;
 }
 
+int64_t hbitmap_iter_next(HBitmapIter *hbi)
+{
+unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
+hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
+int64_t item;
+
+if (cur == 0) {
+cur = hbitmap_iter_skip_words(hbi);
+if (cur == 0) {
+return -1;
+}
+}
+
+/* The next call will resume work from the next bit.  */
+hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
+
+return item << hbi->granularity;
+}
+
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
 {
 unsigned i, bit;
-- 
2.11.1

[Qemu-block] [PATCH v20 04/30] tests: add hbitmap iter test

[Vladimir Sementsov-Ogievskiy]

Test that hbitmap iter is resistant to bitmap resetting.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 tests/test-hbitmap.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 23773d2051..1acb353889 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -909,6 +909,22 @@ static void hbitmap_test_add(const char *testpath,
hbitmap_test_teardown);
 }
 
+static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
+const void *unused)
+{
+HBitmapIter hbi;
+
+hbitmap_test_init(data, L1 * 2, 0);
+hbitmap_set(data->hb, 0, data->size);
+
+hbitmap_iter_init(, data->hb, BITS_PER_LONG - 1);
+
+hbitmap_iter_next();
+
+hbitmap_reset_all(data->hb);
+hbitmap_iter_next();
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
@@ -966,6 +982,9 @@ int main(int argc, char **argv)
  test_hbitmap_serialize_part);
 hbitmap_test_add("/hbitmap/serialize/zeroes",
  test_hbitmap_serialize_zeroes);
+
+hbitmap_test_add("/hbitmap/iter/iter_and_reset",
+ test_hbitmap_iter_and_reset);
 g_test_run();
 
 return 0;
-- 
2.11.1

[Qemu-block] [PATCH v20 12/30] block: refactor bdrv_reopen_commit

[Vladimir Sementsov-Ogievskiy]

Add bs local variable to simplify code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 50ba264143..04af7697dc 100644
--- a/block.c
+++ b/block.c
@@ -2945,9 +2945,11 @@ error:
 void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 {
 BlockDriver *drv;
+BlockDriverState *bs;
 
 assert(reopen_state != NULL);
-drv = reopen_state->bs->drv;
+bs = reopen_state->bs;
+drv = bs->drv;
 assert(drv != NULL);
 
 /* If there are any driver level actions to take */
@@ -2956,13 +2958,13 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 }
 
 /* set BDS specific flags now */
-QDECREF(reopen_state->bs->explicit_options);
+QDECREF(bs->explicit_options);
 
-reopen_state->bs->explicit_options   = reopen_state->explicit_options;
-reopen_state->bs->open_flags = reopen_state->flags;
-reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+bs->explicit_options   = reopen_state->explicit_options;
+bs->open_flags = reopen_state->flags;
+bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
-bdrv_refresh_limits(reopen_state->bs, NULL);
+bdrv_refresh_limits(bs, NULL);
 }
 
 /*
-- 
2.11.1

[Qemu-block] [PATCH v20 06/30] block/dirty-bitmap: add deserialize_ones func

[Vladimir Sementsov-Ogievskiy]

Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
all-ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  7 +++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h   | 15 +++
 util/hbitmap.c   | 17 +
 4 files changed, 42 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 186941cfc3..90af37287f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -499,6 +499,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap 
*bitmap,
 hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
 }
 
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish)
+{
+hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
+}
+
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 {
 hbitmap_deserialize_finish(bitmap->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 7cbe623ba7..1e17729ac2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,6 +70,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
   uint64_t start, uint64_t count,
   bool finish);
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6b04391266..b52304ac29 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 bool finish);
 
 /**
+ * hbitmap_deserialize_ones
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Fills the bitmap with ones.
+ *
+ * If @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish);
+
+/**
  * hbitmap_deserialize_finish
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 0b38817505..0c1591a594 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -551,6 +551,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 }
 }
 
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish)
+{
+uint64_t el_count;
+unsigned long *first;
+
+if (!count) {
+return;
+}
+serialization_chunk(hb, start, count, , _count);
+
+memset(first, 0xff, el_count * sizeof(unsigned long));
+if (finish) {
+hbitmap_deserialize_finish(hb);
+}
+}
+
 void hbitmap_deserialize_finish(HBitmap *bitmap)
 {
 int64_t i, size, prev_size;
-- 
2.11.1

[Qemu-block] [PATCH v20 14/30] qcow2: support .bdrv_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

Realize bdrv_reopen_bitmaps_rw interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 61 
 block/qcow2.c|  2 ++
 block/qcow2.h|  1 +
 3 files changed, 64 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 2c7b057e21..a21fab8ce8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -826,3 +826,64 @@ fail:
 
 return false;
 }
+
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+Qcow2Bitmap *bm;
+GSList *ro_dirty_bitmaps = NULL;
+int ret = 0;
+
+if (s->nb_bitmaps == 0) {
+/* No bitmaps - nothing to do */
+return 0;
+}
+
+if (!can_write(bs)) {
+error_setg(errp, "Can't write to the image on reopening bitmaps rw");
+return -EINVAL;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return -EINVAL;
+}
+
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (!(bm->flags & BME_FLAG_IN_USE)) {
+BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+if (bitmap == NULL) {
+continue;
+}
+
+if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Bitmap %s is not readonly but not marked"
+ "'IN_USE' in the image. Something went wrong,"
+ "all the bitmaps may be corrupted", bm->name);
+ret = -EINVAL;
+goto out;
+}
+
+bm->flags |= BME_FLAG_IN_USE;
+ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+}
+}
+
+if (ro_dirty_bitmaps != NULL) {
+/* in_use flags must be updated */
+ret = update_ext_header_and_dir_in_place(bs, bm_list);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Can't update bitmap directory");
+goto out;
+}
+g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
+}
+
+out:
+g_slist_free(ro_dirty_bitmaps);
+bitmap_list_free(bm_list);
+
+return ret;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index a70d284b75..ec00db7e49 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3596,6 +3596,8 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 67c61de008..3e23bb7361 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -630,5 +630,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   void **refcount_table,
   int64_t *refcount_table_size);
 bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 
 #endif
-- 
2.11.1

Re: [Qemu-block] [PATCH 1/2] qcow2: add reduce image support

[Pavel Butsykin]

On 01.06.2017 17:41, Kevin Wolf wrote:

Am 31.05.2017 um 16:43 hat Pavel Butsykin geschrieben:

This patch adds the reduction of the image file for qcow2. As a result, this
allows us to reduce the virtual image size and free up space on the disk without
copying the image. Image can be fragmented and reduction is done by punching
holes in the image file.

Signed-off-by: Pavel Butsykin 
---
  block/qcow2-cache.c|  8 +
  block/qcow2-cluster.c  | 83 ++
  block/qcow2-refcount.c | 65 +++
  block/qcow2.c  | 40 ++--
  block/qcow2.h  |  4 +++
  qapi/block-core.json   |  4 ++-
  6 files changed, 193 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 1d25147392..da55118ca7 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -411,3 +411,11 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, 
Qcow2Cache *c,
  assert(c->entries[i].offset != 0);
  c->entries[i].dirty = true;
  }
+
+void qcow2_cache_entry_mark_clean(BlockDriverState *bs, Qcow2Cache *c,
+ void *table)
+{
+int i = qcow2_cache_get_table_idx(bs, c, table);
+assert(c->entries[i].offset != 0);
+c->entries[i].dirty = false;
+}


This is an interesting function. We can use it whenever we're not
interested in the content of the table any more. However, we still keep
that data in the cache and may even evict other tables before this one.
The data in the cache also becomes inconsistent with the data in the
file, which should not be a problem in theory (because nobody should be
using it), but it surely could be confusing when debugging something in
the cache.



Good idea!


We can easily improve this a little: Make it qcow2_cache_discard(), a
function that gets a cluster offset, asserts that a table at this
offset isn't in use (not cached or ref == 0), and then just directly
drops it from the cache. This can be called from update_refcount()
whenever a refcount goes to 0, immediately before or after calling
update_refcount_discard() - those two are closely related. Then this
would automatically also be used for L2 tables.



Did I understand correctly? Every time we need to check the incoming
offset to make sure it is offset to L2/refcount table (not to the guest 
data) ?



Adding this mechanism could be a patch of its own

...


Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 02:25, John Snow wrote:


On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:

Hi John!

Look at our discussion about this in v18 thread.

Shortly: readonly is not the same as disabled. disabled= bitmap just

ignores all writes. readonly= writes are not allowed at all.

And I think, I'll try to go through way 2: "dirty" field instead of
"readonly" (look at v18 discussion), as it a bit more flexible.


Not sure which I prefer...

Method 1 is attractive in that it is fairly simple, and enforces fairly
loudly the inability to write to devices with RO bitmaps. It's a 
natural

extension of your current approach.


For now I decided to realize this one, I think I'll publish it today. 
Also, I'm going to rename s/readonly/in_use - to avoid the confuse 
with disabled. So let this field just be mirror of IN_USE in the 
image and just say "persistent storage knows, that bitmap is in use 
and may be dirty".


Finally it would be readonly. in_use is bad for created (not loaded) 
bitmaps. I'll add more descriptive comments for disabled and readonly.




Also, optimization with 'dirty' flag may be added later.


And, also, I don't want to influence this "first write", on which we 
will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less 
performance-demanding place than write.







Method 2 is attractive in that it seems a little more efficient, and is
a little more clever. A dirty flag lets us avoid flushing bitmaps we
never even changed (though we still need to clean up the in_use flags.)

What I wonder about #2 is what happens when a write sneaks in (due to a
bug or a use case we didn't see) on a bitmap attached to a read-only
node. We fail later on invalidate? It shouldn't happen in normal
circumstances, but I worry that the failure mode is messier.


if bitmap is dirty - all ok, the problems will appear when we'll try 
to save it, but these problems are not fatal - bitmap should be 
marked 'in_use' in the image, so it will be lost (the worst case is 
when in_use not set and bitmap is incorrect - it may lead to data 
loss for user)


if it is not dirty - we will fail to write 'in_use' before actual 
write and the whole write will fail.





Well, either way I will be happy for now I think -- pick whichever
option feels easiest or best for you to implement.

Thanks!


On 01.06.2017 02:48, John Snow wrote:

On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


---
   block/dirty-bitmap.c | 28 
   block/io.c   |  8 
   blockdev.c   |  6 ++
   include/block/dirty-bitmap.h |  4 
   4 files changed, 46 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..733f19ca5e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
   int64_t size;   /* Size of the bitmap (Number of
sectors) */
   bool disabled;  /* Bitmap is read-only */
   int active_iterators;   /* How many iterators are 
active */

+bool readonly;  /* Bitmap is read-only and may be
changed only
+   by deserialize* functions */
   QLIST_ENTRY(BdrvDirtyBitmap) list;
   };
   @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap
*bitmap,
  int64_t cur_sector, int64_t 
nr_sectors)

   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));

Not reasonable to add the condition for !readonly into
bdrv_dirty_bitmap_enabled?

As is:

If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is
going to return ACTIVE for such bitmaps, but DISABLED might be more
appropriate to indicate the read-only nature.

If you add this condition into _enabled(), you can skip the extra
assertions you've added here.


hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
   }
   @@ -443,12 +446,14 @@ void 
bdrv_reset_dirty_bitmap(BdrvDirtyBitmap

*bitmap,
int64_t cur_sector, int64_t 
nr_sectors)

   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
   }
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, 
HBitmap **out)

   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   if (!out) {
   hbitmap_reset_all(bitmap->bitmap);
   } else {
@@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, 

Re: [Qemu-block] [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 02:25, John Snow wrote:


On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:

Hi John!

Look at our discussion about this in v18 thread.

Shortly: readonly is not the same as disabled. disabled= bitmap just

ignores all writes. readonly= writes are not allowed at all.

And I think, I'll try to go through way 2: "dirty" field instead of
"readonly" (look at v18 discussion), as it a bit more flexible.


Not sure which I prefer...

Method 1 is attractive in that it is fairly simple, and enforces fairly
loudly the inability to write to devices with RO bitmaps. It's a natural
extension of your current approach.


For now I decided to realize this one, I think I'll publish it today. 
Also, I'm going to rename s/readonly/in_use - to avoid the confuse 
with disabled. So let this field just be mirror of IN_USE in the image 
and just say "persistent storage knows, that bitmap is in use and may 
be dirty".


Also, optimization with 'dirty' flag may be added later.


And, also, I don't want to influence this "first write", on which we 
will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less 
performance-demanding place than write.







Method 2 is attractive in that it seems a little more efficient, and is
a little more clever. A dirty flag lets us avoid flushing bitmaps we
never even changed (though we still need to clean up the in_use flags.)

What I wonder about #2 is what happens when a write sneaks in (due to a
bug or a use case we didn't see) on a bitmap attached to a read-only
node. We fail later on invalidate? It shouldn't happen in normal
circumstances, but I worry that the failure mode is messier.


if bitmap is dirty - all ok, the problems will appear when we'll try 
to save it, but these problems are not fatal - bitmap should be marked 
'in_use' in the image, so it will be lost (the worst case is when 
in_use not set and bitmap is incorrect - it may lead to data loss for 
user)


if it is not dirty - we will fail to write 'in_use' before actual 
write and the whole write will fail.





Well, either way I will be happy for now I think -- pick whichever
option feels easiest or best for you to implement.

Thanks!


On 01.06.2017 02:48, John Snow wrote:

On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


---
   block/dirty-bitmap.c | 28 
   block/io.c   |  8 
   blockdev.c   |  6 ++
   include/block/dirty-bitmap.h |  4 
   4 files changed, 46 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..733f19ca5e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
   int64_t size;   /* Size of the bitmap (Number of
sectors) */
   bool disabled;  /* Bitmap is read-only */
   int active_iterators;   /* How many iterators are 
active */

+bool readonly;  /* Bitmap is read-only and may be
changed only
+   by deserialize* functions */
   QLIST_ENTRY(BdrvDirtyBitmap) list;
   };
   @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap
*bitmap,
  int64_t cur_sector, int64_t nr_sectors)
   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));

Not reasonable to add the condition for !readonly into
bdrv_dirty_bitmap_enabled?

As is:

If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is
going to return ACTIVE for such bitmaps, but DISABLED might be more
appropriate to indicate the read-only nature.

If you add this condition into _enabled(), you can skip the extra
assertions you've added here.


hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
   }
   @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap
*bitmap,
int64_t cur_sector, int64_t 
nr_sectors)

   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
   }
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap 
**out)

   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   if (!out) {
   hbitmap_reset_all(bitmap->bitmap);
   } else {
@@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
cur_sector,
   if (!bdrv_dirty_bitmap_enabled(bitmap)) {
   continue;
   }
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   hbitmap_set(bitmap->bitmap, 

Re: [Qemu-block] [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

02.06.2017 02:25, John Snow wrote:


On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:

Hi John!

Look at our discussion about this in v18 thread.

Shortly: readonly is not the same as disabled. disabled= bitmap just

ignores all writes. readonly= writes are not allowed at all.

And I think, I'll try to go through way 2: "dirty" field instead of
"readonly" (look at v18 discussion), as it a bit more flexible.


Not sure which I prefer...

Method 1 is attractive in that it is fairly simple, and enforces fairly
loudly the inability to write to devices with RO bitmaps. It's a natural
extension of your current approach.


For now I decided to realize this one, I think I'll publish it today. 
Also, I'm going to rename s/readonly/in_use - to avoid the confuse with 
disabled. So let this field just be mirror of IN_USE in the image and 
just say "persistent storage knows, that bitmap is in use and may be dirty".


Also, optimization with 'dirty' flag may be added later.



Method 2 is attractive in that it seems a little more efficient, and is
a little more clever. A dirty flag lets us avoid flushing bitmaps we
never even changed (though we still need to clean up the in_use flags.)

What I wonder about #2 is what happens when a write sneaks in (due to a
bug or a use case we didn't see) on a bitmap attached to a read-only
node. We fail later on invalidate? It shouldn't happen in normal
circumstances, but I worry that the failure mode is messier.


if bitmap is dirty - all ok, the problems will appear when we'll try to 
save it, but these problems are not fatal - bitmap should be marked 
'in_use' in the image, so it will be lost (the worst case is when in_use 
not set and bitmap is incorrect - it may lead to data loss for user)


if it is not dirty - we will fail to write 'in_use' before actual write 
and the whole write will fail.





Well, either way I will be happy for now I think -- pick whichever
option feels easiest or best for you to implement.

Thanks!


On 01.06.2017 02:48, John Snow wrote:

On 05/30/2017 04:17 AM, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   block/dirty-bitmap.c | 28 
   block/io.c   |  8 
   blockdev.c   |  6 ++
   include/block/dirty-bitmap.h |  4 
   4 files changed, 46 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..733f19ca5e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
   int64_t size;   /* Size of the bitmap (Number of
sectors) */
   bool disabled;  /* Bitmap is read-only */
   int active_iterators;   /* How many iterators are active */
+bool readonly;  /* Bitmap is read-only and may be
changed only
+   by deserialize* functions */
   QLIST_ENTRY(BdrvDirtyBitmap) list;
   };
   @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap
*bitmap,
  int64_t cur_sector, int64_t nr_sectors)
   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));

Not reasonable to add the condition for !readonly into
bdrv_dirty_bitmap_enabled?

As is:

If readonly is set to true on a bitmap, bdrv_dirty_bitmap_status is
going to return ACTIVE for such bitmaps, but DISABLED might be more
appropriate to indicate the read-only nature.

If you add this condition into _enabled(), you can skip the extra
assertions you've added here.


   hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
   }
   @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap
*bitmap,
int64_t cur_sector, int64_t nr_sectors)
   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
   }
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
   {
   assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   if (!out) {
   hbitmap_reset_all(bitmap->bitmap);
   } else {
@@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
cur_sector,
   if (!bdrv_dirty_bitmap_enabled(bitmap)) {
   continue;
   }
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
   hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
   }
   }
@@ -540,3 +546,25 @@ int64_t
bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
   {
   return hbitmap_count(bitmap->meta);
   }
+
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
+{
+

Re: [Qemu-block] [PATCH v8 19/20] qcow2: report encryption specific image information

[Alberto Garcia]

On Thu 01 Jun 2017 07:27:33 PM CEST, Daniel P. Berrange wrote:
> Currently 'qemu-img info' reports a simple "encrypted: yes"
> field. This is not very useful now that qcow2 can support
> multiple encryption formats. Users want to know which format
> is in use and some data related to it.

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini]

On 01/06/2017 19:08, Kevin Wolf wrote:
> Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
>> On 01/06/2017 18:28, Kevin Wolf wrote:
 - qed_acquire/qed_release can be removed as you inline stuff, but this
 should not cause bugs so you can either do it as a final patch or let
 me remove it later.
>>> To be honest, I don't completely understand what they are protecting in
>>> the first place. The places where they are look quite strange to me. So
>>> I tried to simply leave them alone.
>>>
>>> What is the reason that we can remove them when we inline stuff?
>>> Shouldn't the inlining be semantically equivalent?
>>
>> You're right, they can be removed when going from callback to direct
>> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
>> does it for the callbacks).
> 
> So if we take qed_read_table_cb() for example:
> 
> qed_acquire(s);
> for (i = 0; i < noffsets; i++) {
> table->offsets[i] = le64_to_cpu(table->offsets[i]);
> }
> qed_release(s);
> 
> First of all, I don't see what it protects. If we wanted to avoid that
> someone else sees table->offsets with wrong endianness, we would be
> taking the lock much too late. And if nobody else knows about the table
> yet, what is there to be locked?

That is the product of a mechanical conversion where all callbacks grew
a qed_acquire/qed_release pair (commit b9e413dd37, "block: explicitly
acquire aiocontext in aio callbacks that need it", 2017-02-21).

In this case:

qed_acquire(s);
bdrv_aio_flush(write_table_cb->s->bs, qed_write_table_cb,
   write_table_cb);
qed_release(s);

the AioContext protects write_table_cb->s->bs.

> But anyway, given your explanation that acquiring the AioContext lock is
> getting replaced by coroutine magic, the qed_acquire/release() pair
> actually can't be removed in patch 2 when the callback is converted to a
> direct call, but only when the whole call path between .bdrv_aio_readv/
> writev and this specific callback is converted, so that we never drop
> out of coroutine context before reaching this code. Correct?

Yes.

> This happens only very late in the series, so it probably also means
> that patch 5 is indeed wrong because it removes the lock too early?

bdrv_qed_co_get_block_status is entirely in coroutine context, so I
think that one is fine.

Paolo

Re: [Qemu-block] virtio-scsi and request splitting

[Peter Lieven]

Am 01.06.2017 um 18:54 schrieb Paolo Bonzini:
>
> On 01/06/2017 16:25, Peter Lieven wrote:
>>>  DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI,
>>> parent_obj.conf.max_sectors,
>>> 0x),
>>>
>>> We could increase it now that we have support for max_xfer_len
>>> passthrough.
>> 0x * 512 is 32MB - 512 byte. This should not limit 1M requests?!
> Yeah, I cannot do math apparently.  Someone needs to debug the guest and
> see where the strange 504K limit comes from.

Okay, here comes the root cause. The kernel honours the max_sectors limit
of 1024 sectors (=512KB), but in virtio-blk we have request merging enabled
per default. If I disable it I get the same strange pattern 516096 + 516096 + 
16384
Byte. If I increase the limit to 2048 sectors, 1M requests go through for 
virtio-blk
and virtio-scsi.

But whats strange is that the kernel properly splits 1M requests into 2* 512KB 
for
IDE, but fails to do so for virtio-blk and virtio-scsi.

Side question: What is needed (Qemu Version and Guest Kernel) to get the 
max_requests limit
in the kernel based on max_xfer_len?

My testing is with Qemu 2.9.0 and Debian 8 (3.16 kernel)

Thanks,
Peter