[Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Fam Zheng]

This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.

block_job_cb is called by block_job_completed, which is always called in
a main loop bottom half in existing block jobs. So we don't need to
worry about thread-safety here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 32b04b4..130b7fb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2248,11 +2248,6 @@ out:
 
 static void block_job_cb(void *opaque, int ret)
 {
-    /* Note that this function may be executed from another AioContext besides
-     * the QEMU main loop.  If you need to access anything that assumes the
-     * QEMU global mutex, use a BH or introduce a mutex.
-     */
-
     BlockDriverState *bs = opaque;
     const char *msg = NULL;
 
-- 
2.6.1

Re: [Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Kevin Wolf]

Am 13.10.2015 um 12:16 hat Fam Zheng geschrieben:
> This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.
> 
> block_job_cb is called by block_job_completed, which is always called in
> a main loop bottom half in existing block jobs. So we don't need to
> worry about thread-safety here.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 32b04b4..130b7fb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2248,11 +2248,6 @@ out:
>  
>  static void block_job_cb(void *opaque, int ret)
>  {
> -    /* Note that this function may be executed from another AioContext besides
> -     * the QEMU main loop.  If you need to access anything that assumes the
> -     * QEMU global mutex, use a BH or introduce a mutex.
> -     */
> -
>      BlockDriverState *bs = opaque;
>      const char *msg = NULL;

Should we instead add a comment that tells you that you _have_ to use
that bottom half because block jobs can be running in an I/O thread?

Kevin

Re: [Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Fam Zheng]

On Tue, 10/13 13:21, Kevin Wolf wrote:
> Am 13.10.2015 um 12:16 hat Fam Zheng geschrieben:
> > This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.
> > 
> > block_job_cb is called by block_job_completed, which is always called in
> > a main loop bottom half in existing block jobs. So we don't need to
> > worry about thread-safety here.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 32b04b4..130b7fb 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2248,11 +2248,6 @@ out:
> >  
> >  static void block_job_cb(void *opaque, int ret)
> >  {
> > -    /* Note that this function may be executed from another AioContext besides
> > -     * the QEMU main loop.  If you need to access anything that assumes the
> > -     * QEMU global mutex, use a BH or introduce a mutex.
> > -     */
> > -
> >      BlockDriverState *bs = opaque;
> >      const char *msg = NULL;
> 
> Should we instead add a comment that tells you that you _have_ to use
> that bottom half because block jobs can be running in an I/O thread?

Probably, but this comment is stale anyway.

Fam

Re: [Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Stefan Hajnoczi]

On Tue, Oct 13, 2015 at 06:16:15PM +0800, Fam Zheng wrote:
> This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.
> 
> block_job_cb is called by block_job_completed, which is always called in
> a main loop bottom half in existing block jobs. So we don't need to
> worry about thread-safety here.

This is not correct.  Search for block_job_completed() callers.

For example, block/stream.c has early exit cases that call
block_job_completed() from the coroutine (i.e. dispatched from a
coroutine in another AioContext+IOThread).

I think you are assuming that all block_job_completed() callers are
called from a function scheduled using block_job_defer_to_main_loop().

Please double-check this.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Fam Zheng]

----- Original Message -----
> On Tue, Oct 13, 2015 at 06:16:15PM +0800, Fam Zheng wrote:
> > This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.
> > 
> > block_job_cb is called by block_job_completed, which is always called in
> > a main loop bottom half in existing block jobs. So we don't need to
> > worry about thread-safety here.
> 
> This is not correct.  Search for block_job_completed() callers.
> 
> For example, block/stream.c has early exit cases that call
> block_job_completed() from the coroutine (i.e. dispatched from a
> coroutine in another AioContext+IOThread).
> 
> I think you are assuming that all block_job_completed() callers are
> called from a function scheduled using block_job_defer_to_main_loop().

No, I'm assuming all block_job_completed() callers are (and they should
be) from main thread. Even the early exit cases in stream are so, because
they are in the same thread as stream_start, which is main thread.

> 
> Please double-check this.
> 
> Thanks,
> Stefan
> 
> 

Re: [Qemu-devel] [PATCH] Revert "blockdev: add note that block_job_cb() must be thread-safe"

[Stefan Hajnoczi]

On Wed, Oct 14, 2015 at 10:51:27PM -0400, Fam Zheng wrote:
> 
> 
> ----- Original Message -----
> > On Tue, Oct 13, 2015 at 06:16:15PM +0800, Fam Zheng wrote:
> > > This reverts commit 723c5d93c51bdb3adbc238ce90195c0864aa6cd5.
> > > 
> > > block_job_cb is called by block_job_completed, which is always called in
> > > a main loop bottom half in existing block jobs. So we don't need to
> > > worry about thread-safety here.
> > 
> > This is not correct.  Search for block_job_completed() callers.
> > 
> > For example, block/stream.c has early exit cases that call
> > block_job_completed() from the coroutine (i.e. dispatched from a
> > coroutine in another AioContext+IOThread).
> > 
> > I think you are assuming that all block_job_completed() callers are
> > called from a function scheduled using block_job_defer_to_main_loop().
> 
> No, I'm assuming all block_job_completed() callers are (and they should
> be) from main thread. Even the early exit cases in stream are so, because
> they are in the same thread as stream_start, which is main thread.

You are right, the early block_job_completed() calls in stream.c happen
in the main thread.

I'm concerned that we have no comments or assertions to check that the
assumption holds.  Luckily only stream.c relies on the trick of calling
block_job_completed() directly but knowing it runs from the main thread.

I'll send a patch to fix stream.c.  It needs to be done anyway since
there is a memory leak in the early block_job_completed() stream.c code.
Will CC you.

Stefan