Re: [Qemu-devel] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB

[Kevin Wolf <kwolf@redhat.com>]

Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                                       | 20 ------------
>  block/block-backend.c                         | 44 +++++++++++++++++++++++----
>  block/io.c                                    | 20 ------------
>  include/block/block.h                         |  2 --
>  stubs/Makefile.objs                           |  2 +-
>  stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
>  6 files changed, 41 insertions(+), 51 deletions(-)
>  rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)
> 
> diff --git a/block.c b/block.c
> index a119840..5eac9a3 100644
> --- a/block.c
> +++ b/block.c
> @@ -2531,26 +2531,6 @@ ro_cleanup:
>      return ret;
>  }
>  
> -int bdrv_commit_all(void)
> -{
> -    BlockDriverState *bs;
> -
> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -        AioContext *aio_context = bdrv_get_aio_context(bs);
> -
> -        aio_context_acquire(aio_context);
> -        if (bs->drv && bs->backing) {
> -            int ret = bdrv_commit(bs);
> -            if (ret < 0) {
> -                aio_context_release(aio_context);
> -                return ret;
> -            }
> -        }
> -        aio_context_release(aio_context);
> -    }
> -    return 0;
> -}
> -
>  /*
>   * Return values:
>   * 0        - success
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a10fe44..a918c35 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -908,11 +908,6 @@ int blk_flush(BlockBackend *blk)
>      return bdrv_flush(blk->bs);
>  }
>  
> -int blk_flush_all(void)
> -{
> -    return bdrv_flush_all();
> -}
> -
>  void blk_drain(BlockBackend *blk)
>  {
>      if (blk->bs) {
> @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
>  
>  int blk_commit_all(void)
>  {
> -    return bdrv_commit_all();
> +    BlockBackend *blk = NULL;
> +
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        AioContext *aio_context = blk_get_aio_context(blk);
> +
> +        aio_context_acquire(aio_context);
> +        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {

blk->bs->drv is already implied by blk_is_available(), so it's
unnecessary, even though it doesn't actively hurt.

Also, using blk_is_available() instead of blk_is_inserted() as in
blk_flush_all() means that a drive with an open tray, but inserted
medium isn't committed. I assume you did this on purpose because you're
using two different functions in this patch, but isn't this a change in
behaviour? If so, and we really want it, it should at least be mentioned
in the commit message.

> +            int ret = bdrv_commit(blk->bs);
> +            if (ret < 0) {
> +                aio_context_release(aio_context);
> +                return ret;
> +            }
> +        }
> +        aio_context_release(aio_context);
> +    }
> +    return 0;
> +}
> +
> +int blk_flush_all(void)
> +{
> +    BlockBackend *blk = NULL;
> +    int result = 0;
> +
> +    while ((blk = blk_all_next(blk)) != NULL) {
> +        AioContext *aio_context = blk_get_aio_context(blk);
> +        int ret;
> +
> +        aio_context_acquire(aio_context);
> +        if (blk_is_inserted(blk)) {
> +            ret = blk_flush(blk);
> +            if (ret < 0 && !result) {
> +                result = ret;
> +            }
> +        }
> +        aio_context_release(aio_context);
> +    }
> +
> +    return result;
>  }

Kevin