| Message ID | 1464974478-23598-6-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/03/2016 11:21 AM, Kevin Wolf wrote: > This changes qcow2 to implement the byte-based .bdrv_co_pwritev > interface rather than the sector-based old one. > > As preallocation uses the same allocation function as normal writes, and > the interface of that function needs to be changed, it is converted in > the same patch. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2-cluster.c | 13 ++++---- > block/qcow2.c | 82 ++++++++++++++++++++++++--------------------------- > block/qcow2.h | 3 +- > trace-events | 6 ++-- > 4 files changed, 49 insertions(+), 55 deletions(-) > > +++ b/block/qcow2-cluster.c > @@ -1261,7 +1261,8 @@ fail: > * Return 0 on success and -errno in error cases > */ > int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > - int *num, uint64_t *host_offset, QCowL2Meta **m) > + unsigned int *bytes, uint64_t *host_offset, > + QCowL2Meta **m) > { > > - *num -= remaining >> BDRV_SECTOR_BITS; > - assert(*num > 0); > + *bytes -= remaining; > + assert(*bytes > 0); The assertion is now weaker. Beforehand, num could go negative. But bytes cannot, all it can do is go to 0. If we wrap around, the assertion won't catch it. Do you want to strengthen it by also asserting that bytes < INT_MAX? > +++ b/block/qcow2.c > @@ -1544,23 +1544,20 @@ fail: > return ret; > } > > -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > - int64_t sector_num, > - int remaining_sectors, > - QEMUIOVector *qiov) > +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, > + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) Worth consistent indentation, while touching it? > + while (bytes != 0) { > > l2meta = NULL; > > trace_qcow2_writev_start_part(qemu_coroutine_self()); > - index_in_cluster = sector_num & (s->cluster_sectors - 1); > - cur_nr_sectors = remaining_sectors; > - if (bs->encrypted && > - cur_nr_sectors > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) { > - cur_nr_sectors = > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster; > + offset_in_cluster = offset_into_cluster(s, offset); > + cur_bytes = MIN(bytes, INT_MAX); > + if (bs->encrypted) { > + cur_bytes = MIN(cur_bytes, > + QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size > + - offset_in_cluster); Again, if the block layer learns to auto-fragment, then we should just inform the block layer about QCOW_MAX_CRYPT_CLUSTERS as our max_transfer limit. > @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > qemu_co_mutex_unlock(&s->lock); > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > trace_qcow2_writev_data(qemu_coroutine_self(), > - (cluster_offset >> 9) + index_in_cluster); > - ret = bdrv_co_writev(bs->file->bs, > - (cluster_offset >> 9) + index_in_cluster, > - cur_nr_sectors, &hd_qiov); > + cluster_offset + offset_in_cluster); > + ret = bdrv_co_pwritev(bs->file->bs, > + cluster_offset + offset_in_cluster, > + cur_bytes, &hd_qiov, 0); s/0/flags/, even if .supported_write_flags stays 0 for now. > @@ -2008,19 +2002,19 @@ static int qcow2_change_backing_file(BlockDriverState *bs, > > static int preallocate(BlockDriverState *bs) > { > - uint64_t nb_sectors; > + uint64_t bytes; > uint64_t offset; > uint64_t host_offset = 0; > - int num; > + unsigned int cur_bytes; > int ret; > QCowL2Meta *meta; > > - nb_sectors = bdrv_nb_sectors(bs); > + bytes = bdrv_getlength(bs); > offset = 0; > > - while (nb_sectors) { > - num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > - ret = qcow2_alloc_cluster_offset(bs, offset, &num, > + while (bytes) { > + cur_bytes = MIN(bytes, INT_MAX); Okay, while bdrv_co_pwritev() should (eventually) let the block layer auto-fragment, here, you are right that you have to fragment yourself since this is not being called from the block layer. > @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs) > uint8_t buf[BDRV_SECTOR_SIZE]; > memset(buf, 0, BDRV_SECTOR_SIZE); > ret = bdrv_write(bs->file->bs, > - (host_offset >> BDRV_SECTOR_BITS) + num - 1, > + ((host_offset + cur_bytes) >> BDRV_SECTOR_BITS) - 1, > buf, 1); Do we still have to call bdrv_write(), or can we convert this to a byte-based interface call?
On 06/03/2016 11:21 AM, Kevin Wolf wrote: > This changes qcow2 to implement the byte-based .bdrv_co_pwritev > interface rather than the sector-based old one. > Subject line consistency: 2/5 has trailing (), this one does not. I prefer 2/5. > As preallocation uses the same allocation function as normal writes, and > the interface of that function needs to be changed, it is converted in > the same patch. >
Am 03.06.2016 um 22:13 hat Eric Blake geschrieben: > On 06/03/2016 11:21 AM, Kevin Wolf wrote: > > This changes qcow2 to implement the byte-based .bdrv_co_pwritev > > interface rather than the sector-based old one. > > > > As preallocation uses the same allocation function as normal writes, and > > the interface of that function needs to be changed, it is converted in > > the same patch. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/qcow2-cluster.c | 13 ++++---- > > block/qcow2.c | 82 ++++++++++++++++++++++++--------------------------- > > block/qcow2.h | 3 +- > > trace-events | 6 ++-- > > 4 files changed, 49 insertions(+), 55 deletions(-) > > > > > +++ b/block/qcow2-cluster.c > > @@ -1261,7 +1261,8 @@ fail: > > * Return 0 on success and -errno in error cases > > */ > > int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > > - int *num, uint64_t *host_offset, QCowL2Meta **m) > > + unsigned int *bytes, uint64_t *host_offset, > > + QCowL2Meta **m) > > { > > > > > - *num -= remaining >> BDRV_SECTOR_BITS; > > - assert(*num > 0); > > + *bytes -= remaining; > > + assert(*bytes > 0); > > The assertion is now weaker. Beforehand, num could go negative. But > bytes cannot, all it can do is go to 0. If we wrap around, the > assertion won't catch it. Do you want to strengthen it by also > asserting that bytes < INT_MAX? Is it useful to assert this, though? We have tons of such loops and we never assert that cur_bytes <= bytes (which is what you would be checking effectively) because it's quite obvious. This assertion was meant to document the less obvious fact that we always make some progress and never return 0 bytes. > > +++ b/block/qcow2.c > > @@ -1544,23 +1544,20 @@ fail: > > return ret; > > } > > > > -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > > - int64_t sector_num, > > - int remaining_sectors, > > - QEMUIOVector *qiov) > > +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, > > + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) > > Worth consistent indentation, while touching it? Will fix. > > + while (bytes != 0) { > > > > l2meta = NULL; > > > > trace_qcow2_writev_start_part(qemu_coroutine_self()); > > - index_in_cluster = sector_num & (s->cluster_sectors - 1); > > - cur_nr_sectors = remaining_sectors; > > - if (bs->encrypted && > > - cur_nr_sectors > > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) { > > - cur_nr_sectors = > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster; > > + offset_in_cluster = offset_into_cluster(s, offset); > > + cur_bytes = MIN(bytes, INT_MAX); > > + if (bs->encrypted) { > > + cur_bytes = MIN(cur_bytes, > > + QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size > > + - offset_in_cluster); > > Again, if the block layer learns to auto-fragment, then we should just > inform the block layer about QCOW_MAX_CRYPT_CLUSTERS as our max_transfer > limit. > > > > @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > > qemu_co_mutex_unlock(&s->lock); > > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > > trace_qcow2_writev_data(qemu_coroutine_self(), > > - (cluster_offset >> 9) + index_in_cluster); > > - ret = bdrv_co_writev(bs->file->bs, > > - (cluster_offset >> 9) + index_in_cluster, > > - cur_nr_sectors, &hd_qiov); > > + cluster_offset + offset_in_cluster); > > + ret = bdrv_co_pwritev(bs->file->bs, > > + cluster_offset + offset_in_cluster, > > + cur_bytes, &hd_qiov, 0); > > s/0/flags/, even if .supported_write_flags stays 0 for now. I don't agree. That might make sense for a passthrough driver like raw, but I don't see a reason to guess for qcow2 that passing flags down is the right thing to do. The only flag that we currently have (FUA) isn't correctly implemented by passing it to the lower layer because qcow2 metadata must be considered. So I would leave changing flags here for the patch that adds some flag to .supported_write_flags. > > @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs) > > uint8_t buf[BDRV_SECTOR_SIZE]; > > memset(buf, 0, BDRV_SECTOR_SIZE); > > ret = bdrv_write(bs->file->bs, > > - (host_offset >> BDRV_SECTOR_BITS) + num - 1, > > + ((host_offset + cur_bytes) >> BDRV_SECTOR_BITS) - 1, > > buf, 1); > > Do we still have to call bdrv_write(), or can we convert this to a > byte-based interface call? Okay, I'll make it a one-byte bdrv_pwrite(). Kevin
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 112f056..9c8378b 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1261,7 +1261,8 @@ fail: * Return 0 on success and -errno in error cases */ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, - int *num, uint64_t *host_offset, QCowL2Meta **m) + unsigned int *bytes, uint64_t *host_offset, + QCowL2Meta **m) { BDRVQcow2State *s = bs->opaque; uint64_t start, remaining; @@ -1269,13 +1270,11 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, uint64_t cur_bytes; int ret; - trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num); - - assert((offset & ~BDRV_SECTOR_MASK) == 0); + trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *bytes); again: start = offset; - remaining = (uint64_t)*num << BDRV_SECTOR_BITS; + remaining = *bytes; cluster_offset = 0; *host_offset = 0; cur_bytes = 0; @@ -1361,8 +1360,8 @@ again: } } - *num -= remaining >> BDRV_SECTOR_BITS; - assert(*num > 0); + *bytes -= remaining; + assert(*bytes > 0); assert(*host_offset != 0); return 0; diff --git a/block/qcow2.c b/block/qcow2.c index b498753..68c6d0d 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1544,23 +1544,20 @@ fail: return ret; } -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, - int64_t sector_num, - int remaining_sectors, - QEMUIOVector *qiov) +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { BDRVQcow2State *s = bs->opaque; - int index_in_cluster; + int offset_in_cluster; int ret; - int cur_nr_sectors; /* number of sectors in current iteration */ + unsigned int cur_bytes; /* number of sectors in current iteration */ uint64_t cluster_offset; QEMUIOVector hd_qiov; uint64_t bytes_done = 0; uint8_t *cluster_data = NULL; QCowL2Meta *l2meta = NULL; - trace_qcow2_writev_start_req(qemu_coroutine_self(), sector_num, - remaining_sectors); + trace_qcow2_writev_start_req(qemu_coroutine_self(), offset, bytes); qemu_iovec_init(&hd_qiov, qiov->niov); @@ -1568,22 +1565,21 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, qemu_co_mutex_lock(&s->lock); - while (remaining_sectors != 0) { + while (bytes != 0) { l2meta = NULL; trace_qcow2_writev_start_part(qemu_coroutine_self()); - index_in_cluster = sector_num & (s->cluster_sectors - 1); - cur_nr_sectors = remaining_sectors; - if (bs->encrypted && - cur_nr_sectors > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) { - cur_nr_sectors = - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster; + offset_in_cluster = offset_into_cluster(s, offset); + cur_bytes = MIN(bytes, INT_MAX); + if (bs->encrypted) { + cur_bytes = MIN(cur_bytes, + QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + - offset_in_cluster); } - ret = qcow2_alloc_cluster_offset(bs, sector_num << 9, - &cur_nr_sectors, &cluster_offset, &l2meta); + ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes, + &cluster_offset, &l2meta); if (ret < 0) { goto fail; } @@ -1591,8 +1587,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, assert((cluster_offset & 511) == 0); qemu_iovec_reset(&hd_qiov); - qemu_iovec_concat(&hd_qiov, qiov, bytes_done, - cur_nr_sectors * 512); + qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes); if (bs->encrypted) { Error *err = NULL; @@ -1611,8 +1606,9 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size); - if (qcow2_encrypt_sectors(s, sector_num, cluster_data, - cluster_data, cur_nr_sectors, + if (qcow2_encrypt_sectors(s, offset >> BDRV_SECTOR_BITS, + cluster_data, cluster_data, + cur_bytes >>BDRV_SECTOR_BITS, true, &err) < 0) { error_free(err); ret = -EIO; @@ -1620,13 +1616,11 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, } qemu_iovec_reset(&hd_qiov); - qemu_iovec_add(&hd_qiov, cluster_data, - cur_nr_sectors * 512); + qemu_iovec_add(&hd_qiov, cluster_data, cur_bytes); } ret = qcow2_pre_write_overlap_check(bs, 0, - cluster_offset + index_in_cluster * BDRV_SECTOR_SIZE, - cur_nr_sectors * BDRV_SECTOR_SIZE); + cluster_offset + offset_in_cluster, cur_bytes); if (ret < 0) { goto fail; } @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, qemu_co_mutex_unlock(&s->lock); BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); trace_qcow2_writev_data(qemu_coroutine_self(), - (cluster_offset >> 9) + index_in_cluster); - ret = bdrv_co_writev(bs->file->bs, - (cluster_offset >> 9) + index_in_cluster, - cur_nr_sectors, &hd_qiov); + cluster_offset + offset_in_cluster); + ret = bdrv_co_pwritev(bs->file->bs, + cluster_offset + offset_in_cluster, + cur_bytes, &hd_qiov, 0); qemu_co_mutex_lock(&s->lock); if (ret < 0) { goto fail; @@ -1663,10 +1657,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, l2meta = next; } - remaining_sectors -= cur_nr_sectors; - sector_num += cur_nr_sectors; - bytes_done += cur_nr_sectors * 512; - trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_nr_sectors); + bytes -= cur_bytes; + offset += cur_bytes; + bytes_done += cur_bytes; + trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_bytes); } ret = 0; @@ -2008,19 +2002,19 @@ static int qcow2_change_backing_file(BlockDriverState *bs, static int preallocate(BlockDriverState *bs) { - uint64_t nb_sectors; + uint64_t bytes; uint64_t offset; uint64_t host_offset = 0; - int num; + unsigned int cur_bytes; int ret; QCowL2Meta *meta; - nb_sectors = bdrv_nb_sectors(bs); + bytes = bdrv_getlength(bs); offset = 0; - while (nb_sectors) { - num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); - ret = qcow2_alloc_cluster_offset(bs, offset, &num, + while (bytes) { + cur_bytes = MIN(bytes, INT_MAX); + ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes, &host_offset, &meta); if (ret < 0) { return ret; @@ -2046,8 +2040,8 @@ static int preallocate(BlockDriverState *bs) /* TODO Preallocate data if requested */ - nb_sectors -= num; - offset += num << BDRV_SECTOR_BITS; + bytes -= cur_bytes; + offset += cur_bytes; } /* @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs) uint8_t buf[BDRV_SECTOR_SIZE]; memset(buf, 0, BDRV_SECTOR_SIZE); ret = bdrv_write(bs->file->bs, - (host_offset >> BDRV_SECTOR_BITS) + num - 1, + ((host_offset + cur_bytes) >> BDRV_SECTOR_BITS) - 1, buf, 1); if (ret < 0) { return ret; @@ -3379,7 +3373,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_set_key = qcow2_set_key, .bdrv_co_preadv = qcow2_co_preadv, - .bdrv_co_writev = qcow2_co_writev, + .bdrv_co_pwritev = qcow2_co_pwritev, .bdrv_co_flush_to_os = qcow2_co_flush_to_os, .bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes, diff --git a/block/qcow2.h b/block/qcow2.h index 175b0c1..b36a7bf 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -539,7 +539,8 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, unsigned int *bytes, uint64_t *cluster_offset); int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, - int *num, uint64_t *host_offset, QCowL2Meta **m); + unsigned int *bytes, uint64_t *host_offset, + QCowL2Meta **m); uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, uint64_t offset, int compressed_size); diff --git a/trace-events b/trace-events index 353454c..81c9ec3 100644 --- a/trace-events +++ b/trace-events @@ -606,16 +606,16 @@ qemu_system_shutdown_request(void) "" qemu_system_powerdown_request(void) "" # block/qcow2.c -qcow2_writev_start_req(void *co, int64_t sector, int nb_sectors) "co %p sector %" PRIx64 " nb_sectors %d" +qcow2_writev_start_req(void *co, int64_t offset, int bytes) "co %p offset %" PRIx64 " bytes %d" qcow2_writev_done_req(void *co, int ret) "co %p ret %d" qcow2_writev_start_part(void *co) "co %p" -qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d" +qcow2_writev_done_part(void *co, int cur_bytes) "co %p cur_bytes %d" qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64 qcow2_pwrite_zeroes_start_req(void *co, int64_t offset, int count) "co %p offset %" PRIx64 " count %d" qcow2_pwrite_zeroes(void *co, int64_t offset, int count) "co %p offset %" PRIx64 " count %d" # block/qcow2-cluster.c -qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offset %" PRIx64 " num %d" +qcow2_alloc_clusters_offset(void *co, uint64_t offset, int bytes) "co %p offset %" PRIx64 " bytes %d" qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offset %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64 qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offset %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64 qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offset %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"
This changes qcow2 to implement the byte-based .bdrv_co_pwritev interface rather than the sector-based old one. As preallocation uses the same allocation function as normal writes, and the interface of that function needs to be changed, it is converted in the same patch. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 13 ++++---- block/qcow2.c | 82 ++++++++++++++++++++++++--------------------------- block/qcow2.h | 3 +- trace-events | 6 ++-- 4 files changed, 49 insertions(+), 55 deletions(-)