Re: [PATCH 4/5] block, migration: add bdrv_flush_vmstate helper

[Eric Blake]

On 6/16/20 11:20 AM, Denis V. Lunev wrote:
> Right now bdrv_fclose() is just calling bdrv_flush().
>
> The problem is that migration code is working inefficently from black

inefficiently, block

> layer terms and are frequently called for very small pieces of not
> properly aligned data. Block layer is capable to work this way, but

s/not properly aligned/unaligned/

> this is very slow.
>
> This patch is a preparation for the introduction of the intermediate
> buffer at block driver state. It would be beneficial to separate
> conventional bdrv_flush() from closing QEMU file from migration code.
>
> The patch also forces bdrv_flush_vmstate() operation inside
> synchronous blk_save_vmstate() operation. This helper is used from
> qemu-io only.

> +++ b/block/block-backend.c
> @@ -2177,16 +2177,20 @@ int blk_truncate(BlockBackend *blk, int64_t offset, 
> bool exact,
>   int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>                        int64_t pos, int size)
>   {
> -    int ret;
> +    int ret, ret2;
>   
>       if (!blk_is_available(blk)) {
>           return -ENOMEDIUM;
>       }
>   
>       ret = bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
> +    ret2 = bdrv_flush_vmstate(blk_bs(blk));

Do you really want to be attempting bdrv_flush_vmstate() even after 
bdrv_save_vmstate() failed?  Better might be...

>       if (ret < 0) {
>           return ret;
>       }

...attempting it here, at which point it looks like the only reason you 
need ret2 is to preserve ret long enough...

> +    if (ret2 < 0) {
> +        return ret2;
> +    }
>   
>       if (ret == size && !blk->enable_write_cache) {

...for this check.  But a quick look at bdrv_save_vmstate() says this 
check is dead: the function can only return negative error or exactly 
size, and we already filtered out negative error above.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org