[Qemu-devel] [PATCH v2 0/2] Fix incomplete aio_preadv byte conversion in mirroring

[Qemu-devel] [PATCH v2 0/2] Fix incomplete aio_preadv byte conversion in mirroring

[Eric Blake]

v2: Split the bug fix from the addition of assertions, add a
BDRV_REQ_MASK constant [famz], stick assertions at the more
common entry points

Eric Blake (2):
  block: Avoid bogus flags during mirroring
  block: Assert that flags are in range

 include/block/block.h | 3 +++
 block/io.c            | 6 ++++++
 block/mirror.c        | 6 ++----
 3 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/2] block: Avoid bogus flags during mirroring

[Eric Blake]

Commit e253f4b8 converted mirroring from sector-based bdrv_aio_*
to byte-based blk_aio_*, but failed to account for the subtle
difference in signatures (the former takes a semi-redundant length,
the latter takes a flags parameter).  Since all of our flags are
currently smaller in size than BDRV_SECTOR_SIZE, it has no ill
effects until we either perform sub-sector mirroring, or we start
asserting that no unexpected flags are set.  I found it while
testing new asserts when qemu-iotests 132 started warning about an
unknown flag 0x200000.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fbbc496..41848b2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -157,8 +157,7 @@ static void mirror_read_complete(void *opaque, int ret)
         return;
     }
     blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, &op->qiov,
-                    op->nb_sectors * BDRV_SECTOR_SIZE,
-                    mirror_write_complete, op);
+                    0, mirror_write_complete, op);
 }

 static inline void mirror_clip_sectors(MirrorBlockJob *s,
@@ -275,8 +274,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
     s->sectors_in_flight += nb_sectors;
     trace_mirror_one_iteration(s, sector_num, nb_sectors);

-    blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov,
-                   nb_sectors * BDRV_SECTOR_SIZE,
+    blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, 0,
                    mirror_read_complete, op);
     return ret;
 }
-- 
2.5.5

[Qemu-devel] [PATCH v2 2/2] block: Assert that flags are in range

[Eric Blake]

Add a new BDRV_REQ_MASK constant, and use it to make sure that
caller flags are always valid.

Tested with 'make check' and with qemu-iotests on both '-raw'
and '-qcow2'; the only failure turned up was fixed in the
previous commit.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h | 3 +++
 block/io.c            | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index a158575..733a8ec 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -65,6 +65,9 @@ typedef enum {
     BDRV_REQ_MAY_UNMAP          = 0x4,
     BDRV_REQ_NO_SERIALISING     = 0x8,
     BDRV_REQ_FUA                = 0x10,
+
+    /* Mask of valid flags */
+    BDRV_REQ_MASK               = 0x1f,
 } BdrvRequestFlags;

 typedef struct BlockSizes {
diff --git a/block/io.c b/block/io.c
index d504443..b4f1235 100644
--- a/block/io.c
+++ b/block/io.c
@@ -802,6 +802,8 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
     int64_t sector_num;
     unsigned int nb_sectors;

+    assert(!(flags & ~BDRV_REQ_MASK));
+
     if (drv->bdrv_co_preadv) {
         return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags);
     }
@@ -841,6 +843,8 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
     unsigned int nb_sectors;
     int ret;

+    assert(!(flags & ~BDRV_REQ_MASK));
+
     if (drv->bdrv_co_pwritev) {
         ret = drv->bdrv_co_pwritev(bs, offset, bytes, qiov,
                                    flags & bs->supported_write_flags);
@@ -967,6 +971,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,

     assert(!qiov || bytes == qiov->size);
     assert((bs->open_flags & BDRV_O_NO_IO) == 0);
+    assert(!(flags & ~BDRV_REQ_MASK));

     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1246,6 +1251,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,

     assert(!qiov || bytes == qiov->size);
     assert((bs->open_flags & BDRV_O_NO_IO) == 0);
+    assert(!(flags & ~BDRV_REQ_MASK));

     waited = wait_serialising_requests(req);
     assert(!waited || !req->serialising);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 2/2] block: Assert that flags are in range

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

On Mon, Jun 13, 2016 at 12:56:35PM -0600, Eric Blake wrote:
> Add a new BDRV_REQ_MASK constant, and use it to make sure that
> caller flags are always valid.
> 
> Tested with 'make check' and with qemu-iotests on both '-raw'
> and '-qcow2'; the only failure turned up was fixed in the
> previous commit.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block.h | 3 +++
>  block/io.c            | 6 ++++++
>  2 files changed, 9 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] Fix incomplete aio_preadv byte conversion in mirroring

[Kevin Wolf]

Am 13.06.2016 um 20:56 hat Eric Blake geschrieben:
> v2: Split the bug fix from the addition of assertions, add a
> BDRV_REQ_MASK constant [famz], stick assertions at the more
> common entry points

Thanks, applied to the block branch.

Kevin