mirror: hold aio_context before bdrv_drain

From: Bin Wu <wu.wubin@huawei.com>

Signed-off-by: Bin Wu <wu.wubin@huawei.com>
---
 block/mirror.c | 2 ++
 1 file changed, 2 insertions(+)

On Wed, 04/01 12:42, Bin Wu wrote:
> From: Bin Wu <wu.wubin@huawei.com>

What's the issue are you fixing? I think the coroutine already is running in
the AioContext of bs.

Fam

> 
> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> ---
>  block/mirror.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 4056164..08372df 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -530,7 +530,9 @@ static void coroutine_fn mirror_run(void *opaque)
>               * mirror_populate runs.
>               */
>              trace_mirror_before_drain(s, cnt);
> +            aio_context_acquire(bdrv_get_aio_context(bs));
>              bdrv_drain(bs);
> +            aio_context_release(bdrv_get_aio_context(bs));
>              cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
>          }
>  
> -- 
> 1.7.12.4
> 
> 
>

On 2015/4/1 16:19, Fam Zheng wrote:
> On Wed, 04/01 12:42, Bin Wu wrote:
>> From: Bin Wu <wu.wubin@huawei.com>
> 
> What's the issue are you fixing? I think the coroutine already is running in
> the AioContext of bs.
> 
> Fam
> 
In the current implementation of bdrv_drain, it should be placed in a critical
section as suggested in the comments above the function: "Note that unlike
bdrv_drain_all(), the caller must hold the BlockDriverState AioContext".

However, the mirror coroutine starting with mirror_run doesn't do this. I just
found qmp_drive_mirror protects the AioCentext, but it is out of the scope of
the mirror coroutine.

Bin

>>
>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>> ---
>>  block/mirror.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 4056164..08372df 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -530,7 +530,9 @@ static void coroutine_fn mirror_run(void *opaque)
>>               * mirror_populate runs.
>>               */
>>              trace_mirror_before_drain(s, cnt);
>> +            aio_context_acquire(bdrv_get_aio_context(bs));
>>              bdrv_drain(bs);
>> +            aio_context_release(bdrv_get_aio_context(bs));
>>              cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
>>          }
>>  
>> -- 
>> 1.7.12.4
>>
>>
>>
> 
> .
>

On Wed, Apr 01, 2015 at 04:49:39PM +0800, Bin Wu wrote:
> 
> On 2015/4/1 16:19, Fam Zheng wrote:
> > On Wed, 04/01 12:42, Bin Wu wrote:
> >> From: Bin Wu <wu.wubin@huawei.com>
> > 
> > What's the issue are you fixing? I think the coroutine already is running in
> > the AioContext of bs.
> > 
> > Fam
> > 
> In the current implementation of bdrv_drain, it should be placed in a critical
> section as suggested in the comments above the function: "Note that unlike
> bdrv_drain_all(), the caller must hold the BlockDriverState AioContext".
> 
> However, the mirror coroutine starting with mirror_run doesn't do this. I just
> found qmp_drive_mirror protects the AioCentext, but it is out of the scope of
> the mirror coroutine.

There are 3 possibilities:

1. qmp_drive_mirror() under QEMU main loop thread.  AioContext is held.

2. IOThread aio_poll().  AioContext is held.

3. QEMU main loop thread when IOThread (dataplane) is not used.  Here
   the AioContext is the global qemu_aio_context.  We don't need to
   acquire it explicitly, we're protected by the global mutex.

If #3 was a problem then virtio-blk.c's bdrv_aio_writev() would also be
a problem, for example.

This patch is unnecessary.

Stefan

On 2015/4/1 19:59, Stefan Hajnoczi wrote:
> On Wed, Apr 01, 2015 at 04:49:39PM +0800, Bin Wu wrote:
>>
>> On 2015/4/1 16:19, Fam Zheng wrote:
>>> On Wed, 04/01 12:42, Bin Wu wrote:
>>>> From: Bin Wu <wu.wubin@huawei.com>
>>>
>>> What's the issue are you fixing? I think the coroutine already is running in
>>> the AioContext of bs.
>>>
>>> Fam
>>>
>> In the current implementation of bdrv_drain, it should be placed in a critical
>> section as suggested in the comments above the function: "Note that unlike
>> bdrv_drain_all(), the caller must hold the BlockDriverState AioContext".
>>
>> However, the mirror coroutine starting with mirror_run doesn't do this. I just
>> found qmp_drive_mirror protects the AioCentext, but it is out of the scope of
>> the mirror coroutine.
> 
> There are 3 possibilities:
> 
> 1. qmp_drive_mirror() under QEMU main loop thread.  AioContext is held.
> 
> 2. IOThread aio_poll().  AioContext is held.
> 
> 3. QEMU main loop thread when IOThread (dataplane) is not used.  Here
>    the AioContext is the global qemu_aio_context.  We don't need to
>    acquire it explicitly, we're protected by the global mutex.
> 
> If #3 was a problem then virtio-blk.c's bdrv_aio_writev() would also be
> a problem, for example.
> 
> This patch is unnecessary.
> 
> Stefan
> 
OK, I see. Thanks