Re: [Qemu-devel] [PATCH v5 01/15] block: Add "file" output parameter to block status query functions

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6182 bytes --]

On 05.01.2016 09:11, Fam Zheng wrote:
> The added parameter can be used to return the BDS pointer which the
> valid offset is referring to. Its value should be ignored unless
> BDRV_BLOCK_OFFSET_VALID in ret is set.
> 
> Until block drivers fill in the right value, let's clear it explicitly
> right before calling .bdrv_get_block_status.
> 
> The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest
> case 102 passing, and will be fixed once all drivers return the right file
> pointer.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c                | 42 ++++++++++++++++++++++++++++--------------
>  block/iscsi.c             |  6 ++++--
>  block/mirror.c            |  3 ++-
>  block/parallels.c         |  2 +-
>  block/qcow.c              |  2 +-
>  block/qcow2.c             |  2 +-
>  block/qed.c               |  3 ++-
>  block/raw-posix.c         |  3 ++-
>  block/raw_bsd.c           |  3 ++-
>  block/sheepdog.c          |  2 +-
>  block/vdi.c               |  2 +-
>  block/vmdk.c              |  2 +-
>  block/vpc.c               |  2 +-
>  block/vvfat.c             |  2 +-
>  include/block/block.h     |  9 ++++++---
>  include/block/block_int.h |  3 ++-
>  qemu-img.c                |  7 +++++--
>  17 files changed, 61 insertions(+), 34 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 63e3678..492c291 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -663,6 +663,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>  int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>  {
>      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
> +    BlockDriverState *file;
>      int n;
>  
>      target_sectors = bdrv_nb_sectors(bs);
> @@ -675,7 +676,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>          if (nb_sectors <= 0) {
>              return 0;
>          }
> -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
>          if (ret < 0) {
>              error_report("error getting block status at sector %" PRId64 ": %s",
>                           sector_num, strerror(-ret));
> @@ -1464,6 +1465,7 @@ int bdrv_flush_all(void)
>  typedef struct BdrvCoGetBlockStatusData {
>      BlockDriverState *bs;
>      BlockDriverState *base;
> +    BlockDriverState **file;
>      int64_t sector_num;
>      int nb_sectors;
>      int *pnum;
> @@ -1488,7 +1490,8 @@ typedef struct BdrvCoGetBlockStatusData {
>   */
>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
> -                                                     int nb_sectors, int *pnum)
> +                                                     int nb_sectors, int *pnum,
> +                                                     BlockDriverState **file)
>  {
>      int64_t total_sectors;
>      int64_t n;
> @@ -1518,16 +1521,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          return ret;
>      }
>  
> -    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    *file = NULL;
> +    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
> +                                            file);
>      if (ret < 0) {
>          *pnum = 0;
> +        *file = NULL;
>          return ret;
>      }
>  
>      if (ret & BDRV_BLOCK_RAW) {
>          assert(ret & BDRV_BLOCK_OFFSET_VALID);
>          return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> -                                     *pnum, pnum);
> +                                     *pnum, pnum, file);
>      }
>  
>      if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
> @@ -1544,13 +1550,14 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          }
>      }
>  
> -    if (bs->file &&
> +    if (bs->file && *file && *file != bs &&

In order to keep iotest 102 working, this line should not be changed at
all in this patch. Because *file is NULL and not changed by any of the
block drivers, the condition will always be false now (breaking iotest 102).

>          (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>          (ret & BDRV_BLOCK_OFFSET_VALID)) {
> +        BlockDriverState *file2;
>          int file_pnum;
>  
> -        ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> -                                        *pnum, &file_pnum);
> +        ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,

And instead of *file, this should remain bs->file->bs.

> +                                        *pnum, &file_pnum, &file2);
>          if (ret2 >= 0) {
>              /* Ignore errors.  This is just providing extra information, it
>               * is useful but not necessary.

[...]

> diff --git a/include/block/block.h b/include/block/block.h
> index db8e096..ab966ad 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -113,7 +113,8 @@ typedef struct HDGeometry {
>   * Allocation status flags
>   * BDRV_BLOCK_DATA: data is read from bs->file or another file
>   * BDRV_BLOCK_ZERO: sectors read as zero
> - * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file or another file
> + *                          as raw data

Hm, this seems so unspecific. We do know exactly where the data is
stored, and that is in the *file BDS returned by
bdrv_get_block_status(). Can we make some reference to that? I don't
know whether "sector stored in *file as raw data" will be clear enough,
though. Maybe "sector stored in the BDS returned as *file by
bdrv_get_block_status()" will suffice.

Max

>   * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
>   *                       layer (as opposed to the backing file)
>   * BDRV_BLOCK_RAW: used internally to indicate that the request


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]