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
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
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
© 2016 - 2025 Red Hat, Inc.