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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Aug 27, 2020 at 04:02:38PM +0300, Denis V. Lunev wrote:
> 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.

Doh, it seems the only caller passes NULL for the errp too,
so it is a redundant parameter. So nothing wrong with your
patch after all.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|