Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper

[Denis V. Lunev]

On 8/27/20 3:58 PM, Daniel P. Berrangé wrote:
> On Thu, Jul 09, 2020 at 04:26:42PM +0300, Denis V. Lunev wrote:
>> Right now bdrv_fclose() is just calling bdrv_flush().
>>
>> The problem is that migration code is working inefficiently from block
>> layer terms and are frequently called for very small pieces of
>> unaligned data. Block layer is capable to work this way, but 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_finalize_vmstate() operation inside
>> synchronous blk_save_vmstate() operation. This helper is used from
>> qemu-io only.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Fam Zheng <fam@euphon.net>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> CC: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>>  block/block-backend.c |  6 +++++-
>>  block/io.c            | 15 +++++++++++++++
>>  include/block/block.h |  5 +++++
>>  migration/savevm.c    |  4 ++++
>>  4 files changed, 29 insertions(+), 1 deletion(-)
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 45c9dd9d8a..d8a94e312c 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -150,6 +150,10 @@ static ssize_t block_get_buffer(void *opaque, uint8_t 
>> *buf, int64_t pos,
>>  
>>  static int bdrv_fclose(void *opaque, Error **errp)
>>  {
>> +    int err = bdrv_finalize_vmstate(opaque);
>> +    if (err < 0) {
>> +        return err;
> This is returning an error without having populating 'errp' which means
> the caller will be missing error diagnosis

but this behaves exactly like the branch below,
bdrv_flush() could return error too and errp
is not filled in the same way.

Den


>> +    }
>>      return bdrv_flush(opaque);
>>  }
> Regards,
> Daniel