Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Fam Zheng <famz@redhat.com>]

On Thu, 09/15 19:34, Denis V. Lunev wrote:
> They should work very similar, covering same areas if backing store is
> shorter than the image. This change is necessary for the followup patch
> switching to bdrv_get_block_status_above() in mirror to avoid assert
> in check_block.
> 
> This change should be made very carefully. Let us assume that we have
> top image and 2 backing stores L0->L1->L2.

Stupid question: which one is top and which are backing?

>   L0: --------------
>   L1: -------
>   L2: -------=======
> The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
> and we should return it as filled in L0 image with properly calculated
> *pnum value.

What '-', '=' and ' ' represent aren't immediately clear to me, could you put a
legend in the message too? Something like:

    '-': allocated
    '=': unallocated
    ' ': beyong EOF

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> ---
>  block/io.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 420944d..067d465 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>          BlockDriverState **file)
>  {
>      BlockDriverState *p;
> -    int64_t ret = 0;
> +    int64_t ret = 0, res = nb_sectors;
>  
>      assert(bs != base);
>      for (p = bs; p != base; p = backing_bs(p)) {
> -        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
> -        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
> -            break;
> +        int sc;
> +        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file);
> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret & BDRV_BLOCK_ALLOCATED) {
> +            *pnum = sc;
> +            return ret;
> +        }
> +
> +        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
> +            res = sc;
>          }
> +
>          /* [sector_num, pnum] unallocated on this layer, which could be only
>           * the first part of [sector_num, nb_sectors].  */
> -        nb_sectors = MIN(nb_sectors, *pnum);
> +        nb_sectors = MIN(nb_sectors, sc);
> +
> +        if (nb_sectors == 0) {
> +            break;
> +        }
>      }
> +
> +    *pnum = res;
>      return ret;
>  }
>  
> -- 
> 2.7.4
> 
>