Re: [Qemu-devel] [PATCH v2 00/12] block: qiov_offset parameter for io

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature