Re: [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
To: Emanuele Giuseppe Esposito <eesposit@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>,
	Eric Blake <eblake@redhat.com>, Fam Zheng <fam@euphon.net>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	"Denis V. Lunev" <den@openvz.org>, Stefan Weil <sw@weilnetz.de>,
	Jeff Cody <codyprime@gmail.com>, Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper
Date: Sat, 26 Nov 2022 00:09:21 +0300	[thread overview]
Message-ID: <45378cfa-0ab8-86a4-f408-d6443c777b5e@yandex-team.ru> (raw)
In-Reply-To: <20221125133518.418328-15-eesposit@redhat.com>

On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
> check if they are running in a coroutine, directly calling the
> coroutine callback if it's the case.
> Except that no coroutine calls such functions, therefore that check
> can be removed, and function creation can be offloaded to
> c_w.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   block/dirty-bitmap.c         | 88 +-----------------------------------
>   block/meson.build            |  1 +
>   include/block/block-common.h |  5 +-
>   include/block/block-io.h     | 10 +++-
>   include/block/dirty-bitmap.h | 10 +++-
>   5 files changed, 22 insertions(+), 92 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bf3dc0512a..21cf592889 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

Please do

   git config diff.orderFile scripts/git.orderfile

so that .h files appear first.

> @@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>    * not fail.
>    * This function doesn't release corresponding BdrvDirtyBitmap.
>    */
> -static int coroutine_fn
> +int coroutine_fn
>   bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
>                                          Error **errp)
>   {

[..]

> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -29,8 +29,6 @@
>   #include "qemu/iov.h"
>   #include "qemu/coroutine.h"
>   #include "block/accounting.h"
> -#include "block/dirty-bitmap.h"
> -#include "block/blockjob.h"
>   #include "qemu/hbitmap.h"
>   #include "qemu/transactions.h"
>   
> @@ -51,6 +49,9 @@
>   #define co_wrapper
>   #define co_wrapper_mixed
>   
> +#include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
> +

that's not good.. we have unclean dependency on include order and these wrapper definitions.

Actually that means that we want separate header file for co_wrapper and co_wrapper_mixed.. But OK, let's keep it as is for now, not hard to fix it later if really needed.

-- 
Best regards,
Vladimir