[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] block/io.c: fix bdrv_child_cb_drained_begin invocations
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH 1/6] block/io.c: fix bdrv_child_cb_drained_begin invocations from a coroutine |
Date: |
Thu, 17 Feb 2022 16:49:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 14/02/2022 12:57, Paolo Bonzini wrote:
> On 2/14/22 11:27, Emanuele Giuseppe Esposito wrote:
>> Anyways, I think that in addition to the fix in this patch, we should
>> also fix bdrv_parent_drained_begin_single(poll=true) in
>> bdrv_replace_child_noperm, with something similar to what is done in
>> bdrv_co_yield_to_drain? ie if we are in coroutine, schedule a BH that
>> runs the same logic but in the main loop, but then somehow wait that it
>> finishes before continuing?
>>
>> Alternatively, we would forbid polling in coroutines at all. And the
>> only place I can see that is using the drain in coroutine is mirror (see
>> below).
>
> I think you should first of all see what breaks if you forbid
> bdrv_replace_child_noperm() from coroutine context.
Checked. Basically, if I add the assertion assert(!qemu_in_coroutine())
only in bdrv_parent_drained_begin_single(), iotests and unit tests run
as intended.
If I add the assertion in bdrv_replace_child_noperm(), so that the
function can't be invoked by coroutines, everything breaks. Starting
from qemu-img, as it uses bdrv_create_co_entry() and therefore
qcow2_co_create_opts() for qcow2 format.
On the other side, if I add subtree_drains throughout the code, we end
up having bdrv_parent_drained_begin_single() called much more
frequently, and since bdrv_replace_child_noperm() *is* called by
coroutines, the drain count won't match, so
bdrv_parent_drained_begin_single() will be called much more frequently
and the assertion will fail.
I am testing the fix agreed with Kevin, i.e. implement something very
similar to bdrv_co_yield_to_drain() in
bdrv_parent_drained_begin_single(), where we just create a bh to do the
work in the main loop, and it seems to be working. Maybe that's the way
to go for bdrv_replace_child_noperm?
Should I get rid of this one or have both fixes in two patches? This
patch just fixes part of the problem, but as Paolo said it's correct
nevertheless.
Emanuele
>
> Drain in coroutines does not poll, it gets out of the coroutine through
> a bottom half before polling. So if bdrv_replace_child_noperm() doesn't
> require it, polling in coroutines can still be forbidden.
>
> This patch is correct nevertheless.
>
> Paolo
>
- Re: [PATCH 5/6] test-bdrv-drain.c: remove test_detach_by_parent_cb(), (continued)
[PATCH 1/6] block/io.c: fix bdrv_child_cb_drained_begin invocations from a coroutine, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH 4/6] test-bdrv-drain.c: adapt test to the coming subtree drains, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH 2/6] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach(), Emanuele Giuseppe Esposito, 2022/02/08
[PATCH 3/6] block.c: bdrv_replace_child_noperm: first call ->attach(), and then add child, Emanuele Giuseppe Esposito, 2022/02/08