Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices

[Paolo Bonzini]

Il 07/10/2013 07:59, Peter Lieven ha scritto:
> if a raw device like an iscsi target or host device is used
> the current implementation makes a second call out to get
> the block status of bs->file.
> 
> Signed-off-by: Peter Lieven <address@hidden>
> ---
> v5: add a generic get_lba_status function in the raw driver which
>     adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
>     handle the callout to bs->file then.
> 
> v4: use a flag to detect the raw driver instead of the strncmp
>     hack
> 
>  block.c               |    4 ++++
>  block/raw_bsd.c       |    3 ++-
>  include/block/block.h |    4 ++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 93e113a..38a589e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn 
> bdrv_co_get_block_status(BlockDriverState *bs,
>          return ret;
>      }
>  
> +    if (ret & BDRV_BLOCK_RAW) {
> +        return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);

Strictly speaking, this should probably do something like this:

  assert(ret & BDRV_BLOCK_OFFSET_VALID);
  return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
                               nb_sectors, pnum);

Or alternatively the raw driver should return just "BDRV_BLOCK_RAW".

As a third option, the raw driver could also return not just
BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
that the answer makes some sense even without going down to bs->file).

But I'll let the block maintainers decide what to do.

Paolo

> +    }
> +
>      if (!(ret & BDRV_BLOCK_DATA)) {
>          if (bdrv_has_zero_init(bs)) {
>              ret |= BDRV_BLOCK_ZERO;
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index d4ace60..308d605 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -62,7 +62,8 @@ static int64_t coroutine_fn 
> raw_co_get_block_status(BlockDriverState *bs,
>                                              int64_t sector_num,
>                                              int nb_sectors, int *pnum)
>  {
> -    return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> +           (sector_num << BDRV_SECTOR_BITS);
>  }
>  
>  static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
> diff --git a/include/block/block.h b/include/block/block.h
> index f808550..003699e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -84,6 +84,9 @@ typedef struct BlockDevOps {
>  /* 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_RAW: used internally to indicate that the request
> + *                 was answered by the raw driver and that one
> + *                 should look in bs->file directly.
>   *
>   * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
>   * bs->file where sector data can be read from as raw data.
> @@ -105,6 +108,7 @@ typedef struct BlockDevOps {
>  #define BDRV_BLOCK_DATA         1
>  #define BDRV_BLOCK_ZERO         2
>  #define BDRV_BLOCK_OFFSET_VALID 4
> +#define BDRV_BLOCK_RAW          8
>  #define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
>  
>  typedef enum {
>