Re: [Qemu-devel] [PATCH 05/12] aio: introduce aio_{disable, enable}_clients

[Kevin Wolf <kwolf@redhat.com>]

Am 09.10.2015 um 07:45 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c         |  3 ++-
>  aio-win32.c         |  3 ++-
>  async.c             | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/block/aio.h | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index d25fcfc..a261892 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_type_disabled(ctx, node->type)) {
>              add_pollfd(node);
>          }
>      }
> diff --git a/aio-win32.c b/aio-win32.c
> index f5ecf57..66cff60 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_type_disabled(ctx, node->type)) {
>              events[count++] = event_notifier_get_handle(node->e);
>          }
>      }
> diff --git a/async.c b/async.c
> index 244bf79..855b9d5 100644
> --- a/async.c
> +++ b/async.c
> @@ -361,3 +361,45 @@ void aio_context_release(AioContext *ctx)
>  {
>      rfifolock_unlock(&ctx->lock);
>  }
> +
> +bool aio_type_disabled(AioContext *ctx, int type)
> +{
> +    int i = 1;
> +    int n = 0;
> +
> +    while (type) {
> +        bool b = type & 0x1;
> +        type >>= 1;
> +        n++;

Any specific reason for leaving client_disable_counters[0] unused?

> +        i <<= 1;

i is never read.

> +        if (!b) {
> +            continue;
> +        }
> +        if (ctx->client_disable_counters[n]) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}

In general I wonder whether this function really needs to take a mask
with possibly multiple set bits instead of just a single type.

> +void aio_disable_enable_clients(AioContext *ctx, int clients_mask,
> +                                bool is_disable)
> +{
> +    int i = 1;
> +    int n = 0;
> +    aio_context_acquire(ctx);
> +
> +    while (clients_mask) {
> +        bool b = clients_mask & 0x1;
> +        clients_mask >>= 1;
> +        n++;
> +        i <<= 1;

This i isn't used either.

> +        if (!b) {
> +            continue;
> +        }
> +        if (ctx->client_disable_counters[n]) {
> +            return true;
> +        }

Wait, why are you checking the state instead of setting it?

How did you test this series?

> +    }
> +    aio_context_release(ctx);
> +}
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 60e796b..b687152 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 client_disable_counters[sizeof(int)];
>  };

sizeof return the size in bytes. I think you mean bits here?

Kevin