| Message ID | 20171004020048.26379-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | make bdrv_get_block_status byte-based | expand |
Am 04.10.2017 um 04:00 hat Eric Blake geschrieben: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Convert another internal > function (no semantic change), and rename it to is_zero() in the > process. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Fam Zheng <famz@redhat.com> > Reviewed-by: John Snow <jsnow@redhat.com> > > --- > v3-v5: no change > v2: rename function, rebase to upstream changes > --- > block/qcow2.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index bcd5c4a34c..e0de46f530 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2972,21 +2972,28 @@ finish: > } > > > -static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > - uint32_t count) > +static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes) > { > int nr; > int64_t res; > + int64_t start; > > - if (start + count > bs->total_sectors) { > - count = bs->total_sectors - start; > + /* Widen to sector boundaries, then clamp to image length, before > + * checking status of underlying sectors */ > + start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE); > + bytes = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE) - start; Why do we still widen to sector boundaries after this series is fully applied? Isn't the whole point that we don't have to do this any more? > + if (start + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) { > + bytes = bs->total_sectors * BDRV_SECTOR_SIZE - start; > } > > - if (!count) { > + if (!bytes) { > return true; > } > - res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL); > - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; > + res = bdrv_get_block_status_above(bs, NULL, start >> BDRV_SECTOR_BITS, > + bytes >> BDRV_SECTOR_BITS, &nr, NULL); > + return res >= 0 && (res & BDRV_BLOCK_ZERO) && > + nr * BDRV_SECTOR_SIZE == bytes; > } Kevin
On 10/10/2017 09:15 AM, Kevin Wolf wrote: > Am 04.10.2017 um 04:00 hat Eric Blake geschrieben: >> We are gradually converting to byte-based interfaces, as they are >> easier to reason about than sector-based. Convert another internal >> function (no semantic change), and rename it to is_zero() in the >> process. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Fam Zheng <famz@redhat.com> >> Reviewed-by: John Snow <jsnow@redhat.com> >> >> +static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes) >> { >> int nr; >> int64_t res; >> + int64_t start; >> >> - if (start + count > bs->total_sectors) { >> - count = bs->total_sectors - start; >> + /* Widen to sector boundaries, then clamp to image length, before >> + * checking status of underlying sectors */ >> + start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE); >> + bytes = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE) - start; > > Why do we still widen to sector boundaries after this series is fully > applied? Isn't the whole point that we don't have to do this any more? The widening is necessary as long as... >> + res = bdrv_get_block_status_above(bs, NULL, start >> BDRV_SECTOR_BITS, >> + bytes >> BDRV_SECTOR_BITS, &nr, NULL); we still shift. But you are right; patch 12/23 should be able to drop the rounding. I'll double check that it still works, but assuming that it does, v6 will include that cleanup.
diff --git a/block/qcow2.c b/block/qcow2.c index bcd5c4a34c..e0de46f530 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2972,21 +2972,28 @@ finish: } -static bool is_zero_sectors(BlockDriverState *bs, int64_t start, - uint32_t count) +static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes) { int nr; int64_t res; + int64_t start; - if (start + count > bs->total_sectors) { - count = bs->total_sectors - start; + /* Widen to sector boundaries, then clamp to image length, before + * checking status of underlying sectors */ + start = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE); + bytes = QEMU_ALIGN_UP(offset + bytes, BDRV_SECTOR_SIZE) - start; + + if (start + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) { + bytes = bs->total_sectors * BDRV_SECTOR_SIZE - start; } - if (!count) { + if (!bytes) { return true; } - res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL); - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; + res = bdrv_get_block_status_above(bs, NULL, start >> BDRV_SECTOR_BITS, + bytes >> BDRV_SECTOR_BITS, &nr, NULL); + return res >= 0 && (res & BDRV_BLOCK_ZERO) && + nr * BDRV_SECTOR_SIZE == bytes; } static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs, @@ -3004,24 +3011,21 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs, } if (head || tail) { - int64_t cl_start = (offset - head) >> BDRV_SECTOR_BITS; uint64_t off; unsigned int nr; assert(head + bytes <= s->cluster_size); /* check whether remainder of cluster already reads as zero */ - if (!(is_zero_sectors(bs, cl_start, - DIV_ROUND_UP(head, BDRV_SECTOR_SIZE)) && - is_zero_sectors(bs, (offset + bytes) >> BDRV_SECTOR_BITS, - DIV_ROUND_UP(-tail & (s->cluster_size - 1), - BDRV_SECTOR_SIZE)))) { + if (!(is_zero(bs, offset - head, head) && + is_zero(bs, offset + bytes, + tail ? s->cluster_size - tail : 0))) { return -ENOTSUP; } qemu_co_mutex_lock(&s->lock); /* We can have new write after previous check */ - offset = cl_start << BDRV_SECTOR_BITS; + offset = QEMU_ALIGN_DOWN(offset, s->cluster_size); bytes = s->cluster_size; nr = s->cluster_size; ret = qcow2_get_cluster_offset(bs, offset, &nr, &off);