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 <d...@openvz.org> > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > >> CC: Kevin Wolf <kw...@redhat.com> > >> CC: Max Reitz <mre...@redhat.com> > >> CC: Stefan Hajnoczi <stefa...@redhat.com> > >> CC: Fam Zheng <f...@euphon.net> > >> CC: Juan Quintela <quint...@redhat.com> > >> CC: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > >> CC: Denis Plotnikov <dplotni...@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 :|