Re: [Qemu-devel] [PATCH v5 1/8] block: Add bdrv_get_block_status_above

[Paolo Bonzini <pbonzini@redhat.com>]

On 26/05/2015 05:36, Fam Zheng wrote:
> Like bdrv_is_allocated_above, this function follows the backing chain until seeing
> BDRV_BLOCK_ALLOCATED.  Base is not included.
> 
> Reimplement bdrv_is_allocated on top.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c            | 53 ++++++++++++++++++++++++++++++++++++++++-----------
>  include/block/block.h |  4 ++++
>  2 files changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index e394d92..a0d9990 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1560,28 +1560,51 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>      return ret;
>  }
>  
> -/* Coroutine wrapper for bdrv_get_block_status() */
> -static void coroutine_fn bdrv_get_block_status_co_entry(void *opaque)
> +static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
> +        BlockDriverState *base,
> +        int64_t sector_num,
> +        int nb_sectors,
> +        int *pnum)
> +{
> +    BlockDriverState *p;
> +    int64_t ret;
> +
> +    assert(bs != base);
> +    for (p = bs; p != base; p = p->backing_hd) {
> +        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);

Since pnum is an output parameter only, *pnum will be set by the last
call in the loop.

This is not what bdrv_is_allocated_above does: you have to set *pnum
(roughly) to the _smallest_ value returned by the calls.  Consider this
case (base == NULL, bs->backing_hd->backing_hd == NULL):

                                 1         2         3
                        123456789012345678901234567890
  bs                    ...........AAAAAAAAAAAA.......
  bs->backing_hd        ...............AAAAAAAAAAAAAAA

Your code would return *pnum == 15, but the right result is *pnum == 11.

Paolo

> +        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
> +            break;
> +        }
> +    }
> +    return ret;
> +}
> +
> +/* Coroutine wrapper for bdrv_get_block_status_above() */
> +static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
>  {
>      BdrvCoGetBlockStatusData *data = opaque;
> -    BlockDriverState *bs = data->bs;
>  
> -    data->ret = bdrv_co_get_block_status(bs, data->sector_num, data->nb_sectors,
> -                                         data->pnum);
> +    data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
> +                                               data->sector_num,
> +                                               data->nb_sectors,
> +                                               data->pnum);
>      data->done = true;
>  }
>  
>  /*
> - * Synchronous wrapper around bdrv_co_get_block_status().
> + * Synchronous wrapper around bdrv_co_get_block_status_above().
>   *
> - * See bdrv_co_get_block_status() for details.
> + * See bdrv_co_get_block_status_above() for details.
>   */
> -int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
> -                              int nb_sectors, int *pnum)
> +int64_t bdrv_get_block_status_above(BlockDriverState *bs,
> +                                    BlockDriverState *base,
> +                                    int64_t sector_num,
> +                                    int nb_sectors, int *pnum)
>  {
>      Coroutine *co;
>      BdrvCoGetBlockStatusData data = {
>          .bs = bs,
> +        .base = base,
>          .sector_num = sector_num,
>          .nb_sectors = nb_sectors,
>          .pnum = pnum,
> @@ -1590,11 +1613,11 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>  
>      if (qemu_in_coroutine()) {
>          /* Fast-path if already in coroutine context */
> -        bdrv_get_block_status_co_entry(&data);
> +        bdrv_get_block_status_above_co_entry(&data);
>      } else {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
>  
> -        co = qemu_coroutine_create(bdrv_get_block_status_co_entry);
> +        co = qemu_coroutine_create(bdrv_get_block_status_above_co_entry);
>          qemu_coroutine_enter(co, &data);
>          while (!data.done) {
>              aio_poll(aio_context, true);
> @@ -1603,6 +1626,14 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>      return data.ret;
>  }
>  
> +int64_t bdrv_get_block_status(BlockDriverState *bs,
> +                              int64_t sector_num,
> +                              int nb_sectors, int *pnum)
> +{
> +    return bdrv_get_block_status_above(bs, bs->backing_hd,
> +                                       sector_num, nb_sectors, pnum);
> +}
> +
>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                                     int nb_sectors, int *pnum)
>  {
> diff --git a/include/block/block.h b/include/block/block.h
> index c1c963e..8a13bed 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -361,6 +361,10 @@ 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);
> +int64_t bdrv_get_block_status_above(BlockDriverState *bs,
> +                                    BlockDriverState *base,
> +                                    int64_t sector_num,
> +                                    int nb_sectors, int *pnum);
>  int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>                        int *pnum);
>  int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>