On 03.12.2016 19:19, Eric Blake wrote: > Passing a byte offset, but sector count, when we ultimately > want to operate on cluster granularity, is madness. Clean up > the interfaces to take byte offset and count. Rename > qcow2_discard_clusters() and qcow2_zero_clusters() to the > shorter qcow2_discard() and qcow2_zero() to make sure backports > don't get confused by changed units. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > > 2.9 material, depends on 'Don't strand clusters near 2G intervals > during commit' > > block/qcow2.h | 8 ++++---- > block/qcow2-cluster.c | 20 +++++++++++--------- > block/qcow2-snapshot.c | 7 +++---- > block/qcow2.c | 22 ++++++++++------------ > 4 files changed, 28 insertions(+), 29 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 1823414..a0d169b 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -543,10 +543,10 @@ uint64_t > qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, > int compressed_size); > > int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m); > -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, > - int nb_sectors, enum qcow2_discard_type type, bool full_discard); > -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int > nb_sectors, > - int flags); > +int qcow2_discard(BlockDriverState *bs, uint64_t offset, uint64_t count, > + enum qcow2_discard_type type, bool full_discard); > +int qcow2_zero(BlockDriverState *bs, uint64_t offset, uint64_t count, > + int flags); > > int qcow2_expand_zero_clusters(BlockDriverState *bs, > BlockDriverAmendStatusCB *status_cb, > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 928c1e2..3ee0815 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1511,19 +1511,17 @@ static int discard_single_l2(BlockDriverState *bs, > uint64_t offset, > return nb_clusters; > } > > -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, > - int nb_sectors, enum qcow2_discard_type type, bool full_discard) > +int qcow2_discard(BlockDriverState *bs, uint64_t offset, uint64_t count, > + enum qcow2_discard_type type, bool full_discard) > { > BDRVQcow2State *s = bs->opaque; > uint64_t end_offset; > uint64_t nb_clusters; > int ret; > > - end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS); > - > - /* Round start up and end down */ > + /* Round start up and end down to cluster boundary */ > + end_offset = start_of_cluster(s, offset + count); > offset = align_offset(offset, s->cluster_size); > - end_offset = start_of_cluster(s, end_offset); > > if (offset > end_offset) { > return 0; > @@ -1595,20 +1593,24 @@ static int zero_single_l2(BlockDriverState *bs, > uint64_t offset, > return nb_clusters; > } > > -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int > nb_sectors, > - int flags) > +int qcow2_zero(BlockDriverState *bs, uint64_t offset, uint64_t count, > + int flags)
Hmm, I personally liked qcow2_zero_clusters() better. qcow2_zero() doesn't really express that it means the verb "to zero". Also, while you are making a good point why the function should be renamed, qcow2_zero_clusters() was much more accurate because offset and count are expected to be cluster-aligned. The only alternative I can come up with would be "qcow2_write_zeroes"; that at least solves the first issue I have with this, but not the second one... > { > BDRVQcow2State *s = bs->opaque; > uint64_t nb_clusters; > int ret; > > + /* Block layer guarantees cluster alignment */ Hm, it's rather qcow2_co_pwrite_zeroes(), isn't it? The block layer will split unaligned requests into head, body and tail and it will still submit head and tail (though separately). As far as I can see, it's qcow2_co_pwrite_zeroes() which then guarantees that only the aligned part gets through to qcow2_zero(). The patch looks good apart from these nit picks, though. Max > + assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); > + assert(QEMU_IS_ALIGNED(count, s->cluster_size)); > + > /* The zero flag is only supported by version 3 and newer */ > if (s->qcow_version < 3) { > return -ENOTSUP; > } > > /* Each L2 table is handled by its own loop iteration */ > - nb_clusters = size_to_clusters(s, nb_sectors << BDRV_SECTOR_BITS); > + nb_clusters = size_to_clusters(s, count); > > s->cache_discards = true;
signature.asc
Description: OpenPGP digital signature