Re: [PATCH v8 7/7] block/io: refactor save/load vmstate
On Tue, Sep 15, 2020 at 07:44:11PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Like for read/write in a previous commit, drop extra indirection layer, > generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Eric Blake > --- > block/coroutines.h| 10 +++ > include/block/block.h | 6 ++-- > block/io.c| 67 ++- > 3 files changed, 42 insertions(+), 41 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v8 7/7] block/io: refactor save/load vmstate
23.09.2020 23:10, Eric Blake wrote: On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: Like for read/write in a previous commit, drop extra indirection layer, generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/coroutines.h | 10 +++ include/block/block.h | 6 ++-- block/io.c | 67 ++- 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/block/coroutines.h b/block/coroutines.h int coroutine_fn -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, - bool is_read) +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BlockDriver *drv = bs->drv; int ret = -ENOTSUP; + if (!drv) { + return -ENOMEDIUM; + } + bdrv_inc_in_flight(bs); - if (!drv) { - ret = -ENOMEDIUM; - } else if (drv->bdrv_load_vmstate) { - if (is_read) { - ret = drv->bdrv_load_vmstate(bs, qiov, pos); - } else { - ret = drv->bdrv_save_vmstate(bs, qiov, pos); - } + if (drv->bdrv_load_vmstate) { + ret = drv->bdrv_load_vmstate(bs, qiov, pos); This one makes sense; } else if (bs->file) { - ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); + ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); } bdrv_dec_in_flight(bs); + return ret; } -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size) +int coroutine_fn +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); - int ret; + BlockDriver *drv = bs->drv; + int ret = -ENOTSUP; - ret = bdrv_writev_vmstate(bs, &qiov, pos); - if (ret < 0) { - return ret; + if (!drv) { + return -ENOMEDIUM; } - return size; -} + bdrv_inc_in_flight(bs); -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) -{ - return bdrv_rw_vmstate(bs, qiov, pos, false); + if (drv->bdrv_load_vmstate) { + ret = drv->bdrv_save_vmstate(bs, qiov, pos); but this one looks awkward. It represents the pre-patch logic, but it would be nicer to check for bdrv_save_vmstate. With that tweak, my R-b still stands. Agree. I had an interesting time applying this patch due to merge conflicts with the new bdrv_primary_bs() changes that landed in the meantime. Thanks a lot! To clarify: did you finally staged the series to send a pull request? Or Stefan should do it? Should I make a v9? -- Best regards, Vladimir
Re: [PATCH v8 7/7] block/io: refactor save/load vmstate
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: Like for read/write in a previous commit, drop extra indirection layer, generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/coroutines.h| 10 +++ include/block/block.h | 6 ++-- block/io.c| 67 ++- 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/block/coroutines.h b/block/coroutines.h int coroutine_fn -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, - bool is_read) +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BlockDriver *drv = bs->drv; int ret = -ENOTSUP; +if (!drv) { +return -ENOMEDIUM; +} + bdrv_inc_in_flight(bs); -if (!drv) { -ret = -ENOMEDIUM; -} else if (drv->bdrv_load_vmstate) { -if (is_read) { -ret = drv->bdrv_load_vmstate(bs, qiov, pos); -} else { -ret = drv->bdrv_save_vmstate(bs, qiov, pos); -} +if (drv->bdrv_load_vmstate) { +ret = drv->bdrv_load_vmstate(bs, qiov, pos); This one makes sense; } else if (bs->file) { -ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); +ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); } bdrv_dec_in_flight(bs); + return ret; } -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size) +int coroutine_fn +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { -QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); -int ret; +BlockDriver *drv = bs->drv; +int ret = -ENOTSUP; -ret = bdrv_writev_vmstate(bs, &qiov, pos); -if (ret < 0) { -return ret; +if (!drv) { +return -ENOMEDIUM; } -return size; -} +bdrv_inc_in_flight(bs); -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) -{ -return bdrv_rw_vmstate(bs, qiov, pos, false); +if (drv->bdrv_load_vmstate) { +ret = drv->bdrv_save_vmstate(bs, qiov, pos); but this one looks awkward. It represents the pre-patch logic, but it would be nicer to check for bdrv_save_vmstate. With that tweak, my R-b still stands. I had an interesting time applying this patch due to merge conflicts with the new bdrv_primary_bs() changes that landed in the meantime. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v8 7/7] block/io: refactor save/load vmstate
Like for read/write in a previous commit, drop extra indirection layer, generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/coroutines.h| 10 +++ include/block/block.h | 6 ++-- block/io.c| 67 ++- 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/block/coroutines.h b/block/coroutines.h index 6c63a819c9..f69179f5ef 100644 --- a/block/coroutines.h +++ b/block/coroutines.h @@ -57,11 +57,9 @@ bdrv_common_block_status_above(BlockDriverState *bs, int64_t *map, BlockDriverState **file); -int coroutine_fn -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, - bool is_read); -int generated_co_wrapper -bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, -bool is_read); +int coroutine_fn bdrv_co_readv_vmstate(BlockDriverState *bs, + QEMUIOVector *qiov, int64_t pos); +int coroutine_fn bdrv_co_writev_vmstate(BlockDriverState *bs, +QEMUIOVector *qiov, int64_t pos); #endif /* BLOCK_COROUTINES_INT_H */ diff --git a/include/block/block.h b/include/block/block.h index b8b4c177de..6cd789724b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -572,8 +572,10 @@ int path_has_protocol(const char *path); int path_is_absolute(const char *path); char *path_combine(const char *base_path, const char *filename); -int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); +int generated_co_wrapper +bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); +int generated_co_wrapper +bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size); diff --git a/block/io.c b/block/io.c index 68d7d9cf80..84f82bc069 100644 --- a/block/io.c +++ b/block/io.c @@ -2491,66 +2491,67 @@ int bdrv_is_allocated_above(BlockDriverState *top, } int coroutine_fn -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, - bool is_read) +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BlockDriver *drv = bs->drv; int ret = -ENOTSUP; +if (!drv) { +return -ENOMEDIUM; +} + bdrv_inc_in_flight(bs); -if (!drv) { -ret = -ENOMEDIUM; -} else if (drv->bdrv_load_vmstate) { -if (is_read) { -ret = drv->bdrv_load_vmstate(bs, qiov, pos); -} else { -ret = drv->bdrv_save_vmstate(bs, qiov, pos); -} +if (drv->bdrv_load_vmstate) { +ret = drv->bdrv_load_vmstate(bs, qiov, pos); } else if (bs->file) { -ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); +ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); } bdrv_dec_in_flight(bs); + return ret; } -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size) +int coroutine_fn +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { -QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); -int ret; +BlockDriver *drv = bs->drv; +int ret = -ENOTSUP; -ret = bdrv_writev_vmstate(bs, &qiov, pos); -if (ret < 0) { -return ret; +if (!drv) { +return -ENOMEDIUM; } -return size; -} +bdrv_inc_in_flight(bs); -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) -{ -return bdrv_rw_vmstate(bs, qiov, pos, false); +if (drv->bdrv_load_vmstate) { +ret = drv->bdrv_save_vmstate(bs, qiov, pos); +} else if (bs->file) { +ret = bdrv_co_writev_vmstate(bs->file->bs, qiov, pos); +} + +bdrv_dec_in_flight(bs); + +return ret; } -int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, +int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size) { QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); -int ret; - -ret = bdrv_readv_vmstate(bs, &qiov, pos); -if (ret < 0) { -return ret; -} +int ret = bdrv_writev_vmstate(bs, &qiov, pos); -return size; +return ret < 0 ? ret : size; } -int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) +int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, + int64_t pos, int size) { -return bdrv_rw_vmstate(bs, qiov, pos, true); +QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); +int ret = bdrv_readv_vmstate(bs, &qiov, pos); + +return ret < 0 ? ret : size; } /