Re: [Qemu-block] [PATCH v3 04/16] block/mirror: Pull out mirror_perform()

[Jeff Cody]

On Wed, Feb 28, 2018 at 07:04:55PM +0100, Max Reitz wrote:
> When converting mirror's I/O to coroutines, we are going to need a point
> where these coroutines are created.  mirror_perform() is going to be
> that point.
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/mirror.c | 51 +--
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f5bf620942..d197c8936e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -82,6 +82,12 @@ typedef struct MirrorOp {
>  uint64_t bytes;
>  } MirrorOp;
>  
> +typedef enum MirrorMethod {
> +MIRROR_METHOD_COPY,
> +MIRROR_METHOD_ZERO,
> +MIRROR_METHOD_DISCARD,
> +} MirrorMethod;
> +
>  static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
>  int error)
>  {
> @@ -321,6 +327,22 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>  }
>  }
>  
> +static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
> +   unsigned bytes, MirrorMethod mirror_method)
> +{
> +switch (mirror_method) {
> +case MIRROR_METHOD_COPY:
> +return mirror_do_read(s, offset, bytes);
> +case MIRROR_METHOD_ZERO:
> +case MIRROR_METHOD_DISCARD:
> +mirror_do_zero_or_discard(s, offset, bytes,
> +  mirror_method == MIRROR_METHOD_DISCARD);
> +return bytes;
> +default:
> +abort();
> +}
> +}
> +
>  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  {
>  BlockDriverState *source = s->source;
> @@ -387,11 +409,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  int ret;
>  int64_t io_bytes;
>  int64_t io_bytes_acct;
> -enum MirrorMethod {
> -MIRROR_METHOD_COPY,
> -MIRROR_METHOD_ZERO,
> -MIRROR_METHOD_DISCARD
> -} mirror_method = MIRROR_METHOD_COPY;
> +MirrorMethod mirror_method = MIRROR_METHOD_COPY;
>  
>  assert(!(offset % s->granularity));
>  ret = bdrv_block_status_above(source, NULL, offset,
> @@ -429,22 +447,11 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  }
>  
>  io_bytes = mirror_clip_bytes(s, offset, io_bytes);
> -switch (mirror_method) {
> -case MIRROR_METHOD_COPY:
> -io_bytes = io_bytes_acct = mirror_do_read(s, offset, io_bytes);
> -break;
> -case MIRROR_METHOD_ZERO:
> -case MIRROR_METHOD_DISCARD:
> -mirror_do_zero_or_discard(s, offset, io_bytes,
> -  mirror_method == 
> MIRROR_METHOD_DISCARD);
> -if (write_zeroes_ok) {
> -io_bytes_acct = 0;
> -} else {
> -io_bytes_acct = io_bytes;
> -}
> -break;
> -default:
> -abort();
> +io_bytes = mirror_perform(s, offset, io_bytes, mirror_method);
> +if (mirror_method != MIRROR_METHOD_COPY && write_zeroes_ok) {
> +io_bytes_acct = 0;
> +} else {
> +io_bytes_acct = io_bytes;
>  }
>  assert(io_bytes);
>  offset += io_bytes;
> @@ -638,7 +645,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
> *s)
>  continue;
>  }
>  
> -mirror_do_zero_or_discard(s, offset, bytes, false);
> +mirror_perform(s, offset, bytes, MIRROR_METHOD_ZERO);
>  offset += bytes;
>  }
>  
> -- 
> 2.14.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH v3 02/16] block: BDS deletion in bdrv_do_drained_begin()

[Jeff Cody]

On Wed, Feb 28, 2018 at 07:04:53PM +0100, Max Reitz wrote:
> Draining a BDS (in the main loop) may cause it to go be deleted.  That
> is rather suboptimal if we still plan to access it afterwards, so let us
> enclose the main body of the function with a bdrv_ref()/bdrv_unref()
> pair.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/io.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index ead12c4136..3b61e26114 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -294,12 +294,27 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool 
> recursive,
> BdrvChild *parent)
>  {
>  BdrvChild *child, *next;
> +bool in_main_loop =
> +qemu_get_current_aio_context() == qemu_get_aio_context();
> +/* bdrv_close() invokes bdrv_drain() with bs->refcnt == 0; then,
> + * we may not invoke bdrv_ref()/bdrv_unref() because the latter
> + * would result in the refcount going back to 0, creating an
> + * infinite loop.
> + * Also, we have to be in the main loop because we may not call
> + * bdrv_unref() elsewhere.  But because of that, the BDS is not in
> + * danger of going away without the bdrv_ref()/bdrv_unref() pair
> + * elsewhere, so we are fine then. */
> +bool add_ref = in_main_loop && bs->refcnt > 0;
>  
>  if (qemu_in_coroutine()) {
>  bdrv_co_yield_to_drain(bs, true, recursive, parent);
>  return;
>  }
>  
> +if (add_ref) {
> +bdrv_ref(bs);
> +}
> +
>  /* Stop things in parent-to-child order */
>  if (atomic_fetch_inc(>quiesce_counter) == 0) {
>  aio_disable_external(bdrv_get_aio_context(bs));
> @@ -315,6 +330,10 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool 
> recursive,
>  bdrv_do_drained_begin(child->bs, true, child);
>  }
>  }
> +
> +if (add_ref) {
> +bdrv_unref(bs);
> +}
>  }
>  
>  void bdrv_drained_begin(BlockDriverState *bs)
> -- 
> 2.14.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH v3 01/16] block: BDS deletion during bdrv_drain_recurse

[Jeff Cody]

On Wed, Feb 28, 2018 at 07:04:52PM +0100, Max Reitz wrote:
> Draining a BDS child may lead to other children of the same parent being
> detached and/or deleted.  We should prepare for the former case (by
> copying the children list before iterating through it) and prevent the
> latter (by bdrv_ref()'ing all nodes if we are in the main loop).
> 
> Signed-off-by: Max Reitz 
> ---
>  block/io.c | 40 ++--
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 4d3d1f640a..ead12c4136 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -189,31 +189,51 @@ static void bdrv_drain_invoke(BlockDriverState *bs, 
> bool begin, bool recursive)
>  
>  static bool bdrv_drain_recurse(BlockDriverState *bs)
>  {
> -BdrvChild *child, *tmp;
> +BdrvChild *child;
>  bool waited;
> +struct BDSToDrain {
> +BlockDriverState *bs;
> +QLIST_ENTRY(BDSToDrain) next;
> +};
> +QLIST_HEAD(, BDSToDrain) bs_list = QLIST_HEAD_INITIALIZER(bs_list);
> +bool in_main_loop =
> +qemu_get_current_aio_context() == qemu_get_aio_context();
>  
>  /* Wait for drained requests to finish */
>  waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
>  
> -QLIST_FOREACH_SAFE(child, >children, next, tmp) {
> -BlockDriverState *bs = child->bs;
> -bool in_main_loop =
> -qemu_get_current_aio_context() == qemu_get_aio_context();
> -assert(bs->refcnt > 0);
> +/* Draining children may result in other children being removed from this
> + * parent and maybe even deleted, so copy the children list first */
> +QLIST_FOREACH(child, >children, next) {
> +struct BDSToDrain *bs2d = g_new0(struct BDSToDrain, 1);
> +
> +bs2d->bs = child->bs;
>  if (in_main_loop) {
>  /* In case the recursive bdrv_drain_recurse processes a
>   * block_job_defer_to_main_loop BH and modifies the graph,
> - * let's hold a reference to bs until we are done.
> + * let's hold a reference to the BDS until we are done.
>   *
>   * IOThread doesn't have such a BH, and it is not safe to call
>   * bdrv_unref without BQL, so skip doing it there.
>   */
> -bdrv_ref(bs);
> +bdrv_ref(bs2d->bs);
>  }
> -waited |= bdrv_drain_recurse(bs);
> +
> +QLIST_INSERT_HEAD(_list, bs2d, next);
> +}
> +
> +while (!QLIST_EMPTY(_list)) {
> +struct BDSToDrain *bs2d = QLIST_FIRST(_list);
> +QLIST_REMOVE(bs2d, next);
> +
> +assert(bs2d->bs->refcnt > 0);
> +
> +waited |= bdrv_drain_recurse(bs2d->bs);
>  if (in_main_loop) {
> -bdrv_unref(bs);
> +bdrv_unref(bs2d->bs);
>  }
> +
> +g_free(bs2d);
>  }
>  
>  return waited;
> -- 
> 2.14.3
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH v3 2/7] qapi: Add qobject_to()

[Max Reitz]

On 2018-03-19 20:36, Eric Blake wrote:
> On 02/26/2018 05:58 AM, Max Reitz wrote:
>> On 2018-02-24 21:57, Eric Blake wrote:
>>> On 02/24/2018 09:40 AM, Max Reitz wrote:
 This is a dynamic casting macro that, given a QObject type, returns an
 object as that type or NULL if the object is of a different type (or
 NULL itself).

> 
 +#define qobject_to(obj, type) \
 +    container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type))
 ?: \
 + QOBJECT((type *)NULL), \
>>>
>>> I guess the third (second?) branch of the ternary is written this way,
>>> rather than the simpler 'NULL', to ensure that 'type' is still something
>>> that can have the QOBJECT() macro applied to it?  Should be okay.
>>
>> It's written this way because of the container_of() around it.  We want
>> the whole expression to return NULL then, and without the QOBJECT()
>> around it, it would only return NULL if offsetof(type, base) == 0 (which
>> it is not necessarily).
>>
>> OTOH, container_of(&((type *)NULL)->base, type, base) is by definition
>> NULL.
>>
>> (QOBJECT(x) is &(x)->base)
> 
> Well, clang's ubsan griped:
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html
> 
> Practically, all of our qtypes have 'base' at offset 0, which means
> (QObject*)addr and (QString*)addr are the same address, even when addr
> is NULL.  But neither QOBJECT() nor container_of() are currently fit to
> run on a NULL pointer, since the 'base' member need not be at offset 0,
> at which point, we'd be converting away from the NULL pointer on the
> &(x)->base conversion, and then back to the NULL pointer on the
> container_of() conversion.  So at the end of the day, we get the right
> results, but we relied on undefined behavior in the interim.
> 
> So here's what I'm squashing in, if you like it (and remembering that I
> already swapped argument order to be qobject_to(type, obj) in my pending
> pull requests):
> 
> diff --git i/include/qapi/qmp/qobject.h w/include/qapi/qmp/qobject.h
> index ea9702270e7..e6ce9347ab8 100644
> --- i/include/qapi/qmp/qobject.h
> +++ w/include/qapi/qmp/qobject.h
> @@ -62,9 +62,8 @@ QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
>     "The QTYPE_CAST_TO_* list needs to be extended");
> 
>  #define qobject_to(type, obj) \
> -    container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \
> - QOBJECT((type *)NULL), \
> - type, base)
> +    ({ QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_,
> type)); \
> +    _tmp ? container_of(_tmp, type, base) : (type *)NULL; })
> 
>  /* Initialize an object to default values */
>  static inline void qobject_init(QObject *obj, QType type)

Yes, that looks good.  Thanks!

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/7] qapi: Add qobject_to()

[Eric Blake]

On 02/26/2018 05:58 AM, Max Reitz wrote:

On 2018-02-24 21:57, Eric Blake wrote:

On 02/24/2018 09:40 AM, Max Reitz wrote:

This is a dynamic casting macro that, given a QObject type, returns an
object as that type or NULL if the object is of a different type (or
NULL itself).




+#define qobject_to(obj, type) \
+    container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type))
?: \
+ QOBJECT((type *)NULL), \


I guess the third (second?) branch of the ternary is written this way,
rather than the simpler 'NULL', to ensure that 'type' is still something
that can have the QOBJECT() macro applied to it?  Should be okay.


It's written this way because of the container_of() around it.  We want
the whole expression to return NULL then, and without the QOBJECT()
around it, it would only return NULL if offsetof(type, base) == 0 (which
it is not necessarily).

OTOH, container_of(&((type *)NULL)->base, type, base) is by definition NULL.

(QOBJECT(x) is &(x)->base)


Well, clang's ubsan griped:
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html

Practically, all of our qtypes have 'base' at offset 0, which means 
(QObject*)addr and (QString*)addr are the same address, even when addr 
is NULL.  But neither QOBJECT() nor container_of() are currently fit to 
run on a NULL pointer, since the 'base' member need not be at offset 0, 
at which point, we'd be converting away from the NULL pointer on the 
&(x)->base conversion, and then back to the NULL pointer on the 
container_of() conversion.  So at the end of the day, we get the right 
results, but we relied on undefined behavior in the interim.


So here's what I'm squashing in, if you like it (and remembering that I 
already swapped argument order to be qobject_to(type, obj) in my pending 
pull requests):


diff --git i/include/qapi/qmp/qobject.h w/include/qapi/qmp/qobject.h
index ea9702270e7..e6ce9347ab8 100644
--- i/include/qapi/qmp/qobject.h
+++ w/include/qapi/qmp/qobject.h
@@ -62,9 +62,8 @@ QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
"The QTYPE_CAST_TO_* list needs to be extended");

 #define qobject_to(type, obj) \
-container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \
- QOBJECT((type *)NULL), \
- type, base)
+({ QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, 
type)); \

+_tmp ? container_of(_tmp, type, base) : (type *)NULL; })

 /* Initialize an object to default values */
 static inline void qobject_init(QObject *obj, QType type)


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

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix test case 185

[Christian Borntraeger]

On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
> On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao  
> wrote:
>> Test case 185 failed since commit 4486e89c219 --- "vl: introduce 
>> vm_shutdown()".
>> It's because of the newly introduced function vm_shutdown calls 
>> bdrv_drain_all,
>> which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
>> that doubles the speed and offset is doubled.
>> Some jobs' status are changed as well.
>>
>> Thus, let's not call bdrv_drain_all in vm_shutdown.
>>
>> Signed-off-by: QingFeng Hao 
>> ---
>>  cpus.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 2e6701795b..ae2962508c 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
>>  qapi_event_send_stop(_abort);
>>  }
>>  }
>> -
>> -bdrv_drain_all();
>> +if (send_stop) {
>> +bdrv_drain_all();
>> +}
> 
> Thanks for looking into this bug!
> 
> This if statement breaks the contract of the function when send_stop
> is false.  Drain ensures that pending I/O completes and that device
> emulation finishes before this function returns.  This is important
> for live migration, where there must be no more guest-related activity
> after vm_stop().
> 
> This patch relies on the caller invoking bdrv_close_all() immediately
> after do_vm_stop(), which is not documented and could lead to more
> bugs in the future.
> 
> I suggest a different solution:
> 
> Test 185 relies on internal QEMU behavior which can change from time
> to time.  Buffer sizes and block job iteration counts are
> implementation details that are not part of qapi-schema.json or other
> documented behavior.
> 
> In fact, the existing test case was already broken in IOThread mode
> since iothread_stop_all() -> bdrv_set_aio_context() also includes a
> bdrv_drain() with the side-effect of an extra blockjob iteration.
> 
> After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
> non-IOThread mode perform the drain.  This has caused the test
> failure.
> 
> Instead of modifying QEMU to keep the same arbitrary internal behavior
> as before, please send a patch that updates tests/qemu-iotests/185.out
> with the latest output.

Wouldnt it be better if the test actually masks out the values the are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset from
common.filter be what we want?

[Qemu-block] [PULL v3 07/38] qapi: Replace qobject_to_X(o) by qobject_to(X, o)

[Eric Blake]

From: Max Reitz 

This patch was generated using the following Coccinelle script:

@@
expression Obj;
@@
(
- qobject_to_qnum(Obj)
+ qobject_to(QNum, Obj)
|
- qobject_to_qstring(Obj)
+ qobject_to(QString, Obj)
|
- qobject_to_qdict(Obj)
+ qobject_to(QDict, Obj)
|
- qobject_to_qlist(Obj)
+ qobject_to(QList, Obj)
|
- qobject_to_qbool(Obj)
+ qobject_to(QBool, Obj)
)

and a bit of manual fix-up for overly long lines and three places in
tests/check-qjson.c that Coccinelle did not find.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Message-Id: <20180224154033.29559-4-mre...@redhat.com>
Reviewed-by: Eric Blake 
[eblake: swap order from qobject_to(o, X), rebase to master, also a fix
to latent false-positive compiler complaint about hw/i386/acpi-build.c]
Signed-off-by: Eric Blake 
---
 tests/libqtest.c|  6 ++---
 block.c |  4 +--
 block/parallels.c   |  2 +-
 block/qapi.c| 12 -
 block/qcow.c|  2 +-
 block/qcow2.c   |  2 +-
 block/qed.c |  2 +-
 block/rbd.c |  8 +++---
 block/sheepdog.c|  2 +-
 block/vhdx.c|  2 +-
 block/vpc.c |  2 +-
 blockdev.c  |  7 ++---
 hw/i386/acpi-build.c| 16 +--
 monitor.c   |  8 +++---
 qapi/qmp-dispatch.c |  2 +-
 qapi/qobject-input-visitor.c| 20 +++---
 qapi/qobject-output-visitor.c   |  4 +--
 qga/main.c  |  2 +-
 qmp.c   |  2 +-
 qobject/json-parser.c   |  2 +-
 qobject/qbool.c |  4 +--
 qobject/qdict.c | 38 +-
 qobject/qjson.c | 10 +++
 qobject/qlist.c |  6 ++---
 qobject/qlit.c  | 10 +++
 qobject/qnum.c  |  6 ++---
 qobject/qstring.c   |  6 ++---
 qom/object.c|  8 +++---
 target/i386/cpu.c   |  2 +-
 target/s390x/cpu_models.c   |  2 +-
 tests/check-qdict.c | 20 +++---
 tests/check-qjson.c | 41 ++--
 tests/check-qlist.c |  4 +--
 tests/check-qlit.c  | 10 +++
 tests/check-qnum.c  |  4 +--
 tests/check-qobject.c   |  2 +-
 tests/check-qstring.c   |  2 +-
 tests/device-introspect-test.c  | 14 +-
 tests/numa-test.c   |  8 +++---
 tests/qom-test.c|  4 +--
 tests/test-char.c   |  2 +-
 tests/test-keyval.c |  8 +++---
 tests/test-qga.c| 19 ++---
 tests/test-qmp-cmds.c   | 12 -
 tests/test-qmp-event.c  | 16 +--
 tests/test-qobject-input-visitor.c  | 10 +++
 tests/test-qobject-output-visitor.c | 54 ++---
 tests/test-x86-cpuid-compat.c   | 17 ++--
 util/keyval.c   |  4 +--
 util/qemu-config.c  |  2 +-
 util/qemu-option.c  |  6 ++---
 51 files changed, 231 insertions(+), 227 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 13c910069b5..200b2b9e92a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -430,7 +430,7 @@ static void qmp_response(JSONMessageParser *parser, GQueue 
*tokens)
 }

 g_assert(!qmp->response);
-qmp->response = qobject_to_qdict(obj);
+qmp->response = qobject_to(QDict, obj);
 g_assert(qmp->response);
 }

@@ -1008,11 +1008,11 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
*machine))
 g_assert(list);

 for (p = qlist_first(list); p; p = qlist_next(p)) {
-minfo = qobject_to_qdict(qlist_entry_obj(p));
+minfo = qobject_to(QDict, qlist_entry_obj(p));
 g_assert(minfo);
 qobj = qdict_get(minfo, "name");
 g_assert(qobj);
-qstr = qobject_to_qstring(qobj);
+qstr = qobject_to(QString, qobj);
 g_assert(qstr);
 mname = qstring_get_str(qstr);
 cb(mname);
diff --git a/block.c b/block.c
index f7f9d8eca74..fd33d5ec43f 100644
--- a/block.c
+++ b/block.c
@@ -1457,7 +1457,7 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 return NULL;
 }

-options = qobject_to_qdict(options_obj);
+options = qobject_to(QDict, options_obj);
 if (!options) {
 qobject_decref(options_obj);
 error_setg(errp, "Invalid JSON object given");
@@ -2433,7 +2433,7 @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef 
*ref, Error **errp)
 }
 visit_complete(v, );

-qdict = 

Re: [Qemu-block] [Qemu-devel] [PATCH v4] file-posix: specify expected filetypes

[John Snow]

On 03/19/2018 11:29 AM, Kevin Wolf wrote:
> Am 13.03.2018 um 18:20 hat John Snow geschrieben:
>>
>>
>> On 01/19/2018 06:03 PM, Eric Blake wrote:
>>> On 01/19/2018 04:47 PM, John Snow wrote:
 Adjust each caller of raw_open_common to specify if they are expecting
 host and character devices or not. Tighten expectations of file types upon
 open in the common code and refuse types that are not expected.

 This has two effects:

 (1) Character and block devices are now considered deprecated for the
 'file' driver, which expects only S_IFREG, and
 (2) no file-posix driver (file, host_cdrom, or host_device) can open
 directories now.

 I don't think there's a legitimate reason to open directories as if
 they were files. This prevents QEMU from opening and attempting to probe
 a directory inode, which can break in exciting ways. One of those ways
 is lseek on ext4/xfs, which will return 0x7fff as the file
 size instead of EISDIR. This can coax QEMU into responding with a
 confusing "file too big" instead of "Hey, that's not a file".

 See: https://bugs.launchpad.net/qemu/+bug/1739304/
 Signed-off-by: John Snow 
 ---
>>>
>>> Reviewed-by: Eric Blake 
>>
>> Whoops, I let this one rot. It could still be considered a bugfix for
>> next week.
> 
> Yes, we should take this as a bugfix. Needs a rebase, though.
> 
> Kevin
> 

You got it.

--js

Re: [Qemu-block] [PATCH v1 1/1] iotests: fix test case 185

[Stefan Hajnoczi]

On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao  wrote:
> Test case 185 failed since commit 4486e89c219 --- "vl: introduce 
> vm_shutdown()".
> It's because of the newly introduced function vm_shutdown calls 
> bdrv_drain_all,
> which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
> that doubles the speed and offset is doubled.
> Some jobs' status are changed as well.
>
> Thus, let's not call bdrv_drain_all in vm_shutdown.
>
> Signed-off-by: QingFeng Hao 
> ---
>  cpus.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 2e6701795b..ae2962508c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
>  qapi_event_send_stop(_abort);
>  }
>  }
> -
> -bdrv_drain_all();
> +if (send_stop) {
> +bdrv_drain_all();
> +}

Thanks for looking into this bug!

This if statement breaks the contract of the function when send_stop
is false.  Drain ensures that pending I/O completes and that device
emulation finishes before this function returns.  This is important
for live migration, where there must be no more guest-related activity
after vm_stop().

This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.

I suggest a different solution:

Test 185 relies on internal QEMU behavior which can change from time
to time.  Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.

In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.

After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain.  This has caused the test
failure.

Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.

Stefan

Re: [Qemu-block] [PATCH] iotests: 163 is not quick

[Kevin Wolf]

Am 10.03.2018 um 22:45 hat Eric Blake geschrieben:
> Testing on ext4, most 'quick' qcow2 tests took less than 5 seconds,
> but 163 took more than 20.  Let's remove it from the quick set.
> 
> Signed-off-by: Eric Blake 

Takes only 11 seconds for me, but that's still longer than most other
tests.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v4] file-posix: specify expected filetypes

[Kevin Wolf]

Am 13.03.2018 um 18:20 hat John Snow geschrieben:
> 
> 
> On 01/19/2018 06:03 PM, Eric Blake wrote:
> > On 01/19/2018 04:47 PM, John Snow wrote:
> >> Adjust each caller of raw_open_common to specify if they are expecting
> >> host and character devices or not. Tighten expectations of file types upon
> >> open in the common code and refuse types that are not expected.
> >>
> >> This has two effects:
> >>
> >> (1) Character and block devices are now considered deprecated for the
> >> 'file' driver, which expects only S_IFREG, and
> >> (2) no file-posix driver (file, host_cdrom, or host_device) can open
> >> directories now.
> >>
> >> I don't think there's a legitimate reason to open directories as if
> >> they were files. This prevents QEMU from opening and attempting to probe
> >> a directory inode, which can break in exciting ways. One of those ways
> >> is lseek on ext4/xfs, which will return 0x7fff as the file
> >> size instead of EISDIR. This can coax QEMU into responding with a
> >> confusing "file too big" instead of "Hey, that's not a file".
> >>
> >> See: https://bugs.launchpad.net/qemu/+bug/1739304/
> >> Signed-off-by: John Snow 
> >> ---
> > 
> > Reviewed-by: Eric Blake 
> 
> Whoops, I let this one rot. It could still be considered a bugfix for
> next week.

Yes, we should take this as a bugfix. Needs a rebase, though.

Kevin

Re: [Qemu-block] [PULL v3 00/46] Block layer patches

[Peter Maydell]

On 19 March 2018 at 11:04, Kevin Wolf  wrote:
> The following changes since commit e1e44a9916b4318e943aecd669e096222cb3eaeb:
>
>   Merge remote-tracking branch 'remotes/xtensa/tags/20180316-xtensa' into 
> staging (2018-03-17 14:15:03 +)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 63ca8406beac44aa59c389ed8578d0c7b3da3402:
>
>   iotests: Avoid realpath, for CentOS 6 (2018-03-19 12:01:39 +0100)
>
> 
> Block layer patches
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH v8 5/9] block: treat BDRV_REQ_ALLOCATE as serialising

[Alberto Garcia]

On Mon 12 Mar 2018 11:16:54 AM CET, Anton Nefedov wrote:
> The idea is that ALLOCATE requests may overlap with other requests.
> Reuse the existing block layer infrastructure for serialising requests.
> Use the following approach:
>   - mark ALLOCATE serialising, so subsequent requests to the area wait
>   - ALLOCATE request itself must never wait if another request is in flight
> already. Return EAGAIN, let the caller reconsider.
>
> Signed-off-by: Anton Nefedov 

Reviewed-by: Alberto Garcia 

> @@ -1498,8 +1507,13 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
> *child,
>  max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, 
> INT_MAX),
> align);
>  
> -waited = wait_serialising_requests(req);
> -assert(!waited || !req->serialising);
> +found = find_or_wait_serialising_requests(req,
> +  !(flags & BDRV_REQ_ALLOCATE));
> +if (found && (flags & BDRV_REQ_ALLOCATE)) {
> +return -EAGAIN;
> +}
> +

Another alternative (perhaps a bit more readable):

if (flags & BDRV_REQ_ALLOCATE) {
if (find_or_wait_serialising_requests(req, false)) {
return -EAGAIN;
}
} else {
bool found = wait_serialising_requests(req);
assert(!found || !req->serialising);
}

but yours is fine, so keep it if you prefer.

Berto

[Qemu-block] [PULL v3 00/46] Block layer patches

[Kevin Wolf]

The following changes since commit e1e44a9916b4318e943aecd669e096222cb3eaeb:

  Merge remote-tracking branch 'remotes/xtensa/tags/20180316-xtensa' into 
staging (2018-03-17 14:15:03 +)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 63ca8406beac44aa59c389ed8578d0c7b3da3402:

  iotests: Avoid realpath, for CentOS 6 (2018-03-19 12:01:39 +0100)


Block layer patches


Eric Blake (1):
  iotests: Avoid realpath, for CentOS 6

Fam Zheng (4):
  block: Fix flags in reopen queue
  iotests: Add regression test for commit base locking
  vvfat: Fix inherit_options flags
  block: Fix leak of ignore_children in error path

Jeff Cody (1):
  block: fix iotest 146 output expectations

John Snow (21):
  blockjobs: fix set-speed kick
  blockjobs: model single jobs as transactions
  Blockjobs: documentation touchup
  blockjobs: add status enum
  blockjobs: add state transition table
  iotests: add pause_wait
  blockjobs: add block_job_verb permission table
  blockjobs: add ABORTING state
  blockjobs: add CONCLUDED state
  blockjobs: add NULL state
  blockjobs: add block_job_dismiss
  blockjobs: ensure abort is called for cancelled jobs
  blockjobs: add commit, abort, clean helpers
  blockjobs: add block_job_txn_apply function
  blockjobs: add prepare callback
  blockjobs: add waiting status
  blockjobs: add PENDING status and event
  blockjobs: add block-job-finalize
  blockjobs: Expose manual property
  iotests: test manual job dismissal
  tests/test-blockjob: test cancellations

Kevin Wolf (14):
  luks: Separate image file creation from formatting
  luks: Create block_crypto_co_create_generic()
  luks: Support .bdrv_co_create
  luks: Turn invalid assertion into check
  luks: Catch integer overflow for huge sizes
  qemu-iotests: Test luks QMP image creation
  parallels: Support .bdrv_co_create
  qemu-iotests: Enable write tests for parallels
  qcow: Support .bdrv_co_create
  qed: Support .bdrv_co_create
  vdi: Make comments consistent with other drivers
  vhdx: Support .bdrv_co_create
  vpc: Support .bdrv_co_create
  vpc: Require aligned size in .bdrv_co_create

Liang Li (1):
  block/mirror: change the semantic of 'force' of block-job-cancel

Max Reitz (3):
  vdi: Pull option parsing from vdi_co_create
  vdi: Move file creation to vdi_co_create_opts
  vdi: Implement .bdrv_co_create

Paolo Bonzini (1):
  iscsi: fix iSER compilation

 qapi/block-core.json  | 363 --
 include/block/blockjob.h  |  71 -
 include/block/blockjob_int.h  |  17 +-
 block.c   |  10 +-
 block/backup.c|   5 +-
 block/commit.c|   2 +-
 block/crypto.c| 150 -
 block/iscsi.c |   2 +-
 block/mirror.c|  12 +-
 block/parallels.c | 199 +--
 block/qcow.c  | 196 +++
 block/qed.c   | 204 
 block/stream.c|   2 +-
 block/vdi.c   | 147 +
 block/vhdx.c  | 216 +++--
 block/vpc.c   | 241 +---
 block/vvfat.c |   2 +-
 blockdev.c|  71 +++--
 blockjob.c| 358 +++--
 tests/test-bdrv-drain.c   |   5 +-
 tests/test-blockjob-txn.c |  27 ++--
 tests/test-blockjob.c | 233 ++-
 block/trace-events|   7 +
 hmp-commands.hx   |   3 +-
 tests/qemu-iotests/030|   6 +-
 tests/qemu-iotests/055|  17 +-
 tests/qemu-iotests/056| 187 ++
 tests/qemu-iotests/056.out|   4 +-
 tests/qemu-iotests/109.out|  24 +--
 tests/qemu-iotests/146.out|   2 +-
 tests/qemu-iotests/153|  12 ++
 tests/qemu-iotests/153.out|   5 +
 tests/qemu-iotests/181|   2 +-
 tests/qemu-iotests/210| 210 
 tests/qemu-iotests/210.out| 136 
 tests/qemu-iotests/check  |  13 +-
 tests/qemu-iotests/common.rc  |   2 +-
 tests/qemu-iotests/group  |   1 +
 tests/qemu-iotests/iotests.py |  12 +-
 39 files changed, 2652 insertions(+), 524 deletions(-)
 create mode 100755 tests/qemu-iotests/210
 create mode 100644 tests/qemu-iotests/210.out

[Qemu-block] [PATCH v1 0/1] iotests: fix test case 185

[QingFeng Hao]

Hi,
This patch is to remove the redundant call to bdrv_drain_all in vm_shutdown.
Thanks!

Test case 185 failed as below:
185 2s ... - output mismatch (see 185.out.bad)
--- /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out 
2018-03-09 01:00:40.451603189 +0100
+++ /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out.bad 
2018-03-19 09:40:22.781603189 +0100
@@ -20,7 +20,7 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
524288, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
1048576, "speed": 65536, "type": "commit"}}

 === Start active commit job and exit qemu ===

@@ -28,7 +28,8 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "commit"}}

 === Start mirror job and exit qemu ===

@@ -37,7 +38,8 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 
4194304, "speed": 65536, "type": "mirror"}}

 === Start backup job and exit qemu ===

@@ -46,7 +48,7 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
65536, "speed": 65536, "type": "backup"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
131072, "speed": 65536, "type": "backup"}}

 === Start streaming job and exit qemu ===

@@ -54,6 +56,6 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
524288, "speed": 65536, "type": "stream"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 
1048576, "speed": 65536, "type": "stream"}}
 No errors were found on the image.
 *** done
Failures: 185
Failed 1 of 1 tests

QingFeng Hao (1):
  iotests: fix test case 185

 cpus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.13.5

[Qemu-block] [PATCH v1 1/1] iotests: fix test case 185

[QingFeng Hao]

Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.

Thus, let's not call bdrv_drain_all in vm_shutdown.

Signed-off-by: QingFeng Hao 
---
 cpus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
 qapi_event_send_stop(_abort);
 }
 }
-
-bdrv_drain_all();
+if (send_stop) {
+bdrv_drain_all();
+}
 replay_disable_events();
 ret = bdrv_flush_all();
 
-- 
2.13.5

[Qemu-block] [PATCH v3 for 2.12 0/3] fix bitmaps migration through shared storage

[Vladimir Sementsov-Ogievskiy]

Hi all.

This fixes bitmaps migration through shared storage. Look at 02 for
details.

The bug introduced in 2.10 with the whole qcow2 bitmaps feature, so
qemu-stable in CC. However I doubt that someone really suffered from this.

Do we need dirty bitmaps at all in inactive case? - that was a question in v2.
And, keeping in mind that we are going to use inactive mode not only for
incoming migration, I'm not sure that answer is NO (but, it may be "NO" for 
2.10, 2.11), so let's fix it in proposed here manner at least for 2.12.

v3: tiny context changes in 01,02
03: instead of a separate test, enable corresponding case in existent one

v2:
   John, thank you for reviewing v1.
   changes:
add John's r-bs, change s/timeout=10/timeout=10.0/ in last patch
and drop old 03 patch, related to this timeout fix.

Vladimir Sementsov-Ogievskiy (3):
  qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
  qcow2: handle reopening bitmaps on bdrv_invalidate_cache
  iotests: enable shared migration cases in 169

 block/qcow2.h  |  2 ++
 block/qcow2-bitmap.c   | 15 ++-
 block/qcow2.c  |  8 +++-
 tests/qemu-iotests/169 |  8 +++-
 4 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.11.1

[Qemu-block] [PATCH v3 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

Add version of qcow2_reopen_bitmaps_rw, which do the same work but
also return a hint about was header updated or not. This will be
used in the following fix for bitmaps reloading after migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 block/qcow2.h|  2 ++
 block/qcow2-bitmap.c | 15 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ccb92a9696..d301f77cea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -671,6 +671,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   void **refcount_table,
   int64_t *refcount_table_size);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3010adb909..6e93ec43e1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1004,7 +1004,8 @@ fail:
 return false;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+ Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2BitmapList *bm_list;
@@ -1012,6 +1013,10 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 GSList *ro_dirty_bitmaps = NULL;
 int ret = 0;
 
+if (header_updated != NULL) {
+*header_updated = false;
+}
+
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
@@ -1055,6 +1060,9 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 error_setg_errno(errp, -ret, "Can't update bitmap directory");
 goto out;
 }
+if (header_updated != NULL) {
+*header_updated = true;
+}
 g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
@@ -1065,6 +1073,11 @@ out:
 return ret;
 }
 
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+{
+return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
-- 
2.11.1

[Qemu-block] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Vladimir Sementsov-Ogievskiy]

Shared migration for dirty bitmaps is fixed by previous patches,
so we can enable the test.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/169 | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 3a8db91f6f..153b10b6e7 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -140,16 +140,14 @@ def inject_test_case(klass, name, method, *args, 
**kwargs):
 mc = operator.methodcaller(method, *args, **kwargs)
 setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
 
-for cmb in list(itertools.product((True, False), repeat=3)):
+for cmb in list(itertools.product((True, False), repeat=4)):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
 name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
 name += '_online' if cmb[2] else '_offline'
-
-# TODO fix shared-storage bitmap migration and enable cases for it
-args = list(cmb) + [False]
+name += '_shared' if cmb[3] else '_nonshared'
 
 inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *args)
+ *list(cmb))
 
 
 if __name__ == '__main__':
-- 
2.11.1

[Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

Consider migration with shared storage. Persistent bitmaps are stored
on bdrv_inactivate. Then, on destination
process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
are already loaded on destination start. In this case we should call
qcow2_reopen_bitmaps_rw instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 block/qcow2.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..6219666d4a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
 }
 
-if (qcow2_load_dirty_bitmaps(bs, _err)) {
+if (bdrv_has_readonly_bitmaps(bs)) {
+if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) 
{
+bool header_updated = false;
+qcow2_reopen_bitmaps_rw_hint(bs, _updated, _err);
+update_header = update_header && !header_updated;
+}
+} else if (qcow2_load_dirty_bitmaps(bs, _err)) {
 update_header = false;
 }
 if (local_err != NULL) {
-- 
2.11.1

[Qemu-block] [PATCH v2] qcow2: add overlap check for bitmap directory

[Vladimir Sementsov-Ogievskiy]

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

If it appropriate for 2.12, let's push it. If not - then for 2.13.


v2: - squash 02 (indentation fix) to 01
- drop comment from qcow2_check_metadata_overlap()
- set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
  bitmap_list_store. I don't think non-inplace case should be changed,
  as it don't touch active bitmap directory.

 block/qcow2.h  | 45 -
 block/qcow2-bitmap.c   |  7 ++-
 block/qcow2-refcount.c | 10 ++
 block/qcow2.c  | 22 ++
 4 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 6f0ff15dd0..896ad08e5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -98,6 +98,7 @@
 #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE "overlap-check.snapshot-table"
 #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
 #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY "overlap-check.bitmap-directory"
 #define QCOW2_OPT_CACHE_SIZE "cache-size"
 #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
 #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
@@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
 } QCow2ClusterType;
 
 typedef enum QCow2MetadataOverlap {
-QCOW2_OL_MAIN_HEADER_BITNR= 0,
-QCOW2_OL_ACTIVE_L1_BITNR  = 1,
-QCOW2_OL_ACTIVE_L2_BITNR  = 2,
-QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
-QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
-QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
-QCOW2_OL_INACTIVE_L1_BITNR= 6,
-QCOW2_OL_INACTIVE_L2_BITNR= 7,
-
-QCOW2_OL_MAX_BITNR= 8,
-
-QCOW2_OL_NONE   = 0,
-QCOW2_OL_MAIN_HEADER= (1 << QCOW2_OL_MAIN_HEADER_BITNR),
-QCOW2_OL_ACTIVE_L1  = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
-QCOW2_OL_ACTIVE_L2  = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
-QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
-QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
-QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
-QCOW2_OL_INACTIVE_L1= (1 << QCOW2_OL_INACTIVE_L1_BITNR),
+QCOW2_OL_MAIN_HEADER_BITNR  = 0,
+QCOW2_OL_ACTIVE_L1_BITNR= 1,
+QCOW2_OL_ACTIVE_L2_BITNR= 2,
+QCOW2_OL_REFCOUNT_TABLE_BITNR   = 3,
+QCOW2_OL_REFCOUNT_BLOCK_BITNR   = 4,
+QCOW2_OL_SNAPSHOT_TABLE_BITNR   = 5,
+QCOW2_OL_INACTIVE_L1_BITNR  = 6,
+QCOW2_OL_INACTIVE_L2_BITNR  = 7,
+QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
+
+QCOW2_OL_MAX_BITNR  = 9,
+
+QCOW2_OL_NONE = 0,
+QCOW2_OL_MAIN_HEADER  = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
+QCOW2_OL_ACTIVE_L1= (1 << QCOW2_OL_ACTIVE_L1_BITNR),
+QCOW2_OL_ACTIVE_L2= (1 << QCOW2_OL_ACTIVE_L2_BITNR),
+QCOW2_OL_REFCOUNT_TABLE   = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
+QCOW2_OL_REFCOUNT_BLOCK   = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
+QCOW2_OL_SNAPSHOT_TABLE   = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
+QCOW2_OL_INACTIVE_L1  = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
 /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
  * reads. */
-QCOW2_OL_INACTIVE_L2= (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+QCOW2_OL_INACTIVE_L2  = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
 } QCow2MetadataOverlap;
 
 /* Perform all overlap checks which can be done in constant time */
 #define QCOW2_OL_CONSTANT \
 (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | QCOW2_OL_REFCOUNT_TABLE | \
- QCOW2_OL_SNAPSHOT_TABLE)
+ QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
 
 /* Perform all overlap checks which don't require disk access */
 #define QCOW2_OL_CACHED \
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f45e46cfbd..fb750ba8d3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState *bs, 
Qcow2BitmapList *bm_list,
 }
 }
 
-ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, dir_size);
+/* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is 
not
+ * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
+ * directory in-place (actually, turn-off the extension), which is checked
+ * in qcow2_check_metadata_overlap() */
+ret = qcow2_pre_write_overlap_check(
+bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, 
dir_size);
 if (ret < 0) {
 goto fail;
 }
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3de1ab51ba..275a303cfa 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2585,6 +2585,16 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
int ign, int64_t offset,
 }
 }
 
+if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
+  

Re: [Qemu-block] [PATCH for-2.12 0/2] qcow2: add overlap check for bitmap directory

[Vladimir Sementsov-Ogievskiy]

17.03.2018 01:21, John Snow wrote:


On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:

Add simple constant overlap check.

Vladimir Sementsov-Ogievskiy (2):
   qcow2: add overlap check for bitmap directory
   qcow2: fix indentation after previous patch

  block/qcow2.h  | 45 -
  block/qcow2-refcount.c | 12 
  block/qcow2.c  | 22 ++
  3 files changed, 50 insertions(+), 29 deletions(-)


Vladimir, do we need this for 2.12 still?
How about "fix bitmaps migration through shared storage"?


Ohh, my fault. Will look at them today.

--
Best regards,
Vladimir