[Qemu-devel] [PATCH 0/8] block: more byte-based cleanups: vectored I/O

[Qemu-devel] [PATCH 0/8] block: more byte-based cleanups: vectored I/O

[Eric Blake]

Based-on: <20180424192506.149089-1-eblake@redhat.com>
([PATCH v2 0/6] block: byte-based AIO read/write)
Based-on: <20180424220157.177385-1-eblake@redhat.com>
([PATCH] block: Merge .bdrv_co_writev{, _flags} in drivers)

My quest continues.  I spent some time pruning qcow down as far
as possible (and was dismayed at how long it took to prove no
iotests regressions); so for the other drivers, I did the bare
minimum to get rid of an interface, but will leave it to those
file owners if they want to get rid of further pointless sector
manipulations in their files.

Next on the chopping block: bdrv_read/bdrv_write.

Eric Blake (8):
  parallels: Switch to byte-based calls
  qcow: Switch get_cluster_offset to be byte-based
  qcow: Switch qcow_co_readv to byte-based calls
  qcow: Switch qcow_co_writev to byte-based calls
  qcow: Switch to a byte-based driver
  replication: Switch to byte-based calls
  vhdx: Switch to byte-based calls
  block: Removed unused sector-based vectored I/O

 include/block/block.h |   4 --
 block/io.c            |  36 ---------------
 block/parallels.c     |  16 ++++---
 block/qcow.c          | 122 +++++++++++++++++++++++++-------------------------
 block/replication.c   |  14 +++---
 block/vhdx.c          |  12 ++---
 6 files changed, 86 insertions(+), 118 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/8] parallels: Switch to byte-based calls

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the last few sector-based calls
into the block layer from the parallels driver.

Ideally, the parallels driver should switch to doing everything
byte-based, but that's a more invasive change that requires a
bit more auditing.

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

diff --git a/block/parallels.c b/block/parallels.c
index 3f74fcb877a..c071c7f759f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -226,14 +226,15 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
         };
         qemu_iovec_init_external(&qiov, &iov, 1);

-        ret = bdrv_co_readv(bs->backing, idx * s->tracks, nb_cow_sectors,
-                            &qiov);
+        ret = bdrv_co_preadv(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE,
+                             nb_cow_bytes, &qiov, 0);
         if (ret < 0) {
             qemu_vfree(iov.iov_base);
             return ret;
         }

-        ret = bdrv_co_writev(bs->file, s->data_end, nb_cow_sectors, &qiov);
+        ret = bdrv_co_pwritev(bs->file, s->data_end * BDRV_SECTOR_SIZE,
+                              nb_cow_bytes, &qiov, 0);
         qemu_vfree(iov.iov_base);
         if (ret < 0) {
             return ret;
@@ -339,7 +340,8 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);

-        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
+        ret = bdrv_co_pwritev(bs->file, position * BDRV_SECTOR_SIZE, nbytes,
+                              &hd_qiov, 0);
         if (ret < 0) {
             break;
         }
@@ -378,7 +380,8 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,

         if (position < 0) {
             if (bs->backing) {
-                ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
+                ret = bdrv_co_preadv(bs->backing, sector_num * BDRV_SECTOR_SIZE,
+                                     nbytes, &hd_qiov, 0);
                 if (ret < 0) {
                     break;
                 }
@@ -386,7 +389,8 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
                 qemu_iovec_memset(&hd_qiov, 0, 0, nbytes);
             }
         } else {
-            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
+            ret = bdrv_co_preadv(bs->file, position * BDRV_SECTOR_SIZE, nbytes,
+                                 &hd_qiov, 0);
             if (ret < 0) {
                 break;
             }
-- 
2.14.3

[Qemu-devel] [PATCH 2/8] qcow: Switch get_cluster_offset to be byte-based

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the internal helper function
get_cluster_offset(), by changing n_start and n_end to by byte
offsets rather than sector indices within the cluster being
allocated.

A later patch will then switch the qcow driver as a whole over
to byte-based operation.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index dd042b8ddbe..32730a8dd91 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -345,8 +345,8 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
  *
  * 0 to not allocate.
  *
- * 1 to allocate a normal cluster (for sector indexes 'n_start' to
- * 'n_end')
+ * 1 to allocate a normal cluster (for byte offsets 'n_start' to
+ * 'n_end' within the cluster)
  *
  * 2 to allocate a compressed cluster of size
  * 'compressed_size'. 'compressed_size' must be > 0 and <
@@ -442,7 +442,7 @@ static int get_cluster_offset(BlockDriverState *bs,
         BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
         /* allocate a new cluster */
         if ((cluster_offset & QCOW_OFLAG_COMPRESSED) &&
-            (n_end - n_start) < s->cluster_sectors) {
+            (n_end - n_start) < s->cluster_size) {
             /* if the cluster is already compressed, we must
                decompress it in the case it is not completely
                overwritten */
@@ -480,16 +480,15 @@ static int get_cluster_offset(BlockDriverState *bs,
                 /* if encrypted, we must initialize the cluster
                    content which won't be written */
                 if (bs->encrypted &&
-                    (n_end - n_start) < s->cluster_sectors) {
-                    uint64_t start_sect;
+                    (n_end - n_start) < s->cluster_size) {
+                    uint64_t start_offset;
                     assert(s->crypto);
-                    start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
-                    for(i = 0; i < s->cluster_sectors; i++) {
+                    start_offset = offset & ~(s->cluster_size - 1);
+                    for (i = 0; i < s->cluster_size; i += BDRV_SECTOR_SIZE) {
                         if (i < n_start || i >= n_end) {
-                            memset(s->cluster_data, 0x00, 512);
+                            memset(s->cluster_data, 0x00, BDRV_SECTOR_SIZE);
                             if (qcrypto_block_encrypt(s->crypto,
-                                                      (start_sect + i) *
-                                                      BDRV_SECTOR_SIZE,
+                                                      start_offset + i,
                                                       s->cluster_data,
                                                       BDRV_SECTOR_SIZE,
                                                       NULL) < 0) {
@@ -497,8 +496,9 @@ static int get_cluster_offset(BlockDriverState *bs,
                             }
                             BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
                             ret = bdrv_pwrite(bs->file,
-                                              cluster_offset + i * 512,
-                                              s->cluster_data, 512);
+                                              cluster_offset + i,
+                                              s->cluster_data,
+                                              BDRV_SECTOR_SIZE);
                             if (ret < 0) {
                                 return ret;
                             }
@@ -758,8 +758,8 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
             n = nb_sectors;
         }
         ret = get_cluster_offset(bs, sector_num << 9, 1, 0,
-                                 index_in_cluster,
-                                 index_in_cluster + n, &cluster_offset);
+                                 index_in_cluster << 9,
+                                 (index_in_cluster + n) << 9, &cluster_offset);
         if (ret < 0) {
             break;
         }
-- 
2.14.3

[Qemu-devel] [PATCH 3/8] qcow: Switch qcow_co_readv to byte-based calls

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the internals of the qcow
driver read function, by iterating over offset/bytes instead of
sector_num/nb_sectors, and repurposing index_in_cluster and n
to be bytes instead of sectors.

A later patch will then switch the qcow driver as a whole over
to byte-based operation.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 32730a8dd91..bf9d80fd227 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -623,6 +623,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
+    int64_t offset = sector_num << BDRV_SECTOR_BITS;
+    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;

     if (qiov->niov > 1) {
         buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
@@ -636,36 +638,36 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,

     qemu_co_mutex_lock(&s->lock);

-    while (nb_sectors != 0) {
+    while (bytes != 0) {
         /* prepare next request */
-        ret = get_cluster_offset(bs, sector_num << 9,
+        ret = get_cluster_offset(bs, offset,
                                  0, 0, 0, 0, &cluster_offset);
         if (ret < 0) {
             break;
         }
-        index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n = s->cluster_sectors - index_in_cluster;
-        if (n > nb_sectors) {
-            n = nb_sectors;
+        index_in_cluster = offset & (s->cluster_size - 1);
+        n = s->cluster_size - index_in_cluster;
+        if (n > bytes) {
+            n = bytes;
         }

         if (!cluster_offset) {
             if (bs->backing) {
                 /* read from the base image */
                 hd_iov.iov_base = (void *)buf;
-                hd_iov.iov_len = n * 512;
+                hd_iov.iov_len = n;
                 qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
                 qemu_co_mutex_unlock(&s->lock);
                 /* qcow2 emits this on bs->file instead of bs->backing */
                 BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
-                ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
+                ret = bdrv_co_preadv(bs->backing, offset, n, &hd_qiov, 0);
                 qemu_co_mutex_lock(&s->lock);
                 if (ret < 0) {
                     break;
                 }
             } else {
                 /* Note: in this case, no need to wait */
-                memset(buf, 0, 512 * n);
+                memset(buf, 0, n);
             }
         } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
             /* add AIO support for compressed blocks ? */
@@ -673,21 +675,19 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
                 ret = -EIO;
                 break;
             }
-            memcpy(buf,
-                   s->cluster_cache + index_in_cluster * 512, 512 * n);
+            memcpy(buf, s->cluster_cache + index_in_cluster, n);
         } else {
             if ((cluster_offset & 511) != 0) {
                 ret = -EIO;
                 break;
             }
             hd_iov.iov_base = (void *)buf;
-            hd_iov.iov_len = n * 512;
+            hd_iov.iov_len = n;
             qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
             qemu_co_mutex_unlock(&s->lock);
             BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-            ret = bdrv_co_readv(bs->file,
-                                (cluster_offset >> 9) + index_in_cluster,
-                                n, &hd_qiov);
+            ret = bdrv_co_preadv(bs->file, cluster_offset + index_in_cluster,
+                                 n, &hd_qiov, 0);
             qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 break;
@@ -695,8 +695,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
             if (bs->encrypted) {
                 assert(s->crypto);
                 if (qcrypto_block_decrypt(s->crypto,
-                                          sector_num * BDRV_SECTOR_SIZE, buf,
-                                          n * BDRV_SECTOR_SIZE, NULL) < 0) {
+                                          offset, buf, n, NULL) < 0) {
                     ret = -EIO;
                     break;
                 }
@@ -704,9 +703,9 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
         }
         ret = 0;

-        nb_sectors -= n;
-        sector_num += n;
-        buf += n * 512;
+        bytes -= n;
+        offset += n;
+        buf += n;
     }

     qemu_co_mutex_unlock(&s->lock);
-- 
2.14.3

[Qemu-devel] [PATCH 4/8] qcow: Switch qcow_co_writev to byte-based calls

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the internals of the qcow
driver write function, by iterating over offset/bytes instead of
sector_num/nb_sectors, and repurposing index_in_cluster and n
to be bytes instead of sectors.

A later patch will then switch the qcow driver as a whole over
to byte-based operation.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index bf9d80fd227..ea81a3bf87d 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -730,6 +730,8 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
+    int64_t offset = sector_num << BDRV_SECTOR_BITS;
+    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;

     assert(!flags);
     s->cluster_cache_offset = -1; /* disable compressed cache */
@@ -749,16 +751,14 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,

     qemu_co_mutex_lock(&s->lock);

-    while (nb_sectors != 0) {
-
-        index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n = s->cluster_sectors - index_in_cluster;
-        if (n > nb_sectors) {
-            n = nb_sectors;
+    while (bytes != 0) {
+        index_in_cluster = offset & (s->cluster_size - 1);
+        n = s->cluster_size - index_in_cluster;
+        if (n > bytes) {
+            n = bytes;
         }
-        ret = get_cluster_offset(bs, sector_num << 9, 1, 0,
-                                 index_in_cluster << 9,
-                                 (index_in_cluster + n) << 9, &cluster_offset);
+        ret = get_cluster_offset(bs, offset, 1, 0, index_in_cluster,
+                                 index_in_cluster + n, &cluster_offset);
         if (ret < 0) {
             break;
         }
@@ -768,30 +768,28 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
         }
         if (bs->encrypted) {
             assert(s->crypto);
-            if (qcrypto_block_encrypt(s->crypto, sector_num * BDRV_SECTOR_SIZE,
-                                      buf, n * BDRV_SECTOR_SIZE, NULL) < 0) {
+            if (qcrypto_block_encrypt(s->crypto, offset, buf, n, NULL) < 0) {
                 ret = -EIO;
                 break;
             }
         }

         hd_iov.iov_base = (void *)buf;
-        hd_iov.iov_len = n * 512;
+        hd_iov.iov_len = n;
         qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
         qemu_co_mutex_unlock(&s->lock);
         BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
-        ret = bdrv_co_writev(bs->file,
-                             (cluster_offset >> 9) + index_in_cluster,
-                             n, &hd_qiov);
+        ret = bdrv_co_pwritev(bs->file, cluster_offset + index_in_cluster,
+                              n, &hd_qiov, 0);
         qemu_co_mutex_lock(&s->lock);
         if (ret < 0) {
             break;
         }
         ret = 0;

-        nb_sectors -= n;
-        sector_num += n;
-        buf += n * 512;
+        bytes -= n;
+        offset += n;
+        buf += n;
     }
     qemu_co_mutex_unlock(&s->lock);

-- 
2.14.3

[Qemu-devel] [PATCH 5/8] qcow: Switch to a byte-based driver

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  The qcow driver is now ready to fully utilize the
byte-based callback interface, as long as we override the default
alignment to still be 512 (needed at least for encryption, but
easier to do everywhere than to audit which sub-sector requests
are handled correctly).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index ea81a3bf87d..75fd75cf976 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -69,7 +69,6 @@ typedef struct QCowHeader {
 typedef struct BDRVQcowState {
     int cluster_bits;
     int cluster_size;
-    int cluster_sectors;
     int l2_bits;
     int l2_size;
     unsigned int l1_size;
@@ -235,7 +234,6 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->cluster_bits = header.cluster_bits;
     s->cluster_size = 1 << s->cluster_bits;
-    s->cluster_sectors = 1 << (s->cluster_bits - 9);
     s->l2_bits = header.l2_bits;
     s->l2_size = 1 << s->l2_bits;
     bs->total_sectors = header.size / 512;
@@ -612,8 +610,18 @@ static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
     return 0;
 }

-static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
-                         int nb_sectors, QEMUIOVector *qiov)
+static void qcow_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    /* At least encrypted images require 512-byte alignment. Apply the
+     * limit universally, rather than just on encrypted images, as
+     * it's easier to let the block layer handle rounding than to
+     * audit this code further. */
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE;
+}
+
+static coroutine_fn int qcow_co_preadv(BlockDriverState *bs, uint64_t offset,
+                                       uint64_t bytes, QEMUIOVector *qiov,
+                                       int flags)
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
@@ -623,9 +631,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
-    int64_t offset = sector_num << BDRV_SECTOR_BITS;
-    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;

+    assert(!flags);
     if (qiov->niov > 1) {
         buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
         if (buf == NULL) {
@@ -718,9 +725,9 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }

-static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
-                                       int nb_sectors, QEMUIOVector *qiov,
-                                       int flags)
+static coroutine_fn int qcow_co_pwritev(BlockDriverState *bs, uint64_t offset,
+                                        uint64_t bytes, QEMUIOVector *qiov,
+                                        int flags)
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
@@ -730,8 +737,6 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
-    int64_t offset = sector_num << BDRV_SECTOR_BITS;
-    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;

     assert(!flags);
     s->cluster_cache_offset = -1; /* disable compressed cache */
@@ -1108,8 +1113,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,

     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        ret = qcow_co_writev(bs, offset >> BDRV_SECTOR_BITS,
-                             bytes >> BDRV_SECTOR_BITS, qiov, 0);
+        ret = qcow_co_pwritev(bs, offset, bytes, qiov, 0);
         if (ret < 0) {
             goto fail;
         }
@@ -1194,9 +1198,10 @@ static BlockDriver bdrv_qcow = {
     .bdrv_co_create_opts    = qcow_co_create_opts,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .supports_backing       = true,
+    .bdrv_refresh_limits    = qcow_refresh_limits,

-    .bdrv_co_readv          = qcow_co_readv,
-    .bdrv_co_writev         = qcow_co_writev,
+    .bdrv_co_preadv         = qcow_co_preadv,
+    .bdrv_co_pwritev        = qcow_co_pwritev,
     .bdrv_co_block_status   = qcow_co_block_status,

     .bdrv_make_empty        = qcow_make_empty,
-- 
2.14.3

[Qemu-devel] [PATCH 6/8] replication: Switch to byte-based calls

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the last few sector-based calls
into the block layer from the replication driver.

Ideally, the replication driver should switch to doing everything
byte-based, but that's a more invasive change that requires a
bit more auditing.

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

diff --git a/block/replication.c b/block/replication.c
index 48148b884a5..131639d0714 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -246,13 +246,14 @@ static coroutine_fn int replication_co_readv(BlockDriverState *bs,
         backup_cow_request_begin(&req, child->bs->job,
                                  sector_num * BDRV_SECTOR_SIZE,
                                  remaining_bytes);
-        ret = bdrv_co_readv(bs->file, sector_num, remaining_sectors,
-                            qiov);
+        ret = bdrv_co_preadv(bs->file, sector_num * BDRV_SECTOR_SIZE,
+                             remaining_bytes, qiov, 0);
         backup_cow_request_end(&req);
         goto out;
     }

-    ret = bdrv_co_readv(bs->file, sector_num, remaining_sectors, qiov);
+    ret = bdrv_co_preadv(bs->file, sector_num * BDRV_SECTOR_SIZE,
+                         remaining_sectors * BDRV_SECTOR_SIZE, qiov, 0);
 out:
     return replication_return_value(s, ret);
 }
@@ -279,8 +280,8 @@ static coroutine_fn int replication_co_writev(BlockDriverState *bs,
     }

     if (ret == 0) {
-        ret = bdrv_co_writev(top, sector_num,
-                             remaining_sectors, qiov);
+        ret = bdrv_co_pwritev(top, sector_num * BDRV_SECTOR_SIZE,
+                              remaining_sectors * BDRV_SECTOR_SIZE, qiov, 0);
         return replication_return_value(s, ret);
     }

@@ -306,7 +307,8 @@ static coroutine_fn int replication_co_writev(BlockDriverState *bs,
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done, count);

         target = ret ? top : base;
-        ret = bdrv_co_writev(target, sector_num, n, &hd_qiov);
+        ret = bdrv_co_pwritev(target, sector_num * BDRV_SECTOR_SIZE,
+                              n * BDRV_SECTOR_SIZE, &hd_qiov, 0);
         if (ret < 0) {
             goto out1;
         }
-- 
2.14.3

[Qemu-devel] [PATCH 7/8] vhdx: Switch to byte-based calls

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Make the change for the last few sector-based calls
into the block layer from the vhdx driver.

Ideally, the vhdx driver should switch to doing everything
byte-based, but that's a more invasive change that requires a
bit more auditing.

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

diff --git a/block/vhdx.c b/block/vhdx.c
index 0b72d895db4..f1dfda6e3e5 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1126,9 +1126,9 @@ static coroutine_fn int vhdx_co_readv(BlockDriverState *bs, int64_t sector_num,
                 break;
             case PAYLOAD_BLOCK_FULLY_PRESENT:
                 qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_readv(bs->file,
-                                    sinfo.file_offset >> BDRV_SECTOR_BITS,
-                                    sinfo.sectors_avail, &hd_qiov);
+                ret = bdrv_co_preadv(bs->file, sinfo.file_offset,
+                                     sinfo.sectors_avail * BDRV_SECTOR_SIZE,
+                                     &hd_qiov, 0);
                 qemu_co_mutex_lock(&s->lock);
                 if (ret < 0) {
                     goto exit;
@@ -1348,9 +1348,9 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
                 }
                 /* block exists, so we can just overwrite it */
                 qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_writev(bs->file,
-                                    sinfo.file_offset >> BDRV_SECTOR_BITS,
-                                    sectors_to_write, &hd_qiov);
+                ret = bdrv_co_pwritev(bs->file, sinfo.file_offset,
+                                      sectors_to_write * BDRV_SECTOR_SIZE,
+                                      &hd_qiov, 0);
                 qemu_co_mutex_lock(&s->lock);
                 if (ret < 0) {
                     goto error_bat_restore;
-- 
2.14.3

[Qemu-devel] [PATCH 8/8] block: Removed unused sector-based vectored I/O

[Eric Blake]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Now that all callers of vectored I/O have been converted
to use our preferred byte-based bdrv_co_p{read,write}v(), we can
delete the unused bdrv_co_{read,write}v().

Furthermore, this gets rid of the signature difference between
the public bdrv_co_writev and the callback .bdrv_co_writev.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h |  4 ----
 block/io.c            | 36 ------------------------------------
 2 files changed, 40 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index cdec3639a35..8b191c23d97 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -278,10 +278,6 @@ int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
 int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
                      const void *buf, int count);
-int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
-                               int nb_sectors, QEMUIOVector *qiov);
-int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
-                               int nb_sectors, QEMUIOVector *qiov);
 /*
  * 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
diff --git a/block/io.c b/block/io.c
index 4fad5ac2fef..7155786eb47 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1339,24 +1339,6 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     return ret;
 }

-static int coroutine_fn bdrv_co_do_readv(BdrvChild *child,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
-    BdrvRequestFlags flags)
-{
-    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
-        return -EINVAL;
-    }
-
-    return bdrv_co_preadv(child, sector_num << BDRV_SECTOR_BITS,
-                          nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
-}
-
-int coroutine_fn bdrv_co_readv(BdrvChild *child, int64_t sector_num,
-                               int nb_sectors, QEMUIOVector *qiov)
-{
-    return bdrv_co_do_readv(child, sector_num, nb_sectors, qiov, 0);
-}
-
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
@@ -1795,24 +1777,6 @@ out:
     return ret;
 }

-static int coroutine_fn bdrv_co_do_writev(BdrvChild *child,
-    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
-    BdrvRequestFlags flags)
-{
-    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
-        return -EINVAL;
-    }
-
-    return bdrv_co_pwritev(child, sector_num << BDRV_SECTOR_BITS,
-                           nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
-}
-
-int coroutine_fn bdrv_co_writev(BdrvChild *child, int64_t sector_num,
-    int nb_sectors, QEMUIOVector *qiov)
-{
-    return bdrv_co_do_writev(child, sector_num, nb_sectors, qiov, 0);
-}
-
 int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
                                        int bytes, BdrvRequestFlags flags)
 {
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH 1/8] parallels: Switch to byte-based calls

[John Snow]

On 04/25/2018 02:32 PM, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the last few sector-based calls
> into the block layer from the parallels driver.
> 
> Ideally, the parallels driver should switch to doing everything
> byte-based, but that's a more invasive change that requires a
> bit more auditing.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Older than a month, so I'm assuming this is dropped for now.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 1/8] parallels: Switch to byte-based calls

[Eric Blake]

On 05/23/2018 02:19 PM, John Snow wrote:
> 
> 
> On 04/25/2018 02:32 PM, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Make the change for the last few sector-based calls
>> into the block layer from the parallels driver.
>>
>> Ideally, the parallels driver should switch to doing everything
>> byte-based, but that's a more invasive change that requires a
>> bit more auditing.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Older than a month, so I'm assuming this is dropped for now.

No, it's still awaiting a review; and still applies without any context 
changes.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/8] parallels: Switch to byte-based calls

[John Snow]

On 05/23/2018 04:09 PM, Eric Blake wrote:
> On 05/23/2018 02:19 PM, John Snow wrote:
>>
>>
>> On 04/25/2018 02:32 PM, Eric Blake wrote:
>>> We are gradually moving away from sector-based interfaces, towards
>>> byte-based.  Make the change for the last few sector-based calls
>>> into the block layer from the parallels driver.
>>>
>>> Ideally, the parallels driver should switch to doing everything
>>> byte-based, but that's a more invasive change that requires a
>>> bit more auditing.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Older than a month, so I'm assuming this is dropped for now.
> 
> No, it's still awaiting a review; and still applies without any context
> changes.

Oh, OK! then I'll count this as my naive ping-by-proxy.

--js

Re: [Qemu-devel] [PATCH 8/8] block: Removed unused sector-based vectored I/O

[Stefan Hajnoczi]

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

On Wed, Apr 25, 2018 at 01:32:23PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Now that all callers of vectored I/O have been converted
> to use our preferred byte-based bdrv_co_p{read,write}v(), we can
> delete the unused bdrv_co_{read,write}v().
> 
> Furthermore, this gets rid of the signature difference between
> the public bdrv_co_writev and the callback .bdrv_co_writev.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block.h |  4 ----
>  block/io.c            | 36 ------------------------------------
>  2 files changed, 40 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 1/8] parallels: Switch to byte-based calls

[Stefan Hajnoczi]

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

On Wed, Apr 25, 2018 at 01:32:16PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the last few sector-based calls
> into the block layer from the parallels driver.
> 
> Ideally, the parallels driver should switch to doing everything
> byte-based, but that's a more invasive change that requires a
> bit more auditing.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/parallels.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)

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

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

Re: [Qemu-devel] [PATCH 1/8] parallels: Switch to byte-based calls

[Denis V. Lunev]

On 04/25/2018 09:32 PM, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the last few sector-based calls
> into the block layer from the parallels driver.
>
> Ideally, the parallels driver should switch to doing everything
> byte-based, but that's a more invasive change that requires a
> bit more auditing.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/parallels.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 3f74fcb877a..c071c7f759f 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -226,14 +226,15 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>          };
>          qemu_iovec_init_external(&qiov, &iov, 1);
>
> -        ret = bdrv_co_readv(bs->backing, idx * s->tracks, nb_cow_sectors,
> -                            &qiov);
> +        ret = bdrv_co_preadv(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE,
> +                             nb_cow_bytes, &qiov, 0);
>          if (ret < 0) {
>              qemu_vfree(iov.iov_base);
>              return ret;
>          }
>
> -        ret = bdrv_co_writev(bs->file, s->data_end, nb_cow_sectors, &qiov);
> +        ret = bdrv_co_pwritev(bs->file, s->data_end * BDRV_SECTOR_SIZE,
> +                              nb_cow_bytes, &qiov, 0);
>          qemu_vfree(iov.iov_base);
>          if (ret < 0) {
>              return ret;
> @@ -339,7 +340,8 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
>          qemu_iovec_reset(&hd_qiov);
>          qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
>
> -        ret = bdrv_co_writev(bs->file, position, n, &hd_qiov);
> +        ret = bdrv_co_pwritev(bs->file, position * BDRV_SECTOR_SIZE, nbytes,
> +                              &hd_qiov, 0);
>          if (ret < 0) {
>              break;
>          }
> @@ -378,7 +380,8 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>
>          if (position < 0) {
>              if (bs->backing) {
> -                ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
> +                ret = bdrv_co_preadv(bs->backing, sector_num * BDRV_SECTOR_SIZE,
> +                                     nbytes, &hd_qiov, 0);
>                  if (ret < 0) {
>                      break;
>                  }
> @@ -386,7 +389,8 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>                  qemu_iovec_memset(&hd_qiov, 0, 0, nbytes);
>              }
>          } else {
> -            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
> +            ret = bdrv_co_preadv(bs->file, position * BDRV_SECTOR_SIZE, nbytes,
> +                                 &hd_qiov, 0);
>              if (ret < 0) {
>                  break;
>              }
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [Qemu-devel] [PATCH 2/8] qcow: Switch get_cluster_offset to be byte-based

[Kevin Wolf]

Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the internal helper function
> get_cluster_offset(), by changing n_start and n_end to by byte
> offsets rather than sector indices within the cluster being
> allocated.
> 
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index dd042b8ddbe..32730a8dd91 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -345,8 +345,8 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   *
>   * 0 to not allocate.
>   *
> - * 1 to allocate a normal cluster (for sector indexes 'n_start' to
> - * 'n_end')
> + * 1 to allocate a normal cluster (for byte offsets 'n_start' to
> + * 'n_end' within the cluster)
>   *
>   * 2 to allocate a compressed cluster of size
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
> @@ -442,7 +442,7 @@ static int get_cluster_offset(BlockDriverState *bs,
>          BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
>          /* allocate a new cluster */
>          if ((cluster_offset & QCOW_OFLAG_COMPRESSED) &&
> -            (n_end - n_start) < s->cluster_sectors) {
> +            (n_end - n_start) < s->cluster_size) {
>              /* if the cluster is already compressed, we must
>                 decompress it in the case it is not completely
>                 overwritten */
> @@ -480,16 +480,15 @@ static int get_cluster_offset(BlockDriverState *bs,
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
>                  if (bs->encrypted &&
> -                    (n_end - n_start) < s->cluster_sectors) {
> -                    uint64_t start_sect;
> +                    (n_end - n_start) < s->cluster_size) {
> +                    uint64_t start_offset;
>                      assert(s->crypto);
> -                    start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> -                    for(i = 0; i < s->cluster_sectors; i++) {
> +                    start_offset = offset & ~(s->cluster_size - 1);
> +                    for (i = 0; i < s->cluster_size; i += BDRV_SECTOR_SIZE) {
>                          if (i < n_start || i >= n_end) {
> -                            memset(s->cluster_data, 0x00, 512);
> +                            memset(s->cluster_data, 0x00, BDRV_SECTOR_SIZE);
>                              if (qcrypto_block_encrypt(s->crypto,
> -                                                      (start_sect + i) *
> -                                                      BDRV_SECTOR_SIZE,
> +                                                      start_offset + i,
>                                                        s->cluster_data,
>                                                        BDRV_SECTOR_SIZE,
>                                                        NULL) < 0) {

This code is still working in blocks of BDRV_SECTOR_SIZE here - which
you do need to keep at least partially because that's the block size
that qcrypto_block_encrypt() works with. qcrypto_block_qcow_encrypt()
even asserts it.

However, this means that even though n_start and n_end are byte-based
now, the code only works correctly with encrypted images if they are
multiples of BDRV_SECTOR_SIZE. This is currently true and we could
assert it, but it would kind of defeat the purpose of the patch.

I suppose you could make unaligned n_start/n_end work if you round down
n_start and round up n_end to the next sector boundary for the
comparison with i. For unaligned requests, we would then write a bit
more than is actually necessary, but I think that's okay because we're
initialising a previously unallocated cluster, so we don't overwrite
valid data.

Kevin

Re: [Qemu-devel] [PATCH 3/8] qcow: Switch qcow_co_readv to byte-based calls

[Kevin Wolf]

Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the internals of the qcow
> driver read function, by iterating over offset/bytes instead of
> sector_num/nb_sectors, and repurposing index_in_cluster and n
> to be bytes instead of sectors.
> 
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 32730a8dd91..bf9d80fd227 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -623,6 +623,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
>      void *orig_buf;
> +    int64_t offset = sector_num << BDRV_SECTOR_BITS;
> +    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;

This should be okay because nb_sectors is limited to INT_MAX / 512, but
I wouldn't be surprised if Coverity complained about a possible
truncation here. Multiplying with BDRV_SECTOR_SIZE instead wouldn't be
any less readable and would avoid the problem.

>      if (qiov->niov > 1) {
>          buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> @@ -636,36 +638,36 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> 
>      qemu_co_mutex_lock(&s->lock);
> 
> -    while (nb_sectors != 0) {
> +    while (bytes != 0) {
>          /* prepare next request */
> -        ret = get_cluster_offset(bs, sector_num << 9,
> +        ret = get_cluster_offset(bs, offset,
>                                   0, 0, 0, 0, &cluster_offset);

This surely fits in a single line now?

>          if (ret < 0) {
>              break;
>          }
> -        index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n = s->cluster_sectors - index_in_cluster;
> -        if (n > nb_sectors) {
> -            n = nb_sectors;
> +        index_in_cluster = offset & (s->cluster_size - 1);
> +        n = s->cluster_size - index_in_cluster;
> +        if (n > bytes) {
> +            n = bytes;
>          }

"index" suggests that it refers to an object larger than a byte. qcow2
renamed the variable to offset_in_cluster when the same change was made.
A new name for a unit change would also make review a bit easier.

The logic looks correct, though.

Kevin

Re: [Qemu-devel] [PATCH 4/8] qcow: Switch qcow_co_writev to byte-based calls

[Kevin Wolf]

Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Make the change for the internals of the qcow
> driver write function, by iterating over offset/bytes instead of
> sector_num/nb_sectors, and repurposing index_in_cluster and n
> to be bytes instead of sectors.
> 
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

The same comments from patch 3 apply here.

Kevin

Re: [Qemu-devel] [PATCH 0/8] block: more byte-based cleanups: vectored I/O

[Kevin Wolf]

Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> Based-on: <20180424192506.149089-1-eblake@redhat.com>
> ([PATCH v2 0/6] block: byte-based AIO read/write)
> Based-on: <20180424220157.177385-1-eblake@redhat.com>
> ([PATCH] block: Merge .bdrv_co_writev{, _flags} in drivers)
> 
> My quest continues.  I spent some time pruning qcow down as far
> as possible (and was dismayed at how long it took to prove no
> iotests regressions); so for the other drivers, I did the bare
> minimum to get rid of an interface, but will leave it to those
> file owners if they want to get rid of further pointless sector
> manipulations in their files.
> 
> Next on the chopping block: bdrv_read/bdrv_write.

Nice series, looks good apart from a few minor comments on the qcow1
conversion.

For v2, can you please make sure to have proper CCs also on the cover
letter?

Kevin

Re: [Qemu-devel] [PATCH 0/8] block: more byte-based cleanups: vectored I/O

[Eric Blake]

On 05/28/2018 06:19 AM, Kevin Wolf wrote:
> Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
>> Based-on: <20180424192506.149089-1-eblake@redhat.com>
>> ([PATCH v2 0/6] block: byte-based AIO read/write)
>> Based-on: <20180424220157.177385-1-eblake@redhat.com>
>> ([PATCH] block: Merge .bdrv_co_writev{, _flags} in drivers)
>>
>> My quest continues.  I spent some time pruning qcow down as far
>> as possible (and was dismayed at how long it took to prove no
>> iotests regressions); so for the other drivers, I did the bare
>> minimum to get rid of an interface, but will leave it to those
>> file owners if they want to get rid of further pointless sector
>> manipulations in their files.
>>
>> Next on the chopping block: bdrv_read/bdrv_write.
> 
> Nice series, looks good apart from a few minor comments on the qcow1
> conversion.

For qcow1, I kept things at 512-byte alignment throughout; as the format 
is not for new users, I find it easier to assert that things are aligned 
than to worry about sub-sector requests.  But yes, I can improve things 
according to your comments for v2.

> 
> For v2, can you please make sure to have proper CCs also on the cover
> letter?

Does git-publish do this automatically?  (If so, it's time for me to 
start using it...)

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

Re: [Qemu-devel] [PATCH 2/8] qcow: Switch get_cluster_offset to be byte-based

[Eric Blake]

On 05/28/2018 05:52 AM, Kevin Wolf wrote:
> Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Make the change for the internal helper function
>> get_cluster_offset(), by changing n_start and n_end to by byte
>> offsets rather than sector indices within the cluster being
>> allocated.
>>
>> A later patch will then switch the qcow driver as a whole over
>> to byte-based operation.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qcow.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>

>> +                    for (i = 0; i < s->cluster_size; i += BDRV_SECTOR_SIZE) {
>>                           if (i < n_start || i >= n_end) {
>> -                            memset(s->cluster_data, 0x00, 512);
>> +                            memset(s->cluster_data, 0x00, BDRV_SECTOR_SIZE);
>>                               if (qcrypto_block_encrypt(s->crypto,
>> -                                                      (start_sect + i) *
>> -                                                      BDRV_SECTOR_SIZE,
>> +                                                      start_offset + i,
>>                                                         s->cluster_data,
>>                                                         BDRV_SECTOR_SIZE,
>>                                                         NULL) < 0) {
> 
> This code is still working in blocks of BDRV_SECTOR_SIZE here - which
> you do need to keep at least partially because that's the block size
> that qcrypto_block_encrypt() works with. qcrypto_block_qcow_encrypt()
> even asserts it.
> 
> However, this means that even though n_start and n_end are byte-based
> now, the code only works correctly with encrypted images if they are
> multiples of BDRV_SECTOR_SIZE. This is currently true and we could
> assert it, but it would kind of defeat the purpose of the patch.

But in patch 5, I intentionally kept bs->bl.request_alignment at 512, so 
I'd rather just assert that n_start and n_end are properly aligned than 
to worry about rounding issues.

> 
> I suppose you could make unaligned n_start/n_end work if you round down
> n_start and round up n_end to the next sector boundary for the
> comparison with i. For unaligned requests, we would then write a bit
> more than is actually necessary, but I think that's okay because we're
> initialising a previously unallocated cluster, so we don't overwrite
> valid data.

The point is that we never have unaligned requests to qcow1.

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

Re: [Qemu-devel] [PATCH 3/8] qcow: Switch qcow_co_readv to byte-based calls

[Eric Blake]

On 05/28/2018 06:04 AM, Kevin Wolf wrote:
> Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Make the change for the internals of the qcow
>> driver read function, by iterating over offset/bytes instead of
>> sector_num/nb_sectors, and repurposing index_in_cluster and n
>> to be bytes instead of sectors.
>>
>> A later patch will then switch the qcow driver as a whole over
>> to byte-based operation.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qcow.c | 39 +++++++++++++++++++--------------------
>>   1 file changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/block/qcow.c b/block/qcow.c
>> index 32730a8dd91..bf9d80fd227 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -623,6 +623,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>>       QEMUIOVector hd_qiov;
>>       uint8_t *buf;
>>       void *orig_buf;
>> +    int64_t offset = sector_num << BDRV_SECTOR_BITS;
>> +    int64_t bytes = nb_sectors << BDRV_SECTOR_BITS;
> 
> This should be okay because nb_sectors is limited to INT_MAX / 512, but
> I wouldn't be surprised if Coverity complained about a possible
> truncation here. Multiplying with BDRV_SECTOR_SIZE instead wouldn't be
> any less readable and would avoid the problem.

Sure, will fix.

> 
>>       if (qiov->niov > 1) {
>>           buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
>> @@ -636,36 +638,36 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>>
>>       qemu_co_mutex_lock(&s->lock);
>>
>> -    while (nb_sectors != 0) {
>> +    while (bytes != 0) {
>>           /* prepare next request */
>> -        ret = get_cluster_offset(bs, sector_num << 9,
>> +        ret = get_cluster_offset(bs, offset,
>>                                    0, 0, 0, 0, &cluster_offset);
> 
> This surely fits in a single line now?
> 
>>           if (ret < 0) {
>>               break;
>>           }
>> -        index_in_cluster = sector_num & (s->cluster_sectors - 1);
>> -        n = s->cluster_sectors - index_in_cluster;
>> -        if (n > nb_sectors) {
>> -            n = nb_sectors;
>> +        index_in_cluster = offset & (s->cluster_size - 1);
>> +        n = s->cluster_size - index_in_cluster;
>> +        if (n > bytes) {
>> +            n = bytes;
>>           }
> 
> "index" suggests that it refers to an object larger than a byte. qcow2
> renamed the variable to offset_in_cluster when the same change was made.
> A new name for a unit change would also make review a bit easier.

Will fix.

> 
> The logic looks correct, though.
> 
> Kevin
> 

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

Re: [Qemu-devel] [PATCH 2/8] qcow: Switch get_cluster_offset to be byte-based

[Kevin Wolf]

Am 29.05.2018 um 17:03 hat Eric Blake geschrieben:
> On 05/28/2018 05:52 AM, Kevin Wolf wrote:
> > Am 25.04.2018 um 20:32 hat Eric Blake geschrieben:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based.  Make the change for the internal helper function
> > > get_cluster_offset(), by changing n_start and n_end to by byte
> > > offsets rather than sector indices within the cluster being
> > > allocated.
> > > 
> > > A later patch will then switch the qcow driver as a whole over
> > > to byte-based operation.
> > > 
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >   block/qcow.c | 28 ++++++++++++++--------------
> > >   1 file changed, 14 insertions(+), 14 deletions(-)
> > > 
> 
> > > +                    for (i = 0; i < s->cluster_size; i += BDRV_SECTOR_SIZE) {
> > >                           if (i < n_start || i >= n_end) {
> > > -                            memset(s->cluster_data, 0x00, 512);
> > > +                            memset(s->cluster_data, 0x00, BDRV_SECTOR_SIZE);
> > >                               if (qcrypto_block_encrypt(s->crypto,
> > > -                                                      (start_sect + i) *
> > > -                                                      BDRV_SECTOR_SIZE,
> > > +                                                      start_offset + i,
> > >                                                         s->cluster_data,
> > >                                                         BDRV_SECTOR_SIZE,
> > >                                                         NULL) < 0) {
> > 
> > This code is still working in blocks of BDRV_SECTOR_SIZE here - which
> > you do need to keep at least partially because that's the block size
> > that qcrypto_block_encrypt() works with. qcrypto_block_qcow_encrypt()
> > even asserts it.
> > 
> > However, this means that even though n_start and n_end are byte-based
> > now, the code only works correctly with encrypted images if they are
> > multiples of BDRV_SECTOR_SIZE. This is currently true and we could
> > assert it, but it would kind of defeat the purpose of the patch.
> 
> But in patch 5, I intentionally kept bs->bl.request_alignment at 512, so I'd
> rather just assert that n_start and n_end are properly aligned than to worry
> about rounding issues.

Yes, I hadn't read the whole series yet. So, sure, adding an assertion
works for me.

Kevin