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

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

[Eric Blake]

My quest continues.  I spent some time pruning sector-based usage
out of qcow 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.

In v2:
- throughout: add collected R-b tags
- patch 2: add assert [Kevin]
- patch 3-4: improve readability [Kevin]
- patch 8: retitle to fix typo [Kashyap]

001/8:[----] [--] 'parallels: Switch to byte-based calls'
002/8:[0005] [FC] 'qcow: Switch get_cluster_offset to be byte-based'
003/8:[0017] [FC] 'qcow: Switch qcow_co_readv to byte-based calls'
004/8:[0016] [FC] 'qcow: Switch qcow_co_writev to byte-based calls'
005/8:[0008] [FC] 'qcow: Switch to a byte-based driver'
006/8:[----] [--] 'replication: Switch to byte-based calls'
007/8:[----] [--] 'vhdx: Switch to byte-based calls'
008/8:[down] 'block: Remove unused sector-based vectored I/O'

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: Remove unused sector-based vectored I/O

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

-- 
2.14.3

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6e9c37f44e1..a761c896d35 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 v2 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.  However, assert that these values are still
sector-aligned (at least qcrypto_block_encrypt() still wants that).
For now we get that alignment for free because we still use
sector-based driver callbacks.

A later patch will then switch the qcow driver as a whole over
to byte-based operation; but will still leave things at sector
alignments as it is not worth auditing the qcow image format
to worry about sub-sector requests.

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

---
v2: assert sector alignment [Max]
---
 block/qcow.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 3ba2ca25ea5..42d1bbb7643 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 sector-aligned 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 <
@@ -440,9 +440,10 @@ static int get_cluster_offset(BlockDriverState *bs,
         if (!allocate)
             return 0;
         BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
+        assert(QEMU_IS_ALIGNED(n_start | n_end, BDRV_SECTOR_SIZE));
         /* 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 +481,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 +497,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 +759,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 v2 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 with a rename of index_in_cluster and
repurposing of n to track 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>

---
v2: prefer 64-bit * over 32-bit <<, rename variable for legibility [Kevin]
---
 block/qcow.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 42d1bbb7643..b90915218ff 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -617,13 +617,15 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
                          int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
-    int index_in_cluster;
+    int offset_in_cluster;
     int ret = 0, n;
     uint64_t cluster_offset;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
+    int64_t offset = sector_num * BDRV_SECTOR_SIZE;
+    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;

     if (qiov->niov > 1) {
         buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
@@ -637,36 +639,35 @@ 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,
-                                 0, 0, 0, 0, &cluster_offset);
+        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;
+        offset_in_cluster = offset & (s->cluster_size - 1);
+        n = s->cluster_size - offset_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 ? */
@@ -674,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 + offset_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 + offset_in_cluster,
+                                 n, &hd_qiov, 0);
             qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 break;
@@ -696,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;
                 }
@@ -705,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 v2 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 with a rename of index_in_cluster and
repurposing of n to track 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>

---
v2: prefer 64-bit * over 23-bit <<, rename variable for legibility [Kevin]
---
 block/qcow.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index b90915218ff..44adeba276c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -723,13 +723,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
                                        int flags)
 {
     BDRVQcowState *s = bs->opaque;
-    int index_in_cluster;
+    int offset_in_cluster;
     uint64_t cluster_offset;
     int ret = 0, n;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
+    int64_t offset = sector_num * BDRV_SECTOR_SIZE;
+    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;

     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) {
+        offset_in_cluster = offset & (s->cluster_size - 1);
+        n = s->cluster_size - offset_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, offset_in_cluster,
+                                 offset_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 + offset_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 v2 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 asserts present
because of encryption, but easier to do everywhere than to audit
which sub-sector requests are handled correctly, especially since
we no longer recommend qcow for new disk images).

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

---
v2: minor rebase to changes earlier in series
---
 block/qcow.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 44adeba276c..55720b006ef 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;
@@ -613,8 +611,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 offset_in_cluster;
@@ -624,9 +632,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_SIZE;
-    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;

+    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 offset_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_SIZE;
-    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;

     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 v2 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 826db7b3049..6349d6958e4 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 v2 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 0b1e21c7501..295d3276120 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 v2 8/8] block: Remove 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 (the
latter still exists, because some drivers still need more work
before they are fully byte-based).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v2: commit typo fix [Kashyap]
---
 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 3894edda9de..fe40d2929ac 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -285,10 +285,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 ca96b487eb8..1d86bfc0072 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1341,24 +1341,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)
 {
@@ -1801,24 +1783,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 v2 1/8] parallels: Switch to byte-based calls

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:39PM -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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/parallels.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 6e9c37f44e1..a761c896d35 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
> 
> 

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:45PM -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 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>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/vhdx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 0b1e21c7501..295d3276120 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
> 

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:44PM -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 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>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/replication.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c
> index 826db7b3049..6349d6958e4 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
> 
> 

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:46PM -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 (the
> latter still exists, because some drivers still need more work
> before they are fully byte-based).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
> v2: commit typo fix [Kashyap]
> ---
>  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 3894edda9de..fe40d2929ac 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -285,10 +285,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 ca96b487eb8..1d86bfc0072 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1341,24 +1341,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)
>  {
> @@ -1801,24 +1783,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 v2 2/8] qcow: Switch get_cluster_offset to be byte-based

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:40PM -0500, Eric Blake wrote:
> 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

s/by\ byte/be\ byte/

> offsets rather than sector indices within the cluster being
> allocated.  However, assert that these values are still
> sector-aligned (at least qcrypto_block_encrypt() still wants that).
> For now we get that alignment for free because we still use
> sector-based driver callbacks.
> 
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation; but will still leave things at sector
> alignments as it is not worth auditing the qcow image format
> to worry about sub-sector requests.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> 
> ---
> v2: assert sector alignment [Max]
> ---
>  block/qcow.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 3ba2ca25ea5..42d1bbb7643 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 sector-aligned 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 <
> @@ -440,9 +440,10 @@ static int get_cluster_offset(BlockDriverState *bs,
>          if (!allocate)
>              return 0;
>          BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
> +        assert(QEMU_IS_ALIGNED(n_start | n_end, BDRV_SECTOR_SIZE));
>          /* 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 +481,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 +497,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 +759,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
> 
> 

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:41PM -0500, Eric Blake wrote:
> 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 with a rename of index_in_cluster and
> repurposing of n to track 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>
> 
> ---
> v2: prefer 64-bit * over 32-bit <<, rename variable for legibility [Kevin]
> ---
>  block/qcow.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 42d1bbb7643..b90915218ff 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -617,13 +617,15 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>                           int nb_sectors, QEMUIOVector *qiov)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int index_in_cluster;
> +    int offset_in_cluster;
>      int ret = 0, n;
>      uint64_t cluster_offset;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
>      void *orig_buf;
> +    int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> +    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
> 
>      if (qiov->niov > 1) {
>          buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> @@ -637,36 +639,35 @@ 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,
> -                                 0, 0, 0, 0, &cluster_offset);
> +        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;
> +        offset_in_cluster = offset & (s->cluster_size - 1);
> +        n = s->cluster_size - offset_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 ? */
> @@ -674,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 + offset_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 + offset_in_cluster,
> +                                 n, &hd_qiov, 0);
>              qemu_co_mutex_lock(&s->lock);
>              if (ret < 0) {
>                  break;
> @@ -696,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;
>                  }
> @@ -705,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
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:42PM -0500, Eric Blake wrote:
> 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 with a rename of index_in_cluster and
> repurposing of n to track 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>
> 
> ---
> v2: prefer 64-bit * over 23-bit <<, rename variable for legibility [Kevin]
> ---
>  block/qcow.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index b90915218ff..44adeba276c 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -723,13 +723,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>                                         int flags)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int index_in_cluster;
> +    int offset_in_cluster;
>      uint64_t cluster_offset;
>      int ret = 0, n;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
>      void *orig_buf;
> +    int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> +    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
> 
>      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) {
> +        offset_in_cluster = offset & (s->cluster_size - 1);
> +        n = s->cluster_size - offset_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, offset_in_cluster,
> +                                 offset_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 + offset_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
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

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

[Jeff Cody]

On Thu, May 31, 2018 at 03:50:43PM -0500, Eric Blake wrote:
> 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 asserts present
> because of encryption, but easier to do everywhere than to audit
> which sub-sector requests are handled correctly, especially since
> we no longer recommend qcow for new disk images).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: minor rebase to changes earlier in series
> ---
>  block/qcow.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 44adeba276c..55720b006ef 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;
> @@ -613,8 +611,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 offset_in_cluster;
> @@ -624,9 +632,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_SIZE;
> -    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
> 
> +    assert(!flags);

Why this assert here and in the _pwritev()?

>      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 offset_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_SIZE;
> -    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
> 
>      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
> 
> 

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

[Eric Blake]

On 06/04/2018 04:33 PM, Jeff Cody wrote:
> On Thu, May 31, 2018 at 03:50:43PM -0500, Eric Blake wrote:
>> 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 asserts present
>> because of encryption, but easier to do everywhere than to audit
>> which sub-sector requests are handled correctly, especially since
>> we no longer recommend qcow for new disk images).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>

>> -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 offset_in_cluster;
>> @@ -624,9 +632,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_SIZE;
>> -    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
>>
>> +    assert(!flags);
> 
> Why this assert here and in the _pwritev()?

We're changing from an interface that didn't have flags to one that 
does, but we are not prepared to handle any flags, so the assert proves 
the block layer doesn't hand us any flags we aren't expecting (there are 
no block layer flags for pread at the moment; and no flags for pwrite 
because we didn't set bs->supported_write_flags).

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

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

[Jeff Cody]

On Tue, Jun 05, 2018 at 06:12:33AM -0500, Eric Blake wrote:
> On 06/04/2018 04:33 PM, Jeff Cody wrote:
> >On Thu, May 31, 2018 at 03:50:43PM -0500, Eric Blake wrote:
> >>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 asserts present
> >>because of encryption, but easier to do everywhere than to audit
> >>which sub-sector requests are handled correctly, especially since
> >>we no longer recommend qcow for new disk images).
> >>
> >>Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> >>-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 offset_in_cluster;
> >>@@ -624,9 +632,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_SIZE;
> >>-    int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
> >>
> >>+    assert(!flags);
> >
> >Why this assert here and in the _pwritev()?
> 
> We're changing from an interface that didn't have flags to one that does,
> but we are not prepared to handle any flags, so the assert proves the block
> layer doesn't hand us any flags we aren't expecting (there are no block
> layer flags for pread at the moment; and no flags for pwrite because we
> didn't set bs->supported_write_flags).


Thanks.

Reviewed-by: Jeff Cody <jcody@redhat.com>