[Qemu-devel] Re: [PATCH 2/2] block: Cache total_sectors to reduce bdrv_getlength calls

[Kevin Wolf <kwolf@redhat.com>]

Am 19.04.2010 14:34, schrieb Stefan Hajnoczi:
> The BlockDriver bdrv_getlength function is called from the I/O code path
> when checking that the request falls within the device.  Unfortunately
> this involves an lseek system call in the raw protocol; every read or
> write request will incur this lseek cost.
> 
> Jan Kiszka <jan.kiszka@siemens.com> identified this issue and its
> latency overhead.  This patch caches device length in the existing
> total_sectors variable so lseek calls can be avoided for fixed size
> devices.
> 
> Growable devices fall back to the full bdrv_getlength code path because
> I have not added logic to detect extending the size of the device in a
> write.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |   28 ++++++++++++++++++++++------
>  1 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index def3400..d5a3ba7 100644
> --- a/block.c
> +++ b/block.c
> @@ -363,6 +363,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>      assert(drv != NULL);
>  
>      bs->file = NULL;
> +    bs->total_sectors = 0;
>      bs->is_temporary = 0;
>      bs->encrypted = 0;
>      bs->valid_key = 0;
> @@ -416,9 +417,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>      }
>  
>      bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
> -    if (drv->bdrv_getlength) {
> -        bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> -    }
> +    bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;

Does this hunk make a difference? If drv->bdrv_getlength == NULL, we'll
just get back the current value.

But now that you sent this hunk for review, one thing about the existing
code: We should probably check the return value of bdrv_getlength.

Otherwise both patches look good to me.

Kevin