* [Qemu-devel] [PATCH] qcow2: Unlock during COW
@ 2011-09-19 15:19 Kevin Wolf
2011-09-22 13:27 ` Frediano Ziglio
0 siblings, 1 reply; 2+ messages in thread
From: Kevin Wolf @ 2011-09-19 15:19 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, freddy77
Unlocking during COW allows for more parallelism. One change it requires is
that buffers are dynamically allocated instead of just using a per-image
buffer.
While touching the code, drop the synchronous qcow2_read() function and replace
it by a bdrv_read() call.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2-cluster.c | 95 +++++++++++++-----------------------------------
1 files changed, 26 insertions(+), 69 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index c79323a..61b8e62 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -289,89 +289,42 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
}
}
-
-static int qcow2_read(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
-{
- BDRVQcowState *s = bs->opaque;
- int ret, index_in_cluster, n, n1;
- uint64_t cluster_offset;
- struct iovec iov;
- QEMUIOVector qiov;
-
- while (nb_sectors > 0) {
- n = nb_sectors;
-
- ret = qcow2_get_cluster_offset(bs, sector_num << 9, &n,
- &cluster_offset);
- if (ret < 0) {
- return ret;
- }
-
- index_in_cluster = sector_num & (s->cluster_sectors - 1);
- if (!cluster_offset) {
- if (bs->backing_hd) {
- /* read from the base image */
- iov.iov_base = buf;
- iov.iov_len = n * 512;
- qemu_iovec_init_external(&qiov, &iov, 1);
-
- n1 = qcow2_backing_read1(bs->backing_hd, &qiov, sector_num, n);
- if (n1 > 0) {
- BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING);
- ret = bdrv_read(bs->backing_hd, sector_num, buf, n1);
- if (ret < 0)
- return -1;
- }
- } else {
- memset(buf, 0, 512 * n);
- }
- } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
- if (qcow2_decompress_cluster(bs, cluster_offset) < 0)
- return -1;
- memcpy(buf, s->cluster_cache + index_in_cluster * 512, 512 * n);
- } else {
- BLKDBG_EVENT(bs->file, BLKDBG_READ);
- ret = bdrv_pread(bs->file, cluster_offset + index_in_cluster * 512, buf, n * 512);
- if (ret != n * 512)
- return -1;
- if (s->crypt_method) {
- qcow2_encrypt_sectors(s, sector_num, buf, buf, n, 0,
- &s->aes_decrypt_key);
- }
- }
- nb_sectors -= n;
- sector_num += n;
- buf += n * 512;
- }
- return 0;
-}
-
static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
uint64_t cluster_offset, int n_start, int n_end)
{
BDRVQcowState *s = bs->opaque;
int n, ret;
+ void *buf;
n = n_end - n_start;
- if (n <= 0)
+ if (n <= 0) {
return 0;
+ }
+
+ buf = qemu_blockalign(bs, n * BDRV_SECTOR_SIZE);
+
BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
- ret = qcow2_read(bs, start_sect + n_start, s->cluster_data, n);
- if (ret < 0)
- return ret;
+ ret = bdrv_read(bs, start_sect + n_start, buf, n);
+ if (ret < 0) {
+ goto out;
+ }
+
if (s->crypt_method) {
qcow2_encrypt_sectors(s, start_sect + n_start,
- s->cluster_data,
- s->cluster_data, n, 1,
+ buf, buf, n, 1,
&s->aes_encrypt_key);
}
+
BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
- ret = bdrv_write(bs->file, (cluster_offset >> 9) + n_start,
- s->cluster_data, n);
- if (ret < 0)
- return ret;
- return 0;
+ ret = bdrv_write(bs->file, (cluster_offset >> 9) + n_start, buf, n);
+ if (ret < 0) {
+ goto out;
+ }
+
+ ret = 0;
+out:
+ qemu_vfree(buf);
+ return ret;
}
@@ -620,7 +573,9 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
start_sect = (m->offset & ~(s->cluster_size - 1)) >> 9;
if (m->n_start) {
cow = true;
+ qemu_co_mutex_unlock(&s->lock);
ret = copy_sectors(bs, start_sect, cluster_offset, 0, m->n_start);
+ qemu_co_mutex_lock(&s->lock);
if (ret < 0)
goto err;
}
@@ -628,8 +583,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
if (m->nb_available & (s->cluster_sectors - 1)) {
uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
cow = true;
+ qemu_co_mutex_unlock(&s->lock);
ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
m->nb_available - end, s->cluster_sectors);
+ qemu_co_mutex_lock(&s->lock);
if (ret < 0)
goto err;
}
--
1.7.6.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2: Unlock during COW
2011-09-19 15:19 [Qemu-devel] [PATCH] qcow2: Unlock during COW Kevin Wolf
@ 2011-09-22 13:27 ` Frediano Ziglio
0 siblings, 0 replies; 2+ messages in thread
From: Frediano Ziglio @ 2011-09-22 13:27 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel
2011/9/19 Kevin Wolf <kwolf@redhat.com>:
> Unlocking during COW allows for more parallelism. One change it requires is
> that buffers are dynamically allocated instead of just using a per-image
> buffer.
>
> While touching the code, drop the synchronous qcow2_read() function and replace
> it by a bdrv_read() call.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2-cluster.c | 95 +++++++++++++-----------------------------------
> 1 files changed, 26 insertions(+), 69 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index c79323a..61b8e62 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -289,89 +289,42 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> }
> }
>
> -
> -static int qcow2_read(BlockDriverState *bs, int64_t sector_num,
> - uint8_t *buf, int nb_sectors)
> -{
> - BDRVQcowState *s = bs->opaque;
> - int ret, index_in_cluster, n, n1;
> - uint64_t cluster_offset;
> - struct iovec iov;
> - QEMUIOVector qiov;
> -
> - while (nb_sectors > 0) {
> - n = nb_sectors;
> -
> - ret = qcow2_get_cluster_offset(bs, sector_num << 9, &n,
> - &cluster_offset);
> - if (ret < 0) {
> - return ret;
> - }
> -
> - index_in_cluster = sector_num & (s->cluster_sectors - 1);
> - if (!cluster_offset) {
> - if (bs->backing_hd) {
> - /* read from the base image */
> - iov.iov_base = buf;
> - iov.iov_len = n * 512;
> - qemu_iovec_init_external(&qiov, &iov, 1);
> -
> - n1 = qcow2_backing_read1(bs->backing_hd, &qiov, sector_num, n);
> - if (n1 > 0) {
> - BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING);
> - ret = bdrv_read(bs->backing_hd, sector_num, buf, n1);
> - if (ret < 0)
> - return -1;
> - }
> - } else {
> - memset(buf, 0, 512 * n);
> - }
> - } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> - if (qcow2_decompress_cluster(bs, cluster_offset) < 0)
> - return -1;
> - memcpy(buf, s->cluster_cache + index_in_cluster * 512, 512 * n);
> - } else {
> - BLKDBG_EVENT(bs->file, BLKDBG_READ);
> - ret = bdrv_pread(bs->file, cluster_offset + index_in_cluster * 512, buf, n * 512);
> - if (ret != n * 512)
> - return -1;
> - if (s->crypt_method) {
> - qcow2_encrypt_sectors(s, sector_num, buf, buf, n, 0,
> - &s->aes_decrypt_key);
> - }
> - }
> - nb_sectors -= n;
> - sector_num += n;
> - buf += n * 512;
> - }
> - return 0;
> -}
> -
> static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
> uint64_t cluster_offset, int n_start, int n_end)
> {
> BDRVQcowState *s = bs->opaque;
> int n, ret;
> + void *buf;
>
> n = n_end - n_start;
> - if (n <= 0)
> + if (n <= 0) {
> return 0;
> + }
> +
> + buf = qemu_blockalign(bs, n * BDRV_SECTOR_SIZE);
> +
> BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
> - ret = qcow2_read(bs, start_sect + n_start, s->cluster_data, n);
> - if (ret < 0)
> - return ret;
> + ret = bdrv_read(bs, start_sect + n_start, buf, n);
> + if (ret < 0) {
> + goto out;
> + }
> +
> if (s->crypt_method) {
> qcow2_encrypt_sectors(s, start_sect + n_start,
> - s->cluster_data,
> - s->cluster_data, n, 1,
> + buf, buf, n, 1,
> &s->aes_encrypt_key);
> }
> +
> BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
> - ret = bdrv_write(bs->file, (cluster_offset >> 9) + n_start,
> - s->cluster_data, n);
> - if (ret < 0)
> - return ret;
> - return 0;
> + ret = bdrv_write(bs->file, (cluster_offset >> 9) + n_start, buf, n);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + ret = 0;
> +out:
> + qemu_vfree(buf);
> + return ret;
> }
>
>
> @@ -620,7 +573,9 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
> start_sect = (m->offset & ~(s->cluster_size - 1)) >> 9;
> if (m->n_start) {
> cow = true;
> + qemu_co_mutex_unlock(&s->lock);
> ret = copy_sectors(bs, start_sect, cluster_offset, 0, m->n_start);
> + qemu_co_mutex_lock(&s->lock);
> if (ret < 0)
> goto err;
> }
> @@ -628,8 +583,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
> if (m->nb_available & (s->cluster_sectors - 1)) {
> uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
> cow = true;
> + qemu_co_mutex_unlock(&s->lock);
> ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
> m->nb_available - end, s->cluster_sectors);
> + qemu_co_mutex_lock(&s->lock);
> if (ret < 0)
> goto err;
> }
> --
> 1.7.6.2
>
>
Seems good, I like the idea to reuse bdrv_read instead of a new function.
However I don't understand why (seems related to range checking in
block.c) 028 iotest fails
Frediano
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-09-22 13:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-19 15:19 [Qemu-devel] [PATCH] qcow2: Unlock during COW Kevin Wolf
2011-09-22 13:27 ` Frediano Ziglio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).