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

[Denis V. Lunev]

On 6/17/20 12:11 AM, Eric Blake wrote:
> 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...
no, I would like to be sure that intermediate state is always cleared at
the end.
In the next patch I am going to put intermediate buffer to
BlockDriverState and thus it has to be removed in any case.

May be flush would be a bad name... clean or finalize?

>
>> +    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.
>
>
hmm. you are right. good point. this check is dead.
Worth to remove :) I'll add this as a separate patch.

Den