[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next()
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next() |
Date: |
Fri, 20 May 2016 09:54:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 |
On 19/05/2016 17:21, Kevin Wolf wrote:
> We need to introduce a separate BdrvNextIterator struct that can keep
> more state than just the current BDS in order to avoid using the bs->blk
> pointer.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
> block.c | 40 +++++++----------------
> block/block-backend.c | 72
> +++++++++++++++++++++++++++++-------------
> block/io.c | 13 ++++----
> block/snapshot.c | 30 +++++++++++-------
> blockdev.c | 3 +-
> include/block/block.h | 3 +-
> include/sysemu/block-backend.h | 1 -
> migration/block.c | 4 ++-
> monitor.c | 6 ++--
> qmp.c | 5 ++-
> 10 files changed, 102 insertions(+), 75 deletions(-)
>
> diff --git a/block.c b/block.c
> index fd4cf81..91bf431 100644
> --- a/block.c
> +++ b/block.c
> @@ -2870,25 +2870,6 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
> return QTAILQ_NEXT(bs, node_list);
> }
>
> -/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
> - * the monitor or attached to a BlockBackend */
> -BlockDriverState *bdrv_next(BlockDriverState *bs)
> -{
> - if (!bs || bs->blk) {
> - bs = blk_next_root_bs(bs);
> - if (bs) {
> - return bs;
> - }
> - }
> -
> - /* Ignore all BDSs that are attached to a BlockBackend here; they have
> been
> - * handled by the above block already */
> - do {
> - bs = bdrv_next_monitor_owned(bs);
> - } while (bs && bs->blk);
> - return bs;
> -}
> -
> const char *bdrv_get_node_name(const BlockDriverState *bs)
> {
> return bs->node_name;
> @@ -3220,10 +3201,11 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
> Error **errp)
>
> void bdrv_invalidate_cache_all(Error **errp)
> {
> - BlockDriverState *bs = NULL;
> + BlockDriverState *bs;
> Error *local_err = NULL;
> + BdrvNextIterator *it = NULL;
>
> - while ((bs = bdrv_next(bs)) != NULL) {
> + while ((it = bdrv_next(it, &bs)) != NULL) {
> AioContext *aio_context = bdrv_get_aio_context(bs);
>
> aio_context_acquire(aio_context);
> @@ -3265,10 +3247,11 @@ static int bdrv_inactivate_recurse(BlockDriverState
> *bs,
> int bdrv_inactivate_all(void)
> {
> BlockDriverState *bs = NULL;
> + BdrvNextIterator *it = NULL;
> int ret = 0;
> int pass;
>
> - while ((bs = bdrv_next(bs)) != NULL) {
> + while ((it = bdrv_next(it, &bs)) != NULL) {
> aio_context_acquire(bdrv_get_aio_context(bs));
> }
>
> @@ -3277,8 +3260,8 @@ int bdrv_inactivate_all(void)
> * the second pass sets the BDRV_O_INACTIVE flag so that no further write
> * is allowed. */
> for (pass = 0; pass < 2; pass++) {
> - bs = NULL;
> - while ((bs = bdrv_next(bs)) != NULL) {
> + it = NULL;
> + while ((it = bdrv_next(it, &bs)) != NULL) {
> ret = bdrv_inactivate_recurse(bs, pass);
> if (ret < 0) {
> goto out;
> @@ -3287,8 +3270,8 @@ int bdrv_inactivate_all(void)
> }
>
> out:
> - bs = NULL;
> - while ((bs = bdrv_next(bs)) != NULL) {
> + it = NULL;
> + while ((it = bdrv_next(it, &bs)) != NULL) {
> aio_context_release(bdrv_get_aio_context(bs));
> }
>
> @@ -3781,10 +3764,11 @@ bool
> bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> */
> bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> {
> - BlockDriverState *bs = NULL;
> + BlockDriverState *bs;
> + BdrvNextIterator *it = NULL;
>
> /* walk down the bs forest recursively */
> - while ((bs = bdrv_next(bs)) != NULL) {
> + while ((it = bdrv_next(it, &bs)) != NULL) {
> bool perm;
>
> /* try to recurse in this top level bs */
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 9dcac97..7f2eeb0 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -75,6 +75,7 @@ static const AIOCBInfo block_backend_aiocb_info = {
> };
>
> static void drive_info_del(DriveInfo *dinfo);
> +static BlockBackend *bdrv_first_blk(BlockDriverState *bs);
>
> /* All BlockBackends */
> static QTAILQ_HEAD(, BlockBackend) block_backends =
> @@ -286,28 +287,50 @@ BlockBackend *blk_next(BlockBackend *blk)
> : QTAILQ_FIRST(&monitor_block_backends);
> }
>
> -/*
> - * Iterates over all BlockDriverStates which are attached to a BlockBackend.
> - * This function is for use by bdrv_next().
> - *
> - * @bs must be NULL or a BDS that is attached to a BB.
> - */
> -BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
> -{
> +struct BdrvNextIterator {
> + enum {
> + BDRV_NEXT_BACKEND_ROOTS,
> + BDRV_NEXT_MONITOR_OWNED,
> + } phase;
> BlockBackend *blk;
> + BlockDriverState *bs;
> +};
>
> - if (bs) {
> - assert(bs->blk);
> - blk = bs->blk;
> - } else {
> - blk = NULL;
> +/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
> + * the monitor or attached to a BlockBackend */
> +BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
> +{
> + if (!it) {
> + it = g_new(BdrvNextIterator, 1);
Hmm, who frees it? (Especially if the caller exits the loop
prematurely, which means you cannot just free it before returning NULL).
I think it's better to:
- allocate the iterator on the stack and make bdrv_next return a BDS *
- and add a bdrv_first function that does this:
> + *it = (BdrvNextIterator) {
> + .phase = BDRV_NEXT_BACKEND_ROOTS,
> + };
and then returns bdrv_next(it);
- if desirable add a macro that abstracts the calls to bdrv_first and
bdrv_next.
Thanks,
Paolo
- [Qemu-block] [PULL 16/31] block: User BdrvChild callback for device name, (continued)
- [Qemu-block] [PULL 16/31] block: User BdrvChild callback for device name, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 15/31] block: Use BdrvChild callbacks for change_media/resize, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 18/31] blockjob: Don't touch BDS iostatus, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 11/31] block: Decouple throttling from BlockDriverState, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 17/31] blockjob: Don't set iostatus of target, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 19/31] block: Remove bdrv_aio_multiwrite(), Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 13/31] Revert "block: Forbid I/O throttling on nodes with multiple parents for 2.6", Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 26/31] qcow2: Fix write_zeroes with partially allocated backing file cluster, Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 20/31] block: Add bdrv_has_blk(), Kevin Wolf, 2016/05/19
- [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Kevin Wolf, 2016/05/19
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(),
Paolo Bonzini <=
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Kevin Wolf, 2016/05/20
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Kevin Wolf, 2016/05/20
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Paolo Bonzini, 2016/05/20
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Kevin Wolf, 2016/05/20
- Re: [Qemu-block] [PULL 21/31] block: Avoid bs->blk in bdrv_next(), Paolo Bonzini, 2016/05/20
[Qemu-block] [PULL 25/31] qcow2: fix condition in is_zero_cluster, Kevin Wolf, 2016/05/19
[Qemu-block] [PULL 28/31] block: clarify error message for qmp-eject, Kevin Wolf, 2016/05/19
[Qemu-block] [PULL 22/31] block: Don't return throttling info in query-named-block-nodes, Kevin Wolf, 2016/05/19
[Qemu-block] [PULL 29/31] qemu-io: Fix recent UI updates, Kevin Wolf, 2016/05/19
[Qemu-block] [PULL 23/31] block: Remove BlockDriverState.blk, Kevin Wolf, 2016/05/19