[PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
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
Reviewed-by: Vladimir Sementsov-Ogievskiy
CC: Kevin Wolf
CC: Max Reitz
CC: Stefan Hajnoczi
CC: Fam Zheng
CC: Juan Quintela
CC: "Dr. David Alan Gilbert"
CC: Denis Plotnikov
---
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/block/block-backend.c b/block/block-backend.c
index 1c6e53bbde..5bb11c8e01 100644
--- a/block/block-backend.c
+++ 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_finalize_vmstate(blk_bs(blk));
if (ret < 0) {
return ret;
}
+if (ret2 < 0) {
+return ret2;
+}
if (!blk->enable_write_cache) {
ret = bdrv_flush(blk_bs(blk));
diff --git a/block/io.c b/block/io.c
index b6564e34c5..be1fac0be8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2724,6 +2724,21 @@ int bdrv_readv_vmstate(BlockDriverState *bs,
QEMUIOVector *qiov, int64_t pos)
return bdrv_rw_vmstate(bs, qiov, pos, true);
}
+static int coroutine_fn bdrv_co_finalize_vmstate(BlockDriverState *bs)
+{
+return 0;
+}
+
+static int coroutine_fn bdrv_finalize_vmstate_co_entry(void *opaque)
+{
+return bdrv_co_finalize_vmstate(opaque);
+}
+
+int bdrv_finalize_vmstate(BlockDriverState *bs)
+{
+return bdrv_run_co(bs, bdrv_finalize_vmstate_co_entry, bs);
+}
+
/**/
/* async I/Os */
diff --git a/include/block/block.h b/include/block/block.h
index bca3bb831c..124eb8d422 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -567,6 +567,11 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t
*buf,
int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
int64_t pos, int size);
+/*
+ * bdrv_finalize_vmstate() is mandatory to commit vmstate changes if
+ * bdrv_save_vmstate() was ever called.
+ */
+int bdrv_finalize_vmstate(BlockDriverState *bs);
void bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
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;
+}
return bdrv_flush(opaque);
}
--
2.17.1
Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
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
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> CC: Kevin Wolf
> CC: Max Reitz
> CC: Stefan Hajnoczi
> CC: Fam Zheng
> CC: Juan Quintela
> CC: "Dr. David Alan Gilbert"
> CC: Denis Plotnikov
> ---
> 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
> +}
> return bdrv_flush(opaque);
> }
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 :|
Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
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
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> CC: Kevin Wolf
>> CC: Max Reitz
>> CC: Stefan Hajnoczi
>> CC: Fam Zheng
>> CC: Juan Quintela
>> CC: "Dr. David Alan Gilbert"
>> CC: Denis Plotnikov
>> ---
>> 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
Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
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
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
> >> CC: Kevin Wolf
> >> CC: Max Reitz
> >> CC: Stefan Hajnoczi
> >> CC: Fam Zheng
> >> CC: Juan Quintela
> >> CC: "Dr. David Alan Gilbert"
> >> CC: Denis Plotnikov
> >> ---
> >> 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 :|
Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
Daniel P. Berrangé writes:
> 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
>> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> >> CC: Kevin Wolf
>> >> CC: Max Reitz
>> >> CC: Stefan Hajnoczi
>> >> CC: Fam Zheng
>> >> CC: Juan Quintela
>> >> CC: "Dr. David Alan Gilbert"
>> >> CC: Denis Plotnikov
>> >> ---
>> >> 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.
Not setting an error on failure is plainly wrong.
If it works because all callers pass NULL, then the obvious fix is to
drop the @errp parameter.
I agree it's not this patch's fault. It needs fixing anyway. If you
have to respin for some other reason, including a fix would be nice.
