1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread

Jie Wang posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1526459780-35100-1-git-send-email-wangjie88@huawei.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
There is a newer version of this series
include/block/aio.h | 8 ++++++++
util/aio-posix.c    | 9 +++++++++
util/aio-win32.c    | 4 ++++
util/async.c        | 1 +
4 files changed, 22 insertions(+)
[Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by Jie Wang 5 years, 11 months ago 
When we call addIOThread, the epollfd created in aio_context_setup,
but not close it in the process of delIOThread, so the epollfd will leak.

Signed-off-by: Jie Wang <wangjie88@huawei.com>
---
 include/block/aio.h | 8 ++++++++
 util/aio-posix.c    | 9 +++++++++
 util/aio-win32.c    | 4 ++++
 util/async.c        | 1 +
 4 files changed, 22 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index a1d6b9e..ae6f354 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -555,6 +555,14 @@ static inline bool in_aio_context_home_thread(AioContext *ctx)
 void aio_context_setup(AioContext *ctx);
 
 /**
+ * aio_context_destroy:
+ * @ctx: the aio context
+ *
+ * Destroy the aio context.
+ */
+void aio_context_destroy(AioContext *ctx);
+
+/**
  * aio_context_set_poll_params:
  * @ctx: the aio context
  * @max_ns: how long to busy poll for, in nanoseconds
diff --git a/util/aio-posix.c b/util/aio-posix.c
index d8f0cb4..0ade2c7 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -713,6 +713,15 @@ void aio_context_setup(AioContext *ctx)
 #endif
 }
 
+void aio_context_destroy(AioContext *ctx)
+{
+#ifdef CONFIG_EPOLL_CREATE1
+    if (ctx->epollfd >= 0) {
+        close(ctx->epollfd);
+    }
+#endif
+}
+
 void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
                                  int64_t grow, int64_t shrink, Error **errp)
 {
diff --git a/util/aio-win32.c b/util/aio-win32.c
index a67b00c..e676a8d 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -407,6 +407,10 @@ void aio_context_setup(AioContext *ctx)
 {
 }
 
+void aio_context_destroy(AioContext *ctx)
+{
+}
+
 void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
                                  int64_t grow, int64_t shrink, Error **errp)
 {
diff --git a/util/async.c b/util/async.c
index 4dd9d95..03f6278 100644
--- a/util/async.c
+++ b/util/async.c
@@ -298,6 +298,7 @@ aio_ctx_finalize(GSource     *source)
     qemu_rec_mutex_destroy(&ctx->lock);
     qemu_lockcnt_destroy(&ctx->list_lock);
     timerlistgroup_deinit(&ctx->tlg);
+    aio_context_destroy(ctx);
 }
 
 static GSourceFuncs aio_source_funcs = {
-- 
1.8.3.1
Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by WangJie (Pluto) 5 years, 11 months ago 
Hi, Peter Xu:
	If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
    Because the ctx->epoll_enabled is false in the moment.
	In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
    so ctx->epoll_enabled have no chance to set true.

On 2018/5/16 16:36, Jie Wang wrote:
> +void aio_context_destroy(AioContext *ctx)
> +{
> +#ifdef CONFIG_EPOLL_CREATE1
> +    if (ctx->epollfd >= 0) {
> +        close(ctx->epollfd);
> +    }
> +#endif
> +}
> +
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>                                   int64_t grow, int64_t shrink, Error **errp)
Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by Peter Xu 5 years, 11 months ago 
On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
> Hi, Peter Xu:
> 	If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
>     Because the ctx->epoll_enabled is false in the moment.
> 	In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
>     so ctx->epoll_enabled have no chance to set true.

I see that epoll_available will only be set if epollfd != -1, so it
seems to me to make more sense if we swap the two variables in
aio_epoll_disable(), from current version:

static void aio_epoll_disable(AioContext *ctx)
{
    ctx->epoll_available = false;
    if (!ctx->epoll_enabled) {
        return;
    }
    ctx->epoll_enabled = false;
    close(ctx->epollfd);
}

To:

static void aio_epoll_disable(AioContext *ctx)
{
    ctx->epoll_enabled = false;
    if (!ctx->epoll_available) {
        return;
    }
    ctx->epoll_available = false;
    close(ctx->epollfd);
}

What do you think?  And Fam?

> 
> On 2018/5/16 16:36, Jie Wang wrote:
> > +void aio_context_destroy(AioContext *ctx)
> > +{
> > +#ifdef CONFIG_EPOLL_CREATE1
> > +    if (ctx->epollfd >= 0) {
> > +        close(ctx->epollfd);
> > +    }
> > +#endif
> > +}
> > +
> >  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> >                                   int64_t grow, int64_t shrink, Error **errp)
> 

-- 
Peter Xu
Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by WangJie (Pluto) 5 years, 11 months ago 
I agree, wait for a reply from Fam

On 2018/5/16 19:43, Peter Xu wrote:
> On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
>> Hi, Peter Xu:
>> 	If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
>>     Because the ctx->epoll_enabled is false in the moment.
>> 	In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
>>     so ctx->epoll_enabled have no chance to set true.
> 
> I see that epoll_available will only be set if epollfd != -1, so it
> seems to me to make more sense if we swap the two variables in
> aio_epoll_disable(), from current version:
> 
> static void aio_epoll_disable(AioContext *ctx)
> {
>     ctx->epoll_available = false;
>     if (!ctx->epoll_enabled) {
>         return;
>     }
>     ctx->epoll_enabled = false;
>     close(ctx->epollfd);
> }
> 
> To:
> 
> static void aio_epoll_disable(AioContext *ctx)
> {
>     ctx->epoll_enabled = false;
>     if (!ctx->epoll_available) {
>         return;
>     }
>     ctx->epoll_available = false;
>     close(ctx->epollfd);
> }
> 
> What do you think?  And Fam?
> 
>>
>> On 2018/5/16 16:36, Jie Wang wrote:
>>> +void aio_context_destroy(AioContext *ctx)
>>> +{
>>> +#ifdef CONFIG_EPOLL_CREATE1
>>> +    if (ctx->epollfd >= 0) {
>>> +        close(ctx->epollfd);
>>> +    }
>>> +#endif
>>> +}
>>> +
>>>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>>>                                   int64_t grow, int64_t shrink, Error **errp)
>>
>
Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by Fam Zheng 5 years, 11 months ago 
On Wed, 05/16 19:43, Peter Xu wrote:
> On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
> > Hi, Peter Xu:
> > 	If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
> >     Because the ctx->epoll_enabled is false in the moment.
> > 	In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
> >     so ctx->epoll_enabled have no chance to set true.
> 
> I see that epoll_available will only be set if epollfd != -1, so it
> seems to me to make more sense if we swap the two variables in
> aio_epoll_disable(), from current version:
> 
> static void aio_epoll_disable(AioContext *ctx)
> {
>     ctx->epoll_available = false;
>     if (!ctx->epoll_enabled) {
>         return;
>     }
>     ctx->epoll_enabled = false;
>     close(ctx->epollfd);
> }
> 
> To:
> 
> static void aio_epoll_disable(AioContext *ctx)
> {
>     ctx->epoll_enabled = false;
>     if (!ctx->epoll_available) {
>         return;
>     }
>     ctx->epoll_available = false;
>     close(ctx->epollfd);
> }
> 
> What do you think?  And Fam?

Looks good.

Fam

> 
> > 
> > On 2018/5/16 16:36, Jie Wang wrote:
> > > +void aio_context_destroy(AioContext *ctx)
> > > +{
> > > +#ifdef CONFIG_EPOLL_CREATE1
> > > +    if (ctx->epollfd >= 0) {
> > > +        close(ctx->epollfd);
> > > +    }
> > > +#endif
> > > +}
> > > +
> > >  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> > >                                   int64_t grow, int64_t shrink, Error **errp)
> > 
> 
> -- 
> Peter Xu
Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
Posted by WangJie (Pluto) 5 years, 11 months ago 
Hi, Peter Xu:
	If call aio_epoll_disable() in aio_context_destroy, aio_epoll_disable() will return before close(ctx->epollfd),
    Because the ctx->epoll_enabled is false in the moment.
	In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
    so ctx->epoll_enabled have no chance to set true.


On 2018/5/16 16:36, Jie Wang wrote:
> When we call addIOThread, the epollfd created in aio_context_setup,
> but not close it in the process of delIOThread, so the epollfd will leak.
> 
> Signed-off-by: Jie Wang <wangjie88@huawei.com>
> ---
>  include/block/aio.h | 8 ++++++++
>  util/aio-posix.c    | 9 +++++++++
>  util/aio-win32.c    | 4 ++++
>  util/async.c        | 1 +
>  4 files changed, 22 insertions(+)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index a1d6b9e..ae6f354 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -555,6 +555,14 @@ static inline bool in_aio_context_home_thread(AioContext *ctx)
>  void aio_context_setup(AioContext *ctx);
>  
>  /**
> + * aio_context_destroy:
> + * @ctx: the aio context
> + *
> + * Destroy the aio context.
> + */
> +void aio_context_destroy(AioContext *ctx);
> +
> +/**
>   * aio_context_set_poll_params:
>   * @ctx: the aio context
>   * @max_ns: how long to busy poll for, in nanoseconds
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index d8f0cb4..0ade2c7 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -713,6 +713,15 @@ void aio_context_setup(AioContext *ctx)
>  #endif
>  }
>  
> +void aio_context_destroy(AioContext *ctx)
> +{
> +#ifdef CONFIG_EPOLL_CREATE1
> +    if (ctx->epollfd >= 0) {
> +        close(ctx->epollfd);
> +    }
> +#endif
> +}
> +
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>                                   int64_t grow, int64_t shrink, Error **errp)
>  {
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index a67b00c..e676a8d 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -407,6 +407,10 @@ void aio_context_setup(AioContext *ctx)
>  {
>  }
>  
> +void aio_context_destroy(AioContext *ctx)
> +{
> +}
> +
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>                                   int64_t grow, int64_t shrink, Error **errp)
>  {
> diff --git a/util/async.c b/util/async.c
> index 4dd9d95..03f6278 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -298,6 +298,7 @@ aio_ctx_finalize(GSource     *source)
>      qemu_rec_mutex_destroy(&ctx->lock);
>      qemu_lockcnt_destroy(&ctx->list_lock);
>      timerlistgroup_deinit(&ctx->tlg);
> +    aio_context_destroy(ctx);
>  }
>  
>  static GSourceFuncs aio_source_funcs = {
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.