Re: [Qemu-devel] [PATCH v4 07/23] block: Convert bdrv_get_block_status() to bytes

[John Snow <jsnow@redhat.com>]

On 09/13/2017 12:03 PM, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
> 
> Changing the name of the function from bdrv_get_block_status() to
> bdrv_block_status() ensures that the compiler enforces that all
> callers are updated.  For now, the io.c layer still assert()s that
> all callers are sector-aligned, but that can be relaxed when a later
> patch implements byte-based block status in the drivers.
> 
> Note that we have an inherent limitation in the BDRV_BLOCK_* return
> values: BDRV_BLOCK_OFFSET_VALID can only return the start of a
> sector, even if we later relax the interface to query for the status
> starting at an intermediate byte; document the obvious interpretation
> that valid offsets are always sector-relative.
> 
> Therefore, for the most part this patch is just the addition of scaling
> at the callers followed by inverse scaling at bdrv_block_status().  But
> some code, particularly bdrv_is_allocated(), gets a lot simpler because
> it no longer has to mess with sectors.
> 
> For ease of review, bdrv_get_block_status_above() will be tackled
> separately.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: clamp bytes to 32-bits, rather than asserting
> v2: rebase to earlier changes
> ---
>  include/block/block.h | 12 +++++++-----
>  block/io.c            | 31 +++++++++++++++++++------------
>  block/qcow2-cluster.c |  2 +-
>  qemu-img.c            | 20 +++++++++++---------
>  4 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index bb3b95d491..7a9a8db588 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -138,8 +138,10 @@ typedef struct HDGeometry {
>   *
>   * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
>   * represent the offset in the returned BDS that is allocated for the
> - * corresponding raw data; however, whether that offset actually contains
> - * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows:
> + * corresponding raw data.  Individual bytes are at the same sector-relative
> + * locations (and thus, this bit cannot be set for mappings which are
> + * not equivalent modulo 512).  However, whether that offset actually
> + * contains data also depends on BDRV_BLOCK_DATA, as follows:
>   *
>   * DATA ZERO OFFSET_VALID
>   *  t    t        t       sectors read as zero, returned file is zero at offset
> @@ -421,9 +423,9 @@ int bdrv_has_zero_init_1(BlockDriverState *bs);
>  int bdrv_has_zero_init(BlockDriverState *bs);
>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
>  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
> -int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
> -                              int nb_sectors, int *pnum,
> -                              BlockDriverState **file);
> +int64_t bdrv_block_status(BlockDriverState *bs, int64_t offset,
> +                          int64_t bytes, int64_t *pnum,
> +                          BlockDriverState **file);
>  int64_t bdrv_get_block_status_above(BlockDriverState *bs,
>                                      BlockDriverState *base,
>                                      int64_t sector_num,
> diff --git a/block/io.c b/block/io.c
> index 638b3890b7..1ed46bcece 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -695,7 +695,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>  {
>      int64_t target_size, ret, bytes, offset = 0;
>      BlockDriverState *bs = child->bs;
> -    int n; /* sectors */
> 
>      target_size = bdrv_getlength(bs);
>      if (target_size < 0) {
> @@ -707,24 +706,23 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>          if (bytes <= 0) {
>              return 0;
>          }
> -        ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS,
> -                                    bytes >> BDRV_SECTOR_BITS, &n, NULL);
> +        ret = bdrv_block_status(bs, offset, bytes, &bytes, NULL);
>          if (ret < 0) {
>              error_report("error getting block status at offset %" PRId64 ": %s",
>                           offset, strerror(-ret));
>              return ret;
>          }
>          if (ret & BDRV_BLOCK_ZERO) {
> -            offset += n * BDRV_SECTOR_BITS;
> +            offset += bytes;
>              continue;
>          }
> -        ret = bdrv_pwrite_zeroes(child, offset, n * BDRV_SECTOR_SIZE, flags);
> +        ret = bdrv_pwrite_zeroes(child, offset, bytes, flags);
>          if (ret < 0) {
>              error_report("error writing zeroes at offset %" PRId64 ": %s",
>                           offset, strerror(-ret));
>              return ret;
>          }
> -        offset += n * BDRV_SECTOR_SIZE;
> +        offset += bytes;
>      }
>  }
> 
> @@ -1983,13 +1981,22 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
>                                            nb_sectors, pnum, file);
>  }
> 
> -int64_t bdrv_get_block_status(BlockDriverState *bs,
> -                              int64_t sector_num,
> -                              int nb_sectors, int *pnum,
> -                              BlockDriverState **file)
> +int64_t bdrv_block_status(BlockDriverState *bs,
> +                          int64_t offset, int64_t bytes, int64_t *pnum,
> +                          BlockDriverState **file)
>  {
> -    return bdrv_get_block_status_above(bs, backing_bs(bs),
> -                                       sector_num, nb_sectors, pnum, file);
> +    int64_t ret;
> +    int n;
> +
> +    assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> +    bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
> +    ret = bdrv_get_block_status_above(bs, backing_bs(bs),
> +                                      offset >> BDRV_SECTOR_BITS,
> +                                      bytes >> BDRV_SECTOR_BITS, &n, file);
> +    if (pnum) {
> +        *pnum = n * BDRV_SECTOR_SIZE;
> +    }

Is it safe to truncate the request in the event that the caller did not
provide a pnum target? that is, how will they know for what range we are
answering?

> +    return ret;
>  }
> 
>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 0d4824993c..d837b3980d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1584,7 +1584,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>           * cluster is already marked as zero, or if it's unallocated and we
>           * don't have a backing file.
>           *
> -         * TODO We might want to use bdrv_get_block_status(bs) here, but we're
> +         * TODO We might want to use bdrv_block_status(bs) here, but we're

thanks for updating comments too :)

>           * holding s->lock, so that doesn't work today.
>           *
>           * If full_discard is true, the sector should not read back as zeroes,
> diff --git a/qemu-img.c b/qemu-img.c
> index 54f7682069..897f80abb3 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1598,9 +1598,14 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
> 
>      if (s->sector_next_status <= sector_num) {
>          if (s->target_has_backing) {
> -            ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
> -                                        sector_num - src_cur_offset,
> -                                        n, &n, NULL);
> +            int64_t count = n * BDRV_SECTOR_SIZE;
> +
> +            ret = bdrv_block_status(blk_bs(s->src[src_cur]),
> +                                    (sector_num - src_cur_offset) *
> +                                    BDRV_SECTOR_SIZE,
> +                                    count, &count, NULL);
> +            assert(ret < 0 || QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
> +            n = count >> BDRV_SECTOR_BITS;
>          } else {
>              ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
>                                                sector_num - src_cur_offset,
> @@ -2677,9 +2682,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
>      int depth;
>      BlockDriverState *file;
>      bool has_offset;
> -    int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> 
> -    assert(bytes < INT_MAX);
>      /* As an optimization, we could cache the current range of unallocated
>       * clusters in each file of the chain, and avoid querying the same
>       * range repeatedly.
> @@ -2687,12 +2690,11 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
> 
>      depth = 0;
>      for (;;) {
> -        ret = bdrv_get_block_status(bs, offset >> BDRV_SECTOR_BITS, nb_sectors,
> -                                    &nb_sectors, &file);
> +        ret = bdrv_block_status(bs, offset, bytes, &bytes, &file);
>          if (ret < 0) {
>              return ret;
>          }
> -        assert(nb_sectors);
> +        assert(bytes);
>          if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
>              break;
>          }
> @@ -2709,7 +2711,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
> 
>      *e = (MapEntry) {
>          .start = offset,
> -        .length = nb_sectors * BDRV_SECTOR_SIZE,
> +        .length = bytes,
>          .data = !!(ret & BDRV_BLOCK_DATA),
>          .zero = !!(ret & BDRV_BLOCK_ZERO),
>          .offset = ret & BDRV_BLOCK_OFFSET_MASK,
> 

Rest appears obviously correct.