Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read

[Max Reitz <mreitz@redhat.com>]

On 2015-02-23 at 09:27, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c |  116 +++++++++++++++++++++++++++--------------------------------
>   1 file changed, 52 insertions(+), 64 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 326c2bb..4e5ba85 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -497,40 +497,70 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>       return 0;
>   }
>   
> -static int vpc_read(BlockDriverState *bs, int64_t sector_num,
> -                    uint8_t *buf, int nb_sectors)
> +static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, int *pnum)

How about just putting the function header here? If you really have to 
move vpc_co_get_block_status() up, I'd rather like it to be in a 
separate patch.

Second, while apparently vpc_read() is actually called in a coroutine, 
that is pretty hard to know. Most importantly, it's not marked as a 
coroutine_fn. Therefore I don't think it's a good idea to call 
vpc_co_get_block_status() directly; I'd vote for either using 
bdrv_get_block_status(), or moving the content of 
vpc_co_get_block_status() to a non-coroutine_fn (it doesn't contain an 
coroutine-related function calls, so this is fine) and then making 
vpc_co_get_block_status() a wrapper around that, or just dropping this 
patch.

The latter I'm proposing because I don't really see what this patch 
improves. The previous vpc_read() function was pretty straightforward, 
too, and I don't think it was unbearably longer.

One could argue that the coroutine_fn stuff doesn't really matter in 
this situation because it doesn't actually do anything right now and 
vpc_co_get_block_status() does not call any other coroutine_fn functions 
in turn; however, it is a semantic contract established by 
include/block/coroutine.h and as far as I remember, Stefan did 
eventually want to have something to error out on compile-time if a 
non-coroutine_fn function calls a coroutine_fn. I don't like breaking 
this contract even if it's not bad in this specific case.

Considering you probably think bdrv_get_block_status() to be too much 
overhead (it will fall down to the protocol layer on VHD_FIXED) and you 
probably find making vpc_read() shorter justified (again, which I don't 
necessarily), I think moving the contents of vpc_co_get_block_status() 
to a non-coroutine_fn might be the best way to go.

>   {
>       BDRVVPCState *s = bs->opaque;
> -    int ret;
> -    int64_t offset;
> -    int64_t sectors, sectors_per_block;
> -    VHDFooter *footer = (VHDFooter *) s->footer_buf;
> +    VHDFooter *footer = (VHDFooter*) s->footer_buf;
> +    int64_t start, offset;
> +    bool allocated;
> +    int n;
>   
>       if (be32_to_cpu(footer->type) == VHD_FIXED) {
> -        return bdrv_read(bs->file, sector_num, buf, nb_sectors);
> +        *pnum = nb_sectors;
> +        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +               (sector_num << BDRV_SECTOR_BITS);
>       }
> -    while (nb_sectors > 0) {
> -        offset = get_sector_offset(bs, sector_num, 0);
>   
> -        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
> -        sectors = sectors_per_block - (sector_num % sectors_per_block);
> -        if (sectors > nb_sectors) {
> -            sectors = nb_sectors;
> +    offset = get_sector_offset(bs, sector_num, 0);
> +    start = offset;
> +    allocated = (offset != -1);
> +    *pnum = 0;
> +
> +    do {
> +        /* All sectors in a block are contiguous (without using the bitmap) */
> +        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
> +          - sector_num;
> +        n = MIN(n, nb_sectors);
> +
> +        *pnum += n;
> +        sector_num += n;
> +        nb_sectors -= n;
> +
> +        if (allocated) {
> +            return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
>           }
> +        if (nb_sectors == 0) {
> +            break;
> +        }
> +        offset = get_sector_offset(bs, sector_num, 0);
> +    } while (offset == -1);
>   
> -        if (offset == -1) {
> -            memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
> -        } else {
> -            ret = bdrv_pread(bs->file, offset, buf,
> -                sectors * BDRV_SECTOR_SIZE);
> -            if (ret != sectors * BDRV_SECTOR_SIZE) {
> +    return 0;
> +}
> +
> +static int vpc_read(BlockDriverState *bs, int64_t sector_num,
> +                    uint8_t *buf, int nb_sectors)
> +{
> +    int ret, n;
> +    int64_t ret2;
> +
> +    while (nb_sectors > 0) {
> +        ret2 = vpc_co_get_block_status(bs, sector_num, nb_sectors, &n);
> +

Superfluous whitespace here.

> +        if (ret2 & BDRV_BLOCK_OFFSET_VALID) {
> +            ret = bdrv_pread(bs->file, ret2 & BDRV_SECTOR_MASK, buf,
> +                             n * BDRV_SECTOR_SIZE);
> +            if (ret != n * BDRV_SECTOR_SIZE) {
>                   return -1;

Please make that "return ret" (and possibly "if (ret < 0)", if you want to).

Max

>               }
> +        } else {
> +            memset(buf, 0x00, n * BDRV_SECTOR_SIZE);
>           }
>   
> -        nb_sectors -= sectors;
> -        sector_num += sectors;
> -        buf += sectors * BDRV_SECTOR_SIZE;
> +        sector_num += n;
> +        nb_sectors -= n;
> +        buf += n * BDRV_SECTOR_SIZE;
>       }
>       return 0;
>   }
> @@ -597,48 +627,6 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
>       return ret;
>   }
>   
> -static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
> -        int64_t sector_num, int nb_sectors, int *pnum)
> -{
> -    BDRVVPCState *s = bs->opaque;
> -    VHDFooter *footer = (VHDFooter*) s->footer_buf;
> -    int64_t start, offset;
> -    bool allocated;
> -    int n;
> -
> -    if (be32_to_cpu(footer->type) == VHD_FIXED) {
> -        *pnum = nb_sectors;
> -        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> -               (sector_num << BDRV_SECTOR_BITS);
> -    }
> -
> -    offset = get_sector_offset(bs, sector_num, 0);
> -    start = offset;
> -    allocated = (offset != -1);
> -    *pnum = 0;
> -
> -    do {
> -        /* All sectors in a block are contiguous (without using the bitmap) */
> -        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
> -          - sector_num;
> -        n = MIN(n, nb_sectors);
> -
> -        *pnum += n;
> -        sector_num += n;
> -        nb_sectors -= n;
> -
> -        if (allocated) {
> -            return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> -        }
> -        if (nb_sectors == 0) {
> -            break;
> -        }
> -        offset = get_sector_offset(bs, sector_num, 0);
> -    } while (offset == -1);
> -
> -    return 0;
> -}
> -
>   /*
>    * Calculates the number of cylinders, heads and sectors per cylinder
>    * based on a given number of sectors. This is the algorithm described