mbox series

[v2,00/12] block: qiov_offset parameter for io

Message ID 20190604161514.262241-1-vsementsov@virtuozzo.com
Headers show
Series block: qiov_offset parameter for io | expand

Message

Vladimir Sementsov-Ogievskiy June 4, 2019, 4:15 p.m. UTC
Hi all!

Here is new parameter qiov_offset for io path, to avoid
a lot of places with same pattern of creating local_qiov or hd_qiov
variables.

These series also includes my
"[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
with some changes [described in 01 and 03 emails]

Vladimir Sementsov-Ogievskiy (12):
  util/iov: introduce qemu_iovec_init_extended
  util/iov: improve qemu_iovec_is_zero
  block/io: refactor padding
  block: define .*_part io handlers in BlockDriver
  block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
  block/io: bdrv_co_do_copy_on_readv: lazy allocation
  block/io: bdrv_aligned_preadv: use and support qiov_offset
  block/io: bdrv_aligned_pwritev: use and support qiov_offset
  block/io: introduce bdrv_co_p{read,write}v_part
  block/qcow2: refactor qcow2_co_preadv to use buffer-based io
  block/qcow2: implement .bdrv_co_preadv_part
  block/qcow2: implement .bdrv_co_pwritev(_compressed)_part

 block/qcow2.h             |   1 +
 include/block/block_int.h |  21 ++
 include/qemu/iov.h        |  10 +-
 block/backup.c            |   2 +-
 block/io.c                | 532 ++++++++++++++++++++++----------------
 block/qcow2-cluster.c     |  14 +-
 block/qcow2.c             | 131 +++++-----
 qemu-img.c                |   4 +-
 util/iov.c                | 153 +++++++++--
 9 files changed, 559 insertions(+), 309 deletions(-)

Comments

no-reply@patchew.org June 4, 2019, 5:48 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20190604161514.262241-1-vsementsov@virtuozzo.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      accel/tcg/trace.o
  CC      crypto/trace.o
  CC      authz/trace.o
/tmp/qemu-test/src/util/iov.c:428:9: error: variable 'mid_niov' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
    if (mid_len) {
        ^~~~~~~
/tmp/qemu-test/src/util/iov.c:433:31: note: uninitialized use occurs here


The full log is available at
http://patchew.org/logs/20190604161514.262241-1-vsementsov@virtuozzo.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Stefan Hajnoczi June 28, 2019, 8:43 a.m. UTC | #2
On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is new parameter qiov_offset for io path, to avoid
> a lot of places with same pattern of creating local_qiov or hd_qiov
> variables.
> 
> These series also includes my
> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
> with some changes [described in 01 and 03 emails]
> 
> Vladimir Sementsov-Ogievskiy (12):
>   util/iov: introduce qemu_iovec_init_extended
>   util/iov: improve qemu_iovec_is_zero
>   block/io: refactor padding
>   block: define .*_part io handlers in BlockDriver
>   block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>   block/io: bdrv_co_do_copy_on_readv: lazy allocation
>   block/io: bdrv_aligned_preadv: use and support qiov_offset
>   block/io: bdrv_aligned_pwritev: use and support qiov_offset
>   block/io: introduce bdrv_co_p{read,write}v_part
>   block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>   block/qcow2: implement .bdrv_co_preadv_part
>   block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
> 
>  block/qcow2.h             |   1 +
>  include/block/block_int.h |  21 ++
>  include/qemu/iov.h        |  10 +-
>  block/backup.c            |   2 +-
>  block/io.c                | 532 ++++++++++++++++++++++----------------
>  block/qcow2-cluster.c     |  14 +-
>  block/qcow2.c             | 131 +++++-----
>  qemu-img.c                |   4 +-
>  util/iov.c                | 153 +++++++++--
>  9 files changed, 559 insertions(+), 309 deletions(-)
> 
> -- 
> 2.18.0
> 
> 

I don't see a significant advantage after taking into account more
complex code (e.g. additional block driver interfaces) and the risk of
introducing new bugs.  A measurable performance improvement would make
this refactoring more attractive.  Still:

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Vladimir Sementsov-Ogievskiy July 25, 2019, 8:28 a.m. UTC | #3
28.06.2019 11:43, Stefan Hajnoczi wrote:
> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is new parameter qiov_offset for io path, to avoid
>> a lot of places with same pattern of creating local_qiov or hd_qiov
>> variables.
>>
>> These series also includes my
>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>> with some changes [described in 01 and 03 emails]
>>
>> Vladimir Sementsov-Ogievskiy (12):
>>    util/iov: introduce qemu_iovec_init_extended
>>    util/iov: improve qemu_iovec_is_zero
>>    block/io: refactor padding
>>    block: define .*_part io handlers in BlockDriver
>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>    block/io: introduce bdrv_co_p{read,write}v_part
>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>    block/qcow2: implement .bdrv_co_preadv_part
>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>
>>   block/qcow2.h             |   1 +
>>   include/block/block_int.h |  21 ++
>>   include/qemu/iov.h        |  10 +-
>>   block/backup.c            |   2 +-
>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>   block/qcow2-cluster.c     |  14 +-
>>   block/qcow2.c             | 131 +++++-----
>>   qemu-img.c                |   4 +-
>>   util/iov.c                | 153 +++++++++--
>>   9 files changed, 559 insertions(+), 309 deletions(-)
>>
>> -- 
>> 2.18.0
>>
>>
> 
> I don't see a significant advantage after taking into account more
> complex code (e.g. additional block driver interfaces) and the risk of
> introducing new bugs.  A measurable performance improvement would make
> this refactoring more attractive.  Still:
> 
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Hmm, I understand your doubt. And I doubt that there will be some significant performance
gain.

What will you say if instead of adding new interfaces I just add qiov_offset parameter to
old ones, and add one boolean field to BlockDriver like .supports_qiov_offset, so, generic
code will use non-zero qiov_offset only for drivers supporting it? And, maybe, add corresponding
asserts to all not-supporting driver handlers?
Stefan Hajnoczi July 29, 2019, 3:24 p.m. UTC | #4
On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is new parameter qiov_offset for io path, to avoid
> a lot of places with same pattern of creating local_qiov or hd_qiov
> variables.
> 
> These series also includes my
> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
> with some changes [described in 01 and 03 emails]
> 
> Vladimir Sementsov-Ogievskiy (12):
>   util/iov: introduce qemu_iovec_init_extended
>   util/iov: improve qemu_iovec_is_zero
>   block/io: refactor padding
>   block: define .*_part io handlers in BlockDriver
>   block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>   block/io: bdrv_co_do_copy_on_readv: lazy allocation
>   block/io: bdrv_aligned_preadv: use and support qiov_offset
>   block/io: bdrv_aligned_pwritev: use and support qiov_offset
>   block/io: introduce bdrv_co_p{read,write}v_part
>   block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>   block/qcow2: implement .bdrv_co_preadv_part
>   block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
> 
>  block/qcow2.h             |   1 +
>  include/block/block_int.h |  21 ++
>  include/qemu/iov.h        |  10 +-
>  block/backup.c            |   2 +-
>  block/io.c                | 532 ++++++++++++++++++++++----------------
>  block/qcow2-cluster.c     |  14 +-
>  block/qcow2.c             | 131 +++++-----
>  qemu-img.c                |   4 +-
>  util/iov.c                | 153 +++++++++--
>  9 files changed, 559 insertions(+), 309 deletions(-)
> 
> -- 
> 2.18.0

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

Stefan
Vladimir Sementsov-Ogievskiy July 29, 2019, 3:34 p.m. UTC | #5
29.07.2019 18:24, Stefan Hajnoczi wrote:
> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is new parameter qiov_offset for io path, to avoid
>> a lot of places with same pattern of creating local_qiov or hd_qiov
>> variables.
>>
>> These series also includes my
>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>> with some changes [described in 01 and 03 emails]
>>
>> Vladimir Sementsov-Ogievskiy (12):
>>    util/iov: introduce qemu_iovec_init_extended
>>    util/iov: improve qemu_iovec_is_zero
>>    block/io: refactor padding
>>    block: define .*_part io handlers in BlockDriver
>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>    block/io: introduce bdrv_co_p{read,write}v_part
>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>    block/qcow2: implement .bdrv_co_preadv_part
>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>
>>   block/qcow2.h             |   1 +
>>   include/block/block_int.h |  21 ++
>>   include/qemu/iov.h        |  10 +-
>>   block/backup.c            |   2 +-
>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>   block/qcow2-cluster.c     |  14 +-
>>   block/qcow2.c             | 131 +++++-----
>>   qemu-img.c                |   4 +-
>>   util/iov.c                | 153 +++++++++--
>>   9 files changed, 559 insertions(+), 309 deletions(-)
>>
>> -- 
>> 2.18.0
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
> 
> Stefan
> 

Thanks! Than I can base my further work on parallelizing qcow2
read/write loops  on it, great! (long ago v1 was
[PATCH 0/7] qcow2: async handling of fragmented io)
Vladimir Sementsov-Ogievskiy Aug. 15, 2019, 11:12 a.m. UTC | #6
29.07.2019 18:24, Stefan Hajnoczi wrote:
> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is new parameter qiov_offset for io path, to avoid
>> a lot of places with same pattern of creating local_qiov or hd_qiov
>> variables.
>>
>> These series also includes my
>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>> with some changes [described in 01 and 03 emails]
>>
>> Vladimir Sementsov-Ogievskiy (12):
>>    util/iov: introduce qemu_iovec_init_extended
>>    util/iov: improve qemu_iovec_is_zero
>>    block/io: refactor padding
>>    block: define .*_part io handlers in BlockDriver
>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>    block/io: introduce bdrv_co_p{read,write}v_part
>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>    block/qcow2: implement .bdrv_co_preadv_part
>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>
>>   block/qcow2.h             |   1 +
>>   include/block/block_int.h |  21 ++
>>   include/qemu/iov.h        |  10 +-
>>   block/backup.c            |   2 +-
>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>   block/qcow2-cluster.c     |  14 +-
>>   block/qcow2.c             | 131 +++++-----
>>   qemu-img.c                |   4 +-
>>   util/iov.c                | 153 +++++++++--
>>   9 files changed, 559 insertions(+), 309 deletions(-)
>>
>> -- 
>> 2.18.0
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
> 
> Stefan
> 

Could you please squash this into 01:

diff --git a/util/iov.c b/util/iov.c
index 0ed75e764c..5059e10431 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -422,7 +422,7 @@ void qemu_iovec_init_extended(
          void *tail_buf, size_t tail_len)
  {
      size_t mid_head, mid_tail;
-    int total_niov, mid_niov;
+    int total_niov, mid_niov = 0;
      struct iovec *p, *mid_iov;

      if (mid_len) {
Stefan Hajnoczi Aug. 22, 2019, 2:48 p.m. UTC | #7
On Thu, Aug 15, 2019 at 11:12:35AM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 29.07.2019 18:24, Stefan Hajnoczi wrote:
> > On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> Hi all!
> >>
> >> Here is new parameter qiov_offset for io path, to avoid
> >> a lot of places with same pattern of creating local_qiov or hd_qiov
> >> variables.
> >>
> >> These series also includes my
> >> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
> >> with some changes [described in 01 and 03 emails]
> >>
> >> Vladimir Sementsov-Ogievskiy (12):
> >>    util/iov: introduce qemu_iovec_init_extended
> >>    util/iov: improve qemu_iovec_is_zero
> >>    block/io: refactor padding
> >>    block: define .*_part io handlers in BlockDriver
> >>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
> >>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
> >>    block/io: bdrv_aligned_preadv: use and support qiov_offset
> >>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
> >>    block/io: introduce bdrv_co_p{read,write}v_part
> >>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
> >>    block/qcow2: implement .bdrv_co_preadv_part
> >>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
> >>
> >>   block/qcow2.h             |   1 +
> >>   include/block/block_int.h |  21 ++
> >>   include/qemu/iov.h        |  10 +-
> >>   block/backup.c            |   2 +-
> >>   block/io.c                | 532 ++++++++++++++++++++++----------------
> >>   block/qcow2-cluster.c     |  14 +-
> >>   block/qcow2.c             | 131 +++++-----
> >>   qemu-img.c                |   4 +-
> >>   util/iov.c                | 153 +++++++++--
> >>   9 files changed, 559 insertions(+), 309 deletions(-)
> >>
> >> -- 
> >> 2.18.0
> > 
> > Thanks, applied to my block tree:
> > https://github.com/stefanha/qemu/commits/block
> > 
> > Stefan
> > 
> 
> Could you please squash this into 01:
> 
> diff --git a/util/iov.c b/util/iov.c
> index 0ed75e764c..5059e10431 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -422,7 +422,7 @@ void qemu_iovec_init_extended(
>           void *tail_buf, size_t tail_len)
>   {
>       size_t mid_head, mid_tail;
> -    int total_niov, mid_niov;
> +    int total_niov, mid_niov = 0;
>       struct iovec *p, *mid_iov;
> 
>       if (mid_len) {

Done.  Sending the pull request today.

Stefan
Stefan Hajnoczi Aug. 22, 2019, 3:50 p.m. UTC | #8
On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is new parameter qiov_offset for io path, to avoid
> a lot of places with same pattern of creating local_qiov or hd_qiov
> variables.
> 
> These series also includes my
> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
> with some changes [described in 01 and 03 emails]
> 
> Vladimir Sementsov-Ogievskiy (12):
>   util/iov: introduce qemu_iovec_init_extended
>   util/iov: improve qemu_iovec_is_zero
>   block/io: refactor padding
>   block: define .*_part io handlers in BlockDriver
>   block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>   block/io: bdrv_co_do_copy_on_readv: lazy allocation
>   block/io: bdrv_aligned_preadv: use and support qiov_offset
>   block/io: bdrv_aligned_pwritev: use and support qiov_offset
>   block/io: introduce bdrv_co_p{read,write}v_part
>   block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>   block/qcow2: implement .bdrv_co_preadv_part
>   block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
> 
>  block/qcow2.h             |   1 +
>  include/block/block_int.h |  21 ++
>  include/qemu/iov.h        |  10 +-
>  block/backup.c            |   2 +-
>  block/io.c                | 532 ++++++++++++++++++++++----------------
>  block/qcow2-cluster.c     |  14 +-
>  block/qcow2.c             | 131 +++++-----
>  qemu-img.c                |   4 +-
>  util/iov.c                | 153 +++++++++--
>  9 files changed, 559 insertions(+), 309 deletions(-)

qemu-iotests 077 fails after I apply this series (including your
uninitialized variable fix).  I'm afraid I can't include it in the block
pull request, sorry!

Stefan
Vladimir Sementsov-Ogievskiy Aug. 22, 2019, 5:24 p.m. UTC | #9
22.08.2019 18:50, Stefan Hajnoczi wrote:
> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is new parameter qiov_offset for io path, to avoid
>> a lot of places with same pattern of creating local_qiov or hd_qiov
>> variables.
>>
>> These series also includes my
>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>> with some changes [described in 01 and 03 emails]
>>
>> Vladimir Sementsov-Ogievskiy (12):
>>    util/iov: introduce qemu_iovec_init_extended
>>    util/iov: improve qemu_iovec_is_zero
>>    block/io: refactor padding
>>    block: define .*_part io handlers in BlockDriver
>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>    block/io: introduce bdrv_co_p{read,write}v_part
>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>    block/qcow2: implement .bdrv_co_preadv_part
>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>
>>   block/qcow2.h             |   1 +
>>   include/block/block_int.h |  21 ++
>>   include/qemu/iov.h        |  10 +-
>>   block/backup.c            |   2 +-
>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>   block/qcow2-cluster.c     |  14 +-
>>   block/qcow2.c             | 131 +++++-----
>>   qemu-img.c                |   4 +-
>>   util/iov.c                | 153 +++++++++--
>>   9 files changed, 559 insertions(+), 309 deletions(-)
> 
> qemu-iotests 077 fails after I apply this series (including your
> uninitialized variable fix).  I'm afraid I can't include it in the block
> pull request, sorry!
> 
> Stefan
> 

Hmm, 77 don't work on master for me:
077      fail       [20:23:57] [20:23:59]                    output mismatch (see 077.out.bad)
--- /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out     2019-04-22 15:06:56.162045432 +0300
+++ /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out.bad 2019-08-22 20:23:59.124122307 +0300
@@ -1,7 +1,15 @@
  QA output created by 077
+==117186==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
+==117186==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffefc071000; bottom 0x7fad7277b000; size: 0x0051898f6000 (350200225792)
+False positive error reports may follow
+For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728

  == Some concurrent requests involving RMW ==
+==117197==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
+==117197==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa4fcc000; bottom 0x7fa93a2c1000; size: 0x00566ad0b000 (371159248896)
+False positive error reports may follow
+For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
  wrote XXX/XXX bytes at offset XXX
  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  wrote XXX/XXX bytes at offset XXX
@@ -66,6 +74,10 @@
  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

  == Verify image content ==
+==117219==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
+==117219==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc722de000; bottom 0x7f0848232000; size: 0x00f42a0ac000 (1048677367808)
+False positive error reports may follow
+For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
  read 512/512 bytes at offset 0
  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  read 512/512 bytes at offset 512
@@ -156,5 +168,9 @@
  1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  read 2048/2048 bytes at offset 71680
  2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+==117229==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
+==117229==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa3342000; bottom 0x7fd85a275000; size: 0x0027490cd000 (168729300992)
+False positive error reports may follow
+For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
  No errors were found on the image.
  *** done
Failures: 077
Failed 1 of 1 tests
Vladimir Sementsov-Ogievskiy Aug. 22, 2019, 5:39 p.m. UTC | #10
22.08.2019 20:24, Vladimir Sementsov-Ogievskiy wrote:
> 22.08.2019 18:50, Stefan Hajnoczi wrote:
>> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is new parameter qiov_offset for io path, to avoid
>>> a lot of places with same pattern of creating local_qiov or hd_qiov
>>> variables.
>>>
>>> These series also includes my
>>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>>> with some changes [described in 01 and 03 emails]
>>>
>>> Vladimir Sementsov-Ogievskiy (12):
>>>    util/iov: introduce qemu_iovec_init_extended
>>>    util/iov: improve qemu_iovec_is_zero
>>>    block/io: refactor padding
>>>    block: define .*_part io handlers in BlockDriver
>>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>>    block/io: introduce bdrv_co_p{read,write}v_part
>>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>>    block/qcow2: implement .bdrv_co_preadv_part
>>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>>
>>>   block/qcow2.h             |   1 +
>>>   include/block/block_int.h |  21 ++
>>>   include/qemu/iov.h        |  10 +-
>>>   block/backup.c            |   2 +-
>>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>>   block/qcow2-cluster.c     |  14 +-
>>>   block/qcow2.c             | 131 +++++-----
>>>   qemu-img.c                |   4 +-
>>>   util/iov.c                | 153 +++++++++--
>>>   9 files changed, 559 insertions(+), 309 deletions(-)
>>
>> qemu-iotests 077 fails after I apply this series (including your
>> uninitialized variable fix).  I'm afraid I can't include it in the block
>> pull request, sorry!
>>
>> Stefan
>>
> 
> Hmm, 77 don't work on master for me:
> 077      fail       [20:23:57] [20:23:59]                    output mismatch (see 077.out.bad)
> --- /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out     2019-04-22 15:06:56.162045432 +0300
> +++ /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out.bad 2019-08-22 20:23:59.124122307 +0300
> @@ -1,7 +1,15 @@
>   QA output created by 077
> +==117186==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> +==117186==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffefc071000; bottom 0x7fad7277b000; size: 0x0051898f6000 (350200225792)
> +False positive error reports may follow
> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> 
>   == Some concurrent requests involving RMW ==
> +==117197==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> +==117197==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa4fcc000; bottom 0x7fa93a2c1000; size: 0x00566ad0b000 (371159248896)
> +False positive error reports may follow
> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>   wrote XXX/XXX bytes at offset XXX
>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   wrote XXX/XXX bytes at offset XXX
> @@ -66,6 +74,10 @@
>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> 
>   == Verify image content ==
> +==117219==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> +==117219==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc722de000; bottom 0x7f0848232000; size: 0x00f42a0ac000 (1048677367808)
> +False positive error reports may follow
> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>   read 512/512 bytes at offset 0
>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   read 512/512 bytes at offset 512
> @@ -156,5 +168,9 @@
>   1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   read 2048/2048 bytes at offset 71680
>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +==117229==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> +==117229==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa3342000; bottom 0x7fd85a275000; size: 0x0027490cd000 (168729300992)
> +False positive error reports may follow
> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>   No errors were found on the image.
>   *** done
> Failures: 077
> Failed 1 of 1 tests
> 
> 
> 

But after "block/io: refactor padding" it hangs instead of this fail.. This is not good
Vladimir Sementsov-Ogievskiy Aug. 22, 2019, 5:59 p.m. UTC | #11
22.08.2019 20:39, Vladimir Sementsov-Ogievskiy wrote:
> 22.08.2019 20:24, Vladimir Sementsov-Ogievskiy wrote:
>> 22.08.2019 18:50, Stefan Hajnoczi wrote:
>>> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> Here is new parameter qiov_offset for io path, to avoid
>>>> a lot of places with same pattern of creating local_qiov or hd_qiov
>>>> variables.
>>>>
>>>> These series also includes my
>>>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>>>> with some changes [described in 01 and 03 emails]
>>>>
>>>> Vladimir Sementsov-Ogievskiy (12):
>>>>    util/iov: introduce qemu_iovec_init_extended
>>>>    util/iov: improve qemu_iovec_is_zero
>>>>    block/io: refactor padding
>>>>    block: define .*_part io handlers in BlockDriver
>>>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
>>>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>>>    block/io: introduce bdrv_co_p{read,write}v_part
>>>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>>>    block/qcow2: implement .bdrv_co_preadv_part
>>>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>>>
>>>>   block/qcow2.h             |   1 +
>>>>   include/block/block_int.h |  21 ++
>>>>   include/qemu/iov.h        |  10 +-
>>>>   block/backup.c            |   2 +-
>>>>   block/io.c                | 532 ++++++++++++++++++++++----------------
>>>>   block/qcow2-cluster.c     |  14 +-
>>>>   block/qcow2.c             | 131 +++++-----
>>>>   qemu-img.c                |   4 +-
>>>>   util/iov.c                | 153 +++++++++--
>>>>   9 files changed, 559 insertions(+), 309 deletions(-)
>>>
>>> qemu-iotests 077 fails after I apply this series (including your
>>> uninitialized variable fix).  I'm afraid I can't include it in the block
>>> pull request, sorry!
>>>
>>> Stefan
>>>
>>
>> Hmm, 77 don't work on master for me:
>> 077      fail       [20:23:57] [20:23:59]                    output mismatch (see 077.out.bad)
>> --- /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out     2019-04-22 15:06:56.162045432 +0300
>> +++ /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out.bad 2019-08-22 20:23:59.124122307 +0300
>> @@ -1,7 +1,15 @@
>>   QA output created by 077
>> +==117186==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>> +==117186==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffefc071000; bottom 0x7fad7277b000; size: 0x0051898f6000 (350200225792)
>> +False positive error reports may follow
>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>
>>   == Some concurrent requests involving RMW ==
>> +==117197==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>> +==117197==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa4fcc000; bottom 0x7fa93a2c1000; size: 0x00566ad0b000 (371159248896)
>> +False positive error reports may follow
>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>   wrote XXX/XXX bytes at offset XXX
>>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   wrote XXX/XXX bytes at offset XXX
>> @@ -66,6 +74,10 @@
>>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>
>>   == Verify image content ==
>> +==117219==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>> +==117219==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc722de000; bottom 0x7f0848232000; size: 0x00f42a0ac000 (1048677367808)
>> +False positive error reports may follow
>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>   read 512/512 bytes at offset 0
>>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   read 512/512 bytes at offset 512
>> @@ -156,5 +168,9 @@
>>   1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   read 2048/2048 bytes at offset 71680
>>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +==117229==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>> +==117229==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa3342000; bottom 0x7fd85a275000; size: 0x0027490cd000 (168729300992)
>> +False positive error reports may follow
>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>   No errors were found on the image.
>>   *** done
>> Failures: 077
>> Failed 1 of 1 tests
>>
>>
>>
> 
> But after "block/io: refactor padding" it hangs instead of this fail.. This is not good
> 
> 

Aha seems it's because 77 has "break pwritev_rmw_after_tail" which may not fire if we have merge_reads = true.

So, for me the following fix for 03 helps (77 fails, but not hangs, so same behavior as on master):

diff --git a/block/io.c b/block/io.c
index 6448f1c503..04e69400d8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1493,13 +1493,23 @@ static int bdrv_padding_rmw_read(BdrvChild *child,

          qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);

-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
+        if (pad->head) {
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
+        }
+        if (pad->merge_reads && pad->tail) {
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
+        }
          ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
                                    align, &local_qiov, 0);
          if (ret < 0) {
              return ret;
          }
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
+        if (pad->head) {
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
+        }
+        if (pad->merge_reads && pad->tail) {
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
+        }


Does it work for you?
Stefan Hajnoczi Aug. 23, 2019, 4:21 p.m. UTC | #12
On Thu, Aug 22, 2019 at 05:59:43PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 22.08.2019 20:39, Vladimir Sementsov-Ogievskiy wrote:
> > 22.08.2019 20:24, Vladimir Sementsov-Ogievskiy wrote:
> >> 22.08.2019 18:50, Stefan Hajnoczi wrote:
> >>> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >>>> Hi all!
> >>>>
> >>>> Here is new parameter qiov_offset for io path, to avoid
> >>>> a lot of places with same pattern of creating local_qiov or hd_qiov
> >>>> variables.
> >>>>
> >>>> These series also includes my
> >>>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
> >>>> with some changes [described in 01 and 03 emails]
> >>>>
> >>>> Vladimir Sementsov-Ogievskiy (12):
> >>>>    util/iov: introduce qemu_iovec_init_extended
> >>>>    util/iov: improve qemu_iovec_is_zero
> >>>>    block/io: refactor padding
> >>>>    block: define .*_part io handlers in BlockDriver
> >>>>    block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
> >>>>    block/io: bdrv_co_do_copy_on_readv: lazy allocation
> >>>>    block/io: bdrv_aligned_preadv: use and support qiov_offset
> >>>>    block/io: bdrv_aligned_pwritev: use and support qiov_offset
> >>>>    block/io: introduce bdrv_co_p{read,write}v_part
> >>>>    block/qcow2: refactor qcow2_co_preadv to use buffer-based io
> >>>>    block/qcow2: implement .bdrv_co_preadv_part
> >>>>    block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
> >>>>
> >>>>   block/qcow2.h             |   1 +
> >>>>   include/block/block_int.h |  21 ++
> >>>>   include/qemu/iov.h        |  10 +-
> >>>>   block/backup.c            |   2 +-
> >>>>   block/io.c                | 532 ++++++++++++++++++++++----------------
> >>>>   block/qcow2-cluster.c     |  14 +-
> >>>>   block/qcow2.c             | 131 +++++-----
> >>>>   qemu-img.c                |   4 +-
> >>>>   util/iov.c                | 153 +++++++++--
> >>>>   9 files changed, 559 insertions(+), 309 deletions(-)
> >>>
> >>> qemu-iotests 077 fails after I apply this series (including your
> >>> uninitialized variable fix).  I'm afraid I can't include it in the block
> >>> pull request, sorry!
> >>>
> >>> Stefan
> >>>
> >>
> >> Hmm, 77 don't work on master for me:
> >> 077      fail       [20:23:57] [20:23:59]                    output mismatch (see 077.out.bad)
> >> --- /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out     2019-04-22 15:06:56.162045432 +0300
> >> +++ /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out.bad 2019-08-22 20:23:59.124122307 +0300
> >> @@ -1,7 +1,15 @@
> >>   QA output created by 077
> >> +==117186==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> >> +==117186==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffefc071000; bottom 0x7fad7277b000; size: 0x0051898f6000 (350200225792)
> >> +False positive error reports may follow
> >> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
> >>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> >>
> >>   == Some concurrent requests involving RMW ==
> >> +==117197==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> >> +==117197==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa4fcc000; bottom 0x7fa93a2c1000; size: 0x00566ad0b000 (371159248896)
> >> +False positive error reports may follow
> >> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
> >>   wrote XXX/XXX bytes at offset XXX
> >>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>   wrote XXX/XXX bytes at offset XXX
> >> @@ -66,6 +74,10 @@
> >>   XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>
> >>   == Verify image content ==
> >> +==117219==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> >> +==117219==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc722de000; bottom 0x7f0848232000; size: 0x00f42a0ac000 (1048677367808)
> >> +False positive error reports may follow
> >> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
> >>   read 512/512 bytes at offset 0
> >>   512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>   read 512/512 bytes at offset 512
> >> @@ -156,5 +168,9 @@
> >>   1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>   read 2048/2048 bytes at offset 71680
> >>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >> +==117229==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
> >> +==117229==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa3342000; bottom 0x7fd85a275000; size: 0x0027490cd000 (168729300992)
> >> +False positive error reports may follow
> >> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
> >>   No errors were found on the image.
> >>   *** done
> >> Failures: 077
> >> Failed 1 of 1 tests
> >>
> >>
> >>
> > 
> > But after "block/io: refactor padding" it hangs instead of this fail.. This is not good
> > 
> > 
> 
> Aha seems it's because 77 has "break pwritev_rmw_after_tail" which may not fire if we have merge_reads = true.
> 
> So, for me the following fix for 03 helps (77 fails, but not hangs, so same behavior as on master):
> 
> diff --git a/block/io.c b/block/io.c
> index 6448f1c503..04e69400d8 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1493,13 +1493,23 @@ static int bdrv_padding_rmw_read(BdrvChild *child,
> 
>           qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
> 
> -        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
> +        if (pad->head) {
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
> +        }
> +        if (pad->merge_reads && pad->tail) {
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
> +        }
>           ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
>                                     align, &local_qiov, 0);
>           if (ret < 0) {
>               return ret;
>           }
> -        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> +        if (pad->head) {
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> +        }
> +        if (pad->merge_reads && pad->tail) {
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> +        }
> 
> 
> Does it work for you?

Yes, it works as expected now, thanks.  I've squashed in your fix and
will send these patches in the next block pull request.

Stefan
Vladimir Sementsov-Ogievskiy Aug. 24, 2019, 10:15 a.m. UTC | #13
23.08.2019 19:21, Stefan Hajnoczi wrote:
> On Thu, Aug 22, 2019 at 05:59:43PM +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 22.08.2019 20:39, Vladimir Sementsov-Ogievskiy wrote:
>>> 22.08.2019 20:24, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.08.2019 18:50, Stefan Hajnoczi wrote:
>>>>> On Tue, Jun 04, 2019 at 07:15:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi all!
>>>>>>
>>>>>> Here is new parameter qiov_offset for io path, to avoid
>>>>>> a lot of places with same pattern of creating local_qiov or hd_qiov
>>>>>> variables.
>>>>>>
>>>>>> These series also includes my
>>>>>> "[Qemu-devel] [PATCH 0/2] block/io: refactor padding"
>>>>>> with some changes [described in 01 and 03 emails]
>>>>>>
>>>>>> Vladimir Sementsov-Ogievskiy (12):
>>>>>>     util/iov: introduce qemu_iovec_init_extended
>>>>>>     util/iov: improve qemu_iovec_is_zero
>>>>>>     block/io: refactor padding
>>>>>>     block: define .*_part io handlers in BlockDriver
>>>>>>     block/io: bdrv_co_do_copy_on_readv: use and support qiov_offset
>>>>>>     block/io: bdrv_co_do_copy_on_readv: lazy allocation
>>>>>>     block/io: bdrv_aligned_preadv: use and support qiov_offset
>>>>>>     block/io: bdrv_aligned_pwritev: use and support qiov_offset
>>>>>>     block/io: introduce bdrv_co_p{read,write}v_part
>>>>>>     block/qcow2: refactor qcow2_co_preadv to use buffer-based io
>>>>>>     block/qcow2: implement .bdrv_co_preadv_part
>>>>>>     block/qcow2: implement .bdrv_co_pwritev(_compressed)_part
>>>>>>
>>>>>>    block/qcow2.h             |   1 +
>>>>>>    include/block/block_int.h |  21 ++
>>>>>>    include/qemu/iov.h        |  10 +-
>>>>>>    block/backup.c            |   2 +-
>>>>>>    block/io.c                | 532 ++++++++++++++++++++++----------------
>>>>>>    block/qcow2-cluster.c     |  14 +-
>>>>>>    block/qcow2.c             | 131 +++++-----
>>>>>>    qemu-img.c                |   4 +-
>>>>>>    util/iov.c                | 153 +++++++++--
>>>>>>    9 files changed, 559 insertions(+), 309 deletions(-)
>>>>>
>>>>> qemu-iotests 077 fails after I apply this series (including your
>>>>> uninitialized variable fix).  I'm afraid I can't include it in the block
>>>>> pull request, sorry!
>>>>>
>>>>> Stefan
>>>>>
>>>>
>>>> Hmm, 77 don't work on master for me:
>>>> 077      fail       [20:23:57] [20:23:59]                    output mismatch (see 077.out.bad)
>>>> --- /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out     2019-04-22 15:06:56.162045432 +0300
>>>> +++ /work/src/qemu/up-block-drop-hd-qiov/tests/qemu-iotests/077.out.bad 2019-08-22 20:23:59.124122307 +0300
>>>> @@ -1,7 +1,15 @@
>>>>    QA output created by 077
>>>> +==117186==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>>>> +==117186==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffefc071000; bottom 0x7fad7277b000; size: 0x0051898f6000 (350200225792)
>>>> +False positive error reports may follow
>>>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>>>    Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>>>
>>>>    == Some concurrent requests involving RMW ==
>>>> +==117197==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>>>> +==117197==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa4fcc000; bottom 0x7fa93a2c1000; size: 0x00566ad0b000 (371159248896)
>>>> +False positive error reports may follow
>>>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>>>    wrote XXX/XXX bytes at offset XXX
>>>>    XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>    wrote XXX/XXX bytes at offset XXX
>>>> @@ -66,6 +74,10 @@
>>>>    XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>
>>>>    == Verify image content ==
>>>> +==117219==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>>>> +==117219==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc722de000; bottom 0x7f0848232000; size: 0x00f42a0ac000 (1048677367808)
>>>> +False positive error reports may follow
>>>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>>>    read 512/512 bytes at offset 0
>>>>    512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>    read 512/512 bytes at offset 512
>>>> @@ -156,5 +168,9 @@
>>>>    1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>    read 2048/2048 bytes at offset 71680
>>>>    2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>> +==117229==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
>>>> +==117229==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fffa3342000; bottom 0x7fd85a275000; size: 0x0027490cd000 (168729300992)
>>>> +False positive error reports may follow
>>>> +For details see http://code.google.com/p/address-sanitizer/issues/detail?id=189
>>>>    No errors were found on the image.
>>>>    *** done
>>>> Failures: 077
>>>> Failed 1 of 1 tests
>>>>
>>>>
>>>>
>>>
>>> But after "block/io: refactor padding" it hangs instead of this fail.. This is not good
>>>
>>>
>>
>> Aha seems it's because 77 has "break pwritev_rmw_after_tail" which may not fire if we have merge_reads = true.
>>
>> So, for me the following fix for 03 helps (77 fails, but not hangs, so same behavior as on master):
>>
>> diff --git a/block/io.c b/block/io.c
>> index 6448f1c503..04e69400d8 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1493,13 +1493,23 @@ static int bdrv_padding_rmw_read(BdrvChild *child,
>>
>>            qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
>>
>> -        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
>> +        if (pad->head) {
>> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
>> +        }
>> +        if (pad->merge_reads && pad->tail) {
>> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
>> +        }
>>            ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
>>                                      align, &local_qiov, 0);
>>            if (ret < 0) {
>>                return ret;
>>            }
>> -        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>> +        if (pad->head) {
>> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>> +        }
>> +        if (pad->merge_reads && pad->tail) {
>> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>> +        }
>>
>>
>> Does it work for you?
> 
> Yes, it works as expected now, thanks.  I've squashed in your fix and
> will send these patches in the next block pull request.
> 

Thank you!