Re: [Qemu-devel] [PATCH] block: Avoid unecessary drv->bdrv_getlength() calls

[Paolo Bonzini <pbonzini@redhat.com>]

Il 29/10/2013 12:35, Kevin Wolf ha scritto:
> The block layer generally keeps the size of an image cached in
> bs->total_sectors so that it doesn't have to perform expensive
> operations to get the size whenever it needs it.
> 
> This doesn't work however when using a backend that can change its size
> without qemu being aware of it, i.e. passthrough of removable media like
> CD-ROMs or floppy disks. For this reason, the caching is disabled when a
> removable device is used.
> 
> It is obvious that checking whether the _guest_ device has removable
> media isn't the right thing to do when we want to know whether the size
> of the host backend can change. To make things worse, non-top-level
> BlockDriverStates never have any device attached, which makes qemu
> assume they are removable, so drv->bdrv_getlength() is always called on
> the protocol layer. In the case of raw-posix, this causes unnecessary
> lseek() system calls, which turned out to be rather expensive.
> 
> This patch completely changes the logic and disables bs->total_sectors
> caching only for certain block driver types, for which a size change is
> expected: host_cdrom and host_floppy; also the raw format in case it
> sits on top of one of these protocols, but in the common case the nested
> bdrv_getlength() call on the protocol driver will use the cache again
> and avoid an expensive drv->bdrv_getlength() call.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 7 ++++---
>  block/raw-posix.c         | 9 ++++++---
>  block/raw_bsd.c           | 1 +
>  include/block/block_int.h | 3 +++
>  4 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 366999b..da88be0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2868,9 +2868,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>      if (!drv)
>          return -ENOMEDIUM;
>  
> -    if (bdrv_dev_has_removable_media(bs)) {
> -        if (drv->bdrv_getlength) {
> -            return drv->bdrv_getlength(bs);
> +    if (drv->has_variable_length) {
> +        int ret = refresh_total_sectors(bs, bs->total_sectors);
> +        if (ret < 0) {
> +            return ret;
>          }
>      }
>      return bs->total_sectors * BDRV_SECTOR_SIZE;
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6f03fbf..f6d48bb 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1715,7 +1715,8 @@ static BlockDriver bdrv_host_floppy = {
>      .bdrv_aio_flush	= raw_aio_flush,
>  
>      .bdrv_truncate      = raw_truncate,
> -    .bdrv_getlength	= raw_getlength,
> +    .bdrv_getlength      = raw_getlength,
> +    .has_variable_length = true,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
>  
> @@ -1824,7 +1825,8 @@ static BlockDriver bdrv_host_cdrom = {
>      .bdrv_aio_flush	= raw_aio_flush,
>  
>      .bdrv_truncate      = raw_truncate,
> -    .bdrv_getlength     = raw_getlength,
> +    .bdrv_getlength      = raw_getlength,
> +    .has_variable_length = true,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
>  
> @@ -1951,7 +1953,8 @@ static BlockDriver bdrv_host_cdrom = {
>      .bdrv_aio_flush	= raw_aio_flush,
>  
>      .bdrv_truncate      = raw_truncate,
> -    .bdrv_getlength     = raw_getlength,
> +    .bdrv_getlength      = raw_getlength,
> +    .has_variable_length = true,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
>  
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 0078c1b..2265dcc 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -178,6 +178,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_co_get_block_status = &raw_co_get_block_status,
>      .bdrv_truncate        = &raw_truncate,
>      .bdrv_getlength       = &raw_getlength,
> +    .has_variable_length  = true,
>      .bdrv_get_info        = &raw_get_info,
>      .bdrv_is_inserted     = &raw_is_inserted,
>      .bdrv_media_changed   = &raw_media_changed,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a48731d..1666066 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -156,8 +156,11 @@ struct BlockDriver {
>  
>      const char *protocol_name;
>      int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
> +
>      int64_t (*bdrv_getlength)(BlockDriverState *bs);
> +    bool has_variable_length;
>      int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> +
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>                                   const uint8_t *buf, int nb_sectors);
>  
> 

raw-win32.c probably needs to have a .has_variable_length=true in
bdrv_host_device.  Apart from that,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>