| Message ID | 20201214170519.223781-3-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd/server: Quiesce coroutines on context switch | expand |
Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > While processing the parents of a BDS, one of the parents may process > the child that's doing the tail recursion, which leads to a BDS being > processed twice. This is especially problematic for the aio_notifiers, > as they might attempt to work on both the old and the new AIO > contexts. > > To avoid this, add the BDS pointer to the ignore list, and check the > child BDS pointer while iterating over the children. > > Signed-off-by: Sergio Lopez <slp@redhat.com> Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ What is the specific scenario where you saw this breaking? Did you have multiple BdrvChild connections between two nodes so that we would go to the parent node through one and then come back to the child node through the other? Maybe if what we really need to do is not processing every edge once, but processing every node once, the list should be changed to contain _only_ BDS objects. But then blk_do_set_aio_context() probably won't work any more because it can't have blk->root ignored any more... Anyway, if we end up changing what the list contains, the comment needs an update, too. Currently it says: * @ignore will accumulate all visited BdrvChild object. The caller is * responsible for freeing the list afterwards. Another option: Split the parents QLIST_FOREACH loop in two. First add all parent BdrvChild objects to the ignore list, remember which of them were newly added, and only after adding all of them call child->klass->set_aio_ctx() for each parent that was previously not on the ignore list. This will avoid that we come back to the same node because all of its incoming edges are ignored now. Kevin > block.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index f1cedac362..bc8a66ab6e 100644 > --- a/block.c > +++ b/block.c > @@ -6465,12 +6465,17 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, > bdrv_drained_begin(bs); > > QLIST_FOREACH(child, &bs->children, next) { > - if (g_slist_find(*ignore, child)) { > + if (g_slist_find(*ignore, child) || g_slist_find(*ignore, child->bs)) { > continue; > } > *ignore = g_slist_prepend(*ignore, child); > bdrv_set_aio_context_ignore(child->bs, new_context, ignore); > } > + /* > + * Add a reference to this BS to the ignore list, so its > + * parents won't attempt to process it again. > + */ > + *ignore = g_slist_prepend(*ignore, bs); > QLIST_FOREACH(child, &bs->parents, next_parent) { > if (g_slist_find(*ignore, child)) { > continue; > -- > 2.26.2 >
On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > While processing the parents of a BDS, one of the parents may process > > the child that's doing the tail recursion, which leads to a BDS being > > processed twice. This is especially problematic for the aio_notifiers, > > as they might attempt to work on both the old and the new AIO > > contexts. > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > child BDS pointer while iterating over the children. > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ I know, it's effective but quite ugly... > What is the specific scenario where you saw this breaking? Did you have > multiple BdrvChild connections between two nodes so that we would go to > the parent node through one and then come back to the child node through > the other? I don't think this is a corner case. If the graph is walked top->down, there's no problem since children are added to the ignore list before getting processed, and siblings don't process each other. But, if the graph is walked bottom->up, a BDS will start processing its parents without adding itself to the ignore list, so there's nothing preventing them from processing it again. I'm pasting here an annotated trace of bdrv_set_aio_context_ignore I generated while triggering the issue: <----- begin ------> bdrv_set_aio_context_ignore: bs=0x555ee2e48030 enter bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing children bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 enter bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 processing children bdrv_set_aio_context_ignore: bs=0x555ee2e52060 enter bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing children bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing parents bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing itself bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 processing parents - We enter b_s_a_c_i with BDS 2fbf660 the first time: bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 enter bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing children - We enter b_s_a_c_i with BDS 3bc0c00, a child of 2fbf660: bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 enter bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 processing children bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 processing parents bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 processing itself - We start processing its parents: bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing parents - We enter b_s_a_c_i with BDS 2e48030, a parent of 2fbf660: bdrv_set_aio_context_ignore: bs=0x555ee2e48030 enter bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing children - We enter b_s_a_c_i with BDS 2fbf660 again, because of parent 2e48030 didn't found us it in the ignore list: bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 enter bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing children bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing parents bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing itself bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing parents bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing itself - BDS 2fbf660 will be processed here a second time, triggering the issue: bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing itself <----- end ------> I suspect this has been happening for a while, and has only surfaced now due to the need to run an AIO context BH in an aio_notifier function that the "nbd/server: Quiesce coroutines on context switch" patch introduces. There the problem is that the first time the aio_notifier AIO detach function is called, it works on the old context (as it should be), and the second one works on the new context (which is wrong). > Maybe if what we really need to do is not processing every edge once, > but processing every node once, the list should be changed to contain > _only_ BDS objects. But then blk_do_set_aio_context() probably won't > work any more because it can't have blk->root ignored any more... I tried that in my first attempt and it broke badly. I didn't take a deeper look at the causes. > Anyway, if we end up changing what the list contains, the comment needs > an update, too. Currently it says: > > * @ignore will accumulate all visited BdrvChild object. The caller is > * responsible for freeing the list afterwards. > > Another option: Split the parents QLIST_FOREACH loop in two. First add > all parent BdrvChild objects to the ignore list, remember which of them > were newly added, and only after adding all of them call > child->klass->set_aio_ctx() for each parent that was previously not on > the ignore list. This will avoid that we come back to the same node > because all of its incoming edges are ignored now. I don't think this strategy will fix the issue illustrated in the trace above, as the BdrvChild pointer of the BDS processing its parents won't be the on ignore list by the time one of its parents starts processing its own children. Thanks, Sergio. > > block.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/block.c b/block.c > > index f1cedac362..bc8a66ab6e 100644 > > --- a/block.c > > +++ b/block.c > > @@ -6465,12 +6465,17 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, > > bdrv_drained_begin(bs); > > > > QLIST_FOREACH(child, &bs->children, next) { > > - if (g_slist_find(*ignore, child)) { > > + if (g_slist_find(*ignore, child) || g_slist_find(*ignore, child->bs)) { > > continue; > > } > > *ignore = g_slist_prepend(*ignore, child); > > bdrv_set_aio_context_ignore(child->bs, new_context, ignore); > > } > > + /* > > + * Add a reference to this BS to the ignore list, so its > > + * parents won't attempt to process it again. > > + */ > > + *ignore = g_slist_prepend(*ignore, bs); > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > if (g_slist_find(*ignore, child)) { > > continue; > > -- > > 2.26.2 > > >
Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > While processing the parents of a BDS, one of the parents may process > > > the child that's doing the tail recursion, which leads to a BDS being > > > processed twice. This is especially problematic for the aio_notifiers, > > > as they might attempt to work on both the old and the new AIO > > > contexts. > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > child BDS pointer while iterating over the children. > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > I know, it's effective but quite ugly... > > > What is the specific scenario where you saw this breaking? Did you have > > multiple BdrvChild connections between two nodes so that we would go to > > the parent node through one and then come back to the child node through > > the other? > > I don't think this is a corner case. If the graph is walked top->down, > there's no problem since children are added to the ignore list before > getting processed, and siblings don't process each other. But, if the > graph is walked bottom->up, a BDS will start processing its parents > without adding itself to the ignore list, so there's nothing > preventing them from processing it again. I don't understand. child is added to ignore before calling the parent callback on it, so how can we come back through the same BdrvChild? QLIST_FOREACH(child, &bs->parents, next_parent) { if (g_slist_find(*ignore, child)) { continue; } assert(child->klass->set_aio_ctx); *ignore = g_slist_prepend(*ignore, child); child->klass->set_aio_ctx(child, new_context, ignore); } > I'm pasting here an annotated trace of bdrv_set_aio_context_ignore I > generated while triggering the issue: > > <----- begin ------> > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 enter > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing children > bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 enter > bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 processing children > bdrv_set_aio_context_ignore: bs=0x555ee2e52060 enter > bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing children > bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing parents > bdrv_set_aio_context_ignore: bs=0x555ee2e52060 processing itself > bdrv_set_aio_context_ignore: bs=0x555ee2e5d420 processing parents > > - We enter b_s_a_c_i with BDS 2fbf660 the first time: > > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 enter > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing children > > - We enter b_s_a_c_i with BDS 3bc0c00, a child of 2fbf660: > > bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 enter > bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 processing children > bdrv_set_aio_context_ignore: bs=0x555ee3bc0c00 processing parents > > - We start processing its parents: > > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing parents > > - We enter b_s_a_c_i with BDS 2e48030, a parent of 2fbf660: > > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 enter > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing children > > - We enter b_s_a_c_i with BDS 2fbf660 again, because of parent > 2e48030 didn't found us it in the ignore list: > > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 enter > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing children > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing parents > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing itself > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing parents > bdrv_set_aio_context_ignore: bs=0x555ee2e48030 processing itself > > - BDS 2fbf660 will be processed here a second time, triggering the > issue: > > bdrv_set_aio_context_ignore: bs=0x555ee2fbf660 processing itself > <----- end ------> You didn't dump the BdrvChild here. I think that would add some information on why we re-entered 0x555ee2fbf660. Maybe you can also add bs->drv->format_name for each node to make the scenario less abstract? So far my reconstruction of the graph is something like this: 0x555ee2e48030 --+ | | | | | +-> 0x555ee2e5d420 -> 0x555ee2e52060 v v | 0x555ee2fbf660 --+ | +-------> 0x555ee3bc0c00 It doesn't look quite trivial, but if 0x555ee2e48030 is the filter node of a block job, it's not hard to imagine either. > I suspect this has been happening for a while, and has only surfaced > now due to the need to run an AIO context BH in an aio_notifier > function that the "nbd/server: Quiesce coroutines on context switch" > patch introduces. There the problem is that the first time the > aio_notifier AIO detach function is called, it works on the old > context (as it should be), and the second one works on the new context > (which is wrong). > > > Maybe if what we really need to do is not processing every edge once, > > but processing every node once, the list should be changed to contain > > _only_ BDS objects. But then blk_do_set_aio_context() probably won't > > work any more because it can't have blk->root ignored any more... > > I tried that in my first attempt and it broke badly. I didn't take a > deeper look at the causes. > > > Anyway, if we end up changing what the list contains, the comment needs > > an update, too. Currently it says: > > > > * @ignore will accumulate all visited BdrvChild object. The caller is > > * responsible for freeing the list afterwards. > > > > Another option: Split the parents QLIST_FOREACH loop in two. First add > > all parent BdrvChild objects to the ignore list, remember which of them > > were newly added, and only after adding all of them call > > child->klass->set_aio_ctx() for each parent that was previously not on > > the ignore list. This will avoid that we come back to the same node > > because all of its incoming edges are ignored now. > > I don't think this strategy will fix the issue illustrated in the > trace above, as the BdrvChild pointer of the BDS processing its > parents won't be the on ignore list by the time one of its parents > starts processing its own children. But why? We do append to the ignore list each time before we recurse into a child or parent node. The only way I see is if you have two separate BdrvChild links between the nodes. Kevin
On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > While processing the parents of a BDS, one of the parents may process > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > as they might attempt to work on both the old and the new AIO > > > > contexts. > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > child BDS pointer while iterating over the children. > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > I know, it's effective but quite ugly... > > > > > What is the specific scenario where you saw this breaking? Did you have > > > multiple BdrvChild connections between two nodes so that we would go to > > > the parent node through one and then come back to the child node through > > > the other? > > > > I don't think this is a corner case. If the graph is walked top->down, > > there's no problem since children are added to the ignore list before > > getting processed, and siblings don't process each other. But, if the > > graph is walked bottom->up, a BDS will start processing its parents > > without adding itself to the ignore list, so there's nothing > > preventing them from processing it again. > > I don't understand. child is added to ignore before calling the parent > callback on it, so how can we come back through the same BdrvChild? > > QLIST_FOREACH(child, &bs->parents, next_parent) { > if (g_slist_find(*ignore, child)) { > continue; > } > assert(child->klass->set_aio_ctx); > *ignore = g_slist_prepend(*ignore, child); > child->klass->set_aio_ctx(child, new_context, ignore); > } Perhaps I'm missing something, but the way I understand it, that loop is adding the BdrvChild pointer of each of its parents, but not the BdrvChild pointer of the BDS that was passed as an argument to b_s_a_c_i. > You didn't dump the BdrvChild here. I think that would add some > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > bs->drv->format_name for each node to make the scenario less abstract? I've generated another trace with more data: bs=0x565505e48030 (backup-top) enter bs=0x565505e48030 (backup-top) processing children bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) bs=0x565505e5d420 (qcow2) enter bs=0x565505e5d420 (qcow2) processing children bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) bs=0x565505e52060 (file) enter bs=0x565505e52060 (file) processing children bs=0x565505e52060 (file) processing parents bs=0x565505e52060 (file) processing itself bs=0x565505e5d420 (qcow2) processing parents bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 bs=0x565505fbf660 (qcow2) enter bs=0x565505fbf660 (qcow2) processing children bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) bs=0x565506bc0c00 (file) enter bs=0x565506bc0c00 (file) processing children bs=0x565506bc0c00 (file) processing parents bs=0x565506bc0c00 (file) processing itself bs=0x565505fbf660 (qcow2) processing parents bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 bs=0x565505e48030 (backup-top) enter bs=0x565505e48030 (backup-top) processing children bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) bs=0x565505fbf660 (qcow2) enter bs=0x565505fbf660 (qcow2) processing children bs=0x565505fbf660 (qcow2) processing parents bs=0x565505fbf660 (qcow2) processing itself bs=0x565505e48030 (backup-top) processing parents bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 bs=0x565505e48030 (backup-top) processing itself bs=0x565505fbf660 (qcow2) processing itself So it seems this is happening: backup-top (5e48030) <---------| (5) | | | | | (6) ------------> qcow2 (5fbf660) | ^ | | (3) | | (4) |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) | |-> (2) file (5e52060) backup-top (5e48030), the BDS that was passed as argument in the first bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) is processing its parents, and the latter is also re-entered when the first one starts processing its children again. Sergio.
Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: > On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > > While processing the parents of a BDS, one of the parents may process > > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > > as they might attempt to work on both the old and the new AIO > > > > > contexts. > > > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > > child BDS pointer while iterating over the children. > > > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > > > I know, it's effective but quite ugly... > > > > > > > What is the specific scenario where you saw this breaking? Did you have > > > > multiple BdrvChild connections between two nodes so that we would go to > > > > the parent node through one and then come back to the child node through > > > > the other? > > > > > > I don't think this is a corner case. If the graph is walked top->down, > > > there's no problem since children are added to the ignore list before > > > getting processed, and siblings don't process each other. But, if the > > > graph is walked bottom->up, a BDS will start processing its parents > > > without adding itself to the ignore list, so there's nothing > > > preventing them from processing it again. > > > > I don't understand. child is added to ignore before calling the parent > > callback on it, so how can we come back through the same BdrvChild? > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > if (g_slist_find(*ignore, child)) { > > continue; > > } > > assert(child->klass->set_aio_ctx); > > *ignore = g_slist_prepend(*ignore, child); > > child->klass->set_aio_ctx(child, new_context, ignore); > > } > > Perhaps I'm missing something, but the way I understand it, that loop > is adding the BdrvChild pointer of each of its parents, but not the > BdrvChild pointer of the BDS that was passed as an argument to > b_s_a_c_i. Generally, the caller has already done that. In the theoretical case that it was the outermost call in the recursion and it hasn't (I couldn't find any such case), I think we should still call the callback for the passed BdrvChild like we currently do. > > You didn't dump the BdrvChild here. I think that would add some > > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > > bs->drv->format_name for each node to make the scenario less abstract? > > I've generated another trace with more data: > > bs=0x565505e48030 (backup-top) enter > bs=0x565505e48030 (backup-top) processing children > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) > bs=0x565505e5d420 (qcow2) enter > bs=0x565505e5d420 (qcow2) processing children > bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) > bs=0x565505e52060 (file) enter > bs=0x565505e52060 (file) processing children > bs=0x565505e52060 (file) processing parents > bs=0x565505e52060 (file) processing itself > bs=0x565505e5d420 (qcow2) processing parents > bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 > bs=0x565505fbf660 (qcow2) enter > bs=0x565505fbf660 (qcow2) processing children > bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) > bs=0x565506bc0c00 (file) enter > bs=0x565506bc0c00 (file) processing children > bs=0x565506bc0c00 (file) processing parents > bs=0x565506bc0c00 (file) processing itself > bs=0x565505fbf660 (qcow2) processing parents > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 > bs=0x565505e48030 (backup-top) enter > bs=0x565505e48030 (backup-top) processing children > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) > bs=0x565505fbf660 (qcow2) enter > bs=0x565505fbf660 (qcow2) processing children > bs=0x565505fbf660 (qcow2) processing parents > bs=0x565505fbf660 (qcow2) processing itself > bs=0x565505e48030 (backup-top) processing parents > bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 > bs=0x565505e48030 (backup-top) processing itself > bs=0x565505fbf660 (qcow2) processing itself Hm, is this complete? Is see no "processing itself" for bs=0x565505e5d420. Or is this because it crashed before getting there? Anyway, trying to reconstruct the block graph with BdrvChild pointers annotated at the edges: BlockBackend | v backup-top ------------------------+ | | | | +-----------------------+ | | 0x5655068b8510 | | 0x565505e3c450 | | | | 0x565505e42090 | | v | | qcow2 ---------------------+ | | | | | | | 0x565505e52060 | | | ??? [1] | | | | | v 0x5655066a34d0 | | | | 0x565505fc7aa0 file v v v v qcow2 (backing) | | 0x565505e41d20 v file [1] This seems to be a BdrvChild with a non-BDS parent. Probably a BdrvChild directly owned by the backup job. > So it seems this is happening: > > backup-top (5e48030) <---------| (5) > | | | > | | (6) ------------> qcow2 (5fbf660) > | ^ | > | (3) | | (4) > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > | > |-> (2) file (5e52060) > > backup-top (5e48030), the BDS that was passed as argument in the first > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > is processing its parents, and the latter is also re-entered when the > first one starts processing its children again. Yes, but look at the BdrvChild pointers, it is through different edges that we come back to the same node. No BdrvChild is used twice. If backup-top had added all of its children to the ignore list before calling into the overlay qcow2, the backing qcow2 wouldn't eventually have called back into backup-top. Kevin
On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: > > On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > > > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > > > While processing the parents of a BDS, one of the parents may process > > > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > > > as they might attempt to work on both the old and the new AIO > > > > > > contexts. > > > > > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > > > child BDS pointer while iterating over the children. > > > > > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > > > > > I know, it's effective but quite ugly... > > > > > > > > > What is the specific scenario where you saw this breaking? Did you have > > > > > multiple BdrvChild connections between two nodes so that we would go to > > > > > the parent node through one and then come back to the child node through > > > > > the other? > > > > > > > > I don't think this is a corner case. If the graph is walked top->down, > > > > there's no problem since children are added to the ignore list before > > > > getting processed, and siblings don't process each other. But, if the > > > > graph is walked bottom->up, a BDS will start processing its parents > > > > without adding itself to the ignore list, so there's nothing > > > > preventing them from processing it again. > > > > > > I don't understand. child is added to ignore before calling the parent > > > callback on it, so how can we come back through the same BdrvChild? > > > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > if (g_slist_find(*ignore, child)) { > > > continue; > > > } > > > assert(child->klass->set_aio_ctx); > > > *ignore = g_slist_prepend(*ignore, child); > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > } > > > > Perhaps I'm missing something, but the way I understand it, that loop > > is adding the BdrvChild pointer of each of its parents, but not the > > BdrvChild pointer of the BDS that was passed as an argument to > > b_s_a_c_i. > > Generally, the caller has already done that. > > In the theoretical case that it was the outermost call in the recursion > and it hasn't (I couldn't find any such case), I think we should still > call the callback for the passed BdrvChild like we currently do. > > > > You didn't dump the BdrvChild here. I think that would add some > > > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > > > bs->drv->format_name for each node to make the scenario less abstract? > > > > I've generated another trace with more data: > > > > bs=0x565505e48030 (backup-top) enter > > bs=0x565505e48030 (backup-top) processing children > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) > > bs=0x565505e5d420 (qcow2) enter > > bs=0x565505e5d420 (qcow2) processing children > > bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) > > bs=0x565505e52060 (file) enter > > bs=0x565505e52060 (file) processing children > > bs=0x565505e52060 (file) processing parents > > bs=0x565505e52060 (file) processing itself > > bs=0x565505e5d420 (qcow2) processing parents > > bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 > > bs=0x565505fbf660 (qcow2) enter > > bs=0x565505fbf660 (qcow2) processing children > > bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) > > bs=0x565506bc0c00 (file) enter > > bs=0x565506bc0c00 (file) processing children > > bs=0x565506bc0c00 (file) processing parents > > bs=0x565506bc0c00 (file) processing itself > > bs=0x565505fbf660 (qcow2) processing parents > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 > > bs=0x565505e48030 (backup-top) enter > > bs=0x565505e48030 (backup-top) processing children > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) > > bs=0x565505fbf660 (qcow2) enter > > bs=0x565505fbf660 (qcow2) processing children > > bs=0x565505fbf660 (qcow2) processing parents > > bs=0x565505fbf660 (qcow2) processing itself > > bs=0x565505e48030 (backup-top) processing parents > > bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 > > bs=0x565505e48030 (backup-top) processing itself > > bs=0x565505fbf660 (qcow2) processing itself > > Hm, is this complete? Is see no "processing itself" for > bs=0x565505e5d420. Or is this because it crashed before getting there? Yes, it crashes there. I forgot to mention that, sorry. > Anyway, trying to reconstruct the block graph with BdrvChild pointers > annotated at the edges: > > BlockBackend > | > v > backup-top ------------------------+ > | | | > | +-----------------------+ | > | 0x5655068b8510 | | 0x565505e3c450 > | | | > | 0x565505e42090 | | > v | | > qcow2 ---------------------+ | | > | | | | > | 0x565505e52060 | | | ??? [1] > | | | | | > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > file v v v v > qcow2 (backing) > | > | 0x565505e41d20 > v > file > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > BdrvChild directly owned by the backup job. > > > So it seems this is happening: > > > > backup-top (5e48030) <---------| (5) > > | | | > > | | (6) ------------> qcow2 (5fbf660) > > | ^ | > > | (3) | | (4) > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > | > > |-> (2) file (5e52060) > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > is processing its parents, and the latter is also re-entered when the > > first one starts processing its children again. > > Yes, but look at the BdrvChild pointers, it is through different edges > that we come back to the same node. No BdrvChild is used twice. > > If backup-top had added all of its children to the ignore list before > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > have called back into backup-top. I've tested a patch that first adds every child to the ignore list, and then processes those that weren't there before, as you suggested on a previous email. With that, the offending qcow2 is not re-entered, so we avoid the crash, but backup-top is still entered twice: bs=0x560db0e3b030 (backup-top) enter bs=0x560db0e3b030 (backup-top) processing children bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) bs=0x560db0fb2660 (qcow2) enter bs=0x560db0fb2660 (qcow2) processing children bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) bs=0x560db1bb3c00 (file) enter bs=0x560db1bb3c00 (file) processing children bs=0x560db1bb3c00 (file) processing parents bs=0x560db1bb3c00 (file) processing itself bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) bs=0x560db0e50420 (qcow2) enter bs=0x560db0e50420 (qcow2) processing children bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) bs=0x560db0e45060 (file) enter bs=0x560db0e45060 (file) processing children bs=0x560db0e45060 (file) processing parents bs=0x560db0e45060 (file) processing itself bs=0x560db0e50420 (qcow2) processing parents bs=0x560db0e50420 (qcow2) processing itself bs=0x560db0fb2660 (qcow2) processing parents bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 bs=0x560db0e3b030 (backup-top) enter bs=0x560db0e3b030 (backup-top) processing children bs=0x560db0e3b030 (backup-top) processing parents bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 bs=0x560db0e3b030 (backup-top) processing itself bs=0x560db0fb2660 (qcow2) processing itself bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) bs=0x560db0e50420 (qcow2) enter bs=0x560db0e3b030 (backup-top) processing parents bs=0x560db0e3b030 (backup-top) processing itself I see that "blk_do_set_aio_context()" passes "blk->root" to "bdrv_child_try_set_aio_context()" so it's already in the ignore list, so I'm not sure what's happening here. Is backup-top is referenced from two different BdrvChild or is "blk->root" not pointing to backup-top's BDS? Thanks, Sergio.
Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > > Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: > > > On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > > > > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > > > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > > > > While processing the parents of a BDS, one of the parents may process > > > > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > > > > as they might attempt to work on both the old and the new AIO > > > > > > > contexts. > > > > > > > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > > > > child BDS pointer while iterating over the children. > > > > > > > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > > > > > > > I know, it's effective but quite ugly... > > > > > > > > > > > What is the specific scenario where you saw this breaking? Did you have > > > > > > multiple BdrvChild connections between two nodes so that we would go to > > > > > > the parent node through one and then come back to the child node through > > > > > > the other? > > > > > > > > > > I don't think this is a corner case. If the graph is walked top->down, > > > > > there's no problem since children are added to the ignore list before > > > > > getting processed, and siblings don't process each other. But, if the > > > > > graph is walked bottom->up, a BDS will start processing its parents > > > > > without adding itself to the ignore list, so there's nothing > > > > > preventing them from processing it again. > > > > > > > > I don't understand. child is added to ignore before calling the parent > > > > callback on it, so how can we come back through the same BdrvChild? > > > > > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > > if (g_slist_find(*ignore, child)) { > > > > continue; > > > > } > > > > assert(child->klass->set_aio_ctx); > > > > *ignore = g_slist_prepend(*ignore, child); > > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > > } > > > > > > Perhaps I'm missing something, but the way I understand it, that loop > > > is adding the BdrvChild pointer of each of its parents, but not the > > > BdrvChild pointer of the BDS that was passed as an argument to > > > b_s_a_c_i. > > > > Generally, the caller has already done that. > > > > In the theoretical case that it was the outermost call in the recursion > > and it hasn't (I couldn't find any such case), I think we should still > > call the callback for the passed BdrvChild like we currently do. > > > > > > You didn't dump the BdrvChild here. I think that would add some > > > > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > > > > bs->drv->format_name for each node to make the scenario less abstract? > > > > > > I've generated another trace with more data: > > > > > > bs=0x565505e48030 (backup-top) enter > > > bs=0x565505e48030 (backup-top) processing children > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) > > > bs=0x565505e5d420 (qcow2) enter > > > bs=0x565505e5d420 (qcow2) processing children > > > bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) > > > bs=0x565505e52060 (file) enter > > > bs=0x565505e52060 (file) processing children > > > bs=0x565505e52060 (file) processing parents > > > bs=0x565505e52060 (file) processing itself > > > bs=0x565505e5d420 (qcow2) processing parents > > > bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 > > > bs=0x565505fbf660 (qcow2) enter > > > bs=0x565505fbf660 (qcow2) processing children > > > bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) > > > bs=0x565506bc0c00 (file) enter > > > bs=0x565506bc0c00 (file) processing children > > > bs=0x565506bc0c00 (file) processing parents > > > bs=0x565506bc0c00 (file) processing itself > > > bs=0x565505fbf660 (qcow2) processing parents > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 > > > bs=0x565505e48030 (backup-top) enter > > > bs=0x565505e48030 (backup-top) processing children > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) > > > bs=0x565505fbf660 (qcow2) enter > > > bs=0x565505fbf660 (qcow2) processing children > > > bs=0x565505fbf660 (qcow2) processing parents > > > bs=0x565505fbf660 (qcow2) processing itself > > > bs=0x565505e48030 (backup-top) processing parents > > > bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 > > > bs=0x565505e48030 (backup-top) processing itself > > > bs=0x565505fbf660 (qcow2) processing itself > > > > Hm, is this complete? Is see no "processing itself" for > > bs=0x565505e5d420. Or is this because it crashed before getting there? > > Yes, it crashes there. I forgot to mention that, sorry. > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers > > annotated at the edges: > > > > BlockBackend > > | > > v > > backup-top ------------------------+ > > | | | > > | +-----------------------+ | > > | 0x5655068b8510 | | 0x565505e3c450 > > | | | > > | 0x565505e42090 | | > > v | | > > qcow2 ---------------------+ | | > > | | | | > > | 0x565505e52060 | | | ??? [1] > > | | | | | > > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > > file v v v v > > qcow2 (backing) > > | > > | 0x565505e41d20 > > v > > file > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > > BdrvChild directly owned by the backup job. > > > > > So it seems this is happening: > > > > > > backup-top (5e48030) <---------| (5) > > > | | | > > > | | (6) ------------> qcow2 (5fbf660) > > > | ^ | > > > | (3) | | (4) > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > > | > > > |-> (2) file (5e52060) > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > > is processing its parents, and the latter is also re-entered when the > > > first one starts processing its children again. > > > > Yes, but look at the BdrvChild pointers, it is through different edges > > that we come back to the same node. No BdrvChild is used twice. > > > > If backup-top had added all of its children to the ignore list before > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > > have called back into backup-top. > > I've tested a patch that first adds every child to the ignore list, > and then processes those that weren't there before, as you suggested > on a previous email. With that, the offending qcow2 is not re-entered, > so we avoid the crash, but backup-top is still entered twice: I think we also need to every parent to the ignore list before calling callbacks, though it doesn't look like this is the problem you're currently seeing. > bs=0x560db0e3b030 (backup-top) enter > bs=0x560db0e3b030 (backup-top) processing children > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) > bs=0x560db0fb2660 (qcow2) enter > bs=0x560db0fb2660 (qcow2) processing children > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) > bs=0x560db1bb3c00 (file) enter > bs=0x560db1bb3c00 (file) processing children > bs=0x560db1bb3c00 (file) processing parents > bs=0x560db1bb3c00 (file) processing itself > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) > bs=0x560db0e50420 (qcow2) enter > bs=0x560db0e50420 (qcow2) processing children > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) > bs=0x560db0e45060 (file) enter > bs=0x560db0e45060 (file) processing children > bs=0x560db0e45060 (file) processing parents > bs=0x560db0e45060 (file) processing itself > bs=0x560db0e50420 (qcow2) processing parents > bs=0x560db0e50420 (qcow2) processing itself > bs=0x560db0fb2660 (qcow2) processing parents > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 > bs=0x560db0e3b030 (backup-top) enter > bs=0x560db0e3b030 (backup-top) processing children > bs=0x560db0e3b030 (backup-top) processing parents > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 > bs=0x560db0e3b030 (backup-top) processing itself > bs=0x560db0fb2660 (qcow2) processing itself > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) > bs=0x560db0e50420 (qcow2) enter > bs=0x560db0e3b030 (backup-top) processing parents > bs=0x560db0e3b030 (backup-top) processing itself > > I see that "blk_do_set_aio_context()" passes "blk->root" to > "bdrv_child_try_set_aio_context()" so it's already in the ignore list, > so I'm not sure what's happening here. Is backup-top is referenced > from two different BdrvChild or is "blk->root" not pointing to > backup-top's BDS? The second time that backup-top is entered, it is not as the BDS of blk->root, but as the parent node of the overlay qcow2. Which is interesting, because last time it was still the backing qcow2, so the change did have _some_ effect. The part that I don't understand is why you still get the line with child=0x560db1b14a20, because when you add all children to the ignore list first, that should have been put into the ignore list as one of the first things in the whole process (when backup-top was first entered). Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, but isn't actually present in backup-top's bs->children? Kevin
On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: > Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: > > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > > > Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: > > > > On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > > > > > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > > > > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > > > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > > > > > While processing the parents of a BDS, one of the parents may process > > > > > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > > > > > as they might attempt to work on both the old and the new AIO > > > > > > > > contexts. > > > > > > > > > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > > > > > child BDS pointer while iterating over the children. > > > > > > > > > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > > > > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > > > > > > > > > I know, it's effective but quite ugly... > > > > > > > > > > > > > What is the specific scenario where you saw this breaking? Did you have > > > > > > > multiple BdrvChild connections between two nodes so that we would go to > > > > > > > the parent node through one and then come back to the child node through > > > > > > > the other? > > > > > > > > > > > > I don't think this is a corner case. If the graph is walked top->down, > > > > > > there's no problem since children are added to the ignore list before > > > > > > getting processed, and siblings don't process each other. But, if the > > > > > > graph is walked bottom->up, a BDS will start processing its parents > > > > > > without adding itself to the ignore list, so there's nothing > > > > > > preventing them from processing it again. > > > > > > > > > > I don't understand. child is added to ignore before calling the parent > > > > > callback on it, so how can we come back through the same BdrvChild? > > > > > > > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > > > if (g_slist_find(*ignore, child)) { > > > > > continue; > > > > > } > > > > > assert(child->klass->set_aio_ctx); > > > > > *ignore = g_slist_prepend(*ignore, child); > > > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > > > } > > > > > > > > Perhaps I'm missing something, but the way I understand it, that loop > > > > is adding the BdrvChild pointer of each of its parents, but not the > > > > BdrvChild pointer of the BDS that was passed as an argument to > > > > b_s_a_c_i. > > > > > > Generally, the caller has already done that. > > > > > > In the theoretical case that it was the outermost call in the recursion > > > and it hasn't (I couldn't find any such case), I think we should still > > > call the callback for the passed BdrvChild like we currently do. > > > > > > > > You didn't dump the BdrvChild here. I think that would add some > > > > > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > > > > > bs->drv->format_name for each node to make the scenario less abstract? > > > > > > > > I've generated another trace with more data: > > > > > > > > bs=0x565505e48030 (backup-top) enter > > > > bs=0x565505e48030 (backup-top) processing children > > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) > > > > bs=0x565505e5d420 (qcow2) enter > > > > bs=0x565505e5d420 (qcow2) processing children > > > > bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) > > > > bs=0x565505e52060 (file) enter > > > > bs=0x565505e52060 (file) processing children > > > > bs=0x565505e52060 (file) processing parents > > > > bs=0x565505e52060 (file) processing itself > > > > bs=0x565505e5d420 (qcow2) processing parents > > > > bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 > > > > bs=0x565505fbf660 (qcow2) enter > > > > bs=0x565505fbf660 (qcow2) processing children > > > > bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) > > > > bs=0x565506bc0c00 (file) enter > > > > bs=0x565506bc0c00 (file) processing children > > > > bs=0x565506bc0c00 (file) processing parents > > > > bs=0x565506bc0c00 (file) processing itself > > > > bs=0x565505fbf660 (qcow2) processing parents > > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 > > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 > > > > bs=0x565505e48030 (backup-top) enter > > > > bs=0x565505e48030 (backup-top) processing children > > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) > > > > bs=0x565505fbf660 (qcow2) enter > > > > bs=0x565505fbf660 (qcow2) processing children > > > > bs=0x565505fbf660 (qcow2) processing parents > > > > bs=0x565505fbf660 (qcow2) processing itself > > > > bs=0x565505e48030 (backup-top) processing parents > > > > bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 > > > > bs=0x565505e48030 (backup-top) processing itself > > > > bs=0x565505fbf660 (qcow2) processing itself > > > > > > Hm, is this complete? Is see no "processing itself" for > > > bs=0x565505e5d420. Or is this because it crashed before getting there? > > > > Yes, it crashes there. I forgot to mention that, sorry. > > > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers > > > annotated at the edges: > > > > > > BlockBackend > > > | > > > v > > > backup-top ------------------------+ > > > | | | > > > | +-----------------------+ | > > > | 0x5655068b8510 | | 0x565505e3c450 > > > | | | > > > | 0x565505e42090 | | > > > v | | > > > qcow2 ---------------------+ | | > > > | | | | > > > | 0x565505e52060 | | | ??? [1] > > > | | | | | > > > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > > > file v v v v > > > qcow2 (backing) > > > | > > > | 0x565505e41d20 > > > v > > > file > > > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > > > BdrvChild directly owned by the backup job. > > > > > > > So it seems this is happening: > > > > > > > > backup-top (5e48030) <---------| (5) > > > > | | | > > > > | | (6) ------------> qcow2 (5fbf660) > > > > | ^ | > > > > | (3) | | (4) > > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > > > | > > > > |-> (2) file (5e52060) > > > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > > > is processing its parents, and the latter is also re-entered when the > > > > first one starts processing its children again. > > > > > > Yes, but look at the BdrvChild pointers, it is through different edges > > > that we come back to the same node. No BdrvChild is used twice. > > > > > > If backup-top had added all of its children to the ignore list before > > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > > > have called back into backup-top. > > > > I've tested a patch that first adds every child to the ignore list, > > and then processes those that weren't there before, as you suggested > > on a previous email. With that, the offending qcow2 is not re-entered, > > so we avoid the crash, but backup-top is still entered twice: > > I think we also need to every parent to the ignore list before calling > callbacks, though it doesn't look like this is the problem you're > currently seeing. I agree. > > bs=0x560db0e3b030 (backup-top) enter > > bs=0x560db0e3b030 (backup-top) processing children > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) > > bs=0x560db0fb2660 (qcow2) enter > > bs=0x560db0fb2660 (qcow2) processing children > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) > > bs=0x560db1bb3c00 (file) enter > > bs=0x560db1bb3c00 (file) processing children > > bs=0x560db1bb3c00 (file) processing parents > > bs=0x560db1bb3c00 (file) processing itself > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) > > bs=0x560db0e50420 (qcow2) enter > > bs=0x560db0e50420 (qcow2) processing children > > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) > > bs=0x560db0e45060 (file) enter > > bs=0x560db0e45060 (file) processing children > > bs=0x560db0e45060 (file) processing parents > > bs=0x560db0e45060 (file) processing itself > > bs=0x560db0e50420 (qcow2) processing parents > > bs=0x560db0e50420 (qcow2) processing itself > > bs=0x560db0fb2660 (qcow2) processing parents > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 > > bs=0x560db0e3b030 (backup-top) enter > > bs=0x560db0e3b030 (backup-top) processing children > > bs=0x560db0e3b030 (backup-top) processing parents > > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 > > bs=0x560db0e3b030 (backup-top) processing itself > > bs=0x560db0fb2660 (qcow2) processing itself > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) > > bs=0x560db0e50420 (qcow2) enter > > bs=0x560db0e3b030 (backup-top) processing parents > > bs=0x560db0e3b030 (backup-top) processing itself > > > > I see that "blk_do_set_aio_context()" passes "blk->root" to > > "bdrv_child_try_set_aio_context()" so it's already in the ignore list, > > so I'm not sure what's happening here. Is backup-top is referenced > > from two different BdrvChild or is "blk->root" not pointing to > > backup-top's BDS? > > The second time that backup-top is entered, it is not as the BDS of > blk->root, but as the parent node of the overlay qcow2. Which is > interesting, because last time it was still the backing qcow2, so the > change did have _some_ effect. > > The part that I don't understand is why you still get the line with > child=0x560db1b14a20, because when you add all children to the ignore > list first, that should have been put into the ignore list as one of the > first things in the whole process (when backup-top was first entered). > > Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, > but isn't actually present in backup-top's bs->children? Exactly, that line corresponds to this chunk of code: <---- begin ----> QLIST_FOREACH(child, &bs->parents, next_parent) { if (g_slist_find(*ignore, child)) { continue; } assert(child->klass->set_aio_ctx); *ignore = g_slist_prepend(*ignore, child); fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); child->klass->set_aio_ctx(child, new_context, ignore); } <---- end ----> Do you think it's safe to re-enter backup-top, or should we look for a way to avoid this? Thanks, Sergio.
Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: > On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: > > Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: > > > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > > > > Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: > > > > > On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: > > > > > > Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: > > > > > > > On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: > > > > > > > > Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: > > > > > > > > > While processing the parents of a BDS, one of the parents may process > > > > > > > > > the child that's doing the tail recursion, which leads to a BDS being > > > > > > > > > processed twice. This is especially problematic for the aio_notifiers, > > > > > > > > > as they might attempt to work on both the old and the new AIO > > > > > > > > > contexts. > > > > > > > > > > > > > > > > > > To avoid this, add the BDS pointer to the ignore list, and check the > > > > > > > > > child BDS pointer while iterating over the children. > > > > > > > > > > > > > > > > > > Signed-off-by: Sergio Lopez <slp@redhat.com> > > > > > > > > > > > > > > > > Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ > > > > > > > > > > > > > > I know, it's effective but quite ugly... > > > > > > > > > > > > > > > What is the specific scenario where you saw this breaking? Did you have > > > > > > > > multiple BdrvChild connections between two nodes so that we would go to > > > > > > > > the parent node through one and then come back to the child node through > > > > > > > > the other? > > > > > > > > > > > > > > I don't think this is a corner case. If the graph is walked top->down, > > > > > > > there's no problem since children are added to the ignore list before > > > > > > > getting processed, and siblings don't process each other. But, if the > > > > > > > graph is walked bottom->up, a BDS will start processing its parents > > > > > > > without adding itself to the ignore list, so there's nothing > > > > > > > preventing them from processing it again. > > > > > > > > > > > > I don't understand. child is added to ignore before calling the parent > > > > > > callback on it, so how can we come back through the same BdrvChild? > > > > > > > > > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > > > > if (g_slist_find(*ignore, child)) { > > > > > > continue; > > > > > > } > > > > > > assert(child->klass->set_aio_ctx); > > > > > > *ignore = g_slist_prepend(*ignore, child); > > > > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > > > > } > > > > > > > > > > Perhaps I'm missing something, but the way I understand it, that loop > > > > > is adding the BdrvChild pointer of each of its parents, but not the > > > > > BdrvChild pointer of the BDS that was passed as an argument to > > > > > b_s_a_c_i. > > > > > > > > Generally, the caller has already done that. > > > > > > > > In the theoretical case that it was the outermost call in the recursion > > > > and it hasn't (I couldn't find any such case), I think we should still > > > > call the callback for the passed BdrvChild like we currently do. > > > > > > > > > > You didn't dump the BdrvChild here. I think that would add some > > > > > > information on why we re-entered 0x555ee2fbf660. Maybe you can also add > > > > > > bs->drv->format_name for each node to make the scenario less abstract? > > > > > > > > > > I've generated another trace with more data: > > > > > > > > > > bs=0x565505e48030 (backup-top) enter > > > > > bs=0x565505e48030 (backup-top) processing children > > > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) > > > > > bs=0x565505e5d420 (qcow2) enter > > > > > bs=0x565505e5d420 (qcow2) processing children > > > > > bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) > > > > > bs=0x565505e52060 (file) enter > > > > > bs=0x565505e52060 (file) processing children > > > > > bs=0x565505e52060 (file) processing parents > > > > > bs=0x565505e52060 (file) processing itself > > > > > bs=0x565505e5d420 (qcow2) processing parents > > > > > bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 > > > > > bs=0x565505fbf660 (qcow2) enter > > > > > bs=0x565505fbf660 (qcow2) processing children > > > > > bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) > > > > > bs=0x565506bc0c00 (file) enter > > > > > bs=0x565506bc0c00 (file) processing children > > > > > bs=0x565506bc0c00 (file) processing parents > > > > > bs=0x565506bc0c00 (file) processing itself > > > > > bs=0x565505fbf660 (qcow2) processing parents > > > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 > > > > > bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 > > > > > bs=0x565505e48030 (backup-top) enter > > > > > bs=0x565505e48030 (backup-top) processing children > > > > > bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) > > > > > bs=0x565505fbf660 (qcow2) enter > > > > > bs=0x565505fbf660 (qcow2) processing children > > > > > bs=0x565505fbf660 (qcow2) processing parents > > > > > bs=0x565505fbf660 (qcow2) processing itself > > > > > bs=0x565505e48030 (backup-top) processing parents > > > > > bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 > > > > > bs=0x565505e48030 (backup-top) processing itself > > > > > bs=0x565505fbf660 (qcow2) processing itself > > > > > > > > Hm, is this complete? Is see no "processing itself" for > > > > bs=0x565505e5d420. Or is this because it crashed before getting there? > > > > > > Yes, it crashes there. I forgot to mention that, sorry. > > > > > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers > > > > annotated at the edges: > > > > > > > > BlockBackend > > > > | > > > > v > > > > backup-top ------------------------+ > > > > | | | > > > > | +-----------------------+ | > > > > | 0x5655068b8510 | | 0x565505e3c450 > > > > | | | > > > > | 0x565505e42090 | | > > > > v | | > > > > qcow2 ---------------------+ | | > > > > | | | | > > > > | 0x565505e52060 | | | ??? [1] > > > > | | | | | > > > > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > > > > file v v v v > > > > qcow2 (backing) > > > > | > > > > | 0x565505e41d20 > > > > v > > > > file > > > > > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > > > > BdrvChild directly owned by the backup job. > > > > > > > > > So it seems this is happening: > > > > > > > > > > backup-top (5e48030) <---------| (5) > > > > > | | | > > > > > | | (6) ------------> qcow2 (5fbf660) > > > > > | ^ | > > > > > | (3) | | (4) > > > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > > > > | > > > > > |-> (2) file (5e52060) > > > > > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > > > > is processing its parents, and the latter is also re-entered when the > > > > > first one starts processing its children again. > > > > > > > > Yes, but look at the BdrvChild pointers, it is through different edges > > > > that we come back to the same node. No BdrvChild is used twice. > > > > > > > > If backup-top had added all of its children to the ignore list before > > > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > > > > have called back into backup-top. > > > > > > I've tested a patch that first adds every child to the ignore list, > > > and then processes those that weren't there before, as you suggested > > > on a previous email. With that, the offending qcow2 is not re-entered, > > > so we avoid the crash, but backup-top is still entered twice: > > > > I think we also need to every parent to the ignore list before calling > > callbacks, though it doesn't look like this is the problem you're > > currently seeing. > > I agree. > > > > bs=0x560db0e3b030 (backup-top) enter > > > bs=0x560db0e3b030 (backup-top) processing children > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) > > > bs=0x560db0fb2660 (qcow2) enter > > > bs=0x560db0fb2660 (qcow2) processing children > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) > > > bs=0x560db1bb3c00 (file) enter > > > bs=0x560db1bb3c00 (file) processing children > > > bs=0x560db1bb3c00 (file) processing parents > > > bs=0x560db1bb3c00 (file) processing itself > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) > > > bs=0x560db0e50420 (qcow2) enter > > > bs=0x560db0e50420 (qcow2) processing children > > > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) > > > bs=0x560db0e45060 (file) enter > > > bs=0x560db0e45060 (file) processing children > > > bs=0x560db0e45060 (file) processing parents > > > bs=0x560db0e45060 (file) processing itself > > > bs=0x560db0e50420 (qcow2) processing parents > > > bs=0x560db0e50420 (qcow2) processing itself > > > bs=0x560db0fb2660 (qcow2) processing parents > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 > > > bs=0x560db0e3b030 (backup-top) enter > > > bs=0x560db0e3b030 (backup-top) processing children > > > bs=0x560db0e3b030 (backup-top) processing parents > > > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 > > > bs=0x560db0e3b030 (backup-top) processing itself > > > bs=0x560db0fb2660 (qcow2) processing itself > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) > > > bs=0x560db0e50420 (qcow2) enter > > > bs=0x560db0e3b030 (backup-top) processing parents > > > bs=0x560db0e3b030 (backup-top) processing itself > > > > > > I see that "blk_do_set_aio_context()" passes "blk->root" to > > > "bdrv_child_try_set_aio_context()" so it's already in the ignore list, > > > so I'm not sure what's happening here. Is backup-top is referenced > > > from two different BdrvChild or is "blk->root" not pointing to > > > backup-top's BDS? > > > > The second time that backup-top is entered, it is not as the BDS of > > blk->root, but as the parent node of the overlay qcow2. Which is > > interesting, because last time it was still the backing qcow2, so the > > change did have _some_ effect. > > > > The part that I don't understand is why you still get the line with > > child=0x560db1b14a20, because when you add all children to the ignore > > list first, that should have been put into the ignore list as one of the > > first things in the whole process (when backup-top was first entered). > > > > Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, > > but isn't actually present in backup-top's bs->children? > > Exactly, that line corresponds to this chunk of code: > > <---- begin ----> > QLIST_FOREACH(child, &bs->parents, next_parent) { > if (g_slist_find(*ignore, child)) { > continue; > } > assert(child->klass->set_aio_ctx); > *ignore = g_slist_prepend(*ignore, child); > fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); > child->klass->set_aio_ctx(child, new_context, ignore); > } > <---- end ----> > > Do you think it's safe to re-enter backup-top, or should we look for a > way to avoid this? I think it should be avoided, but I don't understand why putting all children of backup-top into the ignore list doesn't already avoid it. If backup-top is in the parents list of qcow2, then qcow2 should be in the children list of backup-top and therefore the BdrvChild should already be in the ignore list. The only way I can explain this is that backup-top and qcow2 have different ideas about which BdrvChild objects exist that connect them. Or that the graph changes between both places, but I don't see how that could happen in bdrv_set_aio_context_ignore(). Kevin
17.12.2020 13:58, Kevin Wolf wrote: > Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: >> On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: >>> Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: >>>> On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: >>>>> Am 15.12.2020 um 18:23 hat Sergio Lopez geschrieben: >>>>>> On Tue, Dec 15, 2020 at 04:01:19PM +0100, Kevin Wolf wrote: >>>>>>> Am 15.12.2020 um 14:15 hat Sergio Lopez geschrieben: >>>>>>>> On Tue, Dec 15, 2020 at 01:12:33PM +0100, Kevin Wolf wrote: >>>>>>>>> Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben: >>>>>>>>>> While processing the parents of a BDS, one of the parents may process >>>>>>>>>> the child that's doing the tail recursion, which leads to a BDS being >>>>>>>>>> processed twice. This is especially problematic for the aio_notifiers, >>>>>>>>>> as they might attempt to work on both the old and the new AIO >>>>>>>>>> contexts. >>>>>>>>>> >>>>>>>>>> To avoid this, add the BDS pointer to the ignore list, and check the >>>>>>>>>> child BDS pointer while iterating over the children. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Sergio Lopez <slp@redhat.com> >>>>>>>>> >>>>>>>>> Ugh, so we get a mixed list of BdrvChild and BlockDriverState? :-/ >>>>>>>> >>>>>>>> I know, it's effective but quite ugly... >>>>>>>> >>>>>>>>> What is the specific scenario where you saw this breaking? Did you have >>>>>>>>> multiple BdrvChild connections between two nodes so that we would go to >>>>>>>>> the parent node through one and then come back to the child node through >>>>>>>>> the other? >>>>>>>> >>>>>>>> I don't think this is a corner case. If the graph is walked top->down, >>>>>>>> there's no problem since children are added to the ignore list before >>>>>>>> getting processed, and siblings don't process each other. But, if the >>>>>>>> graph is walked bottom->up, a BDS will start processing its parents >>>>>>>> without adding itself to the ignore list, so there's nothing >>>>>>>> preventing them from processing it again. >>>>>>> >>>>>>> I don't understand. child is added to ignore before calling the parent >>>>>>> callback on it, so how can we come back through the same BdrvChild? >>>>>>> >>>>>>> QLIST_FOREACH(child, &bs->parents, next_parent) { >>>>>>> if (g_slist_find(*ignore, child)) { >>>>>>> continue; >>>>>>> } >>>>>>> assert(child->klass->set_aio_ctx); >>>>>>> *ignore = g_slist_prepend(*ignore, child); >>>>>>> child->klass->set_aio_ctx(child, new_context, ignore); >>>>>>> } >>>>>> >>>>>> Perhaps I'm missing something, but the way I understand it, that loop >>>>>> is adding the BdrvChild pointer of each of its parents, but not the >>>>>> BdrvChild pointer of the BDS that was passed as an argument to >>>>>> b_s_a_c_i. >>>>> >>>>> Generally, the caller has already done that. >>>>> >>>>> In the theoretical case that it was the outermost call in the recursion >>>>> and it hasn't (I couldn't find any such case), I think we should still >>>>> call the callback for the passed BdrvChild like we currently do. >>>>> >>>>>>> You didn't dump the BdrvChild here. I think that would add some >>>>>>> information on why we re-entered 0x555ee2fbf660. Maybe you can also add >>>>>>> bs->drv->format_name for each node to make the scenario less abstract? >>>>>> >>>>>> I've generated another trace with more data: >>>>>> >>>>>> bs=0x565505e48030 (backup-top) enter >>>>>> bs=0x565505e48030 (backup-top) processing children >>>>>> bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e42090 (child->bs=0x565505e5d420) >>>>>> bs=0x565505e5d420 (qcow2) enter >>>>>> bs=0x565505e5d420 (qcow2) processing children >>>>>> bs=0x565505e5d420 (qcow2) calling bsaci child=0x565505e41ea0 (child->bs=0x565505e52060) >>>>>> bs=0x565505e52060 (file) enter >>>>>> bs=0x565505e52060 (file) processing children >>>>>> bs=0x565505e52060 (file) processing parents >>>>>> bs=0x565505e52060 (file) processing itself >>>>>> bs=0x565505e5d420 (qcow2) processing parents >>>>>> bs=0x565505e5d420 (qcow2) calling set_aio_ctx child=0x5655066a34d0 >>>>>> bs=0x565505fbf660 (qcow2) enter >>>>>> bs=0x565505fbf660 (qcow2) processing children >>>>>> bs=0x565505fbf660 (qcow2) calling bsaci child=0x565505e41d20 (child->bs=0x565506bc0c00) >>>>>> bs=0x565506bc0c00 (file) enter >>>>>> bs=0x565506bc0c00 (file) processing children >>>>>> bs=0x565506bc0c00 (file) processing parents >>>>>> bs=0x565506bc0c00 (file) processing itself >>>>>> bs=0x565505fbf660 (qcow2) processing parents >>>>>> bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x565505fc7aa0 >>>>>> bs=0x565505fbf660 (qcow2) calling set_aio_ctx child=0x5655068b8510 >>>>>> bs=0x565505e48030 (backup-top) enter >>>>>> bs=0x565505e48030 (backup-top) processing children >>>>>> bs=0x565505e48030 (backup-top) calling bsaci child=0x565505e3c450 (child->bs=0x565505fbf660) >>>>>> bs=0x565505fbf660 (qcow2) enter >>>>>> bs=0x565505fbf660 (qcow2) processing children >>>>>> bs=0x565505fbf660 (qcow2) processing parents >>>>>> bs=0x565505fbf660 (qcow2) processing itself >>>>>> bs=0x565505e48030 (backup-top) processing parents >>>>>> bs=0x565505e48030 (backup-top) calling set_aio_ctx child=0x565505e402d0 >>>>>> bs=0x565505e48030 (backup-top) processing itself >>>>>> bs=0x565505fbf660 (qcow2) processing itself >>>>> >>>>> Hm, is this complete? Is see no "processing itself" for >>>>> bs=0x565505e5d420. Or is this because it crashed before getting there? >>>> >>>> Yes, it crashes there. I forgot to mention that, sorry. >>>> >>>>> Anyway, trying to reconstruct the block graph with BdrvChild pointers >>>>> annotated at the edges: >>>>> >>>>> BlockBackend >>>>> | >>>>> v >>>>> backup-top ------------------------+ >>>>> | | | >>>>> | +-----------------------+ | >>>>> | 0x5655068b8510 | | 0x565505e3c450 >>>>> | | | >>>>> | 0x565505e42090 | | >>>>> v | | >>>>> qcow2 ---------------------+ | | >>>>> | | | | >>>>> | 0x565505e52060 | | | ??? [1] >>>>> | | | | | >>>>> v 0x5655066a34d0 | | | | 0x565505fc7aa0 >>>>> file v v v v >>>>> qcow2 (backing) >>>>> | >>>>> | 0x565505e41d20 >>>>> v >>>>> file >>>>> >>>>> [1] This seems to be a BdrvChild with a non-BDS parent. Probably a >>>>> BdrvChild directly owned by the backup job. >>>>> >>>>>> So it seems this is happening: >>>>>> >>>>>> backup-top (5e48030) <---------| (5) >>>>>> | | | >>>>>> | | (6) ------------> qcow2 (5fbf660) >>>>>> | ^ | >>>>>> | (3) | | (4) >>>>>> |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) >>>>>> | >>>>>> |-> (2) file (5e52060) >>>>>> >>>>>> backup-top (5e48030), the BDS that was passed as argument in the first >>>>>> bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) >>>>>> is processing its parents, and the latter is also re-entered when the >>>>>> first one starts processing its children again. >>>>> >>>>> Yes, but look at the BdrvChild pointers, it is through different edges >>>>> that we come back to the same node. No BdrvChild is used twice. >>>>> >>>>> If backup-top had added all of its children to the ignore list before >>>>> calling into the overlay qcow2, the backing qcow2 wouldn't eventually >>>>> have called back into backup-top. >>>> >>>> I've tested a patch that first adds every child to the ignore list, >>>> and then processes those that weren't there before, as you suggested >>>> on a previous email. With that, the offending qcow2 is not re-entered, >>>> so we avoid the crash, but backup-top is still entered twice: >>> >>> I think we also need to every parent to the ignore list before calling >>> callbacks, though it doesn't look like this is the problem you're >>> currently seeing. >> >> I agree. >> >>>> bs=0x560db0e3b030 (backup-top) enter >>>> bs=0x560db0e3b030 (backup-top) processing children >>>> bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) >>>> bs=0x560db0fb2660 (qcow2) enter >>>> bs=0x560db0fb2660 (qcow2) processing children >>>> bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) >>>> bs=0x560db1bb3c00 (file) enter >>>> bs=0x560db1bb3c00 (file) processing children >>>> bs=0x560db1bb3c00 (file) processing parents >>>> bs=0x560db1bb3c00 (file) processing itself >>>> bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) >>>> bs=0x560db0e50420 (qcow2) enter >>>> bs=0x560db0e50420 (qcow2) processing children >>>> bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) >>>> bs=0x560db0e45060 (file) enter >>>> bs=0x560db0e45060 (file) processing children >>>> bs=0x560db0e45060 (file) processing parents >>>> bs=0x560db0e45060 (file) processing itself >>>> bs=0x560db0e50420 (qcow2) processing parents >>>> bs=0x560db0e50420 (qcow2) processing itself >>>> bs=0x560db0fb2660 (qcow2) processing parents >>>> bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 >>>> bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 >>>> bs=0x560db0e3b030 (backup-top) enter >>>> bs=0x560db0e3b030 (backup-top) processing children >>>> bs=0x560db0e3b030 (backup-top) processing parents >>>> bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 >>>> bs=0x560db0e3b030 (backup-top) processing itself >>>> bs=0x560db0fb2660 (qcow2) processing itself >>>> bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) >>>> bs=0x560db0e50420 (qcow2) enter >>>> bs=0x560db0e3b030 (backup-top) processing parents >>>> bs=0x560db0e3b030 (backup-top) processing itself >>>> >>>> I see that "blk_do_set_aio_context()" passes "blk->root" to >>>> "bdrv_child_try_set_aio_context()" so it's already in the ignore list, >>>> so I'm not sure what's happening here. Is backup-top is referenced >>>> from two different BdrvChild or is "blk->root" not pointing to >>>> backup-top's BDS? >>> >>> The second time that backup-top is entered, it is not as the BDS of >>> blk->root, but as the parent node of the overlay qcow2. Which is >>> interesting, because last time it was still the backing qcow2, so the >>> change did have _some_ effect. >>> >>> The part that I don't understand is why you still get the line with >>> child=0x560db1b14a20, because when you add all children to the ignore >>> list first, that should have been put into the ignore list as one of the >>> first things in the whole process (when backup-top was first entered). >>> >>> Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, >>> but isn't actually present in backup-top's bs->children? >> >> Exactly, that line corresponds to this chunk of code: >> >> <---- begin ----> >> QLIST_FOREACH(child, &bs->parents, next_parent) { >> if (g_slist_find(*ignore, child)) { >> continue; >> } >> assert(child->klass->set_aio_ctx); >> *ignore = g_slist_prepend(*ignore, child); >> fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); >> child->klass->set_aio_ctx(child, new_context, ignore); >> } >> <---- end ----> >> >> Do you think it's safe to re-enter backup-top, or should we look for a >> way to avoid this? > > I think it should be avoided, but I don't understand why putting all > children of backup-top into the ignore list doesn't already avoid it. If > backup-top is in the parents list of qcow2, then qcow2 should be in the > children list of backup-top and therefore the BdrvChild should already > be in the ignore list. > > The only way I can explain this is that backup-top and qcow2 have > different ideas about which BdrvChild objects exist that connect them. > Or that the graph changes between both places, but I don't see how that > could happen in bdrv_set_aio_context_ignore(). > bdrv_set_aio_context_ignore() do bdrv_drained_begin().. As I reported recently, nothing prevents some job finish and do graph modification during some another drained section. It may be the case. If backup-top involved, I can suppose that graph modification is in backup_clean, when we remove the filter.. Who is making set_aio_context in the issue? I mean, what is the backtrace of bdrv_set_aio_context_ignore()?
Am 17.12.2020 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben: > 17.12.2020 13:58, Kevin Wolf wrote: > > Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: > > > On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: > > > > Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: > > > > > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > > > > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers > > > > > > annotated at the edges: > > > > > > > > > > > > BlockBackend > > > > > > | > > > > > > v > > > > > > backup-top ------------------------+ > > > > > > | | | > > > > > > | +-----------------------+ | > > > > > > | 0x5655068b8510 | | 0x565505e3c450 > > > > > > | | | > > > > > > | 0x565505e42090 | | > > > > > > v | | > > > > > > qcow2 ---------------------+ | | > > > > > > | | | | > > > > > > | 0x565505e52060 | | | ??? [1] > > > > > > | | | | | > > > > > > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > > > > > > file v v v v > > > > > > qcow2 (backing) > > > > > > | > > > > > > | 0x565505e41d20 > > > > > > v > > > > > > file > > > > > > > > > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > > > > > > BdrvChild directly owned by the backup job. > > > > > > > > > > > > > So it seems this is happening: > > > > > > > > > > > > > > backup-top (5e48030) <---------| (5) > > > > > > > | | | > > > > > > > | | (6) ------------> qcow2 (5fbf660) > > > > > > > | ^ | > > > > > > > | (3) | | (4) > > > > > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > > > > > > | > > > > > > > |-> (2) file (5e52060) > > > > > > > > > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > > > > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > > > > > > is processing its parents, and the latter is also re-entered when the > > > > > > > first one starts processing its children again. > > > > > > > > > > > > Yes, but look at the BdrvChild pointers, it is through different edges > > > > > > that we come back to the same node. No BdrvChild is used twice. > > > > > > > > > > > > If backup-top had added all of its children to the ignore list before > > > > > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > > > > > > have called back into backup-top. > > > > > > > > > > I've tested a patch that first adds every child to the ignore list, > > > > > and then processes those that weren't there before, as you suggested > > > > > on a previous email. With that, the offending qcow2 is not re-entered, > > > > > so we avoid the crash, but backup-top is still entered twice: > > > > > > > > I think we also need to every parent to the ignore list before calling > > > > callbacks, though it doesn't look like this is the problem you're > > > > currently seeing. > > > > > > I agree. > > > > > > > > bs=0x560db0e3b030 (backup-top) enter > > > > > bs=0x560db0e3b030 (backup-top) processing children > > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) > > > > > bs=0x560db0fb2660 (qcow2) enter > > > > > bs=0x560db0fb2660 (qcow2) processing children > > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) > > > > > bs=0x560db1bb3c00 (file) enter > > > > > bs=0x560db1bb3c00 (file) processing children > > > > > bs=0x560db1bb3c00 (file) processing parents > > > > > bs=0x560db1bb3c00 (file) processing itself > > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) > > > > > bs=0x560db0e50420 (qcow2) enter > > > > > bs=0x560db0e50420 (qcow2) processing children > > > > > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) > > > > > bs=0x560db0e45060 (file) enter > > > > > bs=0x560db0e45060 (file) processing children > > > > > bs=0x560db0e45060 (file) processing parents > > > > > bs=0x560db0e45060 (file) processing itself > > > > > bs=0x560db0e50420 (qcow2) processing parents > > > > > bs=0x560db0e50420 (qcow2) processing itself > > > > > bs=0x560db0fb2660 (qcow2) processing parents > > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 > > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 > > > > > bs=0x560db0e3b030 (backup-top) enter > > > > > bs=0x560db0e3b030 (backup-top) processing children > > > > > bs=0x560db0e3b030 (backup-top) processing parents > > > > > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 > > > > > bs=0x560db0e3b030 (backup-top) processing itself > > > > > bs=0x560db0fb2660 (qcow2) processing itself > > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) > > > > > bs=0x560db0e50420 (qcow2) enter > > > > > bs=0x560db0e3b030 (backup-top) processing parents > > > > > bs=0x560db0e3b030 (backup-top) processing itself > > > > > > > > > > I see that "blk_do_set_aio_context()" passes "blk->root" to > > > > > "bdrv_child_try_set_aio_context()" so it's already in the ignore list, > > > > > so I'm not sure what's happening here. Is backup-top is referenced > > > > > from two different BdrvChild or is "blk->root" not pointing to > > > > > backup-top's BDS? > > > > > > > > The second time that backup-top is entered, it is not as the BDS of > > > > blk->root, but as the parent node of the overlay qcow2. Which is > > > > interesting, because last time it was still the backing qcow2, so the > > > > change did have _some_ effect. > > > > > > > > The part that I don't understand is why you still get the line with > > > > child=0x560db1b14a20, because when you add all children to the ignore > > > > list first, that should have been put into the ignore list as one of the > > > > first things in the whole process (when backup-top was first entered). > > > > > > > > Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, > > > > but isn't actually present in backup-top's bs->children? > > > > > > Exactly, that line corresponds to this chunk of code: > > > > > > <---- begin ----> > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > if (g_slist_find(*ignore, child)) { > > > continue; > > > } > > > assert(child->klass->set_aio_ctx); > > > *ignore = g_slist_prepend(*ignore, child); > > > fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > } > > > <---- end ----> > > > > > > Do you think it's safe to re-enter backup-top, or should we look for a > > > way to avoid this? > > > > I think it should be avoided, but I don't understand why putting all > > children of backup-top into the ignore list doesn't already avoid it. If > > backup-top is in the parents list of qcow2, then qcow2 should be in the > > children list of backup-top and therefore the BdrvChild should already > > be in the ignore list. > > > > The only way I can explain this is that backup-top and qcow2 have > > different ideas about which BdrvChild objects exist that connect them. > > Or that the graph changes between both places, but I don't see how that > > could happen in bdrv_set_aio_context_ignore(). > > > > bdrv_set_aio_context_ignore() do bdrv_drained_begin().. As I reported > recently, nothing prevents some job finish and do graph modification > during some another drained section. It may be the case. Good point, this might be the same bug then. If everything worked correctly, a job completion could only happen on the outer bdrv_set_aio_context_ignore(). But after that, we are already in a drain section, so the job should be quiesced and a second drain shouldn't cause any additional graph changes. I would have to go back to the other discussion, but I think it was related to block jobs that are already in the completion process and keep moving forward even though they are supposed to be quiesced. If I remember correctly, actually pausing them at this point looked difficult. Maybe what we should then do is letting .drained_poll return true until they have actually fully completed? Ah, but was this something that would deadlock because the job completion callbacks use drain sections themselves? > If backup-top involved, I can suppose that graph modification is in > backup_clean, when we remove the filter.. Who is making > set_aio_context in the issue? I mean, what is the backtrace of > bdrv_set_aio_context_ignore()? Sergio, can you provide the backtrace and also test if the theory with a job completion in the middle of the process is what you actually hit? Kevin
On Thu, Dec 17, 2020 at 11:58:30AM +0100, Kevin Wolf wrote: > Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: > > Do you think it's safe to re-enter backup-top, or should we look for a > > way to avoid this? > > I think it should be avoided, but I don't understand why putting all > children of backup-top into the ignore list doesn't already avoid it. If > backup-top is in the parents list of qcow2, then qcow2 should be in the > children list of backup-top and therefore the BdrvChild should already > be in the ignore list. > > The only way I can explain this is that backup-top and qcow2 have > different ideas about which BdrvChild objects exist that connect them. > Or that the graph changes between both places, but I don't see how that > could happen in bdrv_set_aio_context_ignore(). I've been digging around with gdb, and found that, at that point, the backup-top BDS is actually referenced by two different BdrvChild objects: (gdb) p *(BdrvChild *) 0x560c40f7e400 $84 = {bs = 0x560c40c4c030, name = 0x560c41ca4960 "root", klass = 0x560c3eae7c20 <child_root>, role = 20, opaque = 0x560c41ca4610, perm = 3, shared_perm = 29, has_backup_perm = false, backup_perm = 0, backup_shared_perm = 31, frozen = false, parent_quiesce_counter = 2, next = { le_next = 0x0, le_prev = 0x0}, next_parent = {le_next = 0x0, le_prev = 0x560c40c44338}} (gdb) p sibling $72 = (BdrvChild *) 0x560c40981840 (gdb) p *sibling $73 = {bs = 0x560c40c4c030, name = 0x560c4161be20 "main node", klass = 0x560c3eae6a40 <child_job>, role = 0, opaque = 0x560c4161bc00, perm = 0, shared_perm = 31, has_backup_perm = false, backup_perm = 0, backup_shared_perm = 0, frozen = false, parent_quiesce_counter = 2, next = { le_next = 0x0, le_prev = 0x0}, next_parent = {le_next = 0x560c40c442d0, le_prev = 0x560c40c501c0}} When the chain of calls to switch AIO contexts is started, backup-top is the first one to be processed. blk_do_set_aio_context() instructs bdrv_child_try_set_aio_context() to add blk->root (0x560c40f7e400) as the first element in ignore list, but the referenced BDS is still re-entered through the other BdrvChild (0x560c40981840) by one the children of the latter. I can't think of a way of preventing this other than keeping track of BDS pointers in the ignore list too. Do you think there are any alternatives? Thanks, Sergio.
On Thu, Dec 17, 2020 at 02:06:02PM +0100, Kevin Wolf wrote: > Am 17.12.2020 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben: > > 17.12.2020 13:58, Kevin Wolf wrote: > > > Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: > > > > On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: > > > > > Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: > > > > > > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: > > > > > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers > > > > > > > annotated at the edges: > > > > > > > > > > > > > > BlockBackend > > > > > > > | > > > > > > > v > > > > > > > backup-top ------------------------+ > > > > > > > | | | > > > > > > > | +-----------------------+ | > > > > > > > | 0x5655068b8510 | | 0x565505e3c450 > > > > > > > | | | > > > > > > > | 0x565505e42090 | | > > > > > > > v | | > > > > > > > qcow2 ---------------------+ | | > > > > > > > | | | | > > > > > > > | 0x565505e52060 | | | ??? [1] > > > > > > > | | | | | > > > > > > > v 0x5655066a34d0 | | | | 0x565505fc7aa0 > > > > > > > file v v v v > > > > > > > qcow2 (backing) > > > > > > > | > > > > > > > | 0x565505e41d20 > > > > > > > v > > > > > > > file > > > > > > > > > > > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a > > > > > > > BdrvChild directly owned by the backup job. > > > > > > > > > > > > > > > So it seems this is happening: > > > > > > > > > > > > > > > > backup-top (5e48030) <---------| (5) > > > > > > > > | | | > > > > > > > > | | (6) ------------> qcow2 (5fbf660) > > > > > > > > | ^ | > > > > > > > > | (3) | | (4) > > > > > > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) > > > > > > > > | > > > > > > > > |-> (2) file (5e52060) > > > > > > > > > > > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first > > > > > > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) > > > > > > > > is processing its parents, and the latter is also re-entered when the > > > > > > > > first one starts processing its children again. > > > > > > > > > > > > > > Yes, but look at the BdrvChild pointers, it is through different edges > > > > > > > that we come back to the same node. No BdrvChild is used twice. > > > > > > > > > > > > > > If backup-top had added all of its children to the ignore list before > > > > > > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually > > > > > > > have called back into backup-top. > > > > > > > > > > > > I've tested a patch that first adds every child to the ignore list, > > > > > > and then processes those that weren't there before, as you suggested > > > > > > on a previous email. With that, the offending qcow2 is not re-entered, > > > > > > so we avoid the crash, but backup-top is still entered twice: > > > > > > > > > > I think we also need to every parent to the ignore list before calling > > > > > callbacks, though it doesn't look like this is the problem you're > > > > > currently seeing. > > > > > > > > I agree. > > > > > > > > > > bs=0x560db0e3b030 (backup-top) enter > > > > > > bs=0x560db0e3b030 (backup-top) processing children > > > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) > > > > > > bs=0x560db0fb2660 (qcow2) enter > > > > > > bs=0x560db0fb2660 (qcow2) processing children > > > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) > > > > > > bs=0x560db1bb3c00 (file) enter > > > > > > bs=0x560db1bb3c00 (file) processing children > > > > > > bs=0x560db1bb3c00 (file) processing parents > > > > > > bs=0x560db1bb3c00 (file) processing itself > > > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) > > > > > > bs=0x560db0e50420 (qcow2) enter > > > > > > bs=0x560db0e50420 (qcow2) processing children > > > > > > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) > > > > > > bs=0x560db0e45060 (file) enter > > > > > > bs=0x560db0e45060 (file) processing children > > > > > > bs=0x560db0e45060 (file) processing parents > > > > > > bs=0x560db0e45060 (file) processing itself > > > > > > bs=0x560db0e50420 (qcow2) processing parents > > > > > > bs=0x560db0e50420 (qcow2) processing itself > > > > > > bs=0x560db0fb2660 (qcow2) processing parents > > > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 > > > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 > > > > > > bs=0x560db0e3b030 (backup-top) enter > > > > > > bs=0x560db0e3b030 (backup-top) processing children > > > > > > bs=0x560db0e3b030 (backup-top) processing parents > > > > > > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 > > > > > > bs=0x560db0e3b030 (backup-top) processing itself > > > > > > bs=0x560db0fb2660 (qcow2) processing itself > > > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) > > > > > > bs=0x560db0e50420 (qcow2) enter > > > > > > bs=0x560db0e3b030 (backup-top) processing parents > > > > > > bs=0x560db0e3b030 (backup-top) processing itself > > > > > > > > > > > > I see that "blk_do_set_aio_context()" passes "blk->root" to > > > > > > "bdrv_child_try_set_aio_context()" so it's already in the ignore list, > > > > > > so I'm not sure what's happening here. Is backup-top is referenced > > > > > > from two different BdrvChild or is "blk->root" not pointing to > > > > > > backup-top's BDS? > > > > > > > > > > The second time that backup-top is entered, it is not as the BDS of > > > > > blk->root, but as the parent node of the overlay qcow2. Which is > > > > > interesting, because last time it was still the backing qcow2, so the > > > > > change did have _some_ effect. > > > > > > > > > > The part that I don't understand is why you still get the line with > > > > > child=0x560db1b14a20, because when you add all children to the ignore > > > > > list first, that should have been put into the ignore list as one of the > > > > > first things in the whole process (when backup-top was first entered). > > > > > > > > > > Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, > > > > > but isn't actually present in backup-top's bs->children? > > > > > > > > Exactly, that line corresponds to this chunk of code: > > > > > > > > <---- begin ----> > > > > QLIST_FOREACH(child, &bs->parents, next_parent) { > > > > if (g_slist_find(*ignore, child)) { > > > > continue; > > > > } > > > > assert(child->klass->set_aio_ctx); > > > > *ignore = g_slist_prepend(*ignore, child); > > > > fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); > > > > child->klass->set_aio_ctx(child, new_context, ignore); > > > > } > > > > <---- end ----> > > > > > > > > Do you think it's safe to re-enter backup-top, or should we look for a > > > > way to avoid this? > > > > > > I think it should be avoided, but I don't understand why putting all > > > children of backup-top into the ignore list doesn't already avoid it. If > > > backup-top is in the parents list of qcow2, then qcow2 should be in the > > > children list of backup-top and therefore the BdrvChild should already > > > be in the ignore list. > > > > > > The only way I can explain this is that backup-top and qcow2 have > > > different ideas about which BdrvChild objects exist that connect them. > > > Or that the graph changes between both places, but I don't see how that > > > could happen in bdrv_set_aio_context_ignore(). > > > > > > > bdrv_set_aio_context_ignore() do bdrv_drained_begin().. As I reported > > recently, nothing prevents some job finish and do graph modification > > during some another drained section. It may be the case. > > Good point, this might be the same bug then. > > If everything worked correctly, a job completion could only happen on > the outer bdrv_set_aio_context_ignore(). But after that, we are already > in a drain section, so the job should be quiesced and a second drain > shouldn't cause any additional graph changes. > > I would have to go back to the other discussion, but I think it was > related to block jobs that are already in the completion process and > keep moving forward even though they are supposed to be quiesced. > > If I remember correctly, actually pausing them at this point looked > difficult. Maybe what we should then do is letting .drained_poll return > true until they have actually fully completed? > > Ah, but was this something that would deadlock because the job > completion callbacks use drain sections themselves? > > > If backup-top involved, I can suppose that graph modification is in > > backup_clean, when we remove the filter.. Who is making > > set_aio_context in the issue? I mean, what is the backtrace of > > bdrv_set_aio_context_ignore()? > > Sergio, can you provide the backtrace and also test if the theory with a > job completion in the middle of the process is what you actually hit? No, I'm sure the job is not finishing in the middle of the set_aio_context chain, which is started by a virtio_blk_data_plane_[start|stop], which in turn is triggered by a guest reboot. This is a stack trace that reaches to the point in which backup-top is entered a second time: #0 0x0000560c3e173bbd in child_job_set_aio_ctx (c=<optimized out>, ctx=0x560c40c45630, ignore=0x7f6d4eeb6f40) at ../blockjob.c:159 #1 0x0000560c3e1aefc6 in bdrv_set_aio_context_ignore (bs=0x560c40dc3660, new_context=0x560c40c45630, ignore=0x7f6d4eeb6f40) at ../block.c:6509 #2 0x0000560c3e1aee8a in bdrv_set_aio_context_ignore (bs=bs@entry=0x560c40c4c030, new_context=new_context@entry=0x560c40c45630, ignore=ignore@entry=0x7f6d4eeb6f40) at ../block.c:6487 #3 0x0000560c3e1af503 in bdrv_child_try_set_aio_context (bs=bs@entry=0x560c40c4c030, ctx=ctx@entry=0x560c40c45630, ignore_child=<optimized out>, errp=errp@entry=0x7f6d4eeb6fc8) at ../block.c:6619 #4 0x0000560c3e1e561a in blk_do_set_aio_context (blk=0x560c41ca4610, new_context=0x560c40c45630, update_root_node=update_root_node@entry=true, errp=errp@entry=0x7f6d4eeb6fc8) at ../block/block-backend.c:2027 #5 0x0000560c3e1e740d in blk_set_aio_context (blk=<optimized out>, new_context=<optimized out>, errp=errp@entry=0x7f6d4eeb6fc8) at ../block/block-backend.c:2048 #6 0x0000560c3e10de78 in virtio_blk_data_plane_start (vdev=<optimized out>) at ../hw/block/dataplane/virtio-blk.c:220 #7 0x0000560c3de691d2 in virtio_bus_start_ioeventfd (bus=bus@entry=0x560c41ca1e98) at ../hw/virtio/virtio-bus.c:222 #8 0x0000560c3de4f907 in virtio_pci_start_ioeventfd (proxy=0x560c41c99d90) at ../hw/virtio/virtio-pci.c:1261 #9 0x0000560c3de4f907 in virtio_pci_common_write (opaque=0x560c41c99d90, addr=<optimized out>, val=<optimized out>, size=<optimized out>) at ../hw/virtio/virtio-pci.c:1261 #10 0x0000560c3e145d81 in memory_region_write_accessor (mr=0x560c41c9a770, addr=20, value=<optimized out>, size=1, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:491 #11 0x0000560c3e1447de in access_with_adjusted_size (addr=addr@entry=20, value=value@entry=0x7f6d4eeb71a8, size=size@entry=1, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn= 0x560c3e145c80 <memory_region_write_accessor>, mr=0x560c41c9a770, attrs=...) at ../softmmu/memory.c:552 #12 0x0000560c3e148052 in memory_region_dispatch_write (mr=mr@entry=0x560c41c9a770, addr=20, data=<optimized out>, op=<optimized out>, attrs=attrs@entry=...) at ../softmmu/memory.c:1501 #13 0x0000560c3e06b5b7 in flatview_write_continue (fv=fv@entry=0x7f6d400ed3e0, addr=addr@entry=4261429268, attrs=..., ptr=ptr@entry=0x7f6d71dad028, len=len@entry=1, addr1=<optimized out>, l=<optimized out>, mr=0x560c41c9a770) at /home/BZs/1900326/qemu/include/qemu/host-utils.h:164 #14 0x0000560c3e06b7d6 in flatview_write (fv=0x7f6d400ed3e0, addr=addr@entry=4261429268, attrs=attrs@entry=..., buf=buf@entry=0x7f6d71dad028, len=len@entry=1) at ../softmmu/physmem.c:2799 #15 0x0000560c3e06e330 in address_space_write (as=0x560c3ec0a920 <address_space_memory>, addr=4261429268, attrs=..., buf=buf@entry=0x7f6d71dad028, len=1) at ../softmmu/physmem.c:2891 #16 0x0000560c3e06e3ba in address_space_rw (as=<optimized out>, addr=<optimized out>, attrs=..., attrs@entry=..., buf=buf@entry=0x7f6d71dad028, len=<optimized out>, is_write=<optimized out>) at ../softmmu/physmem.c:2901 #17 0x0000560c3e10021a in kvm_cpu_exec (cpu=cpu@entry=0x560c40d7e0d0) at ../accel/kvm/kvm-all.c:2541 #18 0x0000560c3e1445e5 in kvm_vcpu_thread_fn (arg=arg@entry=0x560c40d7e0d0) at ../accel/kvm/kvm-cpus.c:49 #19 0x0000560c3e2c798a in qemu_thread_start (args=<optimized out>) at ../util/qemu-thread-posix.c:521 #20 0x00007f6d6ba8614a in start_thread () at /lib64/libpthread.so.0 #21 0x00007f6d6b7b8763 in clone () at /lib64/libc.so.6 Thanks, Sergio.
17.12.2020 16:06, Kevin Wolf wrote: > Am 17.12.2020 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 17.12.2020 13:58, Kevin Wolf wrote: >>> Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben: >>>> On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote: >>>>> Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben: >>>>>> On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote: >>>>>>> Anyway, trying to reconstruct the block graph with BdrvChild pointers >>>>>>> annotated at the edges: >>>>>>> >>>>>>> BlockBackend >>>>>>> | >>>>>>> v >>>>>>> backup-top ------------------------+ >>>>>>> | | | >>>>>>> | +-----------------------+ | >>>>>>> | 0x5655068b8510 | | 0x565505e3c450 >>>>>>> | | | >>>>>>> | 0x565505e42090 | | >>>>>>> v | | >>>>>>> qcow2 ---------------------+ | | >>>>>>> | | | | >>>>>>> | 0x565505e52060 | | | ??? [1] >>>>>>> | | | | | >>>>>>> v 0x5655066a34d0 | | | | 0x565505fc7aa0 >>>>>>> file v v v v >>>>>>> qcow2 (backing) >>>>>>> | >>>>>>> | 0x565505e41d20 >>>>>>> v >>>>>>> file >>>>>>> >>>>>>> [1] This seems to be a BdrvChild with a non-BDS parent. Probably a >>>>>>> BdrvChild directly owned by the backup job. >>>>>>> >>>>>>>> So it seems this is happening: >>>>>>>> >>>>>>>> backup-top (5e48030) <---------| (5) >>>>>>>> | | | >>>>>>>> | | (6) ------------> qcow2 (5fbf660) >>>>>>>> | ^ | >>>>>>>> | (3) | | (4) >>>>>>>> |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00) >>>>>>>> | >>>>>>>> |-> (2) file (5e52060) >>>>>>>> >>>>>>>> backup-top (5e48030), the BDS that was passed as argument in the first >>>>>>>> bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660) >>>>>>>> is processing its parents, and the latter is also re-entered when the >>>>>>>> first one starts processing its children again. >>>>>>> >>>>>>> Yes, but look at the BdrvChild pointers, it is through different edges >>>>>>> that we come back to the same node. No BdrvChild is used twice. >>>>>>> >>>>>>> If backup-top had added all of its children to the ignore list before >>>>>>> calling into the overlay qcow2, the backing qcow2 wouldn't eventually >>>>>>> have called back into backup-top. >>>>>> >>>>>> I've tested a patch that first adds every child to the ignore list, >>>>>> and then processes those that weren't there before, as you suggested >>>>>> on a previous email. With that, the offending qcow2 is not re-entered, >>>>>> so we avoid the crash, but backup-top is still entered twice: >>>>> >>>>> I think we also need to every parent to the ignore list before calling >>>>> callbacks, though it doesn't look like this is the problem you're >>>>> currently seeing. >>>> >>>> I agree. >>>> >>>>>> bs=0x560db0e3b030 (backup-top) enter >>>>>> bs=0x560db0e3b030 (backup-top) processing children >>>>>> bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660) >>>>>> bs=0x560db0fb2660 (qcow2) enter >>>>>> bs=0x560db0fb2660 (qcow2) processing children >>>>>> bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00) >>>>>> bs=0x560db1bb3c00 (file) enter >>>>>> bs=0x560db1bb3c00 (file) processing children >>>>>> bs=0x560db1bb3c00 (file) processing parents >>>>>> bs=0x560db1bb3c00 (file) processing itself >>>>>> bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420) >>>>>> bs=0x560db0e50420 (qcow2) enter >>>>>> bs=0x560db0e50420 (qcow2) processing children >>>>>> bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060) >>>>>> bs=0x560db0e45060 (file) enter >>>>>> bs=0x560db0e45060 (file) processing children >>>>>> bs=0x560db0e45060 (file) processing parents >>>>>> bs=0x560db0e45060 (file) processing itself >>>>>> bs=0x560db0e50420 (qcow2) processing parents >>>>>> bs=0x560db0e50420 (qcow2) processing itself >>>>>> bs=0x560db0fb2660 (qcow2) processing parents >>>>>> bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860 >>>>>> bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20 >>>>>> bs=0x560db0e3b030 (backup-top) enter >>>>>> bs=0x560db0e3b030 (backup-top) processing children >>>>>> bs=0x560db0e3b030 (backup-top) processing parents >>>>>> bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0 >>>>>> bs=0x560db0e3b030 (backup-top) processing itself >>>>>> bs=0x560db0fb2660 (qcow2) processing itself >>>>>> bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420) >>>>>> bs=0x560db0e50420 (qcow2) enter >>>>>> bs=0x560db0e3b030 (backup-top) processing parents >>>>>> bs=0x560db0e3b030 (backup-top) processing itself >>>>>> >>>>>> I see that "blk_do_set_aio_context()" passes "blk->root" to >>>>>> "bdrv_child_try_set_aio_context()" so it's already in the ignore list, >>>>>> so I'm not sure what's happening here. Is backup-top is referenced >>>>>> from two different BdrvChild or is "blk->root" not pointing to >>>>>> backup-top's BDS? >>>>> >>>>> The second time that backup-top is entered, it is not as the BDS of >>>>> blk->root, but as the parent node of the overlay qcow2. Which is >>>>> interesting, because last time it was still the backing qcow2, so the >>>>> change did have _some_ effect. >>>>> >>>>> The part that I don't understand is why you still get the line with >>>>> child=0x560db1b14a20, because when you add all children to the ignore >>>>> list first, that should have been put into the ignore list as one of the >>>>> first things in the whole process (when backup-top was first entered). >>>>> >>>>> Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value, >>>>> but isn't actually present in backup-top's bs->children? >>>> >>>> Exactly, that line corresponds to this chunk of code: >>>> >>>> <---- begin ----> >>>> QLIST_FOREACH(child, &bs->parents, next_parent) { >>>> if (g_slist_find(*ignore, child)) { >>>> continue; >>>> } >>>> assert(child->klass->set_aio_ctx); >>>> *ignore = g_slist_prepend(*ignore, child); >>>> fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child); >>>> child->klass->set_aio_ctx(child, new_context, ignore); >>>> } >>>> <---- end ----> >>>> >>>> Do you think it's safe to re-enter backup-top, or should we look for a >>>> way to avoid this? >>> >>> I think it should be avoided, but I don't understand why putting all >>> children of backup-top into the ignore list doesn't already avoid it. If >>> backup-top is in the parents list of qcow2, then qcow2 should be in the >>> children list of backup-top and therefore the BdrvChild should already >>> be in the ignore list. >>> >>> The only way I can explain this is that backup-top and qcow2 have >>> different ideas about which BdrvChild objects exist that connect them. >>> Or that the graph changes between both places, but I don't see how that >>> could happen in bdrv_set_aio_context_ignore(). >>> >> >> bdrv_set_aio_context_ignore() do bdrv_drained_begin().. As I reported >> recently, nothing prevents some job finish and do graph modification >> during some another drained section. It may be the case. > > Good point, this might be the same bug then. > > If everything worked correctly, a job completion could only happen on > the outer bdrv_set_aio_context_ignore(). But after that, we are already > in a drain section, so the job should be quiesced and a second drain > shouldn't cause any additional graph changes. > > I would have to go back to the other discussion, but I think it was > related to block jobs that are already in the completion process and > keep moving forward even though they are supposed to be quiesced. > > If I remember correctly, actually pausing them at this point looked > difficult. Maybe what we should then do is letting .drained_poll return > true until they have actually fully completed? > > Ah, but was this something that would deadlock because the job > completion callbacks use drain sections themselves? Hmm.. I've recently sent good example of dead-lock in email "aio-poll dead-lock".. I don't have better idea than moving all graph modifications (together with corresponding drained sections) into coroutines and protect by global coroutine mutex. > >> If backup-top involved, I can suppose that graph modification is in >> backup_clean, when we remove the filter.. Who is making >> set_aio_context in the issue? I mean, what is the backtrace of >> bdrv_set_aio_context_ignore()? > > Sergio, can you provide the backtrace and also test if the theory with a > job completion in the middle of the process is what you actually hit? > > Kevin >
diff --git a/block.c b/block.c index f1cedac362..bc8a66ab6e 100644 --- a/block.c +++ b/block.c @@ -6465,12 +6465,17 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs, bdrv_drained_begin(bs); QLIST_FOREACH(child, &bs->children, next) { - if (g_slist_find(*ignore, child)) { + if (g_slist_find(*ignore, child) || g_slist_find(*ignore, child->bs)) { continue; } *ignore = g_slist_prepend(*ignore, child); bdrv_set_aio_context_ignore(child->bs, new_context, ignore); } + /* + * Add a reference to this BS to the ignore list, so its + * parents won't attempt to process it again. + */ + *ignore = g_slist_prepend(*ignore, bs); QLIST_FOREACH(child, &bs->parents, next_parent) { if (g_slist_find(*ignore, child)) { continue;
While processing the parents of a BDS, one of the parents may process the child that's doing the tail recursion, which leads to a BDS being processed twice. This is especially problematic for the aio_notifiers, as they might attempt to work on both the old and the new AIO contexts. To avoid this, add the BDS pointer to the ignore list, and check the child BDS pointer while iterating over the children. Signed-off-by: Sergio Lopez <slp@redhat.com> --- block.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)