| Message ID | 20200915164411.20590-1-vsementsov@virtuozzo.com |
|---|---|
| Headers | show |
| Series | coroutines: generate wrapper code | expand |
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 <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > 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.
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: > We are going to keep coroutine-wrappers code (structure-packing > parameters, BDRV_POLL wrapper functions) in separate auto-generated > files. So, we'll need a header with declaration of original _co_ > functions, for those which are static now. As well, we'll need > declarations for wrapper functions. Do these declarations now, as a > preparation step. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > +++ b/block/coroutines.h > +int coroutine_fn bdrv_co_check(BlockDriverState *bs, > + BdrvCheckResult *res, BdrvCheckMode fix); > +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp); > + > +int coroutine_fn > +bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov, > + bool is_write, BdrvRequestFlags flags); Inconsistent on whether the function name is in column 1 or after the return type. But we aren't consistent elsewhre, so I won't bother changing it. R-b still stands
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 <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> 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
On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote: > Most of our coroutine wrappers already follow this convention: > > We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as > the core function, and a wrapper 'bdrv_<something>(<same argument > list>)' which does parameters packing and call bdrv_run_co(). > > The only outsiders are the bdrv_prwv_co and > bdrv_common_block_status_above wrappers. Let's refactor them to behave > as the others, it simplifies further conversion of coroutine wrappers. > > This patch adds indirection layer, but it will be compensated by > further commit, which will drop bdrv_co_prwv together with is_write > logic, to keep read and write path separate. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/io.c | 60 +++++++++++++++++++++++++++++------------------------- > 1 file changed, 32 insertions(+), 28 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote: > We are going to keep coroutine-wrappers code (structure-packing > parameters, BDRV_POLL wrapper functions) in separate auto-generated > files. So, we'll need a header with declaration of original _co_ > functions, for those which are static now. As well, we'll need > declarations for wrapper functions. Do these declarations now, as a > preparation step. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/coroutines.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++ > block.c | 8 +++--- > block/io.c | 34 +++++++++++------------ > 3 files changed, 88 insertions(+), 21 deletions(-) > create mode 100644 block/coroutines.h > > diff --git a/block/coroutines.h b/block/coroutines.h > new file mode 100644 > index 0000000000..9ce1730a09 > --- /dev/null > +++ b/block/coroutines.h > @@ -0,0 +1,67 @@ Maybe also add: /* SPDX-License-Identifier: MIT */ > +/* > + * Block layer I/O functions > + * > + * Copyright (c) 2003 Fabrice Bellard > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#ifndef BLOCK_COROUTINES_INT_H > +#define BLOCK_COROUTINES_INT_H > + > +#include "block/block_int.h" > + > +int coroutine_fn bdrv_co_check(BlockDriverState *bs, > + BdrvCheckResult *res, BdrvCheckMode fix); > +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp); > + > +int coroutine_fn > +bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov, > + bool is_write, BdrvRequestFlags flags); > +int > +bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov, > + bool is_write, BdrvRequestFlags flags); > + > +int coroutine_fn > +bdrv_co_common_block_status_above(BlockDriverState *bs, > + BlockDriverState *base, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file); > +int > +bdrv_common_block_status_above(BlockDriverState *bs, > + BlockDriverState *base, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file); > + Prototypes documentation welcomed, but this is rather scarce in the block APIs, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > +int coroutine_fn > +bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, > + bool is_read); > +int > +bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, > + bool is_read); > + > +#endif /* BLOCK_COROUTINES_INT_H */
On Tue, Sep 15, 2020 at 07:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > The aim of the series is to reduce code-duplication and writing > parameters structure-packing by hand around coroutine function wrappers. > > Benefits: > - no code duplication > - less indirection > > v8: > 04: - rebase on meson build > - script interface is changed to satisfy meson custom_target > - rename script s/coroutine-wrapper.py/block-coroutine-wrapper.py/ > - add docs/devel/block-coroutine-wrapper.rst > > Vladimir Sementsov-Ogievskiy (7): > block: return error-code from bdrv_invalidate_cache > block/io: refactor coroutine wrappers > block: declare some coroutine functions in block/coroutines.h > scripts: add block-coroutine-wrapper.py > block: generate coroutine-wrapper code > block: drop bdrv_prwv > block/io: refactor save/load vmstate > > docs/devel/block-coroutine-wrapper.rst | 54 ++++ > block/block-gen.h | 49 ++++ > block/coroutines.h | 65 +++++ > include/block/block.h | 34 ++- > block.c | 97 ++----- > block/io.c | 336 ++++--------------------- > tests/test-bdrv-drain.c | 2 +- > block/meson.build | 8 + > scripts/block-coroutine-wrapper.py | 187 ++++++++++++++ > 9 files changed, 451 insertions(+), 381 deletions(-) > create mode 100644 docs/devel/block-coroutine-wrapper.rst > create mode 100644 block/block-gen.h > create mode 100644 block/coroutines.h > create mode 100755 scripts/block-coroutine-wrapper.py Please send a v9 and I'll merge it. Stefan
Hi all! The aim of the series is to reduce code-duplication and writing parameters structure-packing by hand around coroutine function wrappers. Benefits: - no code duplication - less indirection v8: 04: - rebase on meson build - script interface is changed to satisfy meson custom_target - rename script s/coroutine-wrapper.py/block-coroutine-wrapper.py/ - add docs/devel/block-coroutine-wrapper.rst Vladimir Sementsov-Ogievskiy (7): block: return error-code from bdrv_invalidate_cache block/io: refactor coroutine wrappers block: declare some coroutine functions in block/coroutines.h scripts: add block-coroutine-wrapper.py block: generate coroutine-wrapper code block: drop bdrv_prwv block/io: refactor save/load vmstate docs/devel/block-coroutine-wrapper.rst | 54 ++++ block/block-gen.h | 49 ++++ block/coroutines.h | 65 +++++ include/block/block.h | 34 ++- block.c | 97 ++----- block/io.c | 336 ++++--------------------- tests/test-bdrv-drain.c | 2 +- block/meson.build | 8 + scripts/block-coroutine-wrapper.py | 187 ++++++++++++++ 9 files changed, 451 insertions(+), 381 deletions(-) create mode 100644 docs/devel/block-coroutine-wrapper.rst create mode 100644 block/block-gen.h create mode 100644 block/coroutines.h create mode 100755 scripts/block-coroutine-wrapper.py