Re: [Qemu-devel] [Qemu-block] [PATCH v5 04/12] aio: introduce aio_{disable, enable}_external

[Jeff Cody <jcody@redhat.com>]

On Wed, Oct 21, 2015 at 10:06:41AM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c         |  3 ++-
>  aio-win32.c         |  3 ++-
>  include/block/aio.h | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index f0f9122..0467f23 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -261,7 +261,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
>  
>      /* fill pollfds */
>      QLIST_FOREACH(node, &ctx->aio_handlers, node) {
> -        if (!node->deleted && node->pfd.events) {
> +        if (!node->deleted && node->pfd.events
> +            && aio_node_check(ctx, node->is_external)) {
>              add_pollfd(node);
>          }
>      }
> diff --git a/aio-win32.c b/aio-win32.c
> index 3110d85..43c4c79 100644
> --- a/aio-win32.c
> +++ b/aio-win32.c
> @@ -309,7 +309,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      /* fill fd sets */
>      count = 0;
>      QLIST_FOREACH(node, &ctx->aio_handlers, node) {
> -        if (!node->deleted && node->io_notify) {
> +        if (!node->deleted && node->io_notify
> +            && aio_node_check(ctx, node->is_external)) {
>              events[count++] = event_notifier_get_handle(node->e);
>          }
>      }
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 12f1141..80151d1 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -122,6 +122,8 @@ struct AioContext {
>  
>      /* TimerLists for calling timers - one per clock type */
>      QEMUTimerListGroup tlg;
> +
> +    int external_disable_cnt;
>  };
>  
>  /**
> @@ -375,4 +377,39 @@ static inline void aio_timer_init(AioContext *ctx,
>   */
>  int64_t aio_compute_timeout(AioContext *ctx);
>  
> +/**
> + * aio_disable_external:
> + * @ctx: the aio context
> + *
> + * Disable the furthur processing of clients.

s/furthur/further

The comment below specifically references external clients - I think
the comments for aio_disable_external / aio_enable_external should be
worded similarly, so there is no confusion.

> + */
> +static inline void aio_disable_external(AioContext *ctx)
> +{
> +    atomic_inc(&ctx->external_disable_cnt);
> +}
> +
> +/**
> + * aio_enable_external:
> + * @ctx: the aio context
> + *
> + * Disable the processing of external clients.

Should this comment read "Enable" instead of "Disable"?

> + */
> +static inline void aio_enable_external(AioContext *ctx)
> +{
> +    atomic_dec(&ctx->external_disable_cnt);

Should we assert(ctx->external_disable_cnt >= 0)?


Additional comment:  the function names aio_enable_external() and
aio_disable_external() may be a bit misleading (particularly
aio_enable_external()).  It doesn't do a blanket enable of external
aio (i.e., it does not just blindly do ctx->external_disable_cnt = 0).

Perhaps something like aio_external_disable_inc/dec()?  (I'm not real
fond of that, either).

Just something for thought.


> +}
> +
> +/**
> + * aio_node_check:
> + * @ctx: the aio context
> + * @is_external: Whether or not the checked node is an external event source.
> + *
> + * Check if the node's is_external flag is okey to be polled by the ctx at this

s/okey/okay

> + * moment. True means green light.
> + */
> +static inline bool aio_node_check(AioContext *ctx, bool is_external)
> +{
> +    return !is_external || !atomic_read(&ctx->external_disable_cnt);
> +}
> +

It seems a little odd to me to have this helper function take the
is_external bool field from the node as the argument - any reason to
do that, rather than pass in the AioHandler and have aio_node_check()
parse whatever fields it deems necessary from it?

>  #endif
> -- 
> 2.4.3
> 
>