Message ID | 1445393209-26545-5-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
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 > >
On Wed, 10/21 11:56, Jeff Cody wrote: > > +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? AioHandler is defined differently for posix and win32, but I didn't want to duplicate this function in two files. Fam
On Thu, Oct 22, 2015 at 10:11:16AM +0800, Fam Zheng wrote: > On Wed, 10/21 11:56, Jeff Cody wrote: > > > +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? > > AioHandler is defined differently for posix and win32, but I didn't want to > duplicate this function in two files. > > Fam That makes sense, thanks.
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. + */ +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. + */ +static inline void aio_enable_external(AioContext *ctx) +{ + atomic_dec(&ctx->external_disable_cnt); +} + +/** + * 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 + * 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); +} + #endif
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(-)