[PATCH v4 00/16] 64bit block-layer: part I

[PATCH v4 00/16] 64bit block-layer: part I

[Vladimir Sementsov-Ogievskiy]

Hi all!

We want 64bit write-zeroes, and for this, convert all io functions to
64bit.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

Please refer to initial cover-letter 
 https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
for more info.

v4: I found, that some more work is needed for block/block-backend, so
decided to make partI, converting block/io

v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
   for BDRV_MAX_LENGTH

changes:
01-05: new
06: add Alberto's r-b
07: new
08-16: rebase, add new-style request check, improve commit-msg, drop r-bs

Based-on: <20201211170812.228643-1-kwolf@redhat.com>

Vladimir Sementsov-Ogievskiy (16):
  block: refactor bdrv_check_request: add errp
  util/iov: make qemu_iovec_init_extended() honest
  block: fix theoretical overflow in bdrv_init_padding()
  block/io: refactor bdrv_pad_request(): move bdrv_pad_request() up
  block/io: bdrv_pad_request(): support qemu_iovec_init_extended failure
  block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit
    bytes
  block/io: improve bdrv_check_request: check qiov too
  block: use int64_t as bytes type in tracked requests
  block/io: use int64_t bytes in driver wrappers
  block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()
  block/io: support int64_t bytes in bdrv_aligned_pwritev()
  block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()
  block/io: support int64_t bytes in bdrv_aligned_preadv()
  block/io: support int64_t bytes in bdrv_co_p{read,write}v_part()
  block/io: support int64_t bytes in read/write wrappers
  block/io: use int64_t bytes in copy_range

 include/block/block.h           |  17 +-
 include/block/block_int.h       |  26 +--
 include/block/throttle-groups.h |   2 +-
 include/qemu/iov.h              |   2 +-
 block/blkverify.c               |   2 +-
 block/file-posix.c              |   2 +-
 block/io.c                      | 274 ++++++++++++++++++++++----------
 block/throttle-groups.c         |   5 +-
 tests/test-write-threshold.c    |   5 +-
 util/iov.c                      |  25 ++-
 block/trace-events              |  12 +-
 11 files changed, 252 insertions(+), 120 deletions(-)

-- 
2.25.4

[PATCH v4 01/16] block: refactor bdrv_check_request: add errp

[Vladimir Sementsov-Ogievskiy]

It's better to pass &error_abort than just assert that result is 0: on
crash, we'll immediately see the reason in the backtrace.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h    |  2 +-
 block/file-posix.c           |  2 +-
 block/io.c                   | 29 ++++++++++++++++++++++-------
 tests/test-write-threshold.c |  5 +++--
 4 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1eeafc118c..ff29f31451 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -93,7 +93,7 @@ typedef struct BdrvTrackedRequest {
     struct BdrvTrackedRequest *waiting_for;
 } BdrvTrackedRequest;
 
-int bdrv_check_request(int64_t offset, int64_t bytes);
+int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
 
 struct BlockDriver {
     const char *format_name;
diff --git a/block/file-posix.c b/block/file-posix.c
index 83e2cc5530..fc35a47832 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2951,7 +2951,7 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
 
         req->bytes = BDRV_MAX_LENGTH - req->offset;
 
-        assert(bdrv_check_request(req->offset, req->bytes) == 0);
+        bdrv_check_request(req->offset, req->bytes, &error_abort);
 
         bdrv_mark_request_serialising(req, bs->bl.request_alignment);
     }
diff --git a/block/io.c b/block/io.c
index 24205f5168..e076236db2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -898,17 +898,34 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
     return waited;
 }
 
-int bdrv_check_request(int64_t offset, int64_t bytes)
+int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
 {
-    if (offset < 0 || bytes < 0) {
+    if (offset < 0) {
+        error_setg(errp, "offset is negative: %" PRIi64, offset);
+        return -EIO;
+    }
+
+    if (bytes < 0) {
+        error_setg(errp, "bytes is negative: %" PRIi64, bytes);
         return -EIO;
     }
 
     if (bytes > BDRV_MAX_LENGTH) {
+        error_setg(errp, "bytes(%" PRIi64 ") exceeds maximum(%" PRIi64 ")",
+                   bytes, BDRV_MAX_LENGTH);
+        return -EIO;
+    }
+
+    if (offset > BDRV_MAX_LENGTH) {
+        error_setg(errp, "offset(%" PRIi64 ") exceeds maximum(%" PRIi64 ")",
+                   offset, BDRV_MAX_LENGTH);
         return -EIO;
     }
 
     if (offset > BDRV_MAX_LENGTH - bytes) {
+        error_setg(errp, "sum of offset(%" PRIi64 ") and bytes(%" PRIi64 ") "
+                   "exceeds maximum(%" PRIi64 ")", offset, bytes,
+                   BDRV_MAX_LENGTH);
         return -EIO;
     }
 
@@ -917,7 +934,7 @@ int bdrv_check_request(int64_t offset, int64_t bytes)
 
 static int bdrv_check_request32(int64_t offset, int64_t bytes)
 {
-    int ret = bdrv_check_request(offset, bytes);
+    int ret = bdrv_check_request(offset, bytes, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -2819,7 +2836,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
         return -EPERM;
     }
 
-    ret = bdrv_check_request(offset, bytes);
+    ret = bdrv_check_request(offset, bytes, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -3221,10 +3238,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
         return -EINVAL;
     }
 
-    ret = bdrv_check_request(offset, 0);
+    ret = bdrv_check_request(offset, 0, errp);
     if (ret < 0) {
-        error_setg(errp, "Required too big image size, it must be not greater "
-                   "than %" PRId64, BDRV_MAX_LENGTH);
         return ret;
     }
 
diff --git a/tests/test-write-threshold.c b/tests/test-write-threshold.c
index 4cf032652d..fc1c45a2eb 100644
--- a/tests/test-write-threshold.c
+++ b/tests/test-write-threshold.c
@@ -7,6 +7,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "block/block_int.h"
 #include "block/write-threshold.h"
 
@@ -64,7 +65,7 @@ static void test_threshold_not_trigger(void)
     req.offset = 1024;
     req.bytes = 1024;
 
-    assert(bdrv_check_request(req.offset, req.bytes) == 0);
+    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
     amount = bdrv_write_threshold_exceeded(&bs, &req);
@@ -84,7 +85,7 @@ static void test_threshold_trigger(void)
     req.offset = (4 * 1024 * 1024) - 1024;
     req.bytes = 2 * 1024;
 
-    assert(bdrv_check_request(req.offset, req.bytes) == 0);
+    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
     amount = bdrv_write_threshold_exceeded(&bs, &req);
-- 
2.25.4

Re: [PATCH v4 01/16] block: refactor bdrv_check_request: add errp

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's better to pass &error_abort than just assert that result is 0: on
> crash, we'll immediately see the reason in the backtrace.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h    |  2 +-
>  block/file-posix.c           |  2 +-
>  block/io.c                   | 29 ++++++++++++++++++++++-------
>  tests/test-write-threshold.c |  5 +++--
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v4 01/16] block: refactor bdrv_check_request: add errp

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's better to pass &error_abort than just assert that result is 0: on
> crash, we'll immediately see the reason in the backtrace.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h    |  2 +-
>  block/file-posix.c           |  2 +-
>  block/io.c                   | 29 ++++++++++++++++++++++-------
>  tests/test-write-threshold.c |  5 +++--
>  4 files changed, 27 insertions(+), 11 deletions(-)

> +++ b/block/io.c
> @@ -898,17 +898,34 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>      return waited;
>  }
>  
> -int bdrv_check_request(int64_t offset, int64_t bytes)
> +int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
>  {

Merge conflicts with 8ac5aab255 here and later in the series, but I
think I managed to resolve all the differences.  I'm planning to queue
this through my NBD tree after subjecting it to more testing.

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

[PATCH v4 02/16] util/iov: make qemu_iovec_init_extended() honest

[Vladimir Sementsov-Ogievskiy]

Actually, we can't extend the io vector in all cases. Handle possible
MAX_IOV and size_t overflows.

For now add assertion to callers (actually they rely on success anyway)
and fix them in the following patch.

Add also some additional good assertions to qemu_iovec_init_slice()
while being here.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/iov.h |  2 +-
 block/io.c         | 10 +++++++---
 util/iov.c         | 25 +++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index b6b283a5e5..9330746680 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -222,7 +222,7 @@ static inline void *qemu_iovec_buf(QEMUIOVector *qiov)
 
 void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
 void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
-void qemu_iovec_init_extended(
+int qemu_iovec_init_extended(
         QEMUIOVector *qiov,
         void *head_buf, size_t head_len,
         QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len,
diff --git a/block/io.c b/block/io.c
index e076236db2..21e8a50725 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1652,13 +1652,17 @@ static bool bdrv_pad_request(BlockDriverState *bs,
                              int64_t *offset, unsigned int *bytes,
                              BdrvRequestPadding *pad)
 {
+    int ret;
+
     if (!bdrv_init_padding(bs, *offset, *bytes, pad)) {
         return false;
     }
 
-    qemu_iovec_init_extended(&pad->local_qiov, pad->buf, pad->head,
-                             *qiov, *qiov_offset, *bytes,
-                             pad->buf + pad->buf_len - pad->tail, pad->tail);
+    ret = qemu_iovec_init_extended(&pad->local_qiov, pad->buf, pad->head,
+                                   *qiov, *qiov_offset, *bytes,
+                                   pad->buf + pad->buf_len - pad->tail,
+                                   pad->tail);
+    assert(ret == 0);
     *bytes += pad->head + pad->tail;
     *offset -= pad->head;
     *qiov = &pad->local_qiov;
diff --git a/util/iov.c b/util/iov.c
index f3a9e92a37..58c7b3eeee 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -415,7 +415,7 @@ int qemu_iovec_subvec_niov(QEMUIOVector *qiov, size_t offset, size_t len)
  * Compile new iovec, combining @head_buf buffer, sub-qiov of @mid_qiov,
  * and @tail_buf buffer into new qiov.
  */
-void qemu_iovec_init_extended(
+int qemu_iovec_init_extended(
         QEMUIOVector *qiov,
         void *head_buf, size_t head_len,
         QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len,
@@ -425,12 +425,24 @@ void qemu_iovec_init_extended(
     int total_niov, mid_niov = 0;
     struct iovec *p, *mid_iov = NULL;
 
+    assert(mid_qiov->niov <= IOV_MAX);
+
+    if (SIZE_MAX - head_len < mid_len ||
+        SIZE_MAX - head_len - mid_len < tail_len)
+    {
+        return -EINVAL;
+    }
+
     if (mid_len) {
         mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,
                              &mid_head, &mid_tail, &mid_niov);
     }
 
     total_niov = !!head_len + mid_niov + !!tail_len;
+    if (total_niov > IOV_MAX) {
+        return -EINVAL;
+    }
+
     if (total_niov == 1) {
         qemu_iovec_init_buf(qiov, NULL, 0);
         p = &qiov->local_iov;
@@ -459,6 +471,8 @@ void qemu_iovec_init_extended(
         p->iov_base = tail_buf;
         p->iov_len = tail_len;
     }
+
+    return 0;
 }
 
 /*
@@ -492,7 +506,14 @@ bool qemu_iovec_is_zero(QEMUIOVector *qiov, size_t offset, size_t bytes)
 void qemu_iovec_init_slice(QEMUIOVector *qiov, QEMUIOVector *source,
                            size_t offset, size_t len)
 {
-    qemu_iovec_init_extended(qiov, NULL, 0, source, offset, len, NULL, 0);
+    int ret;
+
+    assert(source->size >= len);
+    assert(source->size - len >= offset);
+
+    /* We shrink the request, so we can't overflow neither size_t nor MAX_IOV */
+    ret = qemu_iovec_init_extended(qiov, NULL, 0, source, offset, len, NULL, 0);
+    assert(ret == 0);
 }
 
 void qemu_iovec_destroy(QEMUIOVector *qiov)
-- 
2.25.4

Re: [PATCH v4 02/16] util/iov: make qemu_iovec_init_extended() honest

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Actually, we can't extend the io vector in all cases. Handle possible
> MAX_IOV and size_t overflows.
> 
> For now add assertion to callers (actually they rely on success anyway)
> and fix them in the following patch.
> 
> Add also some additional good assertions to qemu_iovec_init_slice()
> while being here.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qemu/iov.h |  2 +-
>  block/io.c         | 10 +++++++---
>  util/iov.c         | 25 +++++++++++++++++++++++--
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 

> @@ -492,7 +506,14 @@ bool qemu_iovec_is_zero(QEMUIOVector *qiov, size_t offset, size_t bytes)
>  void qemu_iovec_init_slice(QEMUIOVector *qiov, QEMUIOVector *source,
>                             size_t offset, size_t len)
>  {
> -    qemu_iovec_init_extended(qiov, NULL, 0, source, offset, len, NULL, 0);
> +    int ret;
> +
> +    assert(source->size >= len);
> +    assert(source->size - len >= offset);
> +
> +    /* We shrink the request, so we can't overflow neither size_t nor MAX_IOV */

We shrink the request, so neither size_t nor MAX_IOV will overflow

> +    ret = qemu_iovec_init_extended(qiov, NULL, 0, source, offset, len, NULL, 0);
> +    assert(ret == 0);
>  }
>  
>  void qemu_iovec_destroy(QEMUIOVector *qiov)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 03/16] block: fix theoretical overflow in bdrv_init_padding()

[Vladimir Sementsov-Ogievskiy]

Calculation of sum may theoretically overflow, so use 64bit type and
add some good assertions.

Use int64_t constantly.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 21e8a50725..d9bc67f1b0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1537,8 +1537,12 @@ static bool bdrv_init_padding(BlockDriverState *bs,
                               int64_t offset, int64_t bytes,
                               BdrvRequestPadding *pad)
 {
-    uint64_t align = bs->bl.request_alignment;
-    size_t sum;
+    int64_t align = bs->bl.request_alignment;
+    int64_t sum;
+
+    bdrv_check_request(offset, bytes, &error_abort);
+    assert(align <= INT_MAX); /* documented in block/block_int.h */
+    assert(align * 2 <= SIZE_MAX); /* so we can allocate the buffer */
 
     memset(pad, 0, sizeof(*pad));
 
-- 
2.25.4

Re: [PATCH v4 03/16] block: fix theoretical overflow in bdrv_init_padding()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Calculation of sum may theoretically overflow, so use 64bit type and
> add some good assertions.
> 
> Use int64_t constantly.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 21e8a50725..d9bc67f1b0 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1537,8 +1537,12 @@ static bool bdrv_init_padding(BlockDriverState *bs,
>                                int64_t offset, int64_t bytes,
>                                BdrvRequestPadding *pad)
>  {
> -    uint64_t align = bs->bl.request_alignment;
> -    size_t sum;
> +    int64_t align = bs->bl.request_alignment;
> +    int64_t sum;
> +
> +    bdrv_check_request(offset, bytes, &error_abort);
> +    assert(align <= INT_MAX); /* documented in block/block_int.h */
> +    assert(align * 2 <= SIZE_MAX); /* so we can allocate the buffer */

Would look better as assert(align <= SIZE_MAX / 2); but not technically
wrong as written because the earlier line proved align is less than 32
bits so align*2 can't overflow 63 bits.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 04/16] block/io: refactor bdrv_pad_request(): move bdrv_pad_request() up

[Vladimir Sementsov-Ogievskiy]

Prepare to the following patch when bdrv_pad_request() will be able to
fail. Update the comments.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index d9bc67f1b0..dcfab267f8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2107,6 +2107,7 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
     uint64_t align = bs->bl.request_alignment;
     BdrvRequestPadding pad;
     int ret;
+    bool padded = false;
 
     trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
 
@@ -2138,20 +2139,32 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
         return 0;
     }
 
+    if (!(flags & BDRV_REQ_ZERO_WRITE)) {
+        /*
+         * Pad request for following read-modify-write cycle.
+         * bdrv_co_do_zero_pwritev() does aligning by itself, so, we do
+         * alignment only if there is no ZERO flag.
+         */
+        padded = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes,
+                                  &pad);
+    }
+
     bdrv_inc_in_flight(bs);
-    /*
-     * Align write if necessary by performing a read-modify-write cycle.
-     * Pad qiov with the read parts and be sure to have a tracked request not
-     * only for bdrv_aligned_pwritev, but also for the reads of the RMW cycle.
-     */
     tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_WRITE);
 
     if (flags & BDRV_REQ_ZERO_WRITE) {
+        assert(!padded);
         ret = bdrv_co_do_zero_pwritev(child, offset, bytes, flags, &req);
         goto out;
     }
 
-    if (bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad)) {
+    if (padded) {
+        /*
+         * Request was unaligned to request_alignment and therefore padded.
+         * We are going to do read-modify-write. User is not prepared to widened
+         * request intersections with other requests, so we serialize the
+         * request.
+         */
         bdrv_mark_request_serialising(&req, align);
         bdrv_padding_rmw_read(child, &req, &pad, false);
     }
-- 
2.25.4

Re: [PATCH v4 04/16] block/io: refactor bdrv_pad_request(): move bdrv_pad_request() up

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Prepare to the following patch when bdrv_pad_request() will be able to

s/Prepare to/Prepare for/

> fail. Update the comments.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 

> -    if (bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad)) {
> +    if (padded) {
> +        /*
> +         * Request was unaligned to request_alignment and therefore padded.
> +         * We are going to do read-modify-write. User is not prepared to widened
> +         * request intersections with other requests, so we serialize the
> +         * request.

We are going to do read-modify-write, and must serialize the request to
prevent interactions of the widened region with other transactions.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 05/16] block/io: bdrv_pad_request(): support qemu_iovec_init_extended failure

[Vladimir Sementsov-Ogievskiy]

Make bdrv_pad_request() honest: return error if
qemu_iovec_init_extended() failed.

Update also bdrv_padding_destroy() to clean the structure for safety.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/block/io.c b/block/io.c
index dcfab267f8..4a057660f8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1637,6 +1637,7 @@ static void bdrv_padding_destroy(BdrvRequestPadding *pad)
         qemu_vfree(pad->buf);
         qemu_iovec_destroy(&pad->local_qiov);
     }
+    memset(pad, 0, sizeof(*pad));
 }
 
 /*
@@ -1646,33 +1647,42 @@ static void bdrv_padding_destroy(BdrvRequestPadding *pad)
  * read of padding, bdrv_padding_rmw_read() should be called separately if
  * needed.
  *
- * All parameters except @bs are in-out: they represent original request at
- * function call and padded (if padding needed) at function finish.
- *
- * Function always succeeds.
+ * Request parameters (@qiov, &qiov_offset, &offset, &bytes) are in-out:
+ *  - on function start they represent original request
+ *  - on failure or when padding is not needed they are unchanged
+ *  - on success when padding is needed they represent padded request
  */
-static bool bdrv_pad_request(BlockDriverState *bs,
-                             QEMUIOVector **qiov, size_t *qiov_offset,
-                             int64_t *offset, unsigned int *bytes,
-                             BdrvRequestPadding *pad)
+static int bdrv_pad_request(BlockDriverState *bs,
+                            QEMUIOVector **qiov, size_t *qiov_offset,
+                            int64_t *offset, unsigned int *bytes,
+                            BdrvRequestPadding *pad, bool *padded)
 {
     int ret;
 
     if (!bdrv_init_padding(bs, *offset, *bytes, pad)) {
-        return false;
+        if (padded) {
+            *padded = false;
+        }
+        return 0;
     }
 
     ret = qemu_iovec_init_extended(&pad->local_qiov, pad->buf, pad->head,
                                    *qiov, *qiov_offset, *bytes,
                                    pad->buf + pad->buf_len - pad->tail,
                                    pad->tail);
-    assert(ret == 0);
+    if (ret < 0) {
+        bdrv_padding_destroy(pad);
+        return ret;
+    }
     *bytes += pad->head + pad->tail;
     *offset -= pad->head;
     *qiov = &pad->local_qiov;
     *qiov_offset = 0;
+    if (padded) {
+        *padded = true;
+    }
 
-    return true;
+    return 0;
 }
 
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
@@ -1722,7 +1732,11 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
         flags |= BDRV_REQ_COPY_ON_READ;
     }
 
-    bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad);
+    ret = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad,
+                           NULL);
+    if (ret < 0) {
+        return ret;
+    }
 
     tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ);
     ret = bdrv_aligned_preadv(child, &req, offset, bytes,
@@ -2145,8 +2159,11 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
          * bdrv_co_do_zero_pwritev() does aligning by itself, so, we do
          * alignment only if there is no ZERO flag.
          */
-        padded = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes,
-                                  &pad);
+        ret = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad,
+                               &padded);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     bdrv_inc_in_flight(bs);
-- 
2.25.4

Re: [PATCH v4 05/16] block/io: bdrv_pad_request(): support qemu_iovec_init_extended failure

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make bdrv_pad_request() honest: return error if
> qemu_iovec_init_extended() failed.
> 
> Update also bdrv_padding_destroy() to clean the structure for safety.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 06/16] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

[Vladimir Sementsov-Ogievskiy]

The function is called from 64bit io handlers, and bytes is just passed
to throttle_account() which is 64bit too (unsigned though). So, let's
convert intermediate argument to 64bit too.

This patch is a first in the 64-bit-blocklayer series, so we are
generally moving to int64_t for both offset and bytes parameters on all
io paths. Main motivation is realization of 64-bit write_zeroes
operation for fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

Patch-correctness audit by Eric Blake:

  Caller has 32-bit, this patch now causes widening which is safe:
  block/block-backend.c: blk_do_preadv() passes 'unsigned int'
  block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
  block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
  block/throttle.c: throttle_co_pdiscard() passes 'int'

  Caller has 64-bit, this patch fixes potential bug where pre-patch
  could narrow, except it's easy enough to trace that callers are still
  capped at 2G actions:
  block/throttle.c: throttle_co_preadv() passes 'uint64_t'
  block/throttle.c: throttle_co_pwritev() passes 'uint64_t'

  Implementation in question: block/throttle-groups.c
  throttle_group_co_io_limits_intercept() takes 'unsigned int bytes'
  and uses it: argument to util/throttle.c throttle_account(uint64_t)

  All safe: it patches a latent bug, and does not introduce any 64-bit
  gotchas once throttle_co_p{read,write}v are relaxed, and assuming
  throttle_account() is not buggy.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 include/block/throttle-groups.h | 2 +-
 block/throttle-groups.c         | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 8bf7d233fa..9541b32432 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -77,7 +77,7 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
 
 void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
-                                                        unsigned int bytes,
+                                                        int64_t bytes,
                                                         bool is_write);
 void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
                                        AioContext *new_context);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index abd16ed9db..fb203c3ced 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -358,12 +358,15 @@ static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
  * @is_write:  the type of operation (read/write)
  */
 void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
-                                                        unsigned int bytes,
+                                                        int64_t bytes,
                                                         bool is_write)
 {
     bool must_wait;
     ThrottleGroupMember *token;
     ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
+
+    assert(bytes >= 0);
+
     qemu_mutex_lock(&tg->lock);
 
     /* First we check if this I/O has to be throttled. */
-- 
2.25.4

[PATCH v4 07/16] block/io: improve bdrv_check_request: check qiov too

[Vladimir Sementsov-Ogievskiy]

Operations with qiov add more restrictions on bytes, let's cover it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4a057660f8..42e687a388 100644
--- a/block/io.c
+++ b/block/io.c
@@ -898,8 +898,14 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
     return waited;
 }
 
-int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
+static int bdrv_check_qiov_request(int64_t offset, int64_t bytes,
+                                   QEMUIOVector *qiov, size_t qiov_offset,
+                                   Error **errp)
 {
+    /*
+     * Check generic offset/bytes correctness
+     */
+
     if (offset < 0) {
         error_setg(errp, "offset is negative: %" PRIi64, offset);
         return -EIO;
@@ -929,12 +935,38 @@ int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
         return -EIO;
     }
 
+    if (!qiov) {
+        return 0;
+    }
+
+    /*
+     * Check qiov and qiov_offset
+     */
+
+    if (qiov_offset > qiov->size) {
+        error_setg(errp, "qiov_offset(%zu) overflow io vector size(%zu)",
+                   qiov_offset, qiov->size);
+        return -EIO;
+    }
+
+    if (bytes > qiov->size - qiov_offset) {
+        error_setg(errp, "bytes(%" PRIi64 ") + qiov_offset(%zu) overflow io "
+                   "vector size(%zu)", bytes, qiov_offset, qiov->size);
+        return -EIO;
+    }
+
     return 0;
 }
 
-static int bdrv_check_request32(int64_t offset, int64_t bytes)
+int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
+{
+    return bdrv_check_qiov_request(offset, bytes, NULL, 0, errp);
+}
+
+static int bdrv_check_request32(int64_t offset, int64_t bytes,
+                                QEMUIOVector *qiov, size_t qiov_offset)
 {
-    int ret = bdrv_check_request(offset, bytes, NULL);
+    int ret = bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -1708,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
         return -ENOMEDIUM;
     }
 
-    ret = bdrv_check_request32(offset, bytes);
+    ret = bdrv_check_request32(offset, bytes, qiov, qiov_offset);
     if (ret < 0) {
         return ret;
     }
@@ -2129,7 +2161,7 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
         return -ENOMEDIUM;
     }
 
-    ret = bdrv_check_request32(offset, bytes);
+    ret = bdrv_check_request32(offset, bytes, qiov, qiov_offset);
     if (ret < 0) {
         return ret;
     }
@@ -3135,7 +3167,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
     if (!dst || !dst->bs || !bdrv_is_inserted(dst->bs)) {
         return -ENOMEDIUM;
     }
-    ret = bdrv_check_request32(dst_offset, bytes);
+    ret = bdrv_check_request32(dst_offset, bytes, NULL, 0);
     if (ret) {
         return ret;
     }
@@ -3146,7 +3178,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
     if (!src || !src->bs || !bdrv_is_inserted(src->bs)) {
         return -ENOMEDIUM;
     }
-    ret = bdrv_check_request32(src_offset, bytes);
+    ret = bdrv_check_request32(src_offset, bytes, NULL, 0);
     if (ret) {
         return ret;
     }
-- 
2.25.4

Re: [PATCH v4 07/16] block/io: improve bdrv_check_request: check qiov too

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Operations with qiov add more restrictions on bytes, let's cover it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 4a057660f8..42e687a388 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -898,8 +898,14 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>      return waited;
>  }
>  
> -int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
> +static int bdrv_check_qiov_request(int64_t offset, int64_t bytes,
> +                                   QEMUIOVector *qiov, size_t qiov_offset,
> +                                   Error **errp)
>  {
> +    /*
> +     * Check generic offset/bytes correctness
> +     */
> +
>      if (offset < 0) {
>          error_setg(errp, "offset is negative: %" PRIi64, offset);
>          return -EIO;
> @@ -929,12 +935,38 @@ int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
>          return -EIO;
>      }
>  
> +    if (!qiov) {
> +        return 0;
> +    }

I guess this short circuit is for write zeroes...

> +
> +    /*
> +     * Check qiov and qiov_offset
> +     */
> +
> +    if (qiov_offset > qiov->size) {
> +        error_setg(errp, "qiov_offset(%zu) overflow io vector size(%zu)",
> +                   qiov_offset, qiov->size);
> +        return -EIO;
> +    }
> +
> +    if (bytes > qiov->size - qiov_offset) {
> +        error_setg(errp, "bytes(%" PRIi64 ") + qiov_offset(%zu) overflow io "
> +                   "vector size(%zu)", bytes, qiov_offset, qiov->size);
> +        return -EIO;
> +    }

Yes, worthwhile additions.

> @@ -3135,7 +3167,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>      if (!dst || !dst->bs || !bdrv_is_inserted(dst->bs)) {
>          return -ENOMEDIUM;
>      }
> -    ret = bdrv_check_request32(dst_offset, bytes);
> +    ret = bdrv_check_request32(dst_offset, bytes, NULL, 0);

...ah, it's also for copy_range; basically any caller that isn't using a
qiov and therefore can't overflow qiov bounds.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 08/16] block: use int64_t as bytes type in tracked requests

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

All requests in block/io must not overflow BDRV_MAX_LENGTH, all
external users of BdrvTrackedRequest already have corresponding
assertions, so we are safe. Add some assertions still.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  4 ++--
 block/io.c                | 14 +++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ff29f31451..04c1e5cb58 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -79,12 +79,12 @@ enum BdrvTrackedRequestType {
 typedef struct BdrvTrackedRequest {
     BlockDriverState *bs;
     int64_t offset;
-    uint64_t bytes;
+    int64_t bytes;
     enum BdrvTrackedRequestType type;
 
     bool serialising;
     int64_t overlap_offset;
-    uint64_t overlap_bytes;
+    int64_t overlap_bytes;
 
     QLIST_ENTRY(BdrvTrackedRequest) list;
     Coroutine *co; /* owner, used for deadlock detection */
diff --git a/block/io.c b/block/io.c
index 42e687a388..5dec6ab925 100644
--- a/block/io.c
+++ b/block/io.c
@@ -717,10 +717,10 @@ static void tracked_request_end(BdrvTrackedRequest *req)
 static void tracked_request_begin(BdrvTrackedRequest *req,
                                   BlockDriverState *bs,
                                   int64_t offset,
-                                  uint64_t bytes,
+                                  int64_t bytes,
                                   enum BdrvTrackedRequestType type)
 {
-    assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
+    bdrv_check_request(offset, bytes, &error_abort);
 
     *req = (BdrvTrackedRequest){
         .bs = bs,
@@ -741,8 +741,10 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
 }
 
 static bool tracked_request_overlaps(BdrvTrackedRequest *req,
-                                     int64_t offset, uint64_t bytes)
+                                     int64_t offset, int64_t bytes)
 {
+    bdrv_check_request(offset, bytes, &error_abort);
+
     /*        aaaa   bbbb */
     if (offset >= req->overlap_offset + req->overlap_bytes) {
         return false;
@@ -798,10 +800,12 @@ bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
 {
     BlockDriverState *bs = req->bs;
     int64_t overlap_offset = req->offset & ~(align - 1);
-    uint64_t overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
-                               - overlap_offset;
+    int64_t overlap_bytes =
+        ROUND_UP(req->offset + req->bytes, align) - overlap_offset;
     bool waited;
 
+    bdrv_check_request(req->offset, req->bytes, &error_abort);
+
     qemu_co_mutex_lock(&bs->reqs_lock);
     if (!req->serialising) {
         qatomic_inc(&req->bs->serialising_in_flight);
-- 
2.25.4

Re: [PATCH v4 08/16] block: use int64_t as bytes type in tracked requests

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> All requests in block/io must not overflow BDRV_MAX_LENGTH, all
> external users of BdrvTrackedRequest already have corresponding
> assertions, so we are safe. Add some assertions still.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |  4 ++--
>  block/io.c                | 14 +++++++++-----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 09/16] block/io: use int64_t bytes in driver wrappers

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, convert driver wrappers parameters which are already 64bit to
signed type.

Requests in block/io.c must never exceed BDRV_MAX_LENGTH (which is less
than INT64_MAX), which makes the conversion to signed 64bit type safe.

Add corresponding assertions.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 5dec6ab925..b2bf18038b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1103,7 +1103,7 @@ static void bdrv_co_io_em_complete(void *opaque, int ret)
 }
 
 static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
-                                           uint64_t offset, uint64_t bytes,
+                                           int64_t offset, int64_t bytes,
                                            QEMUIOVector *qiov,
                                            size_t qiov_offset, int flags)
 {
@@ -1113,6 +1113,7 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
     QEMUIOVector local_qiov;
     int ret;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
     assert(!(flags & ~BDRV_REQ_MASK));
     assert(!(flags & BDRV_REQ_NO_FALLBACK));
 
@@ -1172,7 +1173,7 @@ out:
 }
 
 static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
-                                            uint64_t offset, uint64_t bytes,
+                                            int64_t offset, int64_t bytes,
                                             QEMUIOVector *qiov,
                                             size_t qiov_offset, int flags)
 {
@@ -1182,6 +1183,7 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
     QEMUIOVector local_qiov;
     int ret;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
     assert(!(flags & ~BDRV_REQ_MASK));
     assert(!(flags & BDRV_REQ_NO_FALLBACK));
 
@@ -1252,14 +1254,16 @@ emulate_flags:
 }
 
 static int coroutine_fn
-bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
-                               uint64_t bytes, QEMUIOVector *qiov,
+bdrv_driver_pwritev_compressed(BlockDriverState *bs, int64_t offset,
+                               int64_t bytes, QEMUIOVector *qiov,
                                size_t qiov_offset)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector local_qiov;
     int ret;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
+
     if (!drv) {
         return -ENOMEDIUM;
     }
-- 
2.25.4

Re: [PATCH v4 09/16] block/io: use int64_t bytes in driver wrappers

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, convert driver wrappers parameters which are already 64bit to
> signed type.
> 
> Requests in block/io.c must never exceed BDRV_MAX_LENGTH (which is less
> than INT64_MAX), which makes the conversion to signed 64bit type safe.
> 
> Add corresponding assertions.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 10/16] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, prepare bdrv_co_do_pwrite_zeroes() now.

Callers are safe, as converting int to int64_t is safe. Concentrate on
'bytes' usage in the function (thx to Eric Blake):

    compute 'int tail' via % 'int alignment' - safe
    fragmentation loop 'int num' - still fragments with a cap on
      max_transfer

    use of 'num' within the loop
    MIN(bytes, max_transfer) as well as %alignment - still works, so
         calculations in if (head) {} are safe
    clamp size by 'int max_write_zeroes' - safe
    drv->bdrv_co_pwrite_zeroes(int) - safe because of clamping
    clamp size by 'int max_transfer' - safe
    buf allocation is still clamped to max_transfer
    qemu_iovec_init_buf(size_t) - safe because of clamping
    bdrv_driver_pwritev(uint64_t) - safe

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index b2bf18038b..c6a476559a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -41,7 +41,7 @@
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags);
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags);
 
 static void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
                                       bool ignore_bds_parents)
@@ -1791,7 +1791,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
 }
 
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags)
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
@@ -1806,6 +1806,8 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
                         bs->bl.request_alignment);
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
 
+    bdrv_check_request(offset, bytes, &error_abort);
+
     if (!drv) {
         return -ENOMEDIUM;
     }
@@ -1821,7 +1823,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     assert(max_write_zeroes >= bs->bl.request_alignment);
 
     while (bytes > 0 && !ret) {
-        int num = bytes;
+        int64_t num = bytes;
 
         /* Align request.  Block drivers can expect the "bulk" of the request
          * to be aligned, and that unaligned requests do not cross cluster
-- 
2.25.4

Re: [PATCH v4 10/16] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, prepare bdrv_co_do_pwrite_zeroes() now.
> 
> Callers are safe, as converting int to int64_t is safe. Concentrate on
> 'bytes' usage in the function (thx to Eric Blake):
> 
>     compute 'int tail' via % 'int alignment' - safe
>     fragmentation loop 'int num' - still fragments with a cap on
>       max_transfer
> 
>     use of 'num' within the loop
>     MIN(bytes, max_transfer) as well as %alignment - still works, so
>          calculations in if (head) {} are safe
>     clamp size by 'int max_write_zeroes' - safe
>     drv->bdrv_co_pwrite_zeroes(int) - safe because of clamping
>     clamp size by 'int max_transfer' - safe
>     buf allocation is still clamped to max_transfer
>     qemu_iovec_init_buf(size_t) - safe because of clamping
>     bdrv_driver_pwritev(uint64_t) - safe
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

And thanks for including the gist of my previous audit - that greatly
speeds things up this time through. ;)

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

[PATCH v4 11/16] block/io: support int64_t bytes in bdrv_aligned_pwritev()

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, prepare bdrv_aligned_pwritev() now and convert the dependencies:
bdrv_co_write_req_prepare() and bdrv_co_write_req_finish() to signed
type bytes.

Conversion of bdrv_co_write_req_prepare() and
bdrv_co_write_req_finish() is definitely safe, as all requests in
block/io must not overflow BDRV_MAX_LENGTH. Still add assertions.

For bdrv_aligned_pwritev() 'bytes' type is widened, so callers are
safe. Let's check usage of the parameter inside the function.

Passing to bdrv_co_write_req_prepare() and bdrv_co_write_req_finish()
is OK.

Passing to qemu_iovec_* is OK after new assertion. All other callees
are already updated to int64_t.

Checking alignment is not changed, offset + bytes and qiov_offset +
bytes calculations are safe (thanks to new assertions).

max_transfer is kept to be int for now. It has a default of INT_MAX
here, and some drivers may rely on it. It's to be refactored later.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/io.c b/block/io.c
index c6a476559a..b499998f54 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1904,12 +1904,13 @@ fail:
 }
 
 static inline int coroutine_fn
-bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
+bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
                           BdrvTrackedRequest *req, int flags)
 {
     BlockDriverState *bs = child->bs;
     bool waited;
-    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
+    bdrv_check_request(offset, bytes, &error_abort);
 
     if (bs->read_only) {
         return -EPERM;
@@ -1935,7 +1936,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
 
     assert(req->overlap_offset <= offset);
     assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
-    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
+    assert(offset + bytes <= bs->total_sectors * BDRV_SECTOR_SIZE ||
+           child->perm & BLK_PERM_RESIZE);
 
     switch (req->type) {
     case BDRV_TRACKED_WRITE:
@@ -1956,12 +1958,14 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
 }
 
 static inline void coroutine_fn
-bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, uint64_t bytes,
+bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, int64_t bytes,
                          BdrvTrackedRequest *req, int ret)
 {
     int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
     BlockDriverState *bs = child->bs;
 
+    bdrv_check_request(offset, bytes, &error_abort);
+
     qatomic_inc(&bs->write_gen);
 
     /*
@@ -1998,16 +2002,18 @@ bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, uint64_t bytes,
  * after possibly fragmenting it.
  */
 static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
-    BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
+    BdrvTrackedRequest *req, int64_t offset, int64_t bytes,
     int64_t align, QEMUIOVector *qiov, size_t qiov_offset, int flags)
 {
     BlockDriverState *bs = child->bs;
     BlockDriver *drv = bs->drv;
     int ret;
 
-    uint64_t bytes_remaining = bytes;
+    int64_t bytes_remaining = bytes;
     int max_transfer;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
+
     if (!drv) {
         return -ENOMEDIUM;
     }
@@ -2019,7 +2025,6 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     assert(is_power_of_2(align));
     assert((offset & (align - 1)) == 0);
     assert((bytes & (align - 1)) == 0);
-    assert(!qiov || qiov_offset + bytes <= qiov->size);
     max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, INT_MAX),
                                    align);
 
@@ -2118,7 +2123,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
     assert(!bytes || (offset & (align - 1)) == 0);
     if (bytes >= align) {
         /* Write the aligned part in the middle. */
-        uint64_t aligned_bytes = bytes & ~(align - 1);
+        int64_t aligned_bytes = bytes & ~(align - 1);
         ret = bdrv_aligned_pwritev(child, req, offset, aligned_bytes, align,
                                    NULL, 0, flags);
         if (ret < 0) {
-- 
2.25.4

Re: [PATCH v4 11/16] block/io: support int64_t bytes in bdrv_aligned_pwritev()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, prepare bdrv_aligned_pwritev() now and convert the dependencies:
> bdrv_co_write_req_prepare() and bdrv_co_write_req_finish() to signed
> type bytes.
> 
> Conversion of bdrv_co_write_req_prepare() and
> bdrv_co_write_req_finish() is definitely safe, as all requests in
> block/io must not overflow BDRV_MAX_LENGTH. Still add assertions.
> 
> For bdrv_aligned_pwritev() 'bytes' type is widened, so callers are
> safe. Let's check usage of the parameter inside the function.
> 
> Passing to bdrv_co_write_req_prepare() and bdrv_co_write_req_finish()
> is OK.
> 
> Passing to qemu_iovec_* is OK after new assertion. All other callees
> are already updated to int64_t.
> 
> Checking alignment is not changed, offset + bytes and qiov_offset +
> bytes calculations are safe (thanks to new assertions).
> 
> max_transfer is kept to be int for now. It has a default of INT_MAX
> here, and some drivers may rely on it. It's to be refactored later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 12/16] block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, prepare bdrv_co_do_copy_on_readv() now.

'bytes' type widening, so callers are safe. Look at the function
itself:

bytes, skip_bytes and progress become int64_t.

bdrv_round_to_clusters() is OK, cluster_bytes now may be large.
trace_bdrv_co_do_copy_on_readv() is OK

looping through cluster_bytes is still OK.

pnum is still capped to max_transfer, and to MAX_BOUNCE_BUFFER when we
are going to do COR operation. Therefor calculations in
qemu_iovec_from_buf() and bdrv_driver_preadv() should not change.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c         | 8 +++++---
 block/trace-events | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index b499998f54..d8c07fac56 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1289,7 +1289,7 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, int64_t offset,
 }
 
 static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
-        int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+        int64_t offset, int64_t bytes, QEMUIOVector *qiov,
         size_t qiov_offset, int flags)
 {
     BlockDriverState *bs = child->bs;
@@ -1304,13 +1304,15 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     BlockDriver *drv = bs->drv;
     int64_t cluster_offset;
     int64_t cluster_bytes;
-    size_t skip_bytes;
+    int64_t skip_bytes;
     int ret;
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
                                     BDRV_REQUEST_MAX_BYTES);
-    unsigned int progress = 0;
+    int64_t progress = 0;
     bool skip_write;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
+
     if (!drv) {
         return -ENOMEDIUM;
     }
diff --git a/block/trace-events b/block/trace-events
index 8368f4acb0..a5f6ffb7da 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -14,7 +14,7 @@ blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, int64_t bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %" PRId64 " bytes %" PRId64 " cluster_offset %" PRId64 " cluster_bytes %" PRId64
 bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
 bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
 
-- 
2.25.4

Re: [PATCH v4 12/16] block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, prepare bdrv_co_do_copy_on_readv() now.
> 
> 'bytes' type widening, so callers are safe. Look at the function
> itself:
> 
> bytes, skip_bytes and progress become int64_t.
> 
> bdrv_round_to_clusters() is OK, cluster_bytes now may be large.
> trace_bdrv_co_do_copy_on_readv() is OK
> 
> looping through cluster_bytes is still OK.
> 
> pnum is still capped to max_transfer, and to MAX_BOUNCE_BUFFER when we
> are going to do COR operation. Therefor calculations in
> qemu_iovec_from_buf() and bdrv_driver_preadv() should not change.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c         | 8 +++++---
>  block/trace-events | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 13/16] block/io: support int64_t bytes in bdrv_aligned_preadv()

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, prepare bdrv_aligned_preadv() now.

Make byte variable in bdrv_padding_rmw_read() int64_t, as it defined
only to be passed to bdrv_aligned_preadv().

All bdrv_aligned_preadv() callers are safe as type is widening. Let's
look inside:

 - add a new-style assertion that request is good.
 - callees bdrv_is_allocated(), bdrv_co_do_copy_on_readv() supports
   int64_t bytes
 - conversion of bytes_remaining is OK, as we never has requests
   overflowing BDRV_MAX_LENGTH
 - looping through bytes_remaining is ok, num is updated to int64_t
   - for bdrv_driver_preadv we have same limit of max_transfer
   - qemu_iovec_memset is OK, as bytes+qiov_offset should not overflow
     qiov->size anyway (thanks to bdrv_check_qiov_request())

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index d8c07fac56..93a89a56e3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1453,15 +1453,16 @@ err:
  * reads; any other features must be implemented by the caller.
  */
 static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
-    BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
+    BdrvTrackedRequest *req, int64_t offset, int64_t bytes,
     int64_t align, QEMUIOVector *qiov, size_t qiov_offset, int flags)
 {
     BlockDriverState *bs = child->bs;
     int64_t total_bytes, max_bytes;
     int ret = 0;
-    uint64_t bytes_remaining = bytes;
+    int64_t bytes_remaining = bytes;
     int max_transfer;
 
+    bdrv_check_qiov_request(offset, bytes, qiov, qiov_offset, &error_abort);
     assert(is_power_of_2(align));
     assert((offset & (align - 1)) == 0);
     assert((bytes & (align - 1)) == 0);
@@ -1518,7 +1519,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
     }
 
     while (bytes_remaining) {
-        int num;
+        int64_t num;
 
         if (max_bytes) {
             num = MIN(bytes_remaining, MIN(max_bytes, max_transfer));
@@ -1624,7 +1625,7 @@ static int bdrv_padding_rmw_read(BdrvChild *child,
     assert(req->serialising && pad->buf);
 
     if (pad->head || pad->merge_reads) {
-        uint64_t bytes = pad->merge_reads ? pad->buf_len : align;
+        int64_t bytes = pad->merge_reads ? pad->buf_len : align;
 
         qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
 
-- 
2.25.4

Re: [PATCH v4 13/16] block/io: support int64_t bytes in bdrv_aligned_preadv()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, prepare bdrv_aligned_preadv() now.
> 
> Make byte variable in bdrv_padding_rmw_read() int64_t, as it defined
> only to be passed to bdrv_aligned_preadv().

Reads awkwardly, how about:

Make the byte variable in bdrv_padding_rmw_read() int64_t, as it is only
used for pass-through to bdrv_aligned_preadv().

> 
> All bdrv_aligned_preadv() callers are safe as type is widening. Let's
> look inside:
> 
>  - add a new-style assertion that request is good.
>  - callees bdrv_is_allocated(), bdrv_co_do_copy_on_readv() supports
>    int64_t bytes
>  - conversion of bytes_remaining is OK, as we never has requests

have

>    overflowing BDRV_MAX_LENGTH
>  - looping through bytes_remaining is ok, num is updated to int64_t
>    - for bdrv_driver_preadv we have same limit of max_transfer
>    - qemu_iovec_memset is OK, as bytes+qiov_offset should not overflow
>      qiov->size anyway (thanks to bdrv_check_qiov_request())
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v4 13/16] block/io: support int64_t bytes in bdrv_aligned_preadv()

[Vladimir Sementsov-Ogievskiy]

22.01.2021 19:54, Eric Blake wrote:
> On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
>> We are generally moving to int64_t for both offset and bytes parameters
>> on all io paths.
>>
>> Main motivation is realization of 64-bit write_zeroes operation for
>> fast zeroing large disk chunks, up to the whole disk.
>>
>> We chose signed type, to be consistent with off_t (which is signed) and
>> with possibility for signed return type (where negative value means
>> error).
>>
>> So, prepare bdrv_aligned_preadv() now.
>>
>> Make byte variable in bdrv_padding_rmw_read() int64_t, as it defined
>> only to be passed to bdrv_aligned_preadv().
> 
> Reads awkwardly, how about:
> 
> Make the byte variable in bdrv_padding_rmw_read() int64_t, as it is only
> used for pass-through to bdrv_aligned_preadv().

and also s/byte/bytes/

> 
>>
>> All bdrv_aligned_preadv() callers are safe as type is widening. Let's
>> look inside:
>>
>>   - add a new-style assertion that request is good.
>>   - callees bdrv_is_allocated(), bdrv_co_do_copy_on_readv() supports
>>     int64_t bytes
>>   - conversion of bytes_remaining is OK, as we never has requests
> 
> have
> 
>>     overflowing BDRV_MAX_LENGTH
>>   - looping through bytes_remaining is ok, num is updated to int64_t
>>     - for bdrv_driver_preadv we have same limit of max_transfer
>>     - qemu_iovec_memset is OK, as bytes+qiov_offset should not overflow
>>       qiov->size anyway (thanks to bdrv_check_qiov_request())
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir

[PATCH v4 14/16] block/io: support int64_t bytes in bdrv_co_p{read, write}v_part()

[Vladimir Sementsov-Ogievskiy via]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, prepare bdrv_co_preadv_part() and bdrv_co_pwritev_part() and their
remaining dependencies now.

bdrv_pad_request() is updated simultaneously, as pointer to bytes passed
to it both from bdrv_co_pwritev_part() and bdrv_co_preadv_part().

So, all callers of bdrv_pad_request() are updated to pass 64bit bytes.
bdrv_pad_request() is already good for 64bit requests, add
corresponding assertion.

Look at bdrv_co_preadv_part() and bdrv_co_pwritev_part().
Type is widening, so callers are safe. Let's look inside the functions.

In bdrv_co_preadv_part() and bdrv_aligned_pwritev() we only pass bytes
to other already int64_t interfaces (and some obviously safe
calculations), it's OK.

In bdrv_co_do_zero_pwritev() aligned_bytes may become large now, still
it's passed to bdrv_aligned_pwritev which supports int64_t bytes.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  4 ++--
 block/io.c                | 14 ++++++++------
 block/trace-events        |  4 ++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 04c1e5cb58..55b1039872 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1031,13 +1031,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
-    int64_t offset, unsigned int bytes,
+    int64_t offset, int64_t bytes,
     QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
-    int64_t offset, unsigned int bytes,
+    int64_t offset, int64_t bytes,
     QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
 
 static inline int coroutine_fn bdrv_co_pread(BdrvChild *child,
diff --git a/block/io.c b/block/io.c
index 93a89a56e3..5200658224 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1697,11 +1697,13 @@ static void bdrv_padding_destroy(BdrvRequestPadding *pad)
  */
 static int bdrv_pad_request(BlockDriverState *bs,
                             QEMUIOVector **qiov, size_t *qiov_offset,
-                            int64_t *offset, unsigned int *bytes,
+                            int64_t *offset, int64_t *bytes,
                             BdrvRequestPadding *pad, bool *padded)
 {
     int ret;
 
+    bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort);
+
     if (!bdrv_init_padding(bs, *offset, *bytes, pad)) {
         if (padded) {
             *padded = false;
@@ -1736,7 +1738,7 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
 }
 
 int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
-    int64_t offset, unsigned int bytes,
+    int64_t offset, int64_t bytes,
     QEMUIOVector *qiov, size_t qiov_offset,
     BdrvRequestFlags flags)
 {
@@ -1745,7 +1747,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
     BdrvRequestPadding pad;
     int ret;
 
-    trace_bdrv_co_preadv(bs, offset, bytes, flags);
+    trace_bdrv_co_preadv_part(bs, offset, bytes, flags);
 
     if (!bdrv_is_inserted(bs)) {
         return -ENOMEDIUM;
@@ -2089,7 +2091,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
 
 static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
                                                 int64_t offset,
-                                                unsigned int bytes,
+                                                int64_t bytes,
                                                 BdrvRequestFlags flags,
                                                 BdrvTrackedRequest *req)
 {
@@ -2163,7 +2165,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
 }
 
 int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
-    int64_t offset, unsigned int bytes, QEMUIOVector *qiov, size_t qiov_offset,
+    int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t qiov_offset,
     BdrvRequestFlags flags)
 {
     BlockDriverState *bs = child->bs;
@@ -2173,7 +2175,7 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
     int ret;
     bool padded = false;
 
-    trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
+    trace_bdrv_co_pwritev_part(child->bs, offset, bytes, flags);
 
     if (!bdrv_is_inserted(bs)) {
         return -ENOMEDIUM;
diff --git a/block/trace-events b/block/trace-events
index a5f6ffb7da..91a0f70575 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -11,8 +11,8 @@ blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 
 # io.c
-bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
-bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
+bdrv_co_preadv_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
+bdrv_co_pwritev_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
 bdrv_co_do_copy_on_readv(void *bs, int64_t offset, int64_t bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %" PRId64 " bytes %" PRId64 " cluster_offset %" PRId64 " cluster_bytes %" PRId64
 bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
-- 
2.25.4

Re: [PATCH v4 14/16] block/io: support int64_t bytes in bdrv_co_p{read,write}v_part()

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, prepare bdrv_co_preadv_part() and bdrv_co_pwritev_part() and their
> remaining dependencies now.
> 
> bdrv_pad_request() is updated simultaneously, as pointer to bytes passed
> to it both from bdrv_co_pwritev_part() and bdrv_co_preadv_part().
> 
> So, all callers of bdrv_pad_request() are updated to pass 64bit bytes.
> bdrv_pad_request() is already good for 64bit requests, add
> corresponding assertion.
> 
> Look at bdrv_co_preadv_part() and bdrv_co_pwritev_part().
> Type is widening, so callers are safe. Let's look inside the functions.
> 
> In bdrv_co_preadv_part() and bdrv_aligned_pwritev() we only pass bytes
> to other already int64_t interfaces (and some obviously safe
> calculations), it's OK.
> 
> In bdrv_co_do_zero_pwritev() aligned_bytes may become large now, still
> it's passed to bdrv_aligned_pwritev which supports int64_t bytes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |  4 ++--
>  block/io.c                | 14 ++++++++------
>  block/trace-events        |  4 ++--
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 

> @@ -1745,7 +1747,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>      BdrvRequestPadding pad;
>      int ret;
>  
> -    trace_bdrv_co_preadv(bs, offset, bytes, flags);
> +    trace_bdrv_co_preadv_part(bs, offset, bytes, flags);
>  

> @@ -2173,7 +2175,7 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
>      int ret;
>      bool padded = false;
>  
> -    trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
> +    trace_bdrv_co_pwritev_part(child->bs, offset, bytes, flags);

The change in trace names makes sense, but isn't specifically called out
in the commit message.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 15/16] block/io: support int64_t bytes in read/write wrappers

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

Now, when bdrv_co_preadv_part() and bdrv_co_pwritev_part() updated,
update all their wrappers.

For all of them type of 'bytes' is widening, so callers are safe. We
have update request_fn in blkverify.c simultaneusly. Still it's just a
pointer to on of bdrv_co_pwritev() or bdrv_co_preadv(), and type is
widening for callers of the request_fn anyway.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h     | 11 ++++++-----
 include/block/block_int.h |  4 ++--
 block/blkverify.c         |  2 +-
 block/io.c                | 15 ++++++++-------
 block/trace-events        |  2 +-
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 5b81e33e94..3549125f1d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -390,12 +390,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
-                       int bytes, BdrvRequestFlags flags);
+                       int64_t bytes, BdrvRequestFlags flags);
 int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
-int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
+int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int64_t bytes);
+int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf,
+                int64_t bytes);
 int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
-                     const void *buf, int count);
+                     const void *buf, int64_t bytes);
 /*
  * Efficiently zero a region of the disk image.  Note that this is a regular
  * I/O request like read or write and should have a reasonable size.  This
@@ -403,7 +404,7 @@ int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
  * because it may allocate memory for the entire region.
  */
 int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
-                                       int bytes, BdrvRequestFlags flags);
+                                       int64_t bytes, BdrvRequestFlags flags);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 void bdrv_refresh_filename(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 55b1039872..5e482a8f08 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1028,13 +1028,13 @@ extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
-    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+    int64_t offset, int64_t bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
     int64_t offset, int64_t bytes,
     QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
-    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+    int64_t offset, int64_t bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
 int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
     int64_t offset, int64_t bytes,
diff --git a/block/blkverify.c b/block/blkverify.c
index 4aed53ab59..943e62be9c 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -31,7 +31,7 @@ typedef struct BlkverifyRequest {
     uint64_t bytes;
     int flags;
 
-    int (*request_fn)(BdrvChild *, int64_t, unsigned int, QEMUIOVector *,
+    int (*request_fn)(BdrvChild *, int64_t, int64_t, QEMUIOVector *,
                       BdrvRequestFlags);
 
     int ret;                    /* test image result */
diff --git a/block/io.c b/block/io.c
index 5200658224..34dae81fa7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -983,7 +983,7 @@ static int bdrv_check_request32(int64_t offset, int64_t bytes,
 }
 
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
-                       int bytes, BdrvRequestFlags flags)
+                       int64_t bytes, BdrvRequestFlags flags)
 {
     return bdrv_pwritev(child, offset, bytes, NULL,
                         BDRV_REQ_ZERO_WRITE | flags);
@@ -1031,7 +1031,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 }
 
 /* See bdrv_pwrite() for the return codes */
-int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
+int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int64_t bytes)
 {
     int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
@@ -1051,7 +1051,8 @@ int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
   -EINVAL      Invalid offset or number of bytes
   -EACCES      Trying to write a read-only device
 */
-int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
+int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf,
+                int64_t bytes)
 {
     int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
@@ -1072,7 +1073,7 @@ int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
  * Returns 0 on success, -errno in error cases.
  */
 int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
-                     const void *buf, int count)
+                     const void *buf, int64_t count)
 {
     int ret;
 
@@ -1731,7 +1732,7 @@ static int bdrv_pad_request(BlockDriverState *bs,
 }
 
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
-    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+    int64_t offset, int64_t bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
     return bdrv_co_preadv_part(child, offset, bytes, qiov, 0, flags);
@@ -2158,7 +2159,7 @@ out:
  * Handle a write request in coroutine context
  */
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
-    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+    int64_t offset, int64_t bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
     return bdrv_co_pwritev_part(child, offset, bytes, qiov, 0, flags);
@@ -2251,7 +2252,7 @@ out:
 }
 
 int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
-                                       int bytes, BdrvRequestFlags flags)
+                                       int64_t bytes, BdrvRequestFlags flags)
 {
     trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 
diff --git a/block/trace-events b/block/trace-events
index 91a0f70575..a95d711ade 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -13,7 +13,7 @@ blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 # io.c
 bdrv_co_preadv_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
 bdrv_co_pwritev_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
-bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
+bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int64_t bytes, int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
 bdrv_co_do_copy_on_readv(void *bs, int64_t offset, int64_t bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %" PRId64 " bytes %" PRId64 " cluster_offset %" PRId64 " cluster_bytes %" PRId64
 bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
 bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
-- 
2.25.4

Re: [PATCH v4 15/16] block/io: support int64_t bytes in read/write wrappers

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> Now, when bdrv_co_preadv_part() and bdrv_co_pwritev_part() updated,

s/when/since/, s/updated/have been updated/

> update all their wrappers.
> 
> For all of them type of 'bytes' is widening, so callers are safe. We
> have update request_fn in blkverify.c simultaneusly. Still it's just a

simultaneously

> pointer to on of bdrv_co_pwritev() or bdrv_co_preadv(), and type is

one

> widening for callers of the request_fn anyway.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h     | 11 ++++++-----
>  include/block/block_int.h |  4 ++--
>  block/blkverify.c         |  2 +-
>  block/io.c                | 15 ++++++++-------
>  block/trace-events        |  2 +-
>  5 files changed, 18 insertions(+), 16 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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

[PATCH v4 16/16] block/io: use int64_t bytes in copy_range

[Vladimir Sementsov-Ogievskiy]

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, convert now copy_range parameters which are already 64bit to signed
type.

It's safe as we don't work with requests overflowing BDRV_MAX_LENGTH
(which is less than INT64_MAX), and do check the requests in
bdrv_co_copy_range_internal() (by bdrv_check_request32(), which calls
bdrv_check_request()).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h     |  6 +++---
 include/block/block_int.h | 12 ++++++------
 block/io.c                | 22 +++++++++++-----------
 block/trace-events        |  4 ++--
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3549125f1d..88629eb3a6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -843,8 +843,8 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
  *
  * Returns: 0 if succeeded; negative error code if failed.
  **/
-int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
-                                    BdrvChild *dst, uint64_t dst_offset,
-                                    uint64_t bytes, BdrvRequestFlags read_flags,
+int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
+                                    BdrvChild *dst, int64_t dst_offset,
+                                    int64_t bytes, BdrvRequestFlags read_flags,
                                     BdrvRequestFlags write_flags);
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5e482a8f08..cee5cb5f85 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1343,14 +1343,14 @@ void bdrv_dec_in_flight(BlockDriverState *bs);
 
 void blockdev_close_all_bdrv_states(void);
 
-int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
-                                         BdrvChild *dst, uint64_t dst_offset,
-                                         uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, int64_t src_offset,
+                                         BdrvChild *dst, int64_t dst_offset,
+                                         int64_t bytes,
                                          BdrvRequestFlags read_flags,
                                          BdrvRequestFlags write_flags);
-int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
-                                       BdrvChild *dst, uint64_t dst_offset,
-                                       uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, int64_t src_offset,
+                                       BdrvChild *dst, int64_t dst_offset,
+                                       int64_t bytes,
                                        BdrvRequestFlags read_flags,
                                        BdrvRequestFlags write_flags);
 
diff --git a/block/io.c b/block/io.c
index 34dae81fa7..28680a1f64 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3173,8 +3173,8 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host)
 }
 
 static int coroutine_fn bdrv_co_copy_range_internal(
-        BdrvChild *src, uint64_t src_offset, BdrvChild *dst,
-        uint64_t dst_offset, uint64_t bytes,
+        BdrvChild *src, int64_t src_offset, BdrvChild *dst,
+        int64_t dst_offset, int64_t bytes,
         BdrvRequestFlags read_flags, BdrvRequestFlags write_flags,
         bool recurse_src)
 {
@@ -3252,9 +3252,9 @@ static int coroutine_fn bdrv_co_copy_range_internal(
  *
  * See the comment of bdrv_co_copy_range for the parameter and return value
  * semantics. */
-int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
-                                         BdrvChild *dst, uint64_t dst_offset,
-                                         uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, int64_t src_offset,
+                                         BdrvChild *dst, int64_t dst_offset,
+                                         int64_t bytes,
                                          BdrvRequestFlags read_flags,
                                          BdrvRequestFlags write_flags)
 {
@@ -3268,9 +3268,9 @@ int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
  *
  * See the comment of bdrv_co_copy_range for the parameter and return value
  * semantics. */
-int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
-                                       BdrvChild *dst, uint64_t dst_offset,
-                                       uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, int64_t src_offset,
+                                       BdrvChild *dst, int64_t dst_offset,
+                                       int64_t bytes,
                                        BdrvRequestFlags read_flags,
                                        BdrvRequestFlags write_flags)
 {
@@ -3280,9 +3280,9 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
                                        bytes, read_flags, write_flags, false);
 }
 
-int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
-                                    BdrvChild *dst, uint64_t dst_offset,
-                                    uint64_t bytes, BdrvRequestFlags read_flags,
+int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
+                                    BdrvChild *dst, int64_t dst_offset,
+                                    int64_t bytes, BdrvRequestFlags read_flags,
                                     BdrvRequestFlags write_flags)
 {
     return bdrv_co_copy_range_from(src, src_offset,
diff --git a/block/trace-events b/block/trace-events
index a95d711ade..948df081ba 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -15,8 +15,8 @@ bdrv_co_preadv_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags)
 bdrv_co_pwritev_part(void *bs, int64_t offset, int64_t bytes, unsigned int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int64_t bytes, int flags) "bs %p offset %" PRId64 " bytes %" PRId64 " flags 0x%x"
 bdrv_co_do_copy_on_readv(void *bs, int64_t offset, int64_t bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %" PRId64 " bytes %" PRId64 " cluster_offset %" PRId64 " cluster_bytes %" PRId64
-bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
-bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
+bdrv_co_copy_range_from(void *src, int64_t src_offset, void *dst, int64_t dst_offset, int64_t bytes, int read_flags, int write_flags) "src %p offset %" PRId64 " dst %p offset %" PRId64 " bytes %" PRId64 " rw flags 0x%x 0x%x"
+bdrv_co_copy_range_to(void *src, int64_t src_offset, void *dst, int64_t dst_offset, int64_t bytes, int read_flags, int write_flags) "src %p offset %" PRId64 " dst %p offset %" PRId64 " bytes %" PRId64 " rw flags 0x%x 0x%x"
 
 # stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-- 
2.25.4

Re: [PATCH v4 16/16] block/io: use int64_t bytes in copy_range

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, convert now copy_range parameters which are already 64bit to signed
> type.
> 
> It's safe as we don't work with requests overflowing BDRV_MAX_LENGTH
> (which is less than INT64_MAX), and do check the requests in
> bdrv_co_copy_range_internal() (by bdrv_check_request32(), which calls
> bdrv_check_request()).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h     |  6 +++---
>  include/block/block_int.h | 12 ++++++------
>  block/io.c                | 22 +++++++++++-----------
>  block/trace-events        |  4 ++--
>  4 files changed, 22 insertions(+), 22 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v4 00/16] 64bit block-layer: part I

[Vladimir Sementsov-Ogievskiy]

11.12.2020 21:39, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> We want 64bit write-zeroes, and for this, convert all io functions to
> 64bit.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> Please refer to initial cover-letter
>   https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
> for more info.
> 
> v4: I found, that some more work is needed for block/block-backend, so
> decided to make partI, converting block/io
> 
> v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
>     for BDRV_MAX_LENGTH
> 
> changes:
> 01-05: new
> 06: add Alberto's r-b
> 07: new
> 08-16: rebase, add new-style request check, improve commit-msg, drop r-bs
> 
> Based-on:<20201211170812.228643-1-kwolf@redhat.com>


Now based on master.

-- 
Best regards,
Vladimir

Re: [PATCH v4 00/16] 64bit block-layer: part I

[Vladimir Sementsov-Ogievskiy]

ping

11.12.2020 21:39, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> We want 64bit write-zeroes, and for this, convert all io functions to
> 64bit.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> Please refer to initial cover-letter
>   https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
> for more info.
> 
> v4: I found, that some more work is needed for block/block-backend, so
> decided to make partI, converting block/io
> 
> v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
>     for BDRV_MAX_LENGTH
> 
> changes:
> 01-05: new
> 06: add Alberto's r-b
> 07: new
> 08-16: rebase, add new-style request check, improve commit-msg, drop r-bs
> 
> Based-on: <20201211170812.228643-1-kwolf@redhat.com>
> 
> Vladimir Sementsov-Ogievskiy (16):
>    block: refactor bdrv_check_request: add errp
>    util/iov: make qemu_iovec_init_extended() honest
>    block: fix theoretical overflow in bdrv_init_padding()
>    block/io: refactor bdrv_pad_request(): move bdrv_pad_request() up
>    block/io: bdrv_pad_request(): support qemu_iovec_init_extended failure
>    block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit
>      bytes
>    block/io: improve bdrv_check_request: check qiov too
>    block: use int64_t as bytes type in tracked requests
>    block/io: use int64_t bytes in driver wrappers
>    block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()
>    block/io: support int64_t bytes in bdrv_aligned_pwritev()
>    block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()
>    block/io: support int64_t bytes in bdrv_aligned_preadv()
>    block/io: support int64_t bytes in bdrv_co_p{read,write}v_part()
>    block/io: support int64_t bytes in read/write wrappers
>    block/io: use int64_t bytes in copy_range
> 
>   include/block/block.h           |  17 +-
>   include/block/block_int.h       |  26 +--
>   include/block/throttle-groups.h |   2 +-
>   include/qemu/iov.h              |   2 +-
>   block/blkverify.c               |   2 +-
>   block/file-posix.c              |   2 +-
>   block/io.c                      | 274 ++++++++++++++++++++++----------
>   block/throttle-groups.c         |   5 +-
>   tests/test-write-threshold.c    |   5 +-
>   util/iov.c                      |  25 ++-
>   block/trace-events              |  12 +-
>   11 files changed, 252 insertions(+), 120 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/16] 64bit block-layer: part I

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> We want 64bit write-zeroes, and for this, convert all io functions to
> 64bit.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> Please refer to initial cover-letter 
>  https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
> for more info.
> 
> v4: I found, that some more work is needed for block/block-backend, so
> decided to make partI, converting block/io
> 
> v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
>    for BDRV_MAX_LENGTH
> 
> changes:
> 01-05: new
> 06: add Alberto's r-b
> 07: new
> 08-16: rebase, add new-style request check, improve commit-msg, drop r-bs

I had planned to send a pull request for this series today, but ran into
a snag.  Without this series applied, './check -qcow2' fails 030, 185,
and 297.  With it applied, I now also get a failure in 206.  I'm trying
to bisect which patch caused the problem, but here's the failure:

206   fail       [20:54:54] [20:55:01]   6.9s   (last: 6.7s)  output
mismatch (see 206.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/206.out
+++ 206.out.bad
@@ -180,7 +180,7 @@

 {"execute": "blockdev-create", "arguments": {"job-id": "job0",
"options": {"driver": "qcow2", "file": "node0", "size":
9223372036854775296}}}
 {"return": {}}
-Job failed: Could not resize image: Required too big image size, it
must be not greater than 9223372035781033984
+Job failed: Could not resize image: offset(9223372036854775296) exceeds
maximum(9223372035781033984)
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

Looks like it is just a changed error message, so I can touch up the
correct patch and then repackage the pull request tomorrow (it's too
late for me today).  Oh, and the 0 exit status of ./check when a test
fails is something I see you already plan on fixing...

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

Re: [PATCH v4 00/16] 64bit block-layer: part I

[Vladimir Sementsov-Ogievskiy]

02.02.2021 05:56, Eric Blake wrote:
> On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> We want 64bit write-zeroes, and for this, convert all io functions to
>> 64bit.
>>
>> We chose signed type, to be consistent with off_t (which is signed) and
>> with possibility for signed return type (where negative value means
>> error).
>>
>> Please refer to initial cover-letter
>>   https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
>> for more info.
>>
>> v4: I found, that some more work is needed for block/block-backend, so
>> decided to make partI, converting block/io
>>
>> v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
>>     for BDRV_MAX_LENGTH
>>
>> changes:
>> 01-05: new
>> 06: add Alberto's r-b
>> 07: new
>> 08-16: rebase, add new-style request check, improve commit-msg, drop r-bs
> 
> I had planned to send a pull request for this series today, but ran into
> a snag.  Without this series applied, './check -qcow2' fails 030, 185,
> and 297.  With it applied, I now also get a failure in 206.  I'm trying
> to bisect which patch caused the problem, but here's the failure:
> 
> 206   fail       [20:54:54] [20:55:01]   6.9s   (last: 6.7s)  output
> mismatch (see 206.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/206.out
> +++ 206.out.bad
> @@ -180,7 +180,7 @@
> 
>   {"execute": "blockdev-create", "arguments": {"job-id": "job0",
> "options": {"driver": "qcow2", "file": "node0", "size":
> 9223372036854775296}}}
>   {"return": {}}
> -Job failed: Could not resize image: Required too big image size, it
> must be not greater than 9223372035781033984
> +Job failed: Could not resize image: offset(9223372036854775296) exceeds
> maximum(9223372035781033984)
>   {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>   {"return": {}}
> 
> Looks like it is just a changed error message, so I can touch up the
> correct patch and then repackage the pull request tomorrow (it's too
> late for me today).  Oh, and the 0 exit status of ./check when a test
> fails is something I see you already plan on fixing...
> 

Yes, Kevin have already sent a pull with "iotests: check: return 1 on failure"

-- 
Best regards,
Vladimir

Re: [PATCH v4 00/16] 64bit block-layer: part I

[Eric Blake]

On 2/1/21 8:56 PM, Eric Blake wrote:
> I had planned to send a pull request for this series today, but ran into
> a snag.  Without this series applied, './check -qcow2' fails 030, 185,
> and 297.  With it applied, I now also get a failure in 206.  I'm trying
> to bisect which patch caused the problem, but here's the failure:
> 
> 206   fail       [20:54:54] [20:55:01]   6.9s   (last: 6.7s)  output
> mismatch (see 206.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/206.out
> +++ 206.out.bad
> @@ -180,7 +180,7 @@
> 
>  {"execute": "blockdev-create", "arguments": {"job-id": "job0",
> "options": {"driver": "qcow2", "file": "node0", "size":
> 9223372036854775296}}}
>  {"return": {}}
> -Job failed: Could not resize image: Required too big image size, it
> must be not greater than 9223372035781033984
> +Job failed: Could not resize image: offset(9223372036854775296) exceeds
> maximum(9223372035781033984)
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> 
> Looks like it is just a changed error message, so I can touch up the
> correct patch and then repackage the pull request

Culprit was "block: refactor bdrv_check_request: add errp".  I'm
preparing the pull request now.

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

iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Eric Blake]

On 2/1/21 8:56 PM, Eric Blake wrote:

> I had planned to send a pull request for this series today, but ran into
> a snag.  Without this series applied, './check -qcow2' fails 030, 185,
> and 297.

297 appears to be fixed once Kevin's pull request lands (well, that may
be needing a v2).  185 appears to be just a whitespace difference that
missed fixing in 362ef77f9 and similar:

--- /home/eblake/qemu/tests/qemu-iotests/185.out
+++ 185.out.bad
@@ -89,7 +89,7 @@
                       'format': 'IMGFMT',
                       'sync': 'full',
                       'speed': 65536,
-                      'x-perf': { 'max-chunk': 65536 } } }
+                      'x-perf': {'max-chunk': 65536} } }

030 is a bit tougher to figure out.

030   fail       [09:40:32] [09:40:48]   16.9s  (last: 15.4s) failed,
exit status 1
--- /home/eblake/qemu/tests/qemu-iotests/030.out
+++ 030.out.bad
@@ -1,5 +1,45 @@
-...........................
+WARNING:qemu.machine:qemu received signal 11; command:
"/home/eblake/qemu/build/tests/qemu-iotests/../../qemu-system-x86_64
-display none -vga none -chardev
socket,id=mon,path=/tmp/tmpedy9c_uf/qemu-421866-monitor.sock -mon
chardev=mon,mode=control -qtest
unix:path=/tmp/tmpedy9c_uf/qemu-421866-qtest.sock -accel qtest
-nodefaults -display none -accel qtest -drive
if=virtio,id=drive0,file=/home/eblake/qemu/build/tests/qemu-iotests/scratch/img-8.img,format=qcow2,cache=writeback,aio=threads,backing.backing.backing.backing.backing.backing.backing.backing.node-name=node0,backing.backing.backing.backing.backing.backing.backing.node-name=node1,backing.backing.backing.backing.backing.backing.node-name=node2,backing.backing.backing.backing.backing.node-name=node3,backing.backing.backing.backing.node-name=node4,backing.backing.backing.node-name=node5,backing.backing.node-name=node6,backing.node-name=node7,node-name=node8"
+.............EE.............
+======================================================================
+ERROR: test_stream_parallel (__main__.TestParallelOps)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "/home/eblake/qemu/tests/qemu-iotests/030", line 260, in
test_stream_parallel
+    for event in self.vm.get_qmp_events(wait=True):
+  File
"/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/machine.py",
line 585, in get_qmp_events
+    events = self._qmp.get_events(wait=wait)
+  File "/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/qmp.py",
line 328, in get_events
+    self.__get_events(wait)
+  File "/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/qmp.py",
line 197, in __get_events
+    raise QMPConnectError("Error while reading from socket")
+qemu.qmp.QMPConnectError: Error while reading from socket
+
+======================================================================
+ERROR: test_stream_parallel (__main__.TestParallelOps)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File
"/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/machine.py",
line 477, in _do_shutdown
+    self._soft_shutdown(timeout, has_quit)
+  File
"/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/machine.py",
line 457, in _soft_shutdown
+    self._qmp.cmd('quit')
+  File "/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/qmp.py",
line 278, in cmd
+    return self.cmd_obj(qmp_cmd)
+  File "/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/qmp.py",
line 256, in cmd_obj
+    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
+BrokenPipeError: [Errno 32] Broken pipe
+
+The above exception was the direct cause of the following exception:
+
+Traceback (most recent call last):
+  File "/home/eblake/qemu/tests/qemu-iotests/030", line 227, in tearDown
+    self.vm.shutdown()
+  File
"/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/machine.py",
line 507, in shutdown
+    self._do_shutdown(timeout, has_quit)
+  File
"/home/eblake/qemu/tests/qemu-iotests/../../python/qemu/machine.py",
line 480, in _do_shutdown
+    raise AbnormalShutdown("Could not perform graceful shutdown") \
+qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
+
 ----------------------------------------------------------------------
 Ran 27 tests

-OK
+FAILED (errors=2)


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

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Kevin Wolf]

Am 02.02.2021 um 17:13 hat Eric Blake geschrieben:
> On 2/1/21 8:56 PM, Eric Blake wrote:
> 
> > I had planned to send a pull request for this series today, but ran into
> > a snag.  Without this series applied, './check -qcow2' fails 030, 185,
> > and 297.
> 
> 297 appears to be fixed once Kevin's pull request lands (well, that may
> be needing a v2).  185 appears to be just a whitespace difference that
> missed fixing in 362ef77f9 and similar:
> 
> --- /home/eblake/qemu/tests/qemu-iotests/185.out
> +++ 185.out.bad
> @@ -89,7 +89,7 @@
>                        'format': 'IMGFMT',
>                        'sync': 'full',
>                        'speed': 65536,
> -                      'x-perf': { 'max-chunk': 65536 } } }
> +                      'x-perf': {'max-chunk': 65536} } }
> 
> 030 is a bit tougher to figure out.
> 
> 030   fail       [09:40:32] [09:40:48]   16.9s  (last: 15.4s) failed,
> exit status 1
> --- /home/eblake/qemu/tests/qemu-iotests/030.out
> +++ 030.out.bad
> @@ -1,5 +1,45 @@
> -...........................
> +WARNING:qemu.machine:qemu received signal 11; command:

So some qemu process segfaulted. Did you have a look at the resulting
coredump?

Kevin

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Vladimir Sementsov-Ogievskiy]

02.02.2021 19:23, Kevin Wolf wrote:
> Am 02.02.2021 um 17:13 hat Eric Blake geschrieben:
>> On 2/1/21 8:56 PM, Eric Blake wrote:
>>
>>> I had planned to send a pull request for this series today, but ran into
>>> a snag.  Without this series applied, './check -qcow2' fails 030, 185,
>>> and 297.
>>
>> 297 appears to be fixed once Kevin's pull request lands (well, that may
>> be needing a v2).  185 appears to be just a whitespace difference that
>> missed fixing in 362ef77f9 and similar:
>>
>> --- /home/eblake/qemu/tests/qemu-iotests/185.out
>> +++ 185.out.bad
>> @@ -89,7 +89,7 @@
>>                         'format': 'IMGFMT',
>>                         'sync': 'full',
>>                         'speed': 65536,
>> -                      'x-perf': { 'max-chunk': 65536 } } }
>> +                      'x-perf': {'max-chunk': 65536} } }
>>
>> 030 is a bit tougher to figure out.
>>
>> 030   fail       [09:40:32] [09:40:48]   16.9s  (last: 15.4s) failed,
>> exit status 1
>> --- /home/eblake/qemu/tests/qemu-iotests/030.out
>> +++ 030.out.bad
>> @@ -1,5 +1,45 @@
>> -...........................
>> +WARNING:qemu.machine:qemu received signal 11; command:
> 
> So some qemu process segfaulted. Did you have a look at the resulting
> coredump?
> 
> Kevin
> 

Note that 30 is known to crash sometimes. Look at

"[PATCH RFC 0/5] Fix accidental crash in iotest 30"

https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/

-- 
Best regards,
Vladimir

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Vladimir Sementsov-Ogievskiy]

02.02.2021 19:29, Vladimir Sementsov-Ogievskiy wrote:
> 02.02.2021 19:23, Kevin Wolf wrote:
>> Am 02.02.2021 um 17:13 hat Eric Blake geschrieben:
>>> On 2/1/21 8:56 PM, Eric Blake wrote:
>>>
>>>> I had planned to send a pull request for this series today, but ran into
>>>> a snag.  Without this series applied, './check -qcow2' fails 030, 185,
>>>> and 297.
>>>
>>> 297 appears to be fixed once Kevin's pull request lands (well, that may
>>> be needing a v2).  185 appears to be just a whitespace difference that
>>> missed fixing in 362ef77f9 and similar:
>>>
>>> --- /home/eblake/qemu/tests/qemu-iotests/185.out
>>> +++ 185.out.bad
>>> @@ -89,7 +89,7 @@
>>>                         'format': 'IMGFMT',
>>>                         'sync': 'full',
>>>                         'speed': 65536,
>>> -                      'x-perf': { 'max-chunk': 65536 } } }
>>> +                      'x-perf': {'max-chunk': 65536} } }
>>>
>>> 030 is a bit tougher to figure out.
>>>
>>> 030   fail       [09:40:32] [09:40:48]   16.9s  (last: 15.4s) failed,
>>> exit status 1
>>> --- /home/eblake/qemu/tests/qemu-iotests/030.out
>>> +++ 030.out.bad
>>> @@ -1,5 +1,45 @@
>>> -...........................
>>> +WARNING:qemu.machine:qemu received signal 11; command:
>>
>> So some qemu process segfaulted. Did you have a look at the resulting
>> coredump?
>>
>> Kevin
>>
> 
> Note that 30 is known to crash sometimes. Look at
> 
> "[PATCH RFC 0/5] Fix accidental crash in iotest 30"
> 
> https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/
> 

Also, I see that this series diverged with master.. Did you already rebased it? If so, could you push it in your tree?

-- 
Best regards,
Vladimir

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Peter Maydell]

On Tue, 2 Feb 2021 at 17:09, Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
> Note that 30 is known to crash sometimes. Look at
>
> "[PATCH RFC 0/5] Fix accidental crash in iotest 30"
>
> https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/

It certainly seems to be the least reliable iotest in my experience.
For example it just fell over on ppc64 on MST's latest pullreq merge:

https://lore.kernel.org/qemu-devel/CAFEAcA8aZ6qTLjp00FyqYUwtqk0tAFYUpjW0FeepPMMVfOUbPg@mail.gmail.com/

thanks
-- PMM

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Peter Maydell]

On Tue, 2 Feb 2021 at 22:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 2 Feb 2021 at 17:09, Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
> > Note that 30 is known to crash sometimes. Look at
> >
> > "[PATCH RFC 0/5] Fix accidental crash in iotest 30"
> >
> > https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/
>
> It certainly seems to be the least reliable iotest in my experience.
> For example it just fell over on ppc64 on MST's latest pullreq merge:
>
> https://lore.kernel.org/qemu-devel/CAFEAcA8aZ6qTLjp00FyqYUwtqk0tAFYUpjW0FeepPMMVfOUbPg@mail.gmail.com/

Just saw this same error again, on FreeBSD rather than OpenBSD
this time. Can we please just disable 030 if we can't fix it ?

thanks
-- PMM

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Peter Maydell]

On Wed, 3 Feb 2021 at 10:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 2 Feb 2021 at 22:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 2 Feb 2021 at 17:09, Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> > > Note that 30 is known to crash sometimes. Look at
> > >
> > > "[PATCH RFC 0/5] Fix accidental crash in iotest 30"
> > >
> > > https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/
> >
> > It certainly seems to be the least reliable iotest in my experience.
> > For example it just fell over on ppc64 on MST's latest pullreq merge:
> >
> > https://lore.kernel.org/qemu-devel/CAFEAcA8aZ6qTLjp00FyqYUwtqk0tAFYUpjW0FeepPMMVfOUbPg@mail.gmail.com/
>
> Just saw this same error again, on FreeBSD rather than OpenBSD
> this time. Can we please just disable 030 if we can't fix it ?

And another 030 failure, this time in gitlib CI:
https://gitlab.com/qemu-project/qemu/-/jobs/1008038881

thanks
-- PMM