Re: [Qemu-block] [PATCH] iotests: test clearing unknown autoclear_features by qcow2

[Vladimir Sementsov-Ogievskiy]

10.11.2017 23:02, Kevin Wolf wrote:

Am 10.11.2017 um 18:54 hat Vladimir Sementsov-Ogievskiy geschrieben:

Test clearing unknown autoclear_features by qcow2 on incoming
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all!

This patch shows degradation, added in 2.10 in commit

commit 9c5e6594f15b7364624a3ad40306c396c93a2145
Author: Kevin Wolf 
Date:   Thu May 4 18:52:40 2017 +0200

 block: Fix write/resize permissions for inactive images

The problem:
When on incoming migration with shared storage we reopen image to RW mode
we call bdrv_invalidate_cache it firstly call drv->bdrv_invalidate_cache and
only after it, through "parent->role->activate(parent, _err);" we set
appropriate RW permission.

Because of this, if drv->bdrv_invalidate_cache wants to write something
(image is RW and BDRV_O_INACTIVE is not set) it goes into
"bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed"

One case, when qcow2_invalidate_cache wants to write
something - when it wants to clear some unknown autoclear features. So,
here is a test for it.

Another case is when we try to migrate persistent dirty bitmaps through
shared storage, on invalidate_cache qcow2 will try to set DIRTY bit in
all loaded bitmaps.

Kevin, what do you think? I understand that operation order should be changed
somehow in bdrv_invalidate_cache, but I'm not sure about how "parent->role->.."
things works and can we safely move this part from function end to the middle.

I don't think you need to move the parent->role->activate() callback,
but just the bdrv_set_perm() so that we request the correct permissions
for operation without the BDRV_O_INACTIVE flag.

The following seems to work for me (including a fix for the test case,
we don't seem to get a RESUME event).


hmm, it works for me with RESUME..


I'm not sure about the error paths
yet. We should probably try do undo the permission changes there. I can
have a closer look into this next week.

Kevin


diff --git a/block.c b/block.c
index edc5bb9f9b..f530b630b6 100644
--- a/block.c
+++ b/block.c
@@ -4178,7 +4178,17 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error 
**errp)
  }
  }
  
+/* Update permissions, they may differ for inactive nodes */

  bs->open_flags &= ~BDRV_O_INACTIVE;
+bdrv_get_cumulative_perm(bs, , _perm);
+ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, _err);
+if (ret < 0) {
+bs->open_flags |= BDRV_O_INACTIVE;
+error_propagate(errp, local_err);
+return;
+}
+bdrv_set_perm(bs, perm, shared_perm);
+
  if (bs->drv->bdrv_invalidate_cache) {
  bs->drv->bdrv_invalidate_cache(bs, _err);
  if (local_err) {
@@ -4195,16 +4205,6 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error 
**errp)
  return;
  }
  
-/* Update permissions, they may differ for inactive nodes */

-bdrv_get_cumulative_perm(bs, , _perm);
-ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, _err);
-if (ret < 0) {
-bs->open_flags |= BDRV_O_INACTIVE;
-error_propagate(errp, local_err);
-return;
-}
-bdrv_set_perm(bs, perm, shared_perm);
-
  QLIST_FOREACH(parent, >parents, next_parent) {
  if (parent->role->activate) {
  parent->role->activate(parent, _err);
diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
index 9ffbc723c2..4116ebc92b 100755
--- a/tests/qemu-iotests/196
+++ b/tests/qemu-iotests/196
@@ -53,7 +53,10 @@ class TestInvalidateAutoclear(iotests.QMPTestCase):
  f.write(b'\xff')
  
  self.vm_b.launch()

-self.assertNotEqual(self.vm_b.event_wait("RESUME"), None)
+while True:
+result = self.vm_b.qmp('query-status')
+if result['return']['status'] == 'running':
+break
  
  with open(disk, 'rb') as f:

  f.seek(88)



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 01/10] qemu-iotests: make execution of tests agnostic to test type

[Paolo Bonzini]

On 16/11/2017 18:38, Cleber Rosa wrote:
> check makes a distinction on how it runs Python based tests.  The
> current approach is inconsistent because:
> 
> 1) a large number of Python tests are already set as executable files
> (eg: 030, 040, 041, 044, 045, 055, 056, 057, 065, 093, 118, 147, 149,
> 155, 165 and 194)
> 
> 2) a smaller number of Python tests are not set as executable files
> 
> 3) the true purpose of a shebang line is to make a file executable,
> while it currently is used (inconsistently) as a test type flag
> 
> 4) the same logic could in theory be applied to shell based tests,
> that is, if first line contains a shell shebang, run it with
> "$SHELL $test_file", but it'd be pointless
> 
> IMO, there's no value in the distinction that check makes.  Dropping
> this distinction makes the interface simpler: check requires an
> executable file.
> 
> Signed-off-by: Cleber Rosa 

This is quirky, but I think it makes sense to obey the configure
script's chosen Python interpreter.  Unlike /bin/sh or /bin/bash, there
can be many Python interpreters on a system and the user may not want
the one in /usr/bin/python (think of old RHEL with software collections,
even though our use of Python is generally portable to older versions).

I'd keep it for now; in the longer term, we should set up a virtual
environment so that the shebang line does the right thing.

However, making files executable is certainly a good idea.

Thanks,

Paolo


> ---
>  tests/qemu-iotests/096   | 0
>  tests/qemu-iotests/124   | 0
>  tests/qemu-iotests/129   | 0
>  tests/qemu-iotests/132   | 0
>  tests/qemu-iotests/136   | 0
>  tests/qemu-iotests/139   | 0
>  tests/qemu-iotests/148   | 0
>  tests/qemu-iotests/152   | 0
>  tests/qemu-iotests/163   | 0
>  tests/qemu-iotests/check | 9 ++---
>  10 files changed, 2 insertions(+), 7 deletions(-)
>  mode change 100644 => 100755 tests/qemu-iotests/096
>  mode change 100644 => 100755 tests/qemu-iotests/124
>  mode change 100644 => 100755 tests/qemu-iotests/129
>  mode change 100644 => 100755 tests/qemu-iotests/132
>  mode change 100644 => 100755 tests/qemu-iotests/136
>  mode change 100644 => 100755 tests/qemu-iotests/139
>  mode change 100644 => 100755 tests/qemu-iotests/148
>  mode change 100644 => 100755 tests/qemu-iotests/152
>  mode change 100644 => 100755 tests/qemu-iotests/163
> 
> diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/152 b/tests/qemu-iotests/152
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
> old mode 100644
> new mode 100755
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index e6b6ff7a04..08741328d0 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -741,20 +741,15 @@ do
>  start=`_wallclock`
>  $timestamp && printf %s "[$(date "+%T")]"
>  
> -if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env 
> python" ]; then
> -run_command="$PYTHON $seq"
> -else
> -run_command="./$seq"
> -fi
>  export OUTPUT_DIR=$PWD
>  if $debug; then
>  (cd "$source_iotests";
>  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> -$run_command -d 2>&1 | tee $tmp.out)
> +./$seq -d 2>&1 | tee $tmp.out)
>  else
>  (cd "$source_iotests";
>  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> -$run_command >$tmp.out 2>&1)
> +./$seq >$tmp.out 2>&1)
>  fi
>  sts=$?
>  $timestamp && _timestamp
> 

Re: [Qemu-block] [PATCH 06/10] qemu-iotests: turn owner variable into a comment

[Paolo Bonzini]

On 16/11/2017 18:38, Cleber Rosa wrote:
> This variables has no real use.  To avoid pretending it does, while
> still keeping the information, let's turn it into a comment.
> 
> The format chosen is the one already being used on tests 149 and 194.

I would just delete it...

Paolo

> Signed-off-by: Cleber Rosa 
> ---
>  tests/qemu-iotests/001 | 5 ++---
>  tests/qemu-iotests/002 | 5 ++---
>  tests/qemu-iotests/003 | 5 ++---
>  tests/qemu-iotests/004 | 5 ++---
>  tests/qemu-iotests/005 | 5 ++---
>  tests/qemu-iotests/007 | 5 ++---
>  tests/qemu-iotests/008 | 5 ++---
>  tests/qemu-iotests/009 | 5 ++---
>  tests/qemu-iotests/010 | 5 ++---
>  tests/qemu-iotests/011 | 5 ++---
>  tests/qemu-iotests/012 | 5 ++---
>  tests/qemu-iotests/013 | 5 ++---
>  tests/qemu-iotests/014 | 5 ++---
>  tests/qemu-iotests/015 | 5 ++---
>  tests/qemu-iotests/017 | 5 ++---
>  tests/qemu-iotests/018 | 5 ++---
>  tests/qemu-iotests/019 | 5 ++---
>  tests/qemu-iotests/020 | 5 ++---
>  tests/qemu-iotests/021 | 5 ++---
>  tests/qemu-iotests/022 | 5 ++---
>  tests/qemu-iotests/023 | 5 ++---
>  tests/qemu-iotests/024 | 5 ++---
>  tests/qemu-iotests/025 | 5 ++---
>  tests/qemu-iotests/026 | 5 ++---
>  tests/qemu-iotests/027 | 5 ++---
>  tests/qemu-iotests/028 | 5 ++---
>  tests/qemu-iotests/029 | 5 ++---
>  tests/qemu-iotests/031 | 5 ++---
>  tests/qemu-iotests/032 | 5 ++---
>  tests/qemu-iotests/033 | 5 ++---
>  tests/qemu-iotests/034 | 5 ++---
>  tests/qemu-iotests/035 | 5 ++---
>  tests/qemu-iotests/036 | 5 ++---
>  tests/qemu-iotests/037 | 5 ++---
>  tests/qemu-iotests/038 | 5 ++---
>  tests/qemu-iotests/039 | 5 ++---
>  tests/qemu-iotests/040 | 1 +
>  tests/qemu-iotests/042 | 5 ++---
>  tests/qemu-iotests/043 | 5 ++---
>  tests/qemu-iotests/046 | 5 ++---
>  tests/qemu-iotests/047 | 5 ++---
>  tests/qemu-iotests/048 | 4 ++--
>  tests/qemu-iotests/049 | 5 ++---
>  tests/qemu-iotests/050 | 5 ++---
>  tests/qemu-iotests/051 | 5 ++---
>  tests/qemu-iotests/052 | 5 ++---
>  tests/qemu-iotests/053 | 5 ++---
>  tests/qemu-iotests/054 | 5 ++---
>  tests/qemu-iotests/058 | 5 ++---
>  tests/qemu-iotests/059 | 5 ++---
>  tests/qemu-iotests/060 | 5 ++---
>  tests/qemu-iotests/061 | 5 ++---
>  tests/qemu-iotests/062 | 5 ++---
>  tests/qemu-iotests/063 | 5 ++---
>  tests/qemu-iotests/064 | 5 ++---
>  tests/qemu-iotests/066 | 5 ++---
>  tests/qemu-iotests/067 | 5 ++---
>  tests/qemu-iotests/068 | 5 ++---
>  tests/qemu-iotests/069 | 5 ++---
>  tests/qemu-iotests/070 | 5 ++---
>  tests/qemu-iotests/071 | 5 ++---
>  tests/qemu-iotests/072 | 5 ++---
>  tests/qemu-iotests/073 | 5 ++---
>  tests/qemu-iotests/074 | 3 +--
>  tests/qemu-iotests/075 | 5 ++---
>  tests/qemu-iotests/076 | 5 ++---
>  tests/qemu-iotests/077 | 5 ++---
>  tests/qemu-iotests/078 | 5 ++---
>  tests/qemu-iotests/079 | 5 ++---
>  tests/qemu-iotests/080 | 5 ++---
>  tests/qemu-iotests/081 | 5 ++---
>  tests/qemu-iotests/082 | 5 ++---
>  tests/qemu-iotests/083 | 5 ++---
>  tests/qemu-iotests/084 | 5 ++---
>  tests/qemu-iotests/085 | 5 ++---
>  tests/qemu-iotests/086 | 5 ++---
>  tests/qemu-iotests/087 | 5 ++---
>  tests/qemu-iotests/088 | 5 ++---
>  tests/qemu-iotests/089 | 5 ++---
>  tests/qemu-iotests/090 | 5 ++---
>  tests/qemu-iotests/091 | 5 ++---
>  tests/qemu-iotests/092 | 5 ++---
>  tests/qemu-iotests/094 | 5 ++---
>  tests/qemu-iotests/095 | 4 ++--
>  tests/qemu-iotests/097 | 5 ++---
>  tests/qemu-iotests/098 | 5 ++---
>  tests/qemu-iotests/099 | 5 ++---
>  tests/qemu-iotests/101 | 5 ++---
>  tests/qemu-iotests/102 | 5 ++---
>  tests/qemu-iotests/103 | 5 ++---
>  tests/qemu-iotests/104 | 5 ++---
>  tests/qemu-iotests/105 | 5 ++---
>  tests/qemu-iotests/106 | 5 ++---
>  tests/qemu-iotests/107 | 5 ++---
>  tests/qemu-iotests/108 | 5 ++---
>  tests/qemu-iotests/109 | 5 ++---
>  tests/qemu-iotests/110 | 5 ++---
>  tests/qemu-iotests/111 | 5 ++---
>  tests/qemu-iotests/112 | 5 ++---
>  tests/qemu-iotests/113 | 5 ++---
>  tests/qemu-iotests/114 | 5 ++---
>  tests/qemu-iotests/115 | 5 ++---
>  tests/qemu-iotests/116 | 5 ++---
>  tests/qemu-iotests/117 | 5 ++---
>  tests/qemu-iotests/119 | 5 ++---
>  tests/qemu-iotests/120 | 5 ++---
>  tests/qemu-iotests/121 | 5 ++---
>  tests/qemu-iotests/122 | 5 ++---
>  tests/qemu-iotests/123 | 5 ++---
>  tests/qemu-iotests/125 | 5 ++---
>  tests/qemu-iotests/126 | 5 ++---
>  tests/qemu-iotests/127 | 5 ++---
>  tests/qemu-iotests/128 | 5 ++---
>  tests/qemu-iotests/130 | 5 ++---
>  tests/qemu-iotests/131 | 5 ++---
>  tests/qemu-iotests/133 | 5 ++---
>  tests/qemu-iotests/134 | 5 ++---
>  tests/qemu-iotests/135 | 5 ++---
>  tests/qemu-iotests/137 | 5 ++---
>  tests/qemu-iotests/138 | 5 ++---
>  tests/qemu-iotests/140 | 5 ++---
>  tests/qemu-iotests/141 | 5 ++---
>  tests/qemu-iotests/142 | 5 ++---
>  tests/qemu-iotests/143 | 5 ++---
>  tests/qemu-iotests/144 | 5 ++---
>  tests/qemu-iotests/145 | 5 ++---
>  tests/qemu-iotests/146 | 5 ++---
>  tests/qemu-iotests/150 | 5 ++---
>  tests/qemu-iotests/153 | 5 ++---
>  tests/qemu-iotests/154 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 0/4] qmp dirty bitmap API

[John Snow]

On 11/16/2017 03:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> 16.11.2017 00:20, John Snow wrote:
>>
>> On 11/13/2017 11:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all.
>>>
>>> There are three qmp commands, needed to implement external backup API.
>>>
>>> Using these three commands, client may do all needed bitmap
>>> management by
>>> hand:
>>>
>>> on backup start we need to do a transaction:
>>>   {disable old bitmap, create new bitmap}
>>>
>>> on backup success:
>>>   drop old bitmap
>>>
>>> on backup fail:
>>>   enable old bitmap
>>>   merge new bitmap to old bitmap
>>>   drop new bitmap
>>>
>> Can you give me an example of how you expect these commands to be used,
>> and why they're required?
>>
>> I'm a little weary about how error-prone these commands might be and the
>> potential for incorrect usage seems... high. Why do we require them?
> 
> It is needed for incremental backup. It looks like bad idea to export
> abdicate/reclaim functionality, it is simpler
> and clearer to allow user to merge/enable/disable bitmaps by hand.
> 
> usage is like this:
> 
> 1. we have dirty bitmap bitmap0 for incremental backup.
> 
> 2. prepare image fleecing (create temporary image with backing=our_disk)
> 3. in qmp transaction:
>     - disable bitmap0
>     - create bitmap1
>     - start image fleecing (backup sync=none our_disk -> temp_disk)

This could probably just be its own command, though:

block-job-fleece node=foobar bitmap=bitmap0 etc=etera etc=etera

Could handle forking the bitmap. I'm not sure what the arguments would
look like, but we could name the NBD export here, too. (Assuming the
server is already started and we just need to create the share.)

Then, we can basically do what mirror does:

(1) Cancel
(2) Complete

Cancel would instruct QEMU to keep the bitmap changes (i.e. roll back),
and Complete would instruct QEMU to discard the changes.

This way we don't need to expose commands like split or merge that will
almost always be dangerous to use over QMP.

In fact, a fleecing job would be really convenient even without a
bitmap, because it'd still be nice to have a convenience command for it.
Using an existing infrastructure and understood paradigm is just a bonus.

> 4. export temp_disk (it looks like a snapshot) and bitmap0 through NBD
> 
> === external tool can get bitmap0 and then copy some clusters through
> NBD ===
> 
> on successful backup external tool can drop bitmap0. or can save it and
> reuse somehow
> 
> on failed backup external tool can do the following:
>     - enable bitmap0
>     - merge bitmap1 to bitmap0
>     - drop bitmap1
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[John Snow]

On 11/16/2017 11:50 AM, Dr. David Alan Gilbert wrote:
> * John Snow (js...@redhat.com) wrote:
>>
>>
>> On 10/30/2017 12:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
>>> associated with root nodes and non-root named nodes are migrated.
>>>
>>> If destination qemu is already containing a dirty bitmap with the same name
>>> as a migrated bitmap (for the same node), then, if their granularities are
>>> the same the migration will be done, otherwise the error will be generated.
>>>
>>> If destination qemu doesn't contain such bitmap it will be created.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>  include/migration/misc.h   |   3 +
>>>  migration/migration.h  |   3 +
>>>  migration/block-dirty-bitmap.c | 734 
>>> +
>>
>> Ouch :\
>>
>>>  migration/migration.c  |   3 +
>>>  migration/savevm.c |   2 +
>>>  vl.c   |   1 +
>>>  migration/Makefile.objs|   1 +
>>>  migration/trace-events |  14 +
>>>  8 files changed, 761 insertions(+)
>>>  create mode 100644 migration/block-dirty-bitmap.c
>>>
>>
>> Organizationally, you introduce three new 'public' prototypes:
>>
>> dirty_bitmap_mig_init
>> dirty_bitmap_mig_before_vm_start
>> init_dirty_bitmap_incoming_migration
>>
>> mig_init is advertised in migration/misc.h, the other two are in
>> migration/migration.h.
>> The definitions for all three are in migration/block-dirty-bitmap.c
>>
>> In pure naivety, I find it weird to have something that you use in
>> migration.c and advertised in migration.h actually exist separately in
>> block-dirty-bitmap.c; but maybe this is the sanest thing to do.
> 
> Actually I think that's OK; it makes sense for all of the code for this
> feature to sit in one place,   and there doesn't seem any point creating
> a header just for this one function.
> 

OK, just stood out to me at first. I don't really have better ideas.

>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>> index c079b7771b..9cc539e232 100644
>>> --- a/include/migration/misc.h
>>> +++ b/include/migration/misc.h
>>> @@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
>>>  bool migration_in_postcopy_after_devices(MigrationState *);
>>>  void migration_global_dump(Monitor *mon);
>>>  
>>> +/* migration/block-dirty-bitmap.c */
>>> +void dirty_bitmap_mig_init(void);
>>> +
>>>  #endif
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 50d1f01346..4e3ad04664 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
>>>  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
>>> rbname,
>>>ram_addr_t start, size_t len);
>>>  
>>> +void dirty_bitmap_mig_before_vm_start(void);
>>> +void init_dirty_bitmap_incoming_migration(void);
>>> +
>>>  #endif
>>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>>> new file mode 100644
>>> index 00..53cb20045d
>>> --- /dev/null
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -0,0 +1,734 @@
>>> +/*
>>> + * Block dirty bitmap postcopy migration
>>> + *
>>> + * Copyright IBM, Corp. 2009
>>> + * Copyright (c) 2016-2017 Parallels International GmbH
>>> + *
>>> + * Authors:
>>> + *  Liran Schour   
>>> + *  Vladimir Sementsov-Ogievskiy 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>> + * the COPYING file in the top-level directory.
>>> + * This file is derived from migration/block.c, so it's author and IBM 
>>> copyright
>>> + * are here, although content is quite different.
>>> + *
>>> + * Contributions after 2012-01-13 are licensed under the terms of the
>>> + * GNU GPL, version 2 or (at your option) any later version.
>>> + *
>>> + ****
>>> + *
>>> + * Here postcopy migration of dirty bitmaps is realized. Only named dirty
>>> + * bitmaps, associated with root nodes and non-root named nodes are 
>>> migrated.
>>
>> Put another way, only QMP-addressable bitmaps. Nodes without a name that
>> are not the root have no way to be addressed.
>>
>>> + *
>>> + * If destination qemu is already containing a dirty bitmap with the same 
>>> name
>>
>> "If the destination QEMU already contains a dirty bitmap with the same name"
>>
>>> + * as a migrated bitmap (for the same node), then, if their granularities 
>>> are
>>> + * the same the migration will be done, otherwise the error will be 
>>> generated.
>>
>> "an error"
>>
>>> + *
>>> + * If destination qemu doesn't contain such bitmap it will be created.
>>
>> "If the destination QEMU doesn't contain such a bitmap, it will be created."
>>
>>> + *
>>> + * format of migration:
>>> + *
>>> + * # Header (shared for different chunk types)
>>> + * 1, 2 or 

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[John Snow]

On 11/16/2017 05:24 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.11.2017 04:58, John Snow wrote:
>>
>> On 10/30/2017 12:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
>>> associated with root nodes and non-root named nodes are migrated.
>>>
>>> If destination qemu is already containing a dirty bitmap with the
>>> same name
>>> as a migrated bitmap (for the same node), then, if their
>>> granularities are
>>> the same the migration will be done, otherwise the error will be
>>> generated.
>>>
>>> If destination qemu doesn't contain such bitmap it will be created.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   include/migration/misc.h   |   3 +
>>>   migration/migration.h  |   3 +
>>>   migration/block-dirty-bitmap.c | 734
>>> +
>> Ouch :\
>>
>>>   migration/migration.c  |   3 +
>>>   migration/savevm.c |   2 +
>>>   vl.c   |   1 +
>>>   migration/Makefile.objs    |   1 +
>>>   migration/trace-events |  14 +
>>>   8 files changed, 761 insertions(+)
>>>   create mode 100644 migration/block-dirty-bitmap.c
>>>
>> Organizationally, you introduce three new 'public' prototypes:
>>
>> dirty_bitmap_mig_init
>> dirty_bitmap_mig_before_vm_start
>> init_dirty_bitmap_incoming_migration
>>
>> mig_init is advertised in migration/misc.h, the other two are in
>> migration/migration.h.
>> The definitions for all three are in migration/block-dirty-bitmap.c
>>
>> In pure naivety, I find it weird to have something that you use in
>> migration.c and advertised in migration.h actually exist separately in
>> block-dirty-bitmap.c; but maybe this is the sanest thing to do.
>>
>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>> index c079b7771b..9cc539e232 100644
>>> --- a/include/migration/misc.h
>>> +++ b/include/migration/misc.h
>>> @@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
>>>   bool migration_in_postcopy_after_devices(MigrationState *);
>>>   void migration_global_dump(Monitor *mon);
>>>   +/* migration/block-dirty-bitmap.c */
>>> +void dirty_bitmap_mig_init(void);
>>> +
>>>   #endif
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 50d1f01346..4e3ad04664 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState
>>> *mis,
>>>   void migrate_send_rp_req_pages(MigrationIncomingState *mis, const
>>> char* rbname,
>>>     ram_addr_t start, size_t len);
>>>   +void dirty_bitmap_mig_before_vm_start(void);
>>> +void init_dirty_bitmap_incoming_migration(void);
>>> +
>>>   #endif
>>> diff --git a/migration/block-dirty-bitmap.c
>>> b/migration/block-dirty-bitmap.c
>>> new file mode 100644
>>> index 00..53cb20045d
>>> --- /dev/null
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -0,0 +1,734 @@
>>> +/*
>>> + * Block dirty bitmap postcopy migration
>>> + *
>>> + * Copyright IBM, Corp. 2009
>>> + * Copyright (c) 2016-2017 Parallels International GmbH
>>> + *
>>> + * Authors:
>>> + *  Liran Schour   
>>> + *  Vladimir Sementsov-Ogievskiy 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2. 
>>> See
>>> + * the COPYING file in the top-level directory.
>>> + * This file is derived from migration/block.c, so it's author and
>>> IBM copyright
>>> + * are here, although content is quite different.
>>> + *
>>> + * Contributions after 2012-01-13 are licensed under the terms of the
>>> + * GNU GPL, version 2 or (at your option) any later version.
>>> + *
>>> + *    ***
>>> + *
>>> + * Here postcopy migration of dirty bitmaps is realized. Only named
>>> dirty
>>> + * bitmaps, associated with root nodes and non-root named nodes are
>>> migrated.
>> Put another way, only QMP-addressable bitmaps. Nodes without a name that
>> are not the root have no way to be addressed.
>>
>>> + *
>>> + * If destination qemu is already containing a dirty bitmap with the
>>> same name
>> "If the destination QEMU already contains a dirty bitmap with the same
>> name"
>>
>>> + * as a migrated bitmap (for the same node), then, if their
>>> granularities are
>>> + * the same the migration will be done, otherwise the error will be
>>> generated.
>> "an error"
>>
>>> + *
>>> + * If destination qemu doesn't contain such bitmap it will be created.
>> "If the destination QEMU doesn't contain such a bitmap, it will be
>> created."
>>
>>> + *
>>> + * format of migration:
>>> + *
>>> + * # Header (shared for different chunk types)
>>> + * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
>>> + * [ 1 byte: node name size ] \  flags & DEVICE_NAME
>>> + * [ n bytes: node name ] /
>>> + * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
>>> + * [ n bytes: bitmap name ] /
>>> + *

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[John Snow]

On 11/16/2017 05:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> +    /* if a block is zero we need to flush here since the network
>>> + * bandwidth is now a lot higher than the storage device bandwidth.
>>> + * thus if we queue zero blocks we slow down the migration. */
>> Can you elaborate on this for me?
> 
> it comes from migration/block.c, as it was a prototype for dirty bitmaps
> migration.
> May be the original thought was to give destination storage more time to
> handle
> write-zeros? It may make sense if it can't do it fast but really writes
> zeros.
> 
>>
>>> +    if (flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
>>> +    qemu_fflush(f);
>>> +    } else {
>>> +    qemu_put_be64(f, buf_size);
>>> +    qemu_put_buffer(f, buf, buf_size);
>>> +    }
>>> +
>>> +    g_free(buf);
>>> +} 

Peter, Stefan; do you have any insight on the reason for this blurb's
appearance in migration/block.c? Looks like it showed up in
323004a39d4d8d33c744a5b108f80bfe6402fca3

I'm not sure I understand what the concern was that lead to this, unless
it's simply that without a flush() we can't guarantee progress.

Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)

[John Snow]

On 11/16/2017 10:54 AM, Alberto Garcia wrote:
> On Wed 15 Nov 2017 05:31:20 PM CET, Anton Nefedov wrote:
>>> I have the impression that one major source of headaches is the fact
>>> that the reopen queue contains nodes that don't need to be reopened at
>>> all. Ideally this should be detected early on in bdrv_reopen_queue(), so
>>> there's no chance that the queue contains nodes used by a different
>>> block job. If we had that then op blockers should be enough to prevent
>>> these things. Or am I missing something?
>>>
>> After applying Max's patch I tried the similar approach; that is keep
>> BDSes referenced while they are in the reopen queue.  Now I get the
>> stream job hanging. Somehow one blk_root_drained_begin() is not paired
>> with blk_root_drained_end(). So the job stays paused.
> 
> I can see this if I apply Max's patch and keep refs to BDSs in the
> reopen queue:
> 
> #0  block_job_pause (...) at blockjob.c:130
> #1  0x55c143cb586d in block_job_drained_begin (...) at blockjob.c:227
> #2  0x55c143d08067 in blk_set_dev_ops (...) at block/block-backend.c:887
> #3  0x55c143cb69db in block_job_create (...) at blockjob.c:678
> #4  0x55c143d17c0c in mirror_start_job (...) at block/mirror.c:1177
> 
> There's a ops->drained_begin(opaque) call in blk_set_dev_ops() that
> doesn't seem to be paired. And when you call block_job_start() then it
> yields immediately waiting for the resume (that never arrives).
> 
> John, this change was yours (f4d9cc88ee69a5b04). Any idea?
> 
> Berto
> 

The idea at the time was that if you tell the BlockBackend to drain and
then attach a job to it, once you go to *end* the drained region you'd
have a mismatched begin/end pair.

To allow for some flexibility and to make sure that you *didn't* have a
mismatched begin/end call, what I did was that if you attach dev_ops to
an already drained backend (i.e. we "missed our chance" to issue the
drained_begin) we play catch-up and issue the drained call.

There's no matching call here, because I anticipated whoever initially
bumped that quiesce_counter up to be issuing the drained_end, which will
then be propagated according to the dev_ops structure in place.

Re: [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[John Snow]

On 11/16/2017 04:14 AM, Kashyap Chamarthy wrote:
> On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:
>>
>>
>> On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
>>> On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:
> 
> [...]
> 
 is it covered sufficiently in live-block-operations.rst ?
>>>
>>> I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
>>> synchronization — drive-mirror and blockdev-mirror" section, I simply
>>> seemed to declare: 
>>>
>>> "Issuing the command ``block-job-cancel`` after it emits the event
>>> ``BLOCK_JOB_CANCELLED``"
>>>
>>> As if that's the *only* event it emits, which is clearly not the case.
>>> So while at it, wonder if should I also update it
>>> ('live-block-operations.rst') too.
>>>
>>
>> It's an interesting gotcha that I wasn't really acutely aware of myself,
>> so having it in the doc format for API programmers who aren't
>> necessarily digging through our source sounds like a pleasant courtesy.
> 
> Indeed, will do.  (Just for my own clarity, did you imply: don't update
> it in block-core.json?  FWIW, my first instinct is to check the QAPI
> documentation for such things, that's why I wrote there first :-))
> 

No, definitely update both.

> Thanks for looking.
> 
> [...]
> 

Re: [Qemu-block] [PATCH] iotests: test clearing unknown autoclear_features by qcow2

[John Snow]

On 11/10/2017 03:02 PM, Kevin Wolf wrote:
> Am 10.11.2017 um 18:54 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Test clearing unknown autoclear_features by qcow2 on incoming
>> migration.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>
>> Hi all!
>>
>> This patch shows degradation, added in 2.10 in commit
>>
>> commit 9c5e6594f15b7364624a3ad40306c396c93a2145
>> Author: Kevin Wolf 
>> Date:   Thu May 4 18:52:40 2017 +0200
>>
>> block: Fix write/resize permissions for inactive images
>>
>> The problem:
>> When on incoming migration with shared storage we reopen image to RW mode
>> we call bdrv_invalidate_cache it firstly call drv->bdrv_invalidate_cache and
>> only after it, through "parent->role->activate(parent, _err);" we set
>> appropriate RW permission.
>>
>> Because of this, if drv->bdrv_invalidate_cache wants to write something
>> (image is RW and BDRV_O_INACTIVE is not set) it goes into
>> "bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed"
>>
>> One case, when qcow2_invalidate_cache wants to write
>> something - when it wants to clear some unknown autoclear features. So,
>> here is a test for it.
>>
>> Another case is when we try to migrate persistent dirty bitmaps through
>> shared storage, on invalidate_cache qcow2 will try to set DIRTY bit in
>> all loaded bitmaps.
>>
>> Kevin, what do you think? I understand that operation order should be changed
>> somehow in bdrv_invalidate_cache, but I'm not sure about how 
>> "parent->role->.."
>> things works and can we safely move this part from function end to the 
>> middle.
> 
> I don't think you need to move the parent->role->activate() callback,
> but just the bdrv_set_perm() so that we request the correct permissions
> for operation without the BDRV_O_INACTIVE flag.
> 
> The following seems to work for me (including a fix for the test case,
> we don't seem to get a RESUME event). I'm not sure about the error paths
> yet. We should probably try do undo the permission changes there. I can
> have a closer look into this next week.
> 
> Kevin
> 

What's the status here, something we need to be paying attention to for
2.11?

Re: [Qemu-block] [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version of bdrv_release_dirty_bitmap

[John Snow]

On 11/16/2017 03:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2017 01:52, John Snow wrote:
>>
>> On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> It is needed to realize bdrv_dirty_bitmap_release_successor in
>>> the following patch.
>>>
>> OK, but...
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/dirty-bitmap.c | 25 -
>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 81adbeb6d4..981f99d362 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -326,13 +326,13 @@ static bool
>>> bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
>>>   return !!bdrv_dirty_bitmap_name(bitmap);
>>>   }
>>>   -/* Called with BQL taken.  */
>>> -static void bdrv_do_release_matching_dirty_bitmap(
>>> +/* Called within bdrv_dirty_bitmap_lock..unlock */
>> ...Add this so it will compile:
>>
>> __attribute__((__unused__))
> 
> ok
> 
>>> +static void bdrv_do_release_matching_dirty_bitmap_locked(
>>>   BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>>   bool (*cond)(BdrvDirtyBitmap *bitmap))
>>>   {
>>>   BdrvDirtyBitmap *bm, *next;
>>> -    bdrv_dirty_bitmaps_lock(bs);
>>> +
>>>   QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
>>>   if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
>>>   assert(!bm->active_iterators);
>>> @@ -344,18 +344,33 @@ static void bdrv_do_release_matching_dirty_bitmap(
>>>   g_free(bm);
>>>     if (bitmap) {
>>> -    goto out;
>>> +    return;
>>>   }
>>>   }
>>>   }
>>> +
>>>   if (bitmap) {
>>>   abort();
>>>   }
>> Do we have any style guide rules on using abort() instead of assert()?
>> The rest of this function uses assert, and it'd be less lines to simply
>> write:
>>
>> assert(!bitmap);
>>
>> which I think might also carry better semantic information for coverity
>> beyond an actual runtime conditional branch.
>>
>> (I think. Please correct me if I am wrong, I'm a little hazy on this.)
> 
> agree, but it is a preexisting code, so I'll fix it with an additional
> patch.
> 

You're right. I didn't notice it was pre-existing where I was looking at
it. I'll send my own little fixup. My mistake.

>>
>>> +}
>>>   -out:
>>> +/* Called with BQL taken.  */
>>> +static void bdrv_do_release_matching_dirty_bitmap(
>>> +    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>> +    bool (*cond)(BdrvDirtyBitmap *bitmap))
>>> +{
>>> +    bdrv_dirty_bitmaps_lock(bs);
>>> +    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
>>>   bdrv_dirty_bitmaps_unlock(bs);
>>>   }
>>>   +/* Called within bdrv_dirty_bitmap_lock..unlock */
>>> +static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
>>> +}
>>> +
>>>   /* Called with BQL taken.  */
>>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap)
>>>   {
>>>
>> If you agree with those two changes, you may add:
> 
> ok
> 
>>
>> Reviewed-by: John Snow 
> 
> 

Just make sure it compiles standalone by using the unused attribute and
you can add the RB.

--js

[Qemu-block] [PATCH 05/10] qemu-iotests: define functions used in _cleanup() before its use

[Cleber Rosa]

The functions used in _cleanup() come from common.rc, which currently
gets sourced after _cleanup() is defined and registered as a signal
handler.  When _cleanup() is executed, it has no valid references to
those functions, as BASH won't resolve the reference at that time.

While at it, also fix some tabs versus spaces indentation in
_cleanup() in a few tests.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/001 |  6 +++---
 tests/qemu-iotests/002 |  6 +++---
 tests/qemu-iotests/003 |  6 +++---
 tests/qemu-iotests/004 |  6 +++---
 tests/qemu-iotests/005 |  6 +++---
 tests/qemu-iotests/007 |  6 +++---
 tests/qemu-iotests/008 |  6 +++---
 tests/qemu-iotests/009 |  6 +++---
 tests/qemu-iotests/010 |  6 +++---
 tests/qemu-iotests/011 |  6 +++---
 tests/qemu-iotests/012 |  6 +++---
 tests/qemu-iotests/013 |  8 
 tests/qemu-iotests/014 |  8 
 tests/qemu-iotests/015 |  6 +++---
 tests/qemu-iotests/017 |  8 
 tests/qemu-iotests/018 |  8 
 tests/qemu-iotests/019 |  8 
 tests/qemu-iotests/020 |  8 
 tests/qemu-iotests/021 |  6 +++---
 tests/qemu-iotests/022 |  8 
 tests/qemu-iotests/023 |  8 
 tests/qemu-iotests/024 |  8 
 tests/qemu-iotests/025 |  8 
 tests/qemu-iotests/026 |  8 
 tests/qemu-iotests/027 |  6 +++---
 tests/qemu-iotests/028 | 10 +-
 tests/qemu-iotests/029 |  8 
 tests/qemu-iotests/031 |  8 
 tests/qemu-iotests/032 |  8 
 tests/qemu-iotests/033 |  6 +++---
 tests/qemu-iotests/034 |  6 +++---
 tests/qemu-iotests/035 |  6 +++---
 tests/qemu-iotests/036 |  8 
 tests/qemu-iotests/037 |  6 +++---
 tests/qemu-iotests/038 |  6 +++---
 tests/qemu-iotests/039 |  6 +++---
 tests/qemu-iotests/042 |  6 +++---
 tests/qemu-iotests/043 |  6 +++---
 tests/qemu-iotests/046 |  6 +++---
 tests/qemu-iotests/047 |  6 +++---
 tests/qemu-iotests/048 |  8 
 tests/qemu-iotests/049 |  6 +++---
 tests/qemu-iotests/050 |  6 +++---
 tests/qemu-iotests/051 |  6 +++---
 tests/qemu-iotests/052 |  6 +++---
 tests/qemu-iotests/053 |  6 +++---
 tests/qemu-iotests/054 |  6 +++---
 tests/qemu-iotests/058 |  8 
 tests/qemu-iotests/059 |  6 +++---
 tests/qemu-iotests/060 |  6 +++---
 tests/qemu-iotests/061 |  6 +++---
 tests/qemu-iotests/062 |  6 +++---
 tests/qemu-iotests/063 |  8 
 tests/qemu-iotests/064 |  6 +++---
 tests/qemu-iotests/066 |  6 +++---
 tests/qemu-iotests/068 |  6 +++---
 tests/qemu-iotests/069 |  6 +++---
 tests/qemu-iotests/070 |  6 +++---
 tests/qemu-iotests/071 |  6 +++---
 tests/qemu-iotests/072 |  6 +++---
 tests/qemu-iotests/073 |  6 +++---
 tests/qemu-iotests/074 |  8 
 tests/qemu-iotests/075 |  6 +++---
 tests/qemu-iotests/076 |  6 +++---
 tests/qemu-iotests/077 |  6 +++---
 tests/qemu-iotests/078 |  6 +++---
 tests/qemu-iotests/079 |  6 +++---
 tests/qemu-iotests/080 |  6 +++---
 tests/qemu-iotests/082 |  6 +++---
 tests/qemu-iotests/084 |  6 +++---
 tests/qemu-iotests/085 |  8 
 tests/qemu-iotests/086 |  6 +++---
 tests/qemu-iotests/088 |  6 +++---
 tests/qemu-iotests/089 |  6 +++---
 tests/qemu-iotests/090 |  6 +++---
 tests/qemu-iotests/091 |  8 
 tests/qemu-iotests/092 |  6 +++---
 tests/qemu-iotests/094 |  9 -
 tests/qemu-iotests/095 |  8 
 tests/qemu-iotests/097 |  8 
 tests/qemu-iotests/098 |  8 
 tests/qemu-iotests/099 |  6 +++---
 tests/qemu-iotests/101 |  6 +++---
 tests/qemu-iotests/102 |  8 
 tests/qemu-iotests/103 |  6 +++---
 tests/qemu-iotests/105 |  6 +++---
 tests/qemu-iotests/106 |  6 +++---
 tests/qemu-iotests/107 |  6 +++---
 tests/qemu-iotests/108 |  6 +++---
 tests/qemu-iotests/109 |  8 
 tests/qemu-iotests/110 |  6 +++---
 tests/qemu-iotests/111 |  6 +++---
 tests/qemu-iotests/112 |  6 +++---
 tests/qemu-iotests/113 |  6 +++---
 tests/qemu-iotests/114 |  6 +++---
 tests/qemu-iotests/115 |  6 +++---
 tests/qemu-iotests/116 |  6 +++---
 tests/qemu-iotests/117 | 10 +-
 tests/qemu-iotests/119 |  6 +++---
 tests/qemu-iotests/120 |  6 +++---
 tests/qemu-iotests/121 |  6 +++---
 tests/qemu-iotests/122 |  6 +++---
 tests/qemu-iotests/123 |  6 +++---
 tests/qemu-iotests/125 |  6 +++---
 tests/qemu-iotests/127 |  8 
 tests/qemu-iotests/130 |  8 
 tests/qemu-iotests/131 |  6 +++---
 tests/qemu-iotests/133 |  6 +++---
 tests/qemu-iotests/134 |  8 
 tests/qemu-iotests/135 |  6 +++---
 tests/qemu-iotests/137 |  8 
 tests/qemu-iotests/138 |  6 +++---
 tests/qemu-iotests/140 |  8 
 tests/qemu-iotests/141 |  8 
 tests/qemu-iotests/142 |  6 +++---
 tests/qemu-iotests/143 |  8 
 tests/qemu-iotests/144 |  8 
 tests/qemu-iotests/145 |  6 +++---
 tests/qemu-iotests/146 |  8 
 tests/qemu-iotests/150 |  6 +++---
 tests/qemu-iotests/153 |  8 
 tests/qemu-iotests/154 |  6 +++---
 tests/qemu-iotests/156 |  8 
 tests/qemu-iotests/157 |  6 +++---
 tests/qemu-iotests/158 |  6 +++---

[Qemu-block] [PATCH 09/10] qemu-iotests: remove unused "here" variable

[Cleber Rosa]

Another legacy variable that did not convince me it has any
purpose whatsoever.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/001 | 1 -
 tests/qemu-iotests/002 | 1 -
 tests/qemu-iotests/003 | 1 -
 tests/qemu-iotests/004 | 1 -
 tests/qemu-iotests/005 | 1 -
 tests/qemu-iotests/007 | 1 -
 tests/qemu-iotests/008 | 1 -
 tests/qemu-iotests/009 | 1 -
 tests/qemu-iotests/010 | 1 -
 tests/qemu-iotests/011 | 1 -
 tests/qemu-iotests/012 | 1 -
 tests/qemu-iotests/013 | 1 -
 tests/qemu-iotests/014 | 1 -
 tests/qemu-iotests/015 | 1 -
 tests/qemu-iotests/017 | 1 -
 tests/qemu-iotests/018 | 1 -
 tests/qemu-iotests/019 | 1 -
 tests/qemu-iotests/020 | 1 -
 tests/qemu-iotests/021 | 1 -
 tests/qemu-iotests/022 | 1 -
 tests/qemu-iotests/023 | 1 -
 tests/qemu-iotests/024 | 1 -
 tests/qemu-iotests/025 | 1 -
 tests/qemu-iotests/026 | 1 -
 tests/qemu-iotests/027 | 1 -
 tests/qemu-iotests/028 | 1 -
 tests/qemu-iotests/029 | 1 -
 tests/qemu-iotests/031 | 1 -
 tests/qemu-iotests/032 | 1 -
 tests/qemu-iotests/033 | 1 -
 tests/qemu-iotests/034 | 1 -
 tests/qemu-iotests/035 | 1 -
 tests/qemu-iotests/036 | 1 -
 tests/qemu-iotests/037 | 1 -
 tests/qemu-iotests/038 | 1 -
 tests/qemu-iotests/039 | 1 -
 tests/qemu-iotests/042 | 1 -
 tests/qemu-iotests/043 | 1 -
 tests/qemu-iotests/046 | 1 -
 tests/qemu-iotests/047 | 1 -
 tests/qemu-iotests/049 | 1 -
 tests/qemu-iotests/050 | 1 -
 tests/qemu-iotests/051 | 1 -
 tests/qemu-iotests/052 | 1 -
 tests/qemu-iotests/053 | 1 -
 tests/qemu-iotests/054 | 1 -
 tests/qemu-iotests/058 | 1 -
 tests/qemu-iotests/059 | 1 -
 tests/qemu-iotests/060 | 1 -
 tests/qemu-iotests/061 | 1 -
 tests/qemu-iotests/062 | 1 -
 tests/qemu-iotests/063 | 1 -
 tests/qemu-iotests/064 | 1 -
 tests/qemu-iotests/066 | 1 -
 tests/qemu-iotests/067 | 1 -
 tests/qemu-iotests/068 | 1 -
 tests/qemu-iotests/069 | 1 -
 tests/qemu-iotests/070 | 1 -
 tests/qemu-iotests/071 | 1 -
 tests/qemu-iotests/072 | 1 -
 tests/qemu-iotests/073 | 1 -
 tests/qemu-iotests/075 | 1 -
 tests/qemu-iotests/076 | 1 -
 tests/qemu-iotests/077 | 1 -
 tests/qemu-iotests/078 | 1 -
 tests/qemu-iotests/079 | 1 -
 tests/qemu-iotests/080 | 1 -
 tests/qemu-iotests/081 | 1 -
 tests/qemu-iotests/082 | 1 -
 tests/qemu-iotests/083 | 1 -
 tests/qemu-iotests/084 | 1 -
 tests/qemu-iotests/085 | 1 -
 tests/qemu-iotests/086 | 1 -
 tests/qemu-iotests/087 | 1 -
 tests/qemu-iotests/088 | 1 -
 tests/qemu-iotests/089 | 1 -
 tests/qemu-iotests/090 | 1 -
 tests/qemu-iotests/091 | 1 -
 tests/qemu-iotests/092 | 1 -
 tests/qemu-iotests/094 | 1 -
 tests/qemu-iotests/095 | 1 -
 tests/qemu-iotests/097 | 1 -
 tests/qemu-iotests/098 | 1 -
 tests/qemu-iotests/099 | 1 -
 tests/qemu-iotests/101 | 1 -
 tests/qemu-iotests/102 | 1 -
 tests/qemu-iotests/103 | 1 -
 tests/qemu-iotests/104 | 1 -
 tests/qemu-iotests/105 | 1 -
 tests/qemu-iotests/106 | 1 -
 tests/qemu-iotests/107 | 1 -
 tests/qemu-iotests/108 | 1 -
 tests/qemu-iotests/109 | 1 -
 tests/qemu-iotests/110 | 1 -
 tests/qemu-iotests/111 | 1 -
 tests/qemu-iotests/112 | 1 -
 tests/qemu-iotests/113 | 1 -
 tests/qemu-iotests/114 | 1 -
 tests/qemu-iotests/115 | 1 -
 tests/qemu-iotests/116 | 1 -
 tests/qemu-iotests/117 | 1 -
 tests/qemu-iotests/119 | 1 -
 tests/qemu-iotests/120 | 1 -
 tests/qemu-iotests/121 | 1 -
 tests/qemu-iotests/122 | 1 -
 tests/qemu-iotests/123 | 1 -
 tests/qemu-iotests/125 | 1 -
 tests/qemu-iotests/126 | 1 -
 tests/qemu-iotests/127 | 1 -
 tests/qemu-iotests/128 | 1 -
 tests/qemu-iotests/130 | 1 -
 tests/qemu-iotests/131 | 1 -
 tests/qemu-iotests/133 | 1 -
 tests/qemu-iotests/134 | 1 -
 tests/qemu-iotests/135 | 1 -
 tests/qemu-iotests/137 | 1 -
 tests/qemu-iotests/138 | 1 -
 tests/qemu-iotests/140 | 1 -
 tests/qemu-iotests/141 | 1 -
 tests/qemu-iotests/142 | 1 -
 tests/qemu-iotests/143 | 1 -
 tests/qemu-iotests/144 | 1 -
 tests/qemu-iotests/145 | 1 -
 tests/qemu-iotests/146 | 1 -
 tests/qemu-iotests/150 | 1 -
 tests/qemu-iotests/153 | 1 -
 tests/qemu-iotests/154 | 1 -
 tests/qemu-iotests/156 | 1 -
 tests/qemu-iotests/157 | 1 -
 tests/qemu-iotests/158 | 1 -
 tests/qemu-iotests/159 | 1 -
 tests/qemu-iotests/160 | 1 -
 tests/qemu-iotests/162 | 1 -
 tests/qemu-iotests/170 | 1 -
 tests/qemu-iotests/171 | 1 -
 tests/qemu-iotests/172 | 1 -
 tests/qemu-iotests/173 | 1 -
 tests/qemu-iotests/174 | 1 -
 tests/qemu-iotests/175 | 1 -
 tests/qemu-iotests/176 | 1 -
 tests/qemu-iotests/177 | 1 -
 tests/qemu-iotests/178 | 1 -
 tests/qemu-iotests/179 | 1 -
 tests/qemu-iotests/181 | 1 -
 tests/qemu-iotests/182 | 1 -
 tests/qemu-iotests/183 | 1 -
 tests/qemu-iotests/184 | 1 -
 tests/qemu-iotests/185 | 1 -
 tests/qemu-iotests/186 | 1 -
 tests/qemu-iotests/187 | 1 -
 tests/qemu-iotests/188 | 1 -
 tests/qemu-iotests/189 | 1 -
 tests/qemu-iotests/190 | 1 -
 tests/qemu-iotests/191 | 1 -
 tests/qemu-iotests/192 | 1 -
 tests/qemu-iotests/195 | 1 -
 tests/qemu-iotests/197 | 1 -
 157 files changed, 157 deletions(-)

diff --git a/tests/qemu-iotests/001 b/tests/qemu-iotests/001
index 5830f68ba0..ce671e50e7 100755
--- 

[Qemu-block] [PATCH 10/10] qemu-iotests: add section on how to write a new I/O test

[Cleber Rosa]

This adds some basic information on how to write a new test.  I'm
aware that some of the information in the wiki (Testing/QemuIoTests)
could also belong here.

Since copying content over won't generate much interesting feedback,
the goal here is to get feedback on the sample_test_templates, general
workflow proposed for writing a new test, etc.

After feedback is received, I can go over and sync both sides (wiki
and README).

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/README  | 44 ++--
 tests/qemu-iotests/sample_test_templates/sample.py | 59 ++
 tests/qemu-iotests/sample_test_templates/sample.sh | 40 +++
 3 files changed, 138 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/sample_test_templates/sample.py
 create mode 100755 tests/qemu-iotests/sample_test_templates/sample.sh

diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
index 6079b401ae..efaf72d5e7 100644
--- a/tests/qemu-iotests/README
+++ b/tests/qemu-iotests/README
@@ -1,20 +1,54 @@
+= This is the QEMU I/O test suite =
 
-=== This is the QEMU I/O test suite ===
-
-* Intro
+== Intro ==
 
 This package contains a simple test suite for the I/O layer of qemu.
 It does not require a guest, but only the qemu, qemu-img and qemu-io
 binaries.  This does limit it to exercise the low-level I/O path only
 but no actual block drivers like ide, scsi or virtio.
 
-* Usage
+== Usage ==
 
 Just run ./check to run all tests for the raw image format, or ./check
 -qcow2 to test the qcow2 image format.  The output of ./check -h explains
 additional options to test further image formats or I/O methods.
 
-* Feedback and patches
+== Writing a QEMU I/O test ==
+
+It's a common practice to start with an existing test that may relate
+to the task you have.  QEMU I/O tests are usually written either in
+shell or Python, so it's also a good idea to pick an existing test
+based on your familiarity with those languages and/or what you
+anticipate about your test.
+
+You can find templates available at "sample_test_templates/", or you
+can start with an existing test, such as 001 (shell based) or 030
+(Python based).
+
+After you pick your template, name it as a three-digits file, starting
+with the next available one.  If `ls -1 ??? | sort -rn | head -1` gives
+you "197", name your test "198".  Finally, add an entry to the "group"
+file, which manages just that, test group classification, allowing a
+user to run all "quick" tests, for instance.
+
+=== Test configuration, expected results and reporting ===
+
+The tests are (mostly) standard executable files, that are executed by
+"./check" as such.  Tests get most of their configuration (directly or
+indirectly) by environment variables.  Some of the framework auxiliary
+code (for instance, "common.rc") defines or changes some of the
+environment variables.  You'll come across tests that reference
+"$TEST_IMG", "$QEMU_IO" and other variables.
+
+The expected results for a test are stored in a file named after the
+(sequential) test number.  For test 001, the expected output (stdout +
+stderr) is stored at 001.out.
+
+Tests that finish successfully should exit with status code 0.  To be
+considered a successful run, a test must also produce output that
+matches what is recorded in the expected output file (say, 001.out).
+
+== Feedback and patches ==
 
 Please send improvements to the test suite, general feedback or just
 reports of failing tests cases to qemu-de...@nongnu.org with a CC:
diff --git a/tests/qemu-iotests/sample_test_templates/sample.py 
b/tests/qemu-iotests/sample_test_templates/sample.py
new file mode 100755
index 00..35a7410b5c
--- /dev/null
+++ b/tests/qemu-iotests/sample_test_templates/sample.py
@@ -0,0 +1,59 @@
+#!/usr/bin/env python
+#
+# DESCRIPTION HERE, such as "Test a complex thing using a simple thing"
+#
+# COPYRIGHT NOTICE HERE, such as "Copyright (C)  Yourself, Inc.
+#
+# 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 .
+#
+# Creator/Owner: your.em...@example.com
+#
+
+import iotests
+
+
+class Sample(iotests.QMPTestCase):
+
+def setUp(self):
+"""
+Method that runs before test method(s).  Use it to prepare the
+environment that is common to the all tests, but doesn't
+necessarily mean that is part of the feature testing
+specifics.

[Qemu-block] [PATCH 06/10] qemu-iotests: turn owner variable into a comment

[Cleber Rosa]

This variables has no real use.  To avoid pretending it does, while
still keeping the information, let's turn it into a comment.

The format chosen is the one already being used on tests 149 and 194.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/001 | 5 ++---
 tests/qemu-iotests/002 | 5 ++---
 tests/qemu-iotests/003 | 5 ++---
 tests/qemu-iotests/004 | 5 ++---
 tests/qemu-iotests/005 | 5 ++---
 tests/qemu-iotests/007 | 5 ++---
 tests/qemu-iotests/008 | 5 ++---
 tests/qemu-iotests/009 | 5 ++---
 tests/qemu-iotests/010 | 5 ++---
 tests/qemu-iotests/011 | 5 ++---
 tests/qemu-iotests/012 | 5 ++---
 tests/qemu-iotests/013 | 5 ++---
 tests/qemu-iotests/014 | 5 ++---
 tests/qemu-iotests/015 | 5 ++---
 tests/qemu-iotests/017 | 5 ++---
 tests/qemu-iotests/018 | 5 ++---
 tests/qemu-iotests/019 | 5 ++---
 tests/qemu-iotests/020 | 5 ++---
 tests/qemu-iotests/021 | 5 ++---
 tests/qemu-iotests/022 | 5 ++---
 tests/qemu-iotests/023 | 5 ++---
 tests/qemu-iotests/024 | 5 ++---
 tests/qemu-iotests/025 | 5 ++---
 tests/qemu-iotests/026 | 5 ++---
 tests/qemu-iotests/027 | 5 ++---
 tests/qemu-iotests/028 | 5 ++---
 tests/qemu-iotests/029 | 5 ++---
 tests/qemu-iotests/031 | 5 ++---
 tests/qemu-iotests/032 | 5 ++---
 tests/qemu-iotests/033 | 5 ++---
 tests/qemu-iotests/034 | 5 ++---
 tests/qemu-iotests/035 | 5 ++---
 tests/qemu-iotests/036 | 5 ++---
 tests/qemu-iotests/037 | 5 ++---
 tests/qemu-iotests/038 | 5 ++---
 tests/qemu-iotests/039 | 5 ++---
 tests/qemu-iotests/040 | 1 +
 tests/qemu-iotests/042 | 5 ++---
 tests/qemu-iotests/043 | 5 ++---
 tests/qemu-iotests/046 | 5 ++---
 tests/qemu-iotests/047 | 5 ++---
 tests/qemu-iotests/048 | 4 ++--
 tests/qemu-iotests/049 | 5 ++---
 tests/qemu-iotests/050 | 5 ++---
 tests/qemu-iotests/051 | 5 ++---
 tests/qemu-iotests/052 | 5 ++---
 tests/qemu-iotests/053 | 5 ++---
 tests/qemu-iotests/054 | 5 ++---
 tests/qemu-iotests/058 | 5 ++---
 tests/qemu-iotests/059 | 5 ++---
 tests/qemu-iotests/060 | 5 ++---
 tests/qemu-iotests/061 | 5 ++---
 tests/qemu-iotests/062 | 5 ++---
 tests/qemu-iotests/063 | 5 ++---
 tests/qemu-iotests/064 | 5 ++---
 tests/qemu-iotests/066 | 5 ++---
 tests/qemu-iotests/067 | 5 ++---
 tests/qemu-iotests/068 | 5 ++---
 tests/qemu-iotests/069 | 5 ++---
 tests/qemu-iotests/070 | 5 ++---
 tests/qemu-iotests/071 | 5 ++---
 tests/qemu-iotests/072 | 5 ++---
 tests/qemu-iotests/073 | 5 ++---
 tests/qemu-iotests/074 | 3 +--
 tests/qemu-iotests/075 | 5 ++---
 tests/qemu-iotests/076 | 5 ++---
 tests/qemu-iotests/077 | 5 ++---
 tests/qemu-iotests/078 | 5 ++---
 tests/qemu-iotests/079 | 5 ++---
 tests/qemu-iotests/080 | 5 ++---
 tests/qemu-iotests/081 | 5 ++---
 tests/qemu-iotests/082 | 5 ++---
 tests/qemu-iotests/083 | 5 ++---
 tests/qemu-iotests/084 | 5 ++---
 tests/qemu-iotests/085 | 5 ++---
 tests/qemu-iotests/086 | 5 ++---
 tests/qemu-iotests/087 | 5 ++---
 tests/qemu-iotests/088 | 5 ++---
 tests/qemu-iotests/089 | 5 ++---
 tests/qemu-iotests/090 | 5 ++---
 tests/qemu-iotests/091 | 5 ++---
 tests/qemu-iotests/092 | 5 ++---
 tests/qemu-iotests/094 | 5 ++---
 tests/qemu-iotests/095 | 4 ++--
 tests/qemu-iotests/097 | 5 ++---
 tests/qemu-iotests/098 | 5 ++---
 tests/qemu-iotests/099 | 5 ++---
 tests/qemu-iotests/101 | 5 ++---
 tests/qemu-iotests/102 | 5 ++---
 tests/qemu-iotests/103 | 5 ++---
 tests/qemu-iotests/104 | 5 ++---
 tests/qemu-iotests/105 | 5 ++---
 tests/qemu-iotests/106 | 5 ++---
 tests/qemu-iotests/107 | 5 ++---
 tests/qemu-iotests/108 | 5 ++---
 tests/qemu-iotests/109 | 5 ++---
 tests/qemu-iotests/110 | 5 ++---
 tests/qemu-iotests/111 | 5 ++---
 tests/qemu-iotests/112 | 5 ++---
 tests/qemu-iotests/113 | 5 ++---
 tests/qemu-iotests/114 | 5 ++---
 tests/qemu-iotests/115 | 5 ++---
 tests/qemu-iotests/116 | 5 ++---
 tests/qemu-iotests/117 | 5 ++---
 tests/qemu-iotests/119 | 5 ++---
 tests/qemu-iotests/120 | 5 ++---
 tests/qemu-iotests/121 | 5 ++---
 tests/qemu-iotests/122 | 5 ++---
 tests/qemu-iotests/123 | 5 ++---
 tests/qemu-iotests/125 | 5 ++---
 tests/qemu-iotests/126 | 5 ++---
 tests/qemu-iotests/127 | 5 ++---
 tests/qemu-iotests/128 | 5 ++---
 tests/qemu-iotests/130 | 5 ++---
 tests/qemu-iotests/131 | 5 ++---
 tests/qemu-iotests/133 | 5 ++---
 tests/qemu-iotests/134 | 5 ++---
 tests/qemu-iotests/135 | 5 ++---
 tests/qemu-iotests/137 | 5 ++---
 tests/qemu-iotests/138 | 5 ++---
 tests/qemu-iotests/140 | 5 ++---
 tests/qemu-iotests/141 | 5 ++---
 tests/qemu-iotests/142 | 5 ++---
 tests/qemu-iotests/143 | 5 ++---
 tests/qemu-iotests/144 | 5 ++---
 tests/qemu-iotests/145 | 5 ++---
 tests/qemu-iotests/146 | 5 ++---
 tests/qemu-iotests/150 | 5 ++---
 tests/qemu-iotests/153 | 5 ++---
 tests/qemu-iotests/154 | 5 ++---
 tests/qemu-iotests/156 | 5 ++---
 tests/qemu-iotests/157 | 5 ++---
 tests/qemu-iotests/158 | 5 ++---
 tests/qemu-iotests/159 | 4 ++--
 tests/qemu-iotests/160 | 4 ++--
 tests/qemu-iotests/162 | 5 ++---
 tests/qemu-iotests/170 | 4 ++--
 tests/qemu-iotests/171 | 5 ++---
 tests/qemu-iotests/172 | 5 ++---
 tests/qemu-iotests/173 | 4 ++--

[Qemu-block] [PATCH 07/10] qemu-iotests: remove the concept of $seq.full (and boiler plate code)

[Cleber Rosa]

The $seq.full file, in theory, should contain the full output of a
test error.  In practice, it's only used on a single test, and the
boiler plate code to clean it up plagues all other tests.

Let's remove the concept altogether, and record the failure in the
output itself for the one test using this function.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/001 | 1 -
 tests/qemu-iotests/002 | 1 -
 tests/qemu-iotests/003 | 1 -
 tests/qemu-iotests/004 | 1 -
 tests/qemu-iotests/005 | 1 -
 tests/qemu-iotests/007 | 1 -
 tests/qemu-iotests/008 | 1 -
 tests/qemu-iotests/009 | 1 -
 tests/qemu-iotests/010 | 1 -
 tests/qemu-iotests/011 | 1 -
 tests/qemu-iotests/012 | 1 -
 tests/qemu-iotests/013 | 1 -
 tests/qemu-iotests/014 | 1 -
 tests/qemu-iotests/015 | 1 -
 tests/qemu-iotests/017 | 1 -
 tests/qemu-iotests/018 | 1 -
 tests/qemu-iotests/019 | 1 -
 tests/qemu-iotests/020 | 1 -
 tests/qemu-iotests/021 | 1 -
 tests/qemu-iotests/022 | 1 -
 tests/qemu-iotests/023 | 1 -
 tests/qemu-iotests/024 | 1 -
 tests/qemu-iotests/025 | 1 -
 tests/qemu-iotests/026 | 1 -
 tests/qemu-iotests/027 | 1 -
 tests/qemu-iotests/028 | 1 -
 tests/qemu-iotests/029 | 1 -
 tests/qemu-iotests/031 | 1 -
 tests/qemu-iotests/032 | 1 -
 tests/qemu-iotests/033 | 1 -
 tests/qemu-iotests/034 | 1 -
 tests/qemu-iotests/035 | 1 -
 tests/qemu-iotests/036 | 1 -
 tests/qemu-iotests/037 | 1 -
 tests/qemu-iotests/038 | 1 -
 tests/qemu-iotests/039 | 1 -
 tests/qemu-iotests/042 | 1 -
 tests/qemu-iotests/043 | 1 -
 tests/qemu-iotests/046 | 1 -
 tests/qemu-iotests/047 | 1 -
 tests/qemu-iotests/049 | 1 -
 tests/qemu-iotests/050 | 1 -
 tests/qemu-iotests/051 | 1 -
 tests/qemu-iotests/052 | 1 -
 tests/qemu-iotests/053 | 1 -
 tests/qemu-iotests/054 | 1 -
 tests/qemu-iotests/058 | 1 -
 tests/qemu-iotests/059 | 1 -
 tests/qemu-iotests/060 | 1 -
 tests/qemu-iotests/061 | 1 -
 tests/qemu-iotests/062 | 1 -
 tests/qemu-iotests/063 | 1 -
 tests/qemu-iotests/064 | 1 -
 tests/qemu-iotests/066 | 1 -
 tests/qemu-iotests/067 | 1 -
 tests/qemu-iotests/068 | 1 -
 tests/qemu-iotests/069 | 1 -
 tests/qemu-iotests/070 | 1 -
 tests/qemu-iotests/071 | 1 -
 tests/qemu-iotests/072 | 1 -
 tests/qemu-iotests/073 | 1 -
 tests/qemu-iotests/075 | 1 -
 tests/qemu-iotests/076 | 1 -
 tests/qemu-iotests/077 | 1 -
 tests/qemu-iotests/078 | 1 -
 tests/qemu-iotests/079 | 1 -
 tests/qemu-iotests/080 | 1 -
 tests/qemu-iotests/081 | 1 -
 tests/qemu-iotests/082 | 1 -
 tests/qemu-iotests/083 | 1 -
 tests/qemu-iotests/084 | 1 -
 tests/qemu-iotests/085 | 1 -
 tests/qemu-iotests/086 | 1 -
 tests/qemu-iotests/087 | 1 -
 tests/qemu-iotests/088 | 1 -
 tests/qemu-iotests/089 | 1 -
 tests/qemu-iotests/090 | 1 -
 tests/qemu-iotests/091 | 1 -
 tests/qemu-iotests/092 | 1 -
 tests/qemu-iotests/094 | 1 -
 tests/qemu-iotests/095 | 1 -
 tests/qemu-iotests/097 | 1 -
 tests/qemu-iotests/098 | 1 -
 tests/qemu-iotests/099 | 1 -
 tests/qemu-iotests/101 | 1 -
 tests/qemu-iotests/102 | 1 -
 tests/qemu-iotests/103 | 1 -
 tests/qemu-iotests/104 | 1 -
 tests/qemu-iotests/105 | 1 -
 tests/qemu-iotests/106 | 1 -
 tests/qemu-iotests/107 | 1 -
 tests/qemu-iotests/108 | 1 -
 tests/qemu-iotests/109 | 1 -
 tests/qemu-iotests/110 | 1 -
 tests/qemu-iotests/111 | 1 -
 tests/qemu-iotests/112 | 1 -
 tests/qemu-iotests/113 | 1 -
 tests/qemu-iotests/114 | 1 -
 tests/qemu-iotests/115 | 1 -
 tests/qemu-iotests/116 | 1 -
 tests/qemu-iotests/117 | 1 -
 tests/qemu-iotests/119 | 1 -
 tests/qemu-iotests/120 | 1 -
 tests/qemu-iotests/121 | 1 -
 tests/qemu-iotests/122 | 1 -
 tests/qemu-iotests/123 | 1 -
 tests/qemu-iotests/125 | 1 -
 tests/qemu-iotests/126 | 1 -
 tests/qemu-iotests/127 | 1 -
 tests/qemu-iotests/128 | 1 -
 tests/qemu-iotests/130 | 1 -
 tests/qemu-iotests/131 | 1 -
 tests/qemu-iotests/133 | 1 -
 tests/qemu-iotests/134 | 1 -
 tests/qemu-iotests/135 | 1 -
 tests/qemu-iotests/137 | 1 -
 tests/qemu-iotests/138 | 1 -
 tests/qemu-iotests/140 | 1 -
 tests/qemu-iotests/141 | 1 -
 tests/qemu-iotests/142 | 1 -
 tests/qemu-iotests/143 | 1 -
 tests/qemu-iotests/144 | 1 -
 tests/qemu-iotests/145 | 1 -
 tests/qemu-iotests/146 | 1 -
 tests/qemu-iotests/150 | 1 -
 tests/qemu-iotests/153 | 1 -
 tests/qemu-iotests/154 | 1 -
 tests/qemu-iotests/156 | 1 -
 tests/qemu-iotests/157 | 1 -
 tests/qemu-iotests/158 | 1 -
 tests/qemu-iotests/162 | 1 -
 tests/qemu-iotests/171 | 1 -
 tests/qemu-iotests/172 | 1 -
 tests/qemu-iotests/173 | 1 -
 tests/qemu-iotests/174 | 9 +++--
 tests/qemu-iotests/175 | 1 -
 tests/qemu-iotests/176 | 1 -
 tests/qemu-iotests/178 | 1 -
 tests/qemu-iotests/181 | 1 -
 tests/qemu-iotests/182 | 1 -
 tests/qemu-iotests/183 | 1 -
 tests/qemu-iotests/184 | 1 -
 tests/qemu-iotests/185 | 1 -
 tests/qemu-iotests/186 | 1 -
 tests/qemu-iotests/187 | 1 -
 tests/qemu-iotests/188 | 1 -
 tests/qemu-iotests/189 | 1 -
 tests/qemu-iotests/190 | 1 -
 tests/qemu-iotests/191 | 1 -
 tests/qemu-iotests/192 | 1 -
 tests/qemu-iotests/195 | 1 -
 151 files changed, 7 insertions(+), 152 deletions(-)

diff --git a/tests/qemu-iotests/001 

[Qemu-block] [PATCH 04/10] qemu-iotests: include (source) filters from common.rc

[Cleber Rosa]

There's an explicit dependency from common.rc on common.filters, that
is, it consumes functions defined there.  Just like common.config is
included in common.rc, it makes sense to also sense common.filter.

This drops the requirement on individual tests to include
common.filter, reducing the amount of boiler plate code (while still
making the comment found on almost every test "get standard
environment, filters and checks" accurate.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/001   | 1 -
 tests/qemu-iotests/002   | 1 -
 tests/qemu-iotests/003   | 1 -
 tests/qemu-iotests/004   | 1 -
 tests/qemu-iotests/005   | 1 -
 tests/qemu-iotests/007   | 1 -
 tests/qemu-iotests/008   | 1 -
 tests/qemu-iotests/009   | 1 -
 tests/qemu-iotests/010   | 1 -
 tests/qemu-iotests/011   | 1 -
 tests/qemu-iotests/012   | 1 -
 tests/qemu-iotests/013   | 1 -
 tests/qemu-iotests/014   | 1 -
 tests/qemu-iotests/015   | 1 -
 tests/qemu-iotests/017   | 1 -
 tests/qemu-iotests/018   | 1 -
 tests/qemu-iotests/019   | 1 -
 tests/qemu-iotests/020   | 1 -
 tests/qemu-iotests/021   | 1 -
 tests/qemu-iotests/022   | 1 -
 tests/qemu-iotests/023   | 1 -
 tests/qemu-iotests/024   | 1 -
 tests/qemu-iotests/025   | 1 -
 tests/qemu-iotests/026   | 1 -
 tests/qemu-iotests/027   | 1 -
 tests/qemu-iotests/028   | 1 -
 tests/qemu-iotests/029   | 1 -
 tests/qemu-iotests/031   | 1 -
 tests/qemu-iotests/032   | 1 -
 tests/qemu-iotests/033   | 1 -
 tests/qemu-iotests/034   | 1 -
 tests/qemu-iotests/035   | 1 -
 tests/qemu-iotests/036   | 1 -
 tests/qemu-iotests/037   | 1 -
 tests/qemu-iotests/038   | 1 -
 tests/qemu-iotests/039   | 1 -
 tests/qemu-iotests/042   | 1 -
 tests/qemu-iotests/043   | 1 -
 tests/qemu-iotests/046   | 1 -
 tests/qemu-iotests/047   | 1 -
 tests/qemu-iotests/048   | 1 -
 tests/qemu-iotests/049   | 1 -
 tests/qemu-iotests/050   | 1 -
 tests/qemu-iotests/051   | 1 -
 tests/qemu-iotests/052   | 1 -
 tests/qemu-iotests/053   | 1 -
 tests/qemu-iotests/054   | 1 -
 tests/qemu-iotests/058   | 1 -
 tests/qemu-iotests/059   | 1 -
 tests/qemu-iotests/060   | 1 -
 tests/qemu-iotests/061   | 1 -
 tests/qemu-iotests/062   | 1 -
 tests/qemu-iotests/063   | 1 -
 tests/qemu-iotests/064   | 1 -
 tests/qemu-iotests/066   | 1 -
 tests/qemu-iotests/067   | 1 -
 tests/qemu-iotests/068   | 1 -
 tests/qemu-iotests/069   | 1 -
 tests/qemu-iotests/070   | 1 -
 tests/qemu-iotests/071   | 1 -
 tests/qemu-iotests/072   | 1 -
 tests/qemu-iotests/073   | 1 -
 tests/qemu-iotests/074   | 1 -
 tests/qemu-iotests/075   | 1 -
 tests/qemu-iotests/076   | 1 -
 tests/qemu-iotests/077   | 1 -
 tests/qemu-iotests/078   | 1 -
 tests/qemu-iotests/079   | 1 -
 tests/qemu-iotests/080   | 1 -
 tests/qemu-iotests/081   | 1 -
 tests/qemu-iotests/082   | 1 -
 tests/qemu-iotests/083   | 1 -
 tests/qemu-iotests/084   | 1 -
 tests/qemu-iotests/085   | 1 -
 tests/qemu-iotests/086   | 1 -
 tests/qemu-iotests/087   | 1 -
 tests/qemu-iotests/088   | 1 -
 tests/qemu-iotests/089   | 1 -
 tests/qemu-iotests/090   | 1 -
 tests/qemu-iotests/091   | 1 -
 tests/qemu-iotests/092   | 1 -
 tests/qemu-iotests/094   | 1 -
 tests/qemu-iotests/095   | 1 -
 tests/qemu-iotests/097   | 1 -
 tests/qemu-iotests/098   | 1 -
 tests/qemu-iotests/099   | 1 -
 tests/qemu-iotests/101   | 1 -
 tests/qemu-iotests/102   | 1 -
 tests/qemu-iotests/103   | 1 -
 tests/qemu-iotests/104   | 1 -
 tests/qemu-iotests/105   | 1 -
 tests/qemu-iotests/106   | 1 -
 tests/qemu-iotests/107   | 1 -
 tests/qemu-iotests/108   | 1 -
 tests/qemu-iotests/109   | 1 -
 tests/qemu-iotests/110   | 1 -
 tests/qemu-iotests/111   | 1 -
 tests/qemu-iotests/112   | 1 -
 tests/qemu-iotests/113   | 1 -
 tests/qemu-iotests/114   | 1 -
 tests/qemu-iotests/115   | 1 -
 tests/qemu-iotests/116   | 1 -
 tests/qemu-iotests/117   | 1 -
 tests/qemu-iotests/119   | 1 -
 tests/qemu-iotests/120   | 1 -
 tests/qemu-iotests/121   | 1 -
 tests/qemu-iotests/122   | 1 -
 tests/qemu-iotests/123   | 1 -
 tests/qemu-iotests/125   | 1 -
 tests/qemu-iotests/126   | 1 -
 tests/qemu-iotests/127   | 1 -
 tests/qemu-iotests/128   | 1 -
 tests/qemu-iotests/130   | 1 -
 tests/qemu-iotests/131   | 1 -
 tests/qemu-iotests/133   | 1 -
 tests/qemu-iotests/135   | 1 -
 tests/qemu-iotests/137   | 1 -
 tests/qemu-iotests/138   | 1 -
 tests/qemu-iotests/140   | 1 -
 tests/qemu-iotests/141   | 1 -
 tests/qemu-iotests/142   | 1 -
 tests/qemu-iotests/143   | 1 -
 tests/qemu-iotests/144   | 1 -
 tests/qemu-iotests/145   | 1 -
 tests/qemu-iotests/146   | 1 -
 

[Qemu-block] [PATCH 08/10] qemu-iotests: clean up double comment characters

[Cleber Rosa]

This is a syntactic only change, just to make it consistent with
the style used on all other tests.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/048 | 37 ++---
 tests/qemu-iotests/074 | 40 
 2 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index 531bed217c..3253d77c91 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -1,23 +1,22 @@
 #!/bin/bash
-##
-## qemu-img compare test
-##
-##
-## Copyright (C) 2013 Red Hat, Inc.
-##
-## 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 .
-##
+#
+# qemu-img compare test
+#
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# 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 .
 #
 # Creator/Owner: mreza...@redhat.com
 #
diff --git a/tests/qemu-iotests/074 b/tests/qemu-iotests/074
index d454580067..d09c909bd8 100755
--- a/tests/qemu-iotests/074
+++ b/tests/qemu-iotests/074
@@ -1,24 +1,24 @@
 #!/bin/bash
-##
-## qemu-img compare test (qcow2 only ones)
-##
-##
-## Copyright (C) 2013 Red Hat, Inc.
-##
-## 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 .
-##
-## Creator/Owner: f...@redhat.com
+#
+# qemu-img compare test (qcow2 only ones)
+#
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# 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 .
+#
+# Creator/Owner: f...@redhat.com
 #
 
 seq=`basename $0`
-- 
2.13.6

[Qemu-block] [PATCH 01/10] qemu-iotests: make execution of tests agnostic to test type

[Cleber Rosa]

check makes a distinction on how it runs Python based tests.  The
current approach is inconsistent because:

1) a large number of Python tests are already set as executable files
(eg: 030, 040, 041, 044, 045, 055, 056, 057, 065, 093, 118, 147, 149,
155, 165 and 194)

2) a smaller number of Python tests are not set as executable files

3) the true purpose of a shebang line is to make a file executable,
while it currently is used (inconsistently) as a test type flag

4) the same logic could in theory be applied to shell based tests,
that is, if first line contains a shell shebang, run it with
"$SHELL $test_file", but it'd be pointless

IMO, there's no value in the distinction that check makes.  Dropping
this distinction makes the interface simpler: check requires an
executable file.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/096   | 0
 tests/qemu-iotests/124   | 0
 tests/qemu-iotests/129   | 0
 tests/qemu-iotests/132   | 0
 tests/qemu-iotests/136   | 0
 tests/qemu-iotests/139   | 0
 tests/qemu-iotests/148   | 0
 tests/qemu-iotests/152   | 0
 tests/qemu-iotests/163   | 0
 tests/qemu-iotests/check | 9 ++---
 10 files changed, 2 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 tests/qemu-iotests/096
 mode change 100644 => 100755 tests/qemu-iotests/124
 mode change 100644 => 100755 tests/qemu-iotests/129
 mode change 100644 => 100755 tests/qemu-iotests/132
 mode change 100644 => 100755 tests/qemu-iotests/136
 mode change 100644 => 100755 tests/qemu-iotests/139
 mode change 100644 => 100755 tests/qemu-iotests/148
 mode change 100644 => 100755 tests/qemu-iotests/152
 mode change 100644 => 100755 tests/qemu-iotests/163

diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/152 b/tests/qemu-iotests/152
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
old mode 100644
new mode 100755
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e6b6ff7a04..08741328d0 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -741,20 +741,15 @@ do
 start=`_wallclock`
 $timestamp && printf %s "[$(date "+%T")]"
 
-if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env python" 
]; then
-run_command="$PYTHON $seq"
-else
-run_command="./$seq"
-fi
 export OUTPUT_DIR=$PWD
 if $debug; then
 (cd "$source_iotests";
 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
-$run_command -d 2>&1 | tee $tmp.out)
+./$seq -d 2>&1 | tee $tmp.out)
 else
 (cd "$source_iotests";
 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
-$run_command >$tmp.out 2>&1)
+./$seq >$tmp.out 2>&1)
 fi
 sts=$?
 $timestamp && _timestamp
-- 
2.13.6

[Qemu-block] [PATCH 03/10] qemu-iotests: be strict with expected output

[Cleber Rosa]

The contract between runner (check) and test is one that accepts some
minor differences in the expcted output, as the comparison method
employed is a "diff -w".

This is an exception to a clearer and more straightforward rule of
just requiring the generated output to be *equal* (and not similar) to
the recorded expected output.

Let's drop this exception and make things more predictable.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/026.out | 92 +++---
 tests/qemu-iotests/049.out |  4 +-
 tests/qemu-iotests/061.out |  4 +-
 tests/qemu-iotests/104.out |  2 +-
 tests/qemu-iotests/109.out | 44 +++---
 tests/qemu-iotests/175.out |  2 +-
 tests/qemu-iotests/check   |  4 +-
 7 files changed, 76 insertions(+), 76 deletions(-)

diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 86a50a2e13..38d76418b7 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -3,7 +3,7 @@ Errors while writing 128 kB
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l1_update; errno: 5; imm: off; once: on; write
+Event: l1_update; errno: 5; imm: off; once: on; write 
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -13,7 +13,7 @@ write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l1_update; errno: 5; imm: off; once: off; write
+Event: l1_update; errno: 5; imm: off; once: off; write 
 Failed to flush the L2 table cache: Input/output error
 Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
@@ -31,7 +31,7 @@ write failed: Input/output error
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l1_update; errno: 28; imm: off; once: on; write
+Event: l1_update; errno: 28; imm: off; once: on; write 
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -41,7 +41,7 @@ write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l1_update; errno: 28; imm: off; once: off; write
+Event: l1_update; errno: 28; imm: off; once: off; write 
 Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
@@ -59,7 +59,7 @@ write failed: No space left on device
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_load; errno: 5; imm: off; once: on; write
+Event: l2_load; errno: 5; imm: off; once: on; write 
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
@@ -75,7 +75,7 @@ read failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_load; errno: 5; imm: off; once: off; write
+Event: l2_load; errno: 5; imm: off; once: off; write 
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
@@ -91,7 +91,7 @@ read failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_load; errno: 28; imm: off; once: on; write
+Event: l2_load; errno: 28; imm: off; once: on; write 
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: No space left on device
@@ -107,7 +107,7 @@ read failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_load; errno: 28; imm: off; once: off; write
+Event: l2_load; errno: 28; imm: off; once: off; write 
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: No space left on device
@@ -123,7 +123,7 @@ read failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_update; errno: 5; imm: off; once: on; write
+Event: l2_update; errno: 5; imm: off; once: on; write 
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -133,7 +133,7 @@ write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
-Event: l2_update; errno: 5; imm: off; once: off; write
+Event: l2_update; errno: 5; imm: off; once: off; write 
 Failed to flush the L2 table cache: Input/output error
 Failed to flush the refcount block cache: Input/output error
 write failed: Input/output 

[Qemu-block] [PATCH 00/10] I/O tests cleanups

[Cleber Rosa]

Hi all,

This is a collection of cleanups, simplifications, and hopefully
improvements to the I/O tests.

Please don't mind the "change noise": this almost qualifies as a
collection of trivial patches.  It skips that category (of trivial
patches) because it proposes a few conceptual changes to how tests are
executed and verified.

Thanks!

---

Cleber Rosa (10):
  qemu-iotests: add section on how to write a new I/O test
  qemu-iotests: remove unused "here" variable
  qemu-iotests: clean up double comment characters
  qemu-iotests: remove the concept of $seq.full (and boiler plate code)
  qemu-iotests: turn owner variable into a comment
  qemu-iotests: define functions used in _cleanup() before its use
  qemu-iotests: include (source) filters from common.rc
  qemu-iotests: be strict with expected output
  qemu-iotests: fix filename containing checks
  qemu-iotests: make execution of tests agnostic to test type

 tests/qemu-iotests/001| 14 ++--
 tests/qemu-iotests/002| 14 ++--
 tests/qemu-iotests/003| 14 ++--
 tests/qemu-iotests/004| 14 ++--
 tests/qemu-iotests/005| 14 ++--
 tests/qemu-iotests/007| 14 ++--
 tests/qemu-iotests/008| 14 ++--
 tests/qemu-iotests/009| 14 ++--
 tests/qemu-iotests/010| 14 ++--
 tests/qemu-iotests/011| 14 ++--
 tests/qemu-iotests/012| 14 ++--
 tests/qemu-iotests/013| 16 ++--
 tests/qemu-iotests/014| 16 ++--
 tests/qemu-iotests/015| 14 ++--
 tests/qemu-iotests/017| 16 ++--
 tests/qemu-iotests/018| 16 ++--
 tests/qemu-iotests/019| 16 ++--
 tests/qemu-iotests/020| 16 ++--
 tests/qemu-iotests/021| 14 ++--
 tests/qemu-iotests/022| 16 ++--
 tests/qemu-iotests/023| 16 ++--
 tests/qemu-iotests/024| 16 ++--
 tests/qemu-iotests/025| 16 ++--
 tests/qemu-iotests/026| 16 ++--
 tests/qemu-iotests/026.out| 92 ++---
 tests/qemu-iotests/027| 14 ++--
 tests/qemu-iotests/028| 18 ++--
 tests/qemu-iotests/029| 16 ++--
 tests/qemu-iotests/031| 16 ++--
 tests/qemu-iotests/032| 16 ++--
 tests/qemu-iotests/033| 14 ++--
 tests/qemu-iotests/034| 14 ++--
 tests/qemu-iotests/035| 14 ++--
 tests/qemu-iotests/036| 16 ++--
 tests/qemu-iotests/037| 14 ++--
 tests/qemu-iotests/038| 14 ++--
 tests/qemu-iotests/039| 14 ++--
 tests/qemu-iotests/040|  1 +
 tests/qemu-iotests/042| 14 ++--
 tests/qemu-iotests/043| 14 ++--
 tests/qemu-iotests/046| 14 ++--
 tests/qemu-iotests/047| 14 ++--
 tests/qemu-iotests/048| 52 ++--
 tests/qemu-iotests/049| 14 ++--
 tests/qemu-iotests/049.out|  4 +-
 tests/qemu-iotests/050| 14 ++--
 tests/qemu-iotests/051| 14 ++--
 tests/qemu-iotests/052| 14 ++--
 tests/qemu-iotests/053| 14 ++--
 tests/qemu-iotests/054| 14 ++--
 tests/qemu-iotests/058| 16 ++--
 tests/qemu-iotests/059| 14 ++--
 tests/qemu-iotests/060| 14 ++--
 tests/qemu-iotests/061| 14 ++--
 tests/qemu-iotests/061.out|  4 +-
 tests/qemu-iotests/062| 14 ++--
 tests/qemu-iotests/063| 16 ++--
 tests/qemu-iotests/064| 14 ++--
 tests/qemu-iotests/066| 14 ++--
 tests/qemu-iotests/067|  8 +-
 tests/qemu-iotests/068| 14 ++--
 tests/qemu-iotests/069| 14 ++--
 tests/qemu-iotests/070| 14 ++--
 tests/qemu-iotests/071| 14 ++--
 tests/qemu-iotests/072| 14 ++--
 tests/qemu-iotests/073| 14 ++--
 tests/qemu-iotests/074  

[Qemu-block] [PATCH 02/10] qemu-iotests: fix filename containing checks

[Cleber Rosa]

Commit cce293a2945 moved some functions from common.config to
common.rc, but the error messages still reference the old file
location.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/common.rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index dbae7d74ba..dcabe4c7cc 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -169,12 +169,12 @@ if [ ! -e "$TEST_DIR" ]; then
 fi
 
 if [ ! -d "$TEST_DIR" ]; then
-echo "common.config: Error: \$TEST_DIR ($TEST_DIR) is not a directory"
+echo "common.rc: Error: \$TEST_DIR ($TEST_DIR) is not a directory"
 exit 1
 fi
 
 if [ ! -d "$SAMPLE_IMG_DIR" ]; then
-echo "common.config: Error: \$SAMPLE_IMG_DIR ($SAMPLE_IMG_DIR) is not a 
directory"
+echo "common.rc: Error: \$SAMPLE_IMG_DIR ($SAMPLE_IMG_DIR) is not a 
directory"
 exit 1
 fi
 
-- 
2.13.6

[Qemu-block] [PATCH v2 4/4] iotests: 030: add compressed block-stream test

[Anton Nefedov]

Signed-off-by: Anton Nefedov 
---
 tests/qemu-iotests/030 | 66 +-
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 457984b..831d6f3 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,7 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_img_pipe, qemu_io
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -804,5 +804,69 @@ class TestSetSpeed(iotests.QMPTestCase):
 
 self.cancel_and_wait(resume=True)
 
+class TestCompressed(iotests.QMPTestCase):
+supported_fmts = ['qcow2']
+cluster_size = 64 * 1024;
+image_len = 1 * 1024 * 1024;
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d' % 
TestCompressed.cluster_size, backing_img, str(TestCompressed.image_len))
+qemu_io('-c', 'write -P 1 0 ' + str(TestCompressed.image_len), 
backing_img)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 
'backing_file=%s,cluster_size=%d' % (backing_img, TestCompressed.cluster_size), 
test_img)
+
+# write '3' in every 3rd cluster
+step = 3
+for off in range(0, TestCompressed.image_len, 
TestCompressed.cluster_size * step):
+qemu_io('-c', 'write -P %d %d %d' %
+(step, off, TestCompressed.cluster_size), test_img)
+
+self.vm = iotests.VM().add_drive(test_img)
+self.vm.launch()
+
+def tearDown(self):
+os.remove(test_img)
+os.remove(backing_img)
+
+def _first_divider(self, x, divs):
+return divs[0] if not x%divs[0] else self._first_divider(x, divs[1:])
+
+def test_compressed(self):
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('block-stream', device='drive0', compress=True)
+if iotests.imgfmt not in TestCompressed.supported_fmts:
+self.assert_qmp(
+result, 'error/desc',
+'Compression is not supported for this drive drive0')
+return
+self.assert_qmp(result, 'return', {})
+
+# write '4' in every 4th cluster
+step = 4
+for off in range(0, TestCompressed.image_len, 
TestCompressed.cluster_size * step):
+result = self.vm.qmp('human-monitor-command',
+ command_line=
+ 'qemu-io drive0 "write -P %d %d %d"' %
+ (step, off, TestCompressed.cluster_size))
+self.assert_qmp(result, 'return', "")
+
+self.wait_until_completed()
+self.assert_no_active_block_jobs()
+
+self.vm.shutdown()
+
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size):
+outp = qemu_io('-c', 'read -P %d %d %d' %
+   (self._first_divider(i, [4, 3, 1]),
+i * TestCompressed.cluster_size,
+TestCompressed.cluster_size),
+   test_img)
+self.assertTrue(not 'fail' in outp)
+self.assertTrue('read' in outp and 'at offset' in outp)
+
+self.assertTrue(
+"File contains external, encrypted or compressed clusters."
+in qemu_img_pipe('map', test_img))
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 391c857..42314e9 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...
+
 --
-Ran 23 tests
+Ran 24 tests
 
 OK
-- 
2.7.4

[Qemu-block] [PATCH v2 1/4] qcow2: multiple clusters write compressed

[Anton Nefedov]

From: Pavel Butsykin 

At the moment, qcow2_co_pwritev_compressed can process the requests size
less than or equal to one cluster. This patch added possibility to write
compressed data in the QCOW2 more than one cluster. The implementation
is simple, we just split large requests into separate clusters and write
using existing functionality.

Signed-off-by: Anton Nefedov 
---
 block/qcow2.c | 73 ++-
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c276b24..f1e2759 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3332,11 +3332,9 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
 return 0;
 }
 
-/* XXX: put compressed sectors first, then all the cluster aligned
-   tables to avoid losing bytes in alignment */
 static coroutine_fn int
-qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
-uint64_t bytes, QEMUIOVector *qiov)
+qcow2_co_pwritev_cluster_compressed(BlockDriverState *bs, uint64_t offset,
+uint64_t bytes, QEMUIOVector *qiov)
 {
 BDRVQcow2State *s = bs->opaque;
 QEMUIOVector hd_qiov;
@@ -3346,25 +3344,12 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 uint8_t *buf, *out_buf;
 int64_t cluster_offset;
 
-if (bytes == 0) {
-/* align end of file to a sector boundary to ease reading with
-   sector based I/Os */
-cluster_offset = bdrv_getlength(bs->file->bs);
-if (cluster_offset < 0) {
-return cluster_offset;
-}
-return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
-}
-
-if (offset_into_cluster(s, offset)) {
-return -EINVAL;
-}
+assert(bytes <= s->cluster_size);
+assert(!offset_into_cluster(s, offset));
 
 buf = qemu_blockalign(bs, s->cluster_size);
-if (bytes != s->cluster_size) {
-if (bytes > s->cluster_size ||
-offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
-{
+if (bytes < s->cluster_size) {
+if (offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) {
 qemu_vfree(buf);
 return -EINVAL;
 }
@@ -3444,6 +3429,52 @@ fail:
 return ret;
 }
 
+/* XXX: put compressed sectors first, then all the cluster aligned
+   tables to avoid losing bytes in alignment */
+static coroutine_fn int
+qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
+uint64_t bytes, QEMUIOVector *qiov)
+{
+BDRVQcow2State *s = bs->opaque;
+QEMUIOVector hd_qiov;
+uint64_t curr_off = 0;
+int ret;
+
+if (bytes == 0) {
+/* align end of file to a sector boundary to ease reading with
+   sector based I/Os */
+int64_t cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
+return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
+}
+
+if (offset_into_cluster(s, offset)) {
+return -EINVAL;
+}
+
+qemu_iovec_init(_qiov, qiov->niov);
+do {
+uint32_t chunk_size;
+
+qemu_iovec_reset(_qiov);
+chunk_size = MIN(bytes, s->cluster_size);
+qemu_iovec_concat(_qiov, qiov, curr_off, chunk_size);
+
+ret = qcow2_co_pwritev_cluster_compressed(bs, offset + curr_off,
+  chunk_size, _qiov);
+if (ret < 0) {
+break;
+}
+curr_off += chunk_size;
+bytes -= chunk_size;
+} while (bytes);
+qemu_iovec_destroy(_qiov);
+
+return ret;
+}
+
 static int make_completely_empty(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
-- 
2.7.4

[Qemu-block] [PATCH v2 0/4] compressed block-stream

[Anton Nefedov]

v2:
  - 1st patch omitted (applied to 2.11)
  - remarks applied

v1: http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg02515.html

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.

Anton Nefedov (3):
  block: support compressed write for copy-on-read
  block-stream: add compress option
  iotests: 030: add compressed block-stream test

Pavel Butsykin (1):
  qcow2: multiple clusters write compressed

 qapi/block-core.json   |  4 +++
 include/block/block_int.h  |  4 ++-
 block/io.c | 23 +++
 block/qcow2.c  | 73 +-
 block/stream.c | 16 +++---
 blockdev.c | 13 -
 hmp.c  |  2 ++
 block/trace-events |  2 +-
 hmp-commands.hx|  4 +--
 tests/qemu-iotests/030 | 66 -
 tests/qemu-iotests/030.out |  4 +--
 11 files changed, 172 insertions(+), 39 deletions(-)

-- 
2.7.4

[Qemu-block] [PATCH v2 2/4] block: support compressed write for copy-on-read

[Anton Nefedov]

Signed-off-by: Anton Nefedov 
Reviewed-by: Max Reitz 
---
 block/io.c | 23 +--
 block/trace-events |  2 +-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3d5ef2c..2ba62a3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -953,7 +953,7 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 }
 
 static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
+int64_t offset, unsigned int bytes, QEMUIOVector *qiov, int flags)
 {
 BlockDriverState *bs = child->bs;
 
@@ -988,12 +988,13 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
  * allocating cluster in the image file.  Note that this value may exceed
  * BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
  * is one reason we loop rather than doing it all at once.
+ * Also this is crucial for compressed copy-on-read.
  */
 bdrv_round_to_clusters(bs, offset, bytes, _offset, _bytes);
 skip_bytes = offset - cluster_offset;
 
 trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
-   cluster_offset, cluster_bytes);
+   cluster_offset, cluster_bytes, flags);
 
 bounce_buffer = qemu_try_blockalign(bs,
 MIN(MIN(max_transfer, cluster_bytes),
@@ -1041,8 +1042,13 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
 /* This does not change the data on the disk, it is not
  * necessary to flush even in cache=writethrough mode.
  */
-ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
-  _qiov, 0);
+if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
+ pnum, _qiov);
+} else {
+ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+  _qiov, 0);
+}
 }
 
 if (ret < 0) {
@@ -1107,7 +1113,12 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
  * potential fallback support, if we ever implement any read flags
  * to pass through to drivers.  For now, there aren't any
  * passthrough flags.  */
-assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ)));
+assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
+   BDRV_REQ_WRITE_COMPRESSED)));
+
+/* write compressed only makes sense with copy on read */
+assert(!(flags & BDRV_REQ_WRITE_COMPRESSED) ||
+   (flags & BDRV_REQ_COPY_ON_READ));
 
 /* Handle Copy on Read and associated serialisation */
 if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1132,7 +1143,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
 }
 
 if (!ret || pnum != bytes) {
-ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
+ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov, flags);
 goto out;
 }
 }
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f..12fe188 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -12,7 +12,7 @@ blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned 
int bytes, int flag
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) 
"bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) 
"bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p 
offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t 
cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u 
cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t 
cluster_offset, int64_t cluster_bytes, int flags) "bs %p offset %"PRId64" bytes 
%u cluster_offset %"PRId64" cluster_bytes %"PRId64" flags 0x%x"
 
 # block/stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int 
is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-- 
2.7.4

[Qemu-block] [PATCH v2 3/4] block-stream: add compress option

[Anton Nefedov]

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.

Signed-off-by: Anton Nefedov 
---
 qapi/block-core.json  |  4 
 include/block/block_int.h |  4 +++-
 block/stream.c| 16 
 blockdev.c| 13 -
 hmp.c |  2 ++
 hmp-commands.hx   |  4 ++--
 6 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e34..b7282cd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2007,6 +2007,9 @@
 #
 # @speed:  the maximum speed, in bytes per second
 #
+# @compress: true to compress data; may only be set if the target format
+#supports it (default: false). (Since 2.12)
+#
 # @on-error: the action to take on an error (default report).
 #'stop' and 'enospc' can only be used if the block device
 #supports io-status (see BlockInfo).  Since 1.3.
@@ -2026,6 +2029,7 @@
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
 '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
+'*compress': 'bool',
 '*on-error': 'BlockdevOnError' } }
 
 ##
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a548277..093bf9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -863,6 +863,7 @@ int is_windows_drive(const char *filename);
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @compress: True to compress data.
  * @on_error: The action to take upon error.
  * @errp: Error object.
  *
@@ -875,7 +876,8 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp);
+  int64_t speed, bool compress,
+  BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
diff --git a/block/stream.c b/block/stream.c
index e6f7234..75c9d66 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -38,23 +38,29 @@ typedef struct StreamBlockJob {
 BlockdevOnError on_error;
 char *backing_file_str;
 int bs_flags;
+bool compress;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
 int64_t offset, uint64_t bytes,
-void *buf)
+void *buf, bool compress)
 {
 struct iovec iov = {
 .iov_base = buf,
 .iov_len  = bytes,
 };
 QEMUIOVector qiov;
+int flags = BDRV_REQ_COPY_ON_READ;
+
+if (compress) {
+flags |= BDRV_REQ_WRITE_COMPRESSED;
+}
 
 assert(bytes < SIZE_MAX);
 qemu_iovec_init_external(, , 1);
 
 /* Copy-on-read the unallocated clusters */
-return blk_co_preadv(blk, offset, qiov.size, , BDRV_REQ_COPY_ON_READ);
+return blk_co_preadv(blk, offset, qiov.size, , flags);
 }
 
 typedef struct {
@@ -166,7 +172,7 @@ static void coroutine_fn stream_run(void *opaque)
 }
 trace_stream_one_iteration(s, offset, n, ret);
 if (copy) {
-ret = stream_populate(blk, offset, n, buf);
+ret = stream_populate(blk, offset, n, buf, s->compress);
 }
 if (ret < 0) {
 BlockErrorAction action =
@@ -227,7 +233,8 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp)
+  int64_t speed, bool compress,
+  BlockdevOnError on_error, Error **errp)
 {
 StreamBlockJob *s;
 BlockDriverState *iter;
@@ -267,6 +274,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 s->base = base;
 s->backing_file_str = g_strdup(backing_file_str);
 s->bs_flags = orig_bs_flags;
+s->compress = compress;
 
 s->on_error = on_error;
 trace_stream_start(bs, base, s);
diff --git a/blockdev.c b/blockdev.c
index 56a6b24..ae2a1d2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2968,6 +2968,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
   bool has_base_node, const char *base_node,
   bool has_backing_file, const char *backing_file,
   bool has_speed, int64_t speed,
+  bool has_compress, bool compress,
   bool has_on_error, BlockdevOnError on_error,
   

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[Dr. David Alan Gilbert]

* John Snow (js...@redhat.com) wrote:
> 
> 
> On 10/30/2017 12:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> > Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
> > associated with root nodes and non-root named nodes are migrated.
> > 
> > If destination qemu is already containing a dirty bitmap with the same name
> > as a migrated bitmap (for the same node), then, if their granularities are
> > the same the migration will be done, otherwise the error will be generated.
> > 
> > If destination qemu doesn't contain such bitmap it will be created.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > ---
> >  include/migration/misc.h   |   3 +
> >  migration/migration.h  |   3 +
> >  migration/block-dirty-bitmap.c | 734 
> > +
> 
> Ouch :\
> 
> >  migration/migration.c  |   3 +
> >  migration/savevm.c |   2 +
> >  vl.c   |   1 +
> >  migration/Makefile.objs|   1 +
> >  migration/trace-events |  14 +
> >  8 files changed, 761 insertions(+)
> >  create mode 100644 migration/block-dirty-bitmap.c
> > 
> 
> Organizationally, you introduce three new 'public' prototypes:
> 
> dirty_bitmap_mig_init
> dirty_bitmap_mig_before_vm_start
> init_dirty_bitmap_incoming_migration
> 
> mig_init is advertised in migration/misc.h, the other two are in
> migration/migration.h.
> The definitions for all three are in migration/block-dirty-bitmap.c
> 
> In pure naivety, I find it weird to have something that you use in
> migration.c and advertised in migration.h actually exist separately in
> block-dirty-bitmap.c; but maybe this is the sanest thing to do.

Actually I think that's OK; it makes sense for all of the code for this
feature to sit in one place,   and there doesn't seem any point creating
a header just for this one function.

> > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > index c079b7771b..9cc539e232 100644
> > --- a/include/migration/misc.h
> > +++ b/include/migration/misc.h
> > @@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
> >  bool migration_in_postcopy_after_devices(MigrationState *);
> >  void migration_global_dump(Monitor *mon);
> >  
> > +/* migration/block-dirty-bitmap.c */
> > +void dirty_bitmap_mig_init(void);
> > +
> >  #endif
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 50d1f01346..4e3ad04664 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
> >  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
> > rbname,
> >ram_addr_t start, size_t len);
> >  
> > +void dirty_bitmap_mig_before_vm_start(void);
> > +void init_dirty_bitmap_incoming_migration(void);
> > +
> >  #endif
> > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> > new file mode 100644
> > index 00..53cb20045d
> > --- /dev/null
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -0,0 +1,734 @@
> > +/*
> > + * Block dirty bitmap postcopy migration
> > + *
> > + * Copyright IBM, Corp. 2009
> > + * Copyright (c) 2016-2017 Parallels International GmbH
> > + *
> > + * Authors:
> > + *  Liran Schour   
> > + *  Vladimir Sementsov-Ogievskiy 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + * This file is derived from migration/block.c, so it's author and IBM 
> > copyright
> > + * are here, although content is quite different.
> > + *
> > + * Contributions after 2012-01-13 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> > + *
> > + ****
> > + *
> > + * Here postcopy migration of dirty bitmaps is realized. Only named dirty
> > + * bitmaps, associated with root nodes and non-root named nodes are 
> > migrated.
> 
> Put another way, only QMP-addressable bitmaps. Nodes without a name that
> are not the root have no way to be addressed.
> 
> > + *
> > + * If destination qemu is already containing a dirty bitmap with the same 
> > name
> 
> "If the destination QEMU already contains a dirty bitmap with the same name"
> 
> > + * as a migrated bitmap (for the same node), then, if their granularities 
> > are
> > + * the same the migration will be done, otherwise the error will be 
> > generated.
> 
> "an error"
> 
> > + *
> > + * If destination qemu doesn't contain such bitmap it will be created.
> 
> "If the destination QEMU doesn't contain such a bitmap, it will be created."
> 
> > + *
> > + * format of migration:
> > + *
> > + * # Header (shared for different chunk types)
> > + * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
> > + * [ 1 byte: node name size ] \  flags & DEVICE_NAME
> > + * [ n bytes: node name ] /
> > + * [ 1 

Re: [Qemu-block] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Wouter Verhelst]

On Thu, Nov 16, 2017 at 09:30:41AM -0600, Eric Blake wrote:
> On 11/16/2017 03:51 AM, Wouter Verhelst wrote:
> 
> >> I also remember from talking with Vladimir during KVM Forum last month
> >> that one of the shortfalls of the NBD protocol is that you can only ever
> >> send a length of up to 32 bits on the command side (unless we introduce
> >> structured commands in addition to our current work to add structured
> >> replies);
> > 
> > Yes, and I'm thinking we should do so. This will obviously require more
> > negotiation.
> > 
> > Can be done fairly easily though:
> > - Client negotiates structured replies (don't think it makes sense to do
> >   structured requests without structured replies)
> > - Server sets an extra transmission flag to say "I am capable of
> >   receiving extended requests"
> > - Extended requests have a different magic number, and should have a
> >   "request length" field as well. I'm thinking we make it:
> > 
> > magic  (32b)
> > request length (16b)
> > type   (16b)
> > flags  (64b)
> > handle (64b)
> > from   (64b)
> > data length(64b)
> > (extra data)
> > 
> > Request length in this proposal should always be at least 320.
> 
> 320 bits, but we pass length in bytes, so 40.

Umm. Yes. I didn't sleep very well last night, sorry.

> > I made flags 64 bits rather than 16 as per the current format, because
> > that way everything is aligned on a 4-byte boundary, which makes things
> > a bit easier on some architectures (e.g., I know that sparc doesn't like
> > unaligned 64-bit access). 64 bits for flags looks like a bit of a waste,
> > but then if we're going to waste some bits somewhere, I guess it's best
> > to assign them to flags.
> > 
> > The idea is that "request length" is the length of the data that the
> > client is sending, and "data length" is the length of the range that
> > we're trying to deal with.
> 
> Yep, sounds reasonable.
> 
> > A write request would thus have to have request length be (data length +
> > 320); a read request would have request length be 320, and expect data
> > to be returned of data length bytes.
> 
> Umm, if data length is 64 bits, but request length is 16 bits, we can't
> properly support write requests with that much payload.

mumble mumble. Yes, of course. See above ;-)

> I think it goes
> back to the idea of block sizes: the maximum request that a server is
> willing to accept (advertised via INFO during NBD_OPT_GO) still applies,
> so you can't NBD_CMD_WRITE with more than that maximum size (and
> therefore your maximum request size can't be more than that size plus
> the overhead of the 40 byte header), but OTHER commands that CAN operate
> on larger chunks of the export (WRITE_ZEROES, BLOCK_STATUS, maybe a new
> extension LOCK for allowing fcntl()-like locking semantics) can now be
> accommodated, because they don't have to send large amounts of payload
> along with the request.
> 
> Keeping request length at 16 bits may therefore be too small (if we want
> to allow 32M write payloads), and may require us laying out the
> structured write header slightly differently (maybe fewer flag bits
> after all).

Right, so let's make it:

magic  (32b)
flags  (16b)
type   (16b)
handle (64b)
from   (64b)
data length(64b)
request length (64b)
(extra data)

That actually copies the same structure as the current request header, but:
- data length is extended from 32 bits to 64 bits, so that long requests can be
  made where they make sense. We should make it clear though that block
  size limitations still apply, and that a request to read 2^63 bytes of
  data is likely to be refused by a server.
- request lenght is added, also as a 64-bit value. I could be persuaded
  to make that a 32-bit value rather than a 64-bit one, though.

This way, common code can be used to handle all data up to and including
the "from" field; what's beyond that would need to be decided based upon
the value of the magic field (32-byte data length in case of old
requests, 2 64-byte length values in the other case).

Maybe, by the time we get here, it makes sense to also have "request
length" be 0 for a command that doesn't have any extra data beyond that
header. That would also make one particular case easier to read:

if(req.type == NBD_CMD_WRITE && req.data_length != req.request_length)
return EINVAL;

However, the downside of that is that we include header length in all
other cases (negotiation, structured replies), and this one would then
be the odd one out; I could imagine that might cause too much confusion.
Not sure whether the minor advantage above is worth that.

> Or we can say that NBD_CMD_WRITE still has to use old-style requests.

Let's not :-)

> > A metadata request could then tack on extra data, where request length
> > of 320 implies "all negotiated metadata contexts", but anything more
> > than that would imply there are some metadata IDs passed along.
> 
> 

Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)

[Anton Nefedov]

On 16/11/2017 6:54 PM, Alberto Garcia wrote:

On Wed 15 Nov 2017 05:31:20 PM CET, Anton Nefedov wrote:

I have the impression that one major source of headaches is the fact
that the reopen queue contains nodes that don't need to be reopened at
all. Ideally this should be detected early on in bdrv_reopen_queue(), so
there's no chance that the queue contains nodes used by a different
block job. If we had that then op blockers should be enough to prevent
these things. Or am I missing something?


After applying Max's patch I tried the similar approach; that is keep
BDSes referenced while they are in the reopen queue.  Now I get the
stream job hanging. Somehow one blk_root_drained_begin() is not paired
with blk_root_drained_end(). So the job stays paused.


I can see this if I apply Max's patch and keep refs to BDSs in the
reopen queue:

#0  block_job_pause (...) at blockjob.c:130
#1  0x55c143cb586d in block_job_drained_begin (...) at blockjob.c:227
#2  0x55c143d08067 in blk_set_dev_ops (...) at block/block-backend.c:887
#3  0x55c143cb69db in block_job_create (...) at blockjob.c:678
#4  0x55c143d17c0c in mirror_start_job (...) at block/mirror.c:1177

There's a ops->drained_begin(opaque) call in blk_set_dev_ops() that
doesn't seem to be paired. 



My understanding for now is

  1. in bdrv_drain_all_begin(), BDS gets drained with
 bdrv_parent_drained_begin(), all the parents get
 blk_root_drained_begin(), pause their jobs.
  2. one of the jobs finishes and deletes BB.
  3. in bdrv_drain_all_end(), bdrv_parent_drained_end() is never
 called because even though BDS still exists (referenced in the
 queue), it cannot be accessed as bdrv_next() takes BDS from the
 global BB list (and the corresponding BB is deleted).

Max's patch v1 could have helped:
http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg01835.html

Or, perhaps another approach, keep BlockJob referenced while it is
paused (by block_job_pause/resume_all()). That should prevent it from
deleting the BB.



And when you call block_job_start() then it
yields immediately waiting for the resume (that never arrives).

John, this change was yours (f4d9cc88ee69a5b04). Any idea?

Berto

Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)

[Alberto Garcia]

On Wed 15 Nov 2017 05:31:20 PM CET, Anton Nefedov wrote:
>> I have the impression that one major source of headaches is the fact
>> that the reopen queue contains nodes that don't need to be reopened at
>> all. Ideally this should be detected early on in bdrv_reopen_queue(), so
>> there's no chance that the queue contains nodes used by a different
>> block job. If we had that then op blockers should be enough to prevent
>> these things. Or am I missing something?
>> 
> After applying Max's patch I tried the similar approach; that is keep
> BDSes referenced while they are in the reopen queue.  Now I get the
> stream job hanging. Somehow one blk_root_drained_begin() is not paired
> with blk_root_drained_end(). So the job stays paused.

I can see this if I apply Max's patch and keep refs to BDSs in the
reopen queue:

#0  block_job_pause (...) at blockjob.c:130
#1  0x55c143cb586d in block_job_drained_begin (...) at blockjob.c:227
#2  0x55c143d08067 in blk_set_dev_ops (...) at block/block-backend.c:887
#3  0x55c143cb69db in block_job_create (...) at blockjob.c:678
#4  0x55c143d17c0c in mirror_start_job (...) at block/mirror.c:1177

There's a ops->drained_begin(opaque) call in blk_set_dev_ops() that
doesn't seem to be paired. And when you call block_job_start() then it
yields immediately waiting for the resume (that never arrives).

John, this change was yours (f4d9cc88ee69a5b04). Any idea?

Berto

Re: [Qemu-block] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Eric Blake]

On 11/16/2017 03:51 AM, Wouter Verhelst wrote:

>> I also remember from talking with Vladimir during KVM Forum last month
>> that one of the shortfalls of the NBD protocol is that you can only ever
>> send a length of up to 32 bits on the command side (unless we introduce
>> structured commands in addition to our current work to add structured
>> replies);
> 
> Yes, and I'm thinking we should do so. This will obviously require more
> negotiation.
> 
> Can be done fairly easily though:
> - Client negotiates structured replies (don't think it makes sense to do
>   structured requests without structured replies)
> - Server sets an extra transmission flag to say "I am capable of
>   receiving extended requests"
> - Extended requests have a different magic number, and should have a
>   "request length" field as well. I'm thinking we make it:
> 
> magic  (32b)
> request length (16b)
> type   (16b)
> flags  (64b)
> handle (64b)
> from   (64b)
> data length(64b)
> (extra data)
> 
> Request length in this proposal should always be at least 320.

320 bits, but we pass length in bytes, so 40.

> 
> I made flags 64 bits rather than 16 as per the current format, because
> that way everything is aligned on a 4-byte boundary, which makes things
> a bit easier on some architectures (e.g., I know that sparc doesn't like
> unaligned 64-bit access). 64 bits for flags looks like a bit of a waste,
> but then if we're going to waste some bits somewhere, I guess it's best
> to assign them to flags.
> 
> The idea is that "request length" is the length of the data that the
> client is sending, and "data length" is the length of the range that
> we're trying to deal with.

Yep, sounds reasonable.

> 
> A write request would thus have to have request length be (data length +
> 320); a read request would have request length be 320, and expect data
> to be returned of data length bytes.

Umm, if data length is 64 bits, but request length is 16 bits, we can't
properly support write requests with that much payload.  I think it goes
back to the idea of block sizes: the maximum request that a server is
willing to accept (advertised via INFO during NBD_OPT_GO) still applies,
so you can't NBD_CMD_WRITE with more than that maximum size (and
therefore your maximum request size can't be more than that size plus
the overhead of the 40 byte header), but OTHER commands that CAN operate
on larger chunks of the export (WRITE_ZEROES, BLOCK_STATUS, maybe a new
extension LOCK for allowing fcntl()-like locking semantics) can now be
accommodated, because they don't have to send large amounts of payload
along with the request.

Keeping request length at 16 bits may therefore be too small (if we want
to allow 32M write payloads), and may require us laying out the
structured write header slightly differently (maybe fewer flag bits
after all).  Or we can say that NBD_CMD_WRITE still has to use old-style
requests.

> 
> A metadata request could then tack on extra data, where request length
> of 320 implies "all negotiated metadata contexts", but anything more
> than that would imply there are some metadata IDs passed along.

Again, 40 bytes (not 320 bits), but yeah, that would be the idea.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [ovirt-users] Enabling libgfapi disk access with oVirt 4.2

[Misak Khachatryan]

Hi,

I don't mean on the fly upgrade, just confused that i should stop all
VMs at once, as i understood procedure. If it's possible to do per VM,
it's perfectly OK for me.

Thank you Nir for clarification.

Best regards,
Misak Khachatryan


On Thu, Nov 16, 2017 at 2:05 AM, Nir Soffer  wrote:
> On Wed, Nov 15, 2017 at 8:58 AM Misak Khachatryan  wrote:
>>
>> Hi,
>>
>> will it be a more clean approach? I can't tolerate full stop of all
>> VMs just to enable it, seems too disastrous for real production
>> environment. Will it be some migration mechanisms in future?
>
>
> You can enable it per vm, you don't need to stop all of them. But I think
> we do not support upgrading a machine with running vms, so upgrading
> requires:
>
> 1. migrating vms from the host you want to upgrade
> 2. upgrading the host
> 3. stopping the vm you want to upgrade to libgfapi
> 4. starting this vm on the upgraded host
>
> Theoretically qemu could switch from one disk to another, but I'm not
> sure this is supported when switching to the same disk using different
> transports. I know it is not supported now to mirror a network drive to
> another network drive.
>
> The old disk is using:
>
> 
>  file="/rhev/data-center/mnt/server:_volname/sd_id/images/img_id/vol_id"/>
> 
>  name="qemu" type="raw"/>
> 
>
> The new disk should use:
>
> 
>  protocol="gluster">
> 
> 
>  name="qemu" type="raw"/>
> 
>
> Adding qemu-block mailing list.
>
> Nir
>
>>
>>
>> Best regards,
>> Misak Khachatryan
>>
>>
>> On Fri, Nov 10, 2017 at 12:35 AM, Darrell Budic 
>> wrote:
>> > You do need to stop the VMs and restart them, not just issue a reboot. I
>> > havn’t tried under 4.2 yet, but it works in 4.1.6 that way for me.
>> >
>> > 
>> > From: Alessandro De Salvo 
>> > Subject: Re: [ovirt-users] Enabling libgfapi disk access with oVirt 4.2
>> > Date: November 9, 2017 at 2:35:01 AM CST
>> > To: us...@ovirt.org
>> >
>> >
>> > Hi again,
>> >
>> > OK, tried to stop all the vms, except the engine, set engine-config -s
>> > LibgfApiSupported=true (for 4.2 only) and restarted the engine.
>> >
>> > When I tried restarting the VMs they are still not using gfapi, so it
>> > does
>> > not seem to help.
>> >
>> > Cheers,
>> >
>> >
>> > Alessandro
>> >
>> >
>> >
>> > Il 09/11/17 09:12, Alessandro De Salvo ha scritto:
>> >
>> > Hi,
>> > where should I enable gfapi via the UI?
>> > The only command I tried was engine-config -s LibgfApiSupported=true but
>> > the
>> > result is what is shown in my output below, so it’s set to true for
>> > v4.2. Is
>> > it enough?
>> > I’ll try restarting the engine. Is it really needed to stop all the VMs
>> > and
>> > restart them all? Of course this is a test setup and I can do it, but
>> > for
>> > production clusters in the future it may be a problem.
>> > Thanks,
>> >
>> >Alessandro
>> >
>> > Il giorno 09 nov 2017, alle ore 07:23, Kasturi Narra 
>> > ha
>> > scritto:
>> >
>> > Hi ,
>> >
>> > The procedure to enable gfapi is below.
>> >
>> > 1) stop all the vms running
>> > 2) Enable gfapi via UI or using engine-config command
>> > 3) Restart ovirt-engine service
>> > 4) start the vms.
>> >
>> > Hope you have not missed any !!
>> >
>> > Thanks
>> > kasturi
>> >
>> > On Wed, Nov 8, 2017 at 11:58 PM, Alessandro De Salvo
>> >  wrote:
>> >>
>> >> Hi,
>> >>
>> >> I'm using the latest 4.2 beta release and want to try the gfapi access,
>> >> but I'm currently failing to use it.
>> >>
>> >> My test setup has an external glusterfs cluster v3.12, not managed by
>> >> oVirt.
>> >>
>> >> The compatibility flag is correctly showing gfapi should be enabled
>> >> with
>> >> 4.2:
>> >>
>> >> # engine-config -g LibgfApiSupported
>> >> LibgfApiSupported: false version: 3.6
>> >> LibgfApiSupported: false version: 4.0
>> >> LibgfApiSupported: false version: 4.1
>> >> LibgfApiSupported: true version: 4.2
>> >>
>> >> The data center and cluster have the 4.2 compatibility flags as well.
>> >>
>> >> However, when starting a VM with a disk on gluster I can still see the
>> >> disk is mounted via fuse.
>> >>
>> >> Any clue of what I'm still missing?
>> >>
>> >> Thanks,
>> >>
>> >>
>> >>Alessandro
>> >>
>> >> ___
>> >> Users mailing list
>> >> us...@ovirt.org
>> >> http://lists.ovirt.org/mailman/listinfo/users
>> >
>> >
>> >
>> >
>> > ___
>> > Users mailing list
>> > us...@ovirt.org
>> > http://lists.ovirt.org/mailman/listinfo/users
>> >
>> >
>> > ___
>> > Users mailing list
>> > us...@ovirt.org
>> > http://lists.ovirt.org/mailman/listinfo/users
>> >
>> >
>> >
>> > 

Re: [Qemu-block] [PATCH for-2.11] throttle-groups: forget timer and schedule next TGM on detach

[Stefan Hajnoczi]

On Thu, Nov 16, 2017 at 11:21:50AM +, Stefan Hajnoczi wrote:
> tg->any_timer_armed[] must be cleared when detaching pending timers from
> the AioContext.  Failure to do so leads to hung I/O because it looks
> like there are still timers pending when in fact they have been removed.
> 
> Other ThrottleGroupMembers might have requests pending too so it's
> necessary to schedule the next TGM so it can set a timer.
> 
> This patch fixes hung I/O when QEMU is launched with drives that are in
> the same throttling group:
> 
>   (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct bs=512 &
>   (guest)$ dd if=/dev/zero of=/dev/vdc oflag=direct bs=512 &
>   (qemu) stop
>   (qemu) cont
>   ...I/O is stuck...
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/throttle-groups.c | 12 
>  1 file changed, 12 insertions(+)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [Libguestfs] [Qemu-devel] [qemu-img] support for XVA

[Max Reitz]

On 2017-11-16 11:08, Gandalf Corvotempesta wrote:
> 2017-11-15 23:55 GMT+01:00 Max Reitz :
>> https://xanclic.moe/convert-xva.rb -- does this work?
>> (It seems to works on the two example images I found...)
>>
>> An example is in the code, you use it like this:
>>
>> $ ./convert-xva.rb ~/Downloads/stats-appliance-2.36.020502.xva Ref:73
> 
> 
> It doesn't work on huge images:
> 
> # time convert-xva.rb 20171115_193814_efff_.xva Ref:10 -O qcow2 disk1.qcow2
> /usr/bin/convert-xva.rb:119:in `exec': Argument list too long -
> qemu-img (Errno::E2BIG)
> from /usr/bin/convert-xva.rb:119:in `'
> 
> real 9m41.414s
> user 0m19.280s
> sys 0m3.260s

Well, there is this:

https://github.com/XanClic/qemu.rb/blob/master/examples/convert-xva-online.rb

That should get around that restriction, but currently it is stupidly
slow (because it just issues all 1 MB copy operations simultaneously),
and it doesn't use qemu-img convert.  Instead, it uses a real qemu with
block jobs.  Ideally, you'd use it like this:

$ ./convert-xva-online.rb ~/Downloads/stats-appliance-2.36.020502.xva
Ref:73  81936 M
$ qemu-img create -f qcow2 [whatever options you need] disk1.qcow2 \
81936M
Formatting 'disk1.qcow2', fmt=qcow2 size=85916123136 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./convert-xva-online.rb ~/Downloads/stats-appliance-2.36.020502.xva \
Ref:73 \
'{"driver":"qcow2",
  "detect-zeroes":"unmap",
  "discard":"unmap",
  "file":{"driver":"file","filename":"disk1.qcow2"}}'
Adding block devices...
Starting block jobs...
100.00 % of jobs completed

(Maybe I can get it faster.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Richard W.M. Jones]

I think it's even possible to modify that shell script to pipe into
nbdkit and from there to ‘qemu-img convert’.  I'm too lazy to actually
do that right now, but the basic idea is here:

  https://rwmj.wordpress.com/2014/10/14/streaming-nbd-server/

You'll probably have to add ‘-m 1’ to the qemu-img convert command line.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-block] [PATCH 5/5] iotest 030: add compressed block-stream test

[Anton Nefedov]

On 15/11/2017 10:51 PM, Max Reitz wrote:

On 2017-11-14 11:16, Anton Nefedov wrote:

Signed-off-by: Anton Nefedov 
---
  tests/qemu-iotests/030 | 69 +-
  tests/qemu-iotests/030.out |  4 +--
  2 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1883894..52cbe5d 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,7 @@
  import time
  import os
  import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_img_pipe, qemu_io
  
  backing_img = os.path.join(iotests.test_dir, 'backing.img')

  mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
  
  self.cancel_and_wait()
  
+class TestCompressed(iotests.QMPTestCase):

+supported_fmts = ['qcow2']
+cluster_size = 64 * 1024;
+image_len = 1 * 1024 * 1024;
+
+def setUp(self):
+qemu_img('create', backing_img, str(TestCompressed.image_len))


First, this is missing the '-f raw', if you want to keep it in line with
the next command.


+qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + 
str(TestCompressed.image_len), backing_img)


But why would you want this to be raw?



Copied this from another test in this file :)
The source image format does not really matter. Will fix though.


+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, test_img)
+
+# write '4' in every 4th cluster
+step=4


I'd prefer spaces around operators.  (Also in _pattern() and in the call
to pattern ([1,4,2] should be [1, 4, 2]).)


+for i in range(TestCompressed.image_len / TestCompressed.cluster_size 
/ step + 1):


As far as I know, range() has an actual step parameter, maybe that would
be simpler -- and I don't know what the +1 is supposed to mean.

How about "for ofs in range(0, TestCompressed.image_len,
 TestCompressed.cluster_size * step)"?
Would that work?



It does, thank you.


+qemu_io('-c', 'write -P %d %d %d' %
+(step, step * i * TestCompressed.cluster_size,> +  
   TestCompressed.cluster_size),
+test_img)
+
+self.vm = iotests.VM().add_drive(test_img)
+self.vm.launch()
+
+def tearDown(self):
+os.remove(test_img)
+os.remove(backing_img)
+
+def _pattern(self, x, divs):
+return divs[-1] if not x%divs[-1] else self._pattern(x, divs[:-1])


I have no idea what this function does.

...OK, having looked into how it's used, I understand better.  A comment
would certainly be helpful, though.

Maybe also call it differently.  It doesn't really return a pattern.
What this function does is it returns the first integer from @divs
(starting from the end, which is weird) which is a divider of @x.  So
maybe call it _first_divider(self, x, dividers), that would help already.



Yeah, I really had to comment.

Exactly, it starts from the end as I meant it to accept numbers in the 
order they were written. So 'first_divider' wouldn't be right.

Not that important though, will rename and reorder the input.



+
+def test_compressed(self):
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('block-stream', device='drive0', compress=True)
+if iotests.imgfmt not in TestCompressed.supported_fmts:
+self.assert_qmp(
+result, 'error/desc',
+'Compression is not supported for this drive drive0')
+return
+self.assert_qmp(result, 'return', {})
+
+# write '2' in every 2nd cluster
+step = 2
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size 
/ step + 1):
+result = self.vm.qmp('human-monitor-command',
+ command_line=
+ 'qemu-io drive0 \"write -P %d %d %d\"' %


Just " instead of \" would be enough, I think.


Done.


+ (step, step * i * TestCompressed.cluster_size,
+  TestCompressed.cluster_size))
+self.assert_qmp(result, 'return', "")
+
+self.wait_until_completed()
+self.assert_no_active_block_jobs()
+
+self.vm.shutdown()
+
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size):
+outp = qemu_io('-c', 'read -P %d %d %d' %
+   (self._pattern(i, [1,4,2]),


The 4 is useless, because everything that's divisible by 4 is divisible
by 2 already.

Also, I don't quite see what this should achieve exactly.  You first
write the backing image full of 1s, OK.  Then you write 4s to every
fourth cluster in the top image.  Then you start streaming, and then you
write 2s to ever second cluster in the top image, which overwrites all
of the 4s you wrote.

Do you 

Re: [Qemu-block] [PATCH for-2.11] throttle-groups: forget timer and schedule next TGM on detach

[Alberto Garcia]

On Thu 16 Nov 2017 12:21:50 PM CET, Stefan Hajnoczi wrote:
> tg->any_timer_armed[] must be cleared when detaching pending timers from
> the AioContext.  Failure to do so leads to hung I/O because it looks
> like there are still timers pending when in fact they have been removed.
>
> Other ThrottleGroupMembers might have requests pending too so it's
> necessary to schedule the next TGM so it can set a timer.
>
> This patch fixes hung I/O when QEMU is launched with drives that are in
> the same throttling group:
>
>   (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct bs=512 &
>   (guest)$ dd if=/dev/zero of=/dev/vdc oflag=direct bs=512 &
>   (qemu) stop
>   (qemu) cont
>   ...I/O is stuck...
>
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Tomáš Golembiovský]

On Thu, 16 Nov 2017 13:56:16 +0100
Tomáš Golembiovský  wrote:

> On Wed, 15 Nov 2017 21:41:20 +0100
> Gandalf Corvotempesta  wrote:
> 
> > 2017-11-15 21:29 GMT+01:00 Richard W.M. Jones :  
> > > Gandalf, is there an XVA file publically available (pref. not
> > > too big) that we can look at?  
> > 
> > I can try to provide one, but it's simple:
> > 
> > # tar tvf 20160630_124823_aa72_.xva.gz | head -n 50
> > -- 0/0   42353 1970-01-01 01:00 ova.xml
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/
> > -- 0/0  40 1970-01-01 01:00 Ref:175/.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0001
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0001.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0003
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0003.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0004
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0004.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0005
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0005.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0006
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0006.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0007
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0007.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0009
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0009.checksum
> > -- 0/0 1048576 1970-01-01 01:00 Ref:175/0010
> > -- 0/0  40 1970-01-01 01:00 Ref:175/0010.checksum
> > 
> > 
> > You can ignore the ova.xml and just use the "Ref:175" directory.
> > Inside the XVA you'll fine one "Ref" directory for each virtual disk
> > (ref number is different for each disk)
> > Inside each Ref directory, you'll get tons of 1MB file, corrisponding
> > to the RAW image.
> > You have to merge these files in a single raw file with just an
> > exception: numbers are not sequential.
> > as you can see above, we have: , 0001, 0003
> > 
> > The 0002 is missing, because it's totally blank. XenServer doesn't
> > export any empty block, thus it will skip the corrisponding 1MB file.
> > When building the raw image, you have to replace empty blocks with 1MB
> > full of zeros.
> > 
> > This is (in addition to the tar extract) the most time-consuming part.
> > Currently I'm rebuilding a 250GB image, with tons of files to be
> > merge.
> > If qemu-img can be patched to automatically convert this kind of
> > format, I can save about 3 hours (30 minutes for extracting the
> > tarball, and about 2 hours to merge 250-300GB image)
> >   
> 
> Hi,
> 
> I like what Richard and Max came up with. Pretty nifty solutions.
> While there's nothing wrong with them I decided to take my own shot at
> it. Since the blocks in tar file are pieces of raw image there is no
> conversion happening. All in all it's just moving bytes from one place
> to another. That means there shouldn't be a need for any heavy
> machinery, right? :)
> 
> Attached is a shell script that uses dd to do the byte-shuffling.
> I'm pretty sure this could even be safely parallelized by running
> multiple instances of dd at the same time (e.g. with xargs). But I did
> not try that.

Oh yes, and one more thing: as with the other solutions I didn't bother
reading the XML for the target size. So resize may be necessary
afterwards.

Tomas

-- 
Tomáš Golembiovský 

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Tomáš Golembiovský]

On Wed, 15 Nov 2017 21:41:20 +0100
Gandalf Corvotempesta  wrote:

> 2017-11-15 21:29 GMT+01:00 Richard W.M. Jones :
> > Gandalf, is there an XVA file publically available (pref. not
> > too big) that we can look at?
> 
> I can try to provide one, but it's simple:
> 
> # tar tvf 20160630_124823_aa72_.xva.gz | head -n 50
> -- 0/0   42353 1970-01-01 01:00 ova.xml
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/
> -- 0/0  40 1970-01-01 01:00 Ref:175/.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0001
> -- 0/0  40 1970-01-01 01:00 Ref:175/0001.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0003
> -- 0/0  40 1970-01-01 01:00 Ref:175/0003.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0004
> -- 0/0  40 1970-01-01 01:00 Ref:175/0004.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0005
> -- 0/0  40 1970-01-01 01:00 Ref:175/0005.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0006
> -- 0/0  40 1970-01-01 01:00 Ref:175/0006.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0007
> -- 0/0  40 1970-01-01 01:00 Ref:175/0007.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0009
> -- 0/0  40 1970-01-01 01:00 Ref:175/0009.checksum
> -- 0/0 1048576 1970-01-01 01:00 Ref:175/0010
> -- 0/0  40 1970-01-01 01:00 Ref:175/0010.checksum
> 
> 
> You can ignore the ova.xml and just use the "Ref:175" directory.
> Inside the XVA you'll fine one "Ref" directory for each virtual disk
> (ref number is different for each disk)
> Inside each Ref directory, you'll get tons of 1MB file, corrisponding
> to the RAW image.
> You have to merge these files in a single raw file with just an
> exception: numbers are not sequential.
> as you can see above, we have: , 0001, 0003
> 
> The 0002 is missing, because it's totally blank. XenServer doesn't
> export any empty block, thus it will skip the corrisponding 1MB file.
> When building the raw image, you have to replace empty blocks with 1MB
> full of zeros.
> 
> This is (in addition to the tar extract) the most time-consuming part.
> Currently I'm rebuilding a 250GB image, with tons of files to be
> merge.
> If qemu-img can be patched to automatically convert this kind of
> format, I can save about 3 hours (30 minutes for extracting the
> tarball, and about 2 hours to merge 250-300GB image)
> 

Hi,

I like what Richard and Max came up with. Pretty nifty solutions.
While there's nothing wrong with them I decided to take my own shot at
it. Since the blocks in tar file are pieces of raw image there is no
conversion happening. All in all it's just moving bytes from one place
to another. That means there shouldn't be a need for any heavy
machinery, right? :)

Attached is a shell script that uses dd to do the byte-shuffling.
I'm pretty sure this could even be safely parallelized by running
multiple instances of dd at the same time (e.g. with xargs). But I did
not try that.


Tomas

-- 
Tomáš Golembiovský 


xva2raw.sh
Description: application/shellscript

Re: [Qemu-block] [PATCH v4] throttle-groups: drain before detaching ThrottleState

[Stefan Hajnoczi]

On Mon, Nov 13, 2017 at 2:29 PM, Alberto Garcia  wrote:
> On Fri 10 Nov 2017 04:19:34 PM CET, Stefan Hajnoczi wrote:
>> I/O requests hang after stop/cont commands at least since QEMU 2.10.0
>> with -drive iops=100:
>>
>>   (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
>>   (qemu) stop
>>   (qemu) cont
>>   ...I/O is stuck...
>>
>> This happens because blk_set_aio_context() detaches the ThrottleState
>> while requests may still be in flight:
>>
>>   if (tgm->throttle_state) {
>>   throttle_group_detach_aio_context(tgm);
>>   throttle_group_attach_aio_context(tgm, new_context);
>>   }
>>
>> This patch encloses the detach/attach calls in a drained region so no
>> I/O request is left hanging.  Also add assertions so we don't make the
>> same mistake again in the future.
>
> I'm wondering about the implications of this change... is it possible
> now to bypass the I/O limits simply by stopping and quickly resuming the
> VM? And is that a problem?

bdrv_set_aio_context() already drains so this patch doesn't change
existing behavior with respect to bypassing throttling.

It's not ideal that certain VM lifecycle operations temporarily
disable throttling, but it's a trade-off since synchronous drain is
usually performance sensitive and should not take a long time.
Perhaps there are ways to improve the situation, I haven't studied it
in detail.

Stefan

[Qemu-block] [PATCH for-2.11] throttle-groups: forget timer and schedule next TGM on detach

[Stefan Hajnoczi]

tg->any_timer_armed[] must be cleared when detaching pending timers from
the AioContext.  Failure to do so leads to hung I/O because it looks
like there are still timers pending when in fact they have been removed.

Other ThrottleGroupMembers might have requests pending too so it's
necessary to schedule the next TGM so it can set a timer.

This patch fixes hung I/O when QEMU is launched with drives that are in
the same throttling group:

  (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct bs=512 &
  (guest)$ dd if=/dev/zero of=/dev/vdc oflag=direct bs=512 &
  (qemu) stop
  (qemu) cont
  ...I/O is stuck...

Signed-off-by: Stefan Hajnoczi 
---
 block/throttle-groups.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 2587f19ca3..f26bcb5eee 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -593,13 +593,25 @@ void 
throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
 
 void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
 {
+ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
 ThrottleTimers *tt = >throttle_timers;
+int i;
 
 /* Requests must have been drained */
 assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
 assert(qemu_co_queue_empty(>throttled_reqs[0]));
 assert(qemu_co_queue_empty(>throttled_reqs[1]));
 
+/* Kick off next ThrottleGroupMember, if necessary */
+qemu_mutex_lock(>lock);
+for (i = 0; i < 2; i++) {
+if (timer_pending(tt->timers[i])) {
+tg->any_timer_armed[i] = false;
+schedule_next_request(tgm, i);
+}
+}
+qemu_mutex_unlock(>lock);
+
 throttle_timers_detach_aio_context(tt);
 tgm->aio_context = NULL;
 }
-- 
2.13.6

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

15.11.2017 04:58, John Snow wrote:


On 10/30/2017 12:33 PM, Vladimir Sementsov-Ogievskiy wrote:

Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
associated with root nodes and non-root named nodes are migrated.

If destination qemu is already containing a dirty bitmap with the same name
as a migrated bitmap (for the same node), then, if their granularities are
the same the migration will be done, otherwise the error will be generated.

If destination qemu doesn't contain such bitmap it will be created.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/migration/misc.h   |   3 +
  migration/migration.h  |   3 +
  migration/block-dirty-bitmap.c | 734 +

Ouch :\


  migration/migration.c  |   3 +
  migration/savevm.c |   2 +
  vl.c   |   1 +
  migration/Makefile.objs|   1 +
  migration/trace-events |  14 +
  8 files changed, 761 insertions(+)
  create mode 100644 migration/block-dirty-bitmap.c


Organizationally, you introduce three new 'public' prototypes:

dirty_bitmap_mig_init
dirty_bitmap_mig_before_vm_start
init_dirty_bitmap_incoming_migration

mig_init is advertised in migration/misc.h, the other two are in
migration/migration.h.
The definitions for all three are in migration/block-dirty-bitmap.c

In pure naivety, I find it weird to have something that you use in
migration.c and advertised in migration.h actually exist separately in
block-dirty-bitmap.c; but maybe this is the sanest thing to do.


diff --git a/include/migration/misc.h b/include/migration/misc.h
index c079b7771b..9cc539e232 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
  bool migration_in_postcopy_after_devices(MigrationState *);
  void migration_global_dump(Monitor *mon);
  
+/* migration/block-dirty-bitmap.c */

+void dirty_bitmap_mig_init(void);
+
  #endif
diff --git a/migration/migration.h b/migration/migration.h
index 50d1f01346..4e3ad04664 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
rbname,
ram_addr_t start, size_t len);
  
+void dirty_bitmap_mig_before_vm_start(void);

+void init_dirty_bitmap_incoming_migration(void);
+
  #endif
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
new file mode 100644
index 00..53cb20045d
--- /dev/null
+++ b/migration/block-dirty-bitmap.c
@@ -0,0 +1,734 @@
+/*
+ * Block dirty bitmap postcopy migration
+ *
+ * Copyright IBM, Corp. 2009
+ * Copyright (c) 2016-2017 Parallels International GmbH
+ *
+ * Authors:
+ *  Liran Schour   
+ *  Vladimir Sementsov-Ogievskiy 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ * This file is derived from migration/block.c, so it's author and IBM 
copyright
+ * are here, although content is quite different.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
+ ****
+ *
+ * Here postcopy migration of dirty bitmaps is realized. Only named dirty
+ * bitmaps, associated with root nodes and non-root named nodes are migrated.

Put another way, only QMP-addressable bitmaps. Nodes without a name that
are not the root have no way to be addressed.


+ *
+ * If destination qemu is already containing a dirty bitmap with the same name

"If the destination QEMU already contains a dirty bitmap with the same name"


+ * as a migrated bitmap (for the same node), then, if their granularities are
+ * the same the migration will be done, otherwise the error will be generated.

"an error"


+ *
+ * If destination qemu doesn't contain such bitmap it will be created.

"If the destination QEMU doesn't contain such a bitmap, it will be created."


+ *
+ * format of migration:
+ *
+ * # Header (shared for different chunk types)
+ * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
+ * [ 1 byte: node name size ] \  flags & DEVICE_NAME
+ * [ n bytes: node name ] /
+ * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
+ * [ n bytes: bitmap name ] /
+ *
+ * # Start of bitmap migration (flags & START)
+ * header
+ * be64: granularity
+ * 1 byte: bitmap flags (corresponds to BdrvDirtyBitmap)
+ *   bit 0-  bitmap is enabled
+ *   bit 1-  bitmap is persistent
+ *   bit 2-  bitmap is autoloading
+ *   bits 3-7 - reserved, must be zero
+ *
+ * # Complete of bitmap migration (flags & COMPLETE)
+ * header
+ *
+ * # Data chunk of bitmap migration
+ * header
+ * be64: start sector
+ * be32: number of sectors
+ * [ be64: buffer size  ] \ ! (flags & ZEROES)
+ * [ n 

Re: [Qemu-block] [PATCH 4/5] block-stream: add compress option

[Anton Nefedov]

On 15/11/2017 10:16 PM, Max Reitz wrote:

On 2017-11-14 11:16, Anton Nefedov wrote:

Signed-off-by: Anton Nefedov 
---
  qapi/block-core.json  |  4 
  include/block/block_int.h |  4 +++-
  block/stream.c| 16 
  blockdev.c| 13 -
  hmp.c |  2 ++
  hmp-commands.hx   |  4 ++--
  6 files changed, 35 insertions(+), 8 deletions(-)


Looks good to me overall, two notes below.

Max


diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e34..b0d022f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2007,6 +2007,9 @@
  #
  # @speed:  the maximum speed, in bytes per second
  #
+# @compress: true to compress data, if the target format supports it.
+#(default: false). Since 2.12.
+#


Too many full stops (one before, one after the parentheses).  Also, this
makes it sound a bit like it would still be possible to specify this
parameter even if the target doesn't support it (and it would just be
ignored then), but that's not true.

Also, the format most parameter documentation follows for block-stream
is more "@parameter: Description. Default. (Since version)".

So I'd suggest:

"true to compress data; may only be set if the target format supports
it.  Defaults to false if omitted.  (Since 2.12)"

But I won't argue too much over the format, so the following is OK, too:

"true to compress data; may only be set if the target format supports it
(default: false). (Since 2.12)"



Thanks, will fix.

[..]


@@ -3034,11 +3039,17 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
  goto out;
  }
  
+if (compress && bs->drv->bdrv_co_pwritev_compressed == NULL) {

+error_setg(errp, "Compression is not supported for this drive %s",
+   bdrv_get_device_name(bs));


I think just device instead of bdrv_get_device_name(bs) is better (or
bdrv_get_device_or_node_name(bs) at least).



device should be enough indeed, done.

/Anton

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Richard W.M. Jones]

On Thu, Nov 16, 2017 at 11:07:10AM +0100, Gandalf Corvotempesta wrote:
> 2017-11-16 11:01 GMT+01:00 Richard W.M. Jones :
> > As mentioned before you can use this to do a qemu-img convert using
> > captive nbdkit:
> >
> >   $ nbdkit -U - \
> >   perl script=./xva-reader.pl file=./debian8cloud.xva size=4294967296 \
> >   --run 'qemu-img convert -f raw $nbd -O qcow2 /var/tmp/output.qcow2 -p'
> 
> 
> What if XVA is hosting 2 or more partitions ? Which partition are you
> converting with this command ?

See "XXX" comments in the code, left as an exercise to the reader :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Gandalf Corvotempesta]

2017-11-16 11:01 GMT+01:00 Richard W.M. Jones :
> As mentioned before you can use this to do a qemu-img convert using
> captive nbdkit:
>
>   $ nbdkit -U - \
>   perl script=./xva-reader.pl file=./debian8cloud.xva size=4294967296 \
>   --run 'qemu-img convert -f raw $nbd -O qcow2 /var/tmp/output.qcow2 -p'


What if XVA is hosting 2 or more partitions ? Which partition are you
converting with this command ?

Re: [Qemu-block] [Libguestfs] [Qemu-devel] [qemu-img] support for XVA

[Gandalf Corvotempesta]

2017-11-15 23:55 GMT+01:00 Max Reitz :
> https://xanclic.moe/convert-xva.rb -- does this work?
> (It seems to works on the two example images I found...)
>
> An example is in the code, you use it like this:
>
> $ ./convert-xva.rb ~/Downloads/stats-appliance-2.36.020502.xva Ref:73


It doesn't work on huge images:

# time convert-xva.rb 20171115_193814_efff_.xva Ref:10 -O qcow2 disk1.qcow2
/usr/bin/convert-xva.rb:119:in `exec': Argument list too long -
qemu-img (Errno::E2BIG)
from /usr/bin/convert-xva.rb:119:in `'

real 9m41.414s
user 0m19.280s
sys 0m3.260s

Re: [Qemu-block] [PATCH 3/5] block: support compressed write for copy-on-read

[Anton Nefedov]

On 15/11/2017 9:49 PM, Max Reitz wrote:

On 2017-11-14 11:16, Anton Nefedov wrote:

Signed-off-by: Anton Nefedov 
---
  block/io.c | 30 --
  block/trace-events |  2 +-
  2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3d5ef2c..93c6b24 100644
--- a/block/io.c
+++ b/block/io.c


[...]


@@ -1209,6 +1220,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
  return ret;
  }
  
+/* write compressed only makes sense with copy on read */

+if ((flags & BDRV_REQ_WRITE_COMPRESSED) &&
+!(flags & BDRV_REQ_COPY_ON_READ))
+{
+return -EINVAL;
+}
+


I think the assertion in bdrv_aligned_preadv() should be enough, but
either way:

Reviewed-by: Max Reitz 



Ok, and it will fail more loudly. Will remove.


  bdrv_inc_in_flight(bs);
  
  /* Don't do copy-on-read if we read data before write operation */

Re: [Qemu-block] [PATCH 0/5] compressed block-stream

[Anton Nefedov]

On 16/11/2017 6:26 AM, Fam Zheng wrote:

On Tue, 11/14 13:16, Anton Nefedov wrote:

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.


I haven't looked at the patches yet so maybe the answer is obvious, but still:
would the "compress" option be still necessary if what we have is
blockdev-stream instead?

Fam



Sorry, I can't see blockdev-stream available now. How that would be
different? Would it imply the blocks are pulled up to the top (active)
image?

Or did you mean block-commit? The option might be useful there as well,
and needs to be implemented separately

/Anton

Re: [Qemu-block] [Qemu-devel] [Libguestfs] [qemu-img] support for XVA

[Richard W.M. Jones]

Here's my solution, as a nbdkit plugin written in Perl.

As with Max's solution I don't bother to parse the virtual size out of
the XML file, so you need to specify that on the command line
otherwise the disk will be truncated to the largest extent stored in
the file.  Also the ‘.xva’ file must not be compressed.

  $ nbdkit perl script=./xva-reader.pl file=./debian8cloud.xva size=4294967296

  $ guestfish --ro --format=raw -a nbd://localhost -i
  
  Welcome to guestfish, the guest filesystem shell for
  editing virtual machine filesystems and disk images.
  
  Type: 'help' for help on commands
'man' to read the manual
'quit' to quit the shell
  
  Operating system: 8.2
  /dev/sda1 mounted on /
  
  > ll /
  total 100
  drwxr-xr-x 22 root root  4096 Jan  8  2016 .
  drwxr-xr-x 19 root root  4096 Nov 16 09:50 ..
  drwxrwxr-x  2 root root  4096 Jan  8  2016 bin
  drwxr-xr-x  3 root root  4096 Jan  8  2016 boot
  drwxr-xr-x  4 root root  4096 Jan  8  2016 dev
  drwxr-xr-x 87 root root  4096 Jan  8  2016 etc
  drwxr-xr-x  3 root root  4096 Jan  8  2016 home
  lrwxrwxrwx  1 root root31 Jan  8  2016 initrd.img -> 
/boot/initrd.img-3.16.0-4-amd64
  drwxr-xr-x 14 root root  4096 Jan  8  2016 lib
  drwxr-xr-x  2 root root  4096 Jan  8  2016 lib64
  drwx--  2 root root 16384 Jan  8  2016 lost+found
  drwxr-xr-x  3 root root  4096 Jan  8  2016 media
  drwxr-xr-x  2 root root  4096 Jan  8  2016 mnt
  drwxr-xr-x  2 root root  4096 Jan  8  2016 opt
  drwxr-xr-x  2 root root  4096 May  4  2015 proc
  drwx--  2 root root  4096 Jan  8  2016 root
  drwxr-xr-x  2 root root  4096 Jan  8  2016 run
  drwxr-xr-x  2 root root  4096 Jan  8  2016 sbin
  drwxr-xr-x  2 root root  4096 Jan  8  2016 srv
  drwxr-xr-x  2 root root  4096 Apr  6  2015 sys
  drwxrwxrwt  7 root root  4096 Jan  8  2016 tmp
  drwxr-xr-x 10 root root  4096 Jan  8  2016 usr
  drwxr-xr-x 11 root root  4096 Jan  8  2016 var
  lrwxrwxrwx  1 root root27 Jan  8  2016 vmlinuz -> 
boot/vmlinuz-3.16.0-4-amd64

I even managed to boot the Debian 8 guest from the sample .xva file:

  $ qemu-system-x86_64 -cpu host -machine accel=kvm:tcg -m 2048 -drive 
file=nbd:localhost:10809,format=raw,if=virtio,snapshot=on

although it was pretty slow ...

As mentioned before you can use this to do a qemu-img convert using
captive nbdkit:

  $ nbdkit -U - \
  perl script=./xva-reader.pl file=./debian8cloud.xva size=4294967296 \
  --run 'qemu-img convert -f raw $nbd -O qcow2 /var/tmp/output.qcow2 -p'

HTH,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html


xva-reader.pl
Description: Perl program

Re: [Qemu-block] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Wouter Verhelst]

On Tue, Nov 14, 2017 at 01:06:17PM -0600, Eric Blake wrote:
> On 11/14/2017 11:37 AM, Wouter Verhelst wrote:
> > On Tue, Nov 14, 2017 at 10:41:39AM -0600, Eric Blake wrote:
> >> Another thought - with structured replies, we finally have a way to let
> >> the client ask for the server to send resize information whenever the
> >> server wants, rather than having to be polled by a new client request
> >> all the time.  This is possible by having the server reply with a chunk
> >> without the NBD_REPLY_FLAG_DONE bit, for as many times as it wants,
> >> (that is, the server never officially ends the response to the single
> >> client request for on-going status, until the client sends an
> >> NBD_CMD_DISC).
> > 
> > Hrm, yeah, that could work.
> > 
> > Minor downside of this would be that a client would now be expected to
> > continue listening "forever" (probably needs to do a blocking read() or
> > a select() on the socket), whereas with the current situation a client
> > could get away with only reading for as long as it expects data.
> > 
> > I don't think that should be a blocker, but it might be something we
> > might want to document.
> > 
> >> I don't think the server should go into this mode without a flag bit
> >> from the client requesting it (as it potentially ties up a thread that
> >> could otherwise be used for parallel processing of other requests),
> > 
> > Yeah. I think we should probably initiate this with a BLOCK_STATUS
> > message that has a flag with which we mean "don't stop sending data on
> > the given region for contexts that support it".
> 
> Now we're mixing NBD_CMD_BLOCK_STATUS and NBD_CMD_RESIZE;

Eh, right -- I had forgotten about RESIZE, actually :-)

> I was thinking of the open-ended command for being informed of all
> server-side-initiated size changes in response to RESIZE; but your mention of
> an open-ended BLOCK_STATUS has an interesting connotation of being able to
> get live updates as sections of a file are dirtied.

For instance, or whatever other metadata we end up sending through
BLOCK_STATUS.

> I also remember from talking with Vladimir during KVM Forum last month
> that one of the shortfalls of the NBD protocol is that you can only ever
> send a length of up to 32 bits on the command side (unless we introduce
> structured commands in addition to our current work to add structured
> replies);

Yes, and I'm thinking we should do so. This will obviously require more
negotiation.

Can be done fairly easily though:
- Client negotiates structured replies (don't think it makes sense to do
  structured requests without structured replies)
- Server sets an extra transmission flag to say "I am capable of
  receiving extended requests"
- Extended requests have a different magic number, and should have a
  "request length" field as well. I'm thinking we make it:

magic  (32b)
request length (16b)
type   (16b)
flags  (64b)
handle (64b)
from   (64b)
data length(64b)
(extra data)

Request length in this proposal should always be at least 320.

I made flags 64 bits rather than 16 as per the current format, because
that way everything is aligned on a 4-byte boundary, which makes things
a bit easier on some architectures (e.g., I know that sparc doesn't like
unaligned 64-bit access). 64 bits for flags looks like a bit of a waste,
but then if we're going to waste some bits somewhere, I guess it's best
to assign them to flags.

The idea is that "request length" is the length of the data that the
client is sending, and "data length" is the length of the range that
we're trying to deal with.

A write request would thus have to have request length be (data length +
320); a read request would have request length be 320, and expect data
to be returned of data length bytes.

A metadata request could then tack on extra data, where request length
of 320 implies "all negotiated metadata contexts", but anything more
than that would imply there are some metadata IDs passed along.

etc.

Thoughts?

[...]
> > However, I could imagine that there might be some cases wherein a server
> > might be able to go into such a mode for two or more metadata contexts,
> > and where a client might want to go into that mode for one of them but
> > not all of them, while still wanting some information from them.
> > 
> > This could be covered with metadata context syntax, but it's annoying
> > and shouldn't be necessary.
> > 
> > I'm starting to think I made a mistake when I said NBD_CMD_BLOCK_STATUS
> > can't take a metadata context ID. Okay, there's no space for it, but
> > that shouldn't have been a blocker.
> > 
> > Thoughts?
> 
> Nothing says the server has to reply the same length of information when
> replying for multiple selected metadata contexts; but if we allow
> different reply sizes all in one query, we may also need some way to
> easily tell that the server has stopped sending metadata for one context
> even though it is still providing additional 

Re: [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:
> 
> 
> On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
> > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:

[...]

> >> is it covered sufficiently in live-block-operations.rst ?
> > 
> > I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
> > synchronization — drive-mirror and blockdev-mirror" section, I simply
> > seemed to declare: 
> > 
> > "Issuing the command ``block-job-cancel`` after it emits the event
> > ``BLOCK_JOB_CANCELLED``"
> > 
> > As if that's the *only* event it emits, which is clearly not the case.
> > So while at it, wonder if should I also update it
> > ('live-block-operations.rst') too.
> > 
> 
> It's an interesting gotcha that I wasn't really acutely aware of myself,
> so having it in the doc format for API programmers who aren't
> necessarily digging through our source sounds like a pleasant courtesy.

Indeed, will do.  (Just for my own clarity, did you imply: don't update
it in block-core.json?  FWIW, my first instinct is to check the QAPI
documentation for such things, that's why I wrote there first :-))

Thanks for looking.

[...]

-- 
/kashyap

Re: [Qemu-block] [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version of bdrv_release_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

11.11.2017 01:52, John Snow wrote:


On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:

It is needed to realize bdrv_dirty_bitmap_release_successor in
the following patch.


OK, but...


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/dirty-bitmap.c | 25 -
  1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 81adbeb6d4..981f99d362 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -326,13 +326,13 @@ static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap 
*bitmap)
  return !!bdrv_dirty_bitmap_name(bitmap);
  }
  
-/* Called with BQL taken.  */

-static void bdrv_do_release_matching_dirty_bitmap(
+/* Called within bdrv_dirty_bitmap_lock..unlock */

...Add this so it will compile:

__attribute__((__unused__))


ok


+static void bdrv_do_release_matching_dirty_bitmap_locked(
  BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
  bool (*cond)(BdrvDirtyBitmap *bitmap))
  {
  BdrvDirtyBitmap *bm, *next;
-bdrv_dirty_bitmaps_lock(bs);
+
  QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
  if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
  assert(!bm->active_iterators);
@@ -344,18 +344,33 @@ static void bdrv_do_release_matching_dirty_bitmap(
  g_free(bm);
  
  if (bitmap) {

-goto out;
+return;
  }
  }
  }
+
  if (bitmap) {
  abort();
  }

Do we have any style guide rules on using abort() instead of assert()?
The rest of this function uses assert, and it'd be less lines to simply
write:

assert(!bitmap);

which I think might also carry better semantic information for coverity
beyond an actual runtime conditional branch.

(I think. Please correct me if I am wrong, I'm a little hazy on this.)


agree, but it is a preexisting code, so I'll fix it with an additional 
patch.





+}
  
-out:

+/* Called with BQL taken.  */
+static void bdrv_do_release_matching_dirty_bitmap(
+BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+bool (*cond)(BdrvDirtyBitmap *bitmap))
+{
+bdrv_dirty_bitmaps_lock(bs);
+bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
  bdrv_dirty_bitmaps_unlock(bs);
  }
  
+/* Called within bdrv_dirty_bitmap_lock..unlock */

+static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap)
+{
+bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
+}
+
  /* Called with BQL taken.  */
  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {


If you agree with those two changes, you may add:


ok



Reviewed-by: John Snow 



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 for-2.11] nbd/server: Fix error reporting for bad requests

[Vladimir Sementsov-Ogievskiy]

16.11.2017 00:35, Eric Blake wrote:

The NBD spec says an attempt to NBD_CMD_TRIM on a read-only
export should fail with EPERM, as a trim has the potential
to change disk contents, but we were relying on the block
layer to catch that for us, which might not always give the
right error (and even if it does, it does not let us pass
back a sane message for structured replies).

The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of
bounds should fail with ENOSPC, not EINVAL.

Our check for u64 offset + u32 length wraparound up front is
pointless; nothing uses offset until after the second round
of sanity checks, and we can just as easily ensure there is
no wraparound by checking whether offset is in bounds (since
a disk size cannot exceed off_t which is 63 bits, adding a
32-bit number for a valid offset can't overflow).


looks like here is another problem which you've fixed:
with old code connection would be lost if this check fails in case of
CMD_WRITE, as req->complete would be false.



Solve all of these issues by some code motion and improved
request validation.

Signed-off-by: Eric Blake 
---
v2: actually commit the compiler-error fixes before submitting...

  nbd/server.c | 36 
  1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index df771fd42f..7d6801b427 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1366,15 +1366,6 @@ static int nbd_co_receive_request(NBDRequestData *req, 
NBDRequest *request,
  return -EIO;
  }

-/* Check for sanity in the parameters, part 1.  Defer as many
- * checks as possible until after reading any NBD_CMD_WRITE
- * payload, so we can try and keep the connection alive.  */


I don't understand the comment. Opposite: to keep connection alive we 
must read the payload, even in case of sanity check fail.



-if ((request->from + request->len) < request->from) {
-error_setg(errp,
-   "integer overflow detected, you're probably being 
attacked");
-return -EINVAL;
-}
-
  if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
  if (request->len > NBD_MAX_BUFFER_SIZE) {
  error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",


related idea here: if request->len > NBD_MAX_BUFFER_SIZE or if we failed 
to allocate buffer in following if,
we can call nbd_drop to read CMD_WRITE payload and set req->complete = 
true;, to keep connection in this

cases.

However, it may be done later.


@@ -1399,12 +1390,21 @@ static int nbd_co_receive_request(NBDRequestData *req, 
NBDRequest *request,
request->len);
  }

-/* Sanity checks, part 2. */
-if (request->from + request->len > client->exp->size) {
+/* Sanity checks. */
+if (client->exp->nbdflags & NBD_FLAG_READ_ONLY &&
+(request->type == NBD_CMD_WRITE ||
+ request->type == NBD_CMD_WRITE_ZEROES ||
+ request->type == NBD_CMD_TRIM)) {
+error_setg(errp, "Export is read-only");
+return -EROFS;
+}
+if (request->from > client->exp->size ||
+request->from + request->len > client->exp->size) {
  error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" 
PRIu32
 ", Size: %" PRIu64, request->from, request->len,
 (uint64_t)client->exp->size);
-return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
+return (request->type == NBD_CMD_WRITE ||
+request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL;
  }
  valid_flags = NBD_CMD_FLAG_FUA;
  if (request->type == NBD_CMD_READ && client->structured_reply) {
@@ -1482,12 +1482,6 @@ static coroutine_fn void nbd_trip(void *opaque)

  break;
  case NBD_CMD_WRITE:
-if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-error_setg(_err, "Export is read-only");
-ret = -EROFS;
-break;
-}
-
  flags = 0;
  if (request.flags & NBD_CMD_FLAG_FUA) {
  flags |= BDRV_REQ_FUA;
@@ -1500,12 +1494,6 @@ static coroutine_fn void nbd_trip(void *opaque)

  break;
  case NBD_CMD_WRITE_ZEROES:
-if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-error_setg(_err, "Export is read-only");
-ret = -EROFS;
-break;
-}
-
  flags = 0;
  if (request.flags & NBD_CMD_FLAG_FUA) {
  flags |= BDRV_REQ_FUA;



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 0/4] qmp dirty bitmap API

[Vladimir Sementsov-Ogievskiy]

16.11.2017 00:20, John Snow wrote:


On 11/13/2017 11:20 AM, Vladimir Sementsov-Ogievskiy wrote:

Hi all.

There are three qmp commands, needed to implement external backup API.

Using these three commands, client may do all needed bitmap management by
hand:

on backup start we need to do a transaction:
  {disable old bitmap, create new bitmap}

on backup success:
  drop old bitmap

on backup fail:
  enable old bitmap
  merge new bitmap to old bitmap
  drop new bitmap


Can you give me an example of how you expect these commands to be used,
and why they're required?

I'm a little weary about how error-prone these commands might be and the
potential for incorrect usage seems... high. Why do we require them?


It is needed for incremental backup. It looks like bad idea to export 
abdicate/reclaim functionality, it is simpler

and clearer to allow user to merge/enable/disable bitmaps by hand.

usage is like this:

1. we have dirty bitmap bitmap0 for incremental backup.

2. prepare image fleecing (create temporary image with backing=our_disk)
3. in qmp transaction:
    - disable bitmap0
    - create bitmap1
    - start image fleecing (backup sync=none our_disk -> temp_disk)
4. export temp_disk (it looks like a snapshot) and bitmap0 through NBD

=== external tool can get bitmap0 and then copy some clusters through 
NBD ===


on successful backup external tool can drop bitmap0. or can save it and 
reuse somehow


on failed backup external tool can do the following:
    - enable bitmap0
    - merge bitmap1 to bitmap0
    - drop bitmap1

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 1/4 for-2.11?] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

13.11.2017 20:50, Eric Blake wrote:

On 11/13/2017 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:

Like other setters here these functions should take a lock.

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

Should this patch be in 2.11?



these functions are unused now, so, no, it's not necessary




diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bd04e991b1..2a0bcd9e51 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -397,15 +397,19 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
  /* Called with BQL taken.  */
  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
  {
+bdrv_dirty_bitmap_lock(bitmap);
  assert(!bdrv_dirty_bitmap_frozen(bitmap));
  bitmap->disabled = true;
+bdrv_dirty_bitmap_unlock(bitmap);

Why do we need this lock in addition to BQL?


  }
  
  /* Called with BQL taken.  */

  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
  {
+bdrv_dirty_bitmap_lock(bitmap);
  assert(!bdrv_dirty_bitmap_frozen(bitmap));
  bitmap->disabled = false;
+bdrv_dirty_bitmap_unlock(bitmap);

Again, why do we need this in addition to BQL?

The commit message needs more details about a scenario where our
existing BQL lock is insufficient to prevent misuse of bitmap->disabled.




--
Best regards,
Vladimir