[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] block.c: bdrv_replace_child_noperm: first call ->attach(
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH 3/6] block.c: bdrv_replace_child_noperm: first call ->attach(), and then add child |
Date: |
Mon, 14 Feb 2022 11:37:15 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 11/02/2022 13:34, Kevin Wolf wrote:
> Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben:
>> Doing the opposite can make adding the child node to a non-drained node,
>> as apply_subtree_drain is only done in ->attach() and thus make
>> assert_bdrv_graph_writable fail.
>>
>> This can happen for example during a transaction rollback (test 245,
>> test_io_with_graph_changes):
>> 1. a node is removed from the graph, thus it is undrained
>> 2. then something happens, and we need to roll back the transactions
>> through tran_abort()
>> 3. at this point, the current code would first attach the undrained node
>> to the graph via QLIST_INSERT_HEAD, and then call ->attach() that
>> will take care of restoring the drain with apply_subtree_drain(),
>> leaving the node undrained between the two operations.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>> block.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index ec346a7e2e..08a6e3a4ef 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2872,8 +2872,6 @@ static void bdrv_replace_child_noperm(BdrvChild
>> **childp,
>> }
>>
>> if (new_bs) {
>> - assert_bdrv_graph_writable(new_bs);
>> - QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
>>
>> /*
>> * Detaching the old node may have led to the new node's
>> @@ -2890,6 +2888,10 @@ static void bdrv_replace_child_noperm(BdrvChild
>> **childp,
>> if (child->klass->attach) {
>> child->klass->attach(child);
>> }
>> +
>> + assert_bdrv_graph_writable(new_bs);
>> + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
>> +
>> }
>
> Extra empty line. Looks good otherwise.
>
> Does this also mean that the order in bdrv_child_cb_attach/detach() is
> wrong? Or maybe adding a new node to bs->children is okay even when the
> child node isn't drained.
No I don't think it's wrong. In fact, if we are just replacing a node
(so old_bs and new_bs are both != NULL), the child will be just removed
and then re-added to the same children's list of the same parent
(child->opaque).
Whether adding a new node to bs->children requires a drain or not is
still under debate in the other serie with Vladimir. We'll see about
that, but in the meanwhile this is just a safe fix that makes sure that
*if* drains are added, everything will always stay under proper drain.
Emanuele
>
> Kevin
>
- Re: [PATCH 1/6] block/io.c: fix bdrv_child_cb_drained_begin invocations from a coroutine, (continued)
- [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
- [PATCH 6/6] jobs: ensure sleep in job_sleep_ns is fully performed, Emanuele Giuseppe Esposito, 2022/02/08