blockjob: Make block_job_pause_all() keep a reference to the jobs
cover blockjob: Make block_job_pause_all() keep a reference to the jobs
Hi,
this patch fixes the crash reported by Anton Nefedov here:
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00159.html
I can reproduce it easily with the change he mentions there, or by tweaking iotest 030 as I show here:
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.html
I'm not convinced that this is the best solution, though. As Fam says the block layer is getting complex and I think this can be solved in a different way if the code is properly rewritten. Even with this solution I think it would make sense to assert that the block job's pause count is always 0 when the job is about to be destroyed and perhaps keep a reference while it's being paused. But that's a bigger change and we're close to the release so I opted for this more conservative approach.
Regards,
Berto
Alberto Garcia (1):
blockjob: Make block_job_pause_all() keep a reference to the jobs
blockjob.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Signature · 1 line
2.11.0
PATCH 1/1 blockjob: Make block_job_pause_all() keep a reference to the jobs
Starting from commit 40840e419be31e6a32e6ea24511c74b389d5e0e4 we are pausing all block jobs during bdrv_reopen_multiple() to prevent any of them from finishing and removing nodes from the graph while they are being reopened.
It turns out that pausing a block job doesn't necessarily prevent it from finishing: a paused block job can still run its exit function from the main loop and call block_job_completed(). The mirror block job in particular always goes to the main loop while it is paused (by virtue of the bdrv_drained_begin() call in mirror_run()).
Destroying a paused block job during bdrv_reopen_multiple() has two consequences:
1) The references to the nodes involved in the job are released, possibly destroying some of them. If those nodes were in the reopen queue this would trigger the problem originally described in commit 40840e419be, crashing QEMU.
2) At the end of bdrv_reopen_multiple(), bdrv_drain_all_end() would not be doing all necessary bdrv_parent_drained_end() calls.
I can reproduce problem 1) easily with iotest 030 by increasing STREAM_BUFFER_SIZE from 512KB to 8MB in block/stream.c, or by tweaking the iotest like in this example:
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.html
This patch keeps an additional reference to all block jobs between block_job_pause_all() and block_job_resume_all(), guaranteeing that they are kept alive.
--- blockjob.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
blockjob.c +5 -2
@@ -730,6 +730,7 @@ void block_job_pause_all(void)AioContext *aio_context =blk_get_aio_context (job->blk);aio_context_acquire (aio_context); +block_job_ref (job);block_job_pause (job);aio_context_release (aio_context); }
@@ -808,12 +809,14 @@ void coroutine_fn block_job_pause_point(BlockJob *job)void block_job_resume_all (void ) { -BlockJob *job =NULL ; -while ((job =block_job_next (job))) { +BlockJob *job, *next; + + QLIST_FOREACH_SAFE(job, &block_jobs, job_list, next) {AioContext *aio_context =blk_get_aio_context (job->blk);aio_context_acquire (aio_context);block_job_resume (job); +block_job_unref (job);aio_context_release (aio_context); } }
Signature · 1 line
2.11.0
Am 29.11.2017 um 18:56 hat Alberto Garcia geschrieben:
Quoted 11 lines
Starting from commit
40840e419be31e6a32e6ea24511c74b389d5e0e4we are pausing all block jobs duringbdrv_reopen_multiple()to prevent any of them from finishing and removing nodes from the graph while they are being reopened.It turns out that pausing a block job doesn't necessarily prevent it from finishing: a paused block job can still run its exit function from the main loop and call
block_job_completed(). The mirror block job in particular always goes to the main loop while it is paused (by virtue of thebdrv_drained_begin()call inmirror_run()).
Quoted 7 lines
Destroying a paused block job during
bdrv_reopen_multiple()has two consequences:1) The references to the nodes involved in the job are released, possibly destroying some of them. If those nodes were in the reopen queue this would trigger the problem originally described in commit
40840e419be, crashing QEMU.
This specific problem could be avoided by making the BDS reference in the reopen queue strong, i.e. bdrv_ref() in bdrv_reopen_queue_child() and bdrv_unref() only at the end of bdrv_reopen_multiple().
Quoted 2 lines
2) At the end of
bdrv_reopen_multiple(),bdrv_drain_all_end()would not be doing all necessarybdrv_parent_drained_end()calls.
If I understand correctly, you don't have a reproducer here. I'm not convinced that this one actually exists because the functions that do the graph modifications (specficically bdrv_replace_child_noperm), automatically drain and undrain nodes as necessary.
Quoted 9 lines
I can reproduce problem 1) easily with iotest 030 by increasing
STREAM_BUFFER_SIZEfrom 512KB to 8MB inblock/stream.c, or by tweaking the iotest like in this example:https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.htmlThis patch keeps an additional reference to all block jobs between
block_job_pause_all()andblock_job_resume_all(), guaranteeing that they are kept alive.
This might be okay as a stopgap solution if this is a real problem in practice because we don't have a better solution right now. However, I haven't seen any sign of there being an -rc4, so we're probably too late for 2.11 anyway.
It's certainly not a full solution because keeping a reference to a block job does not prevent it from completing, but only from being freed. Most block jobs do graph modifications, including dropping the references to nodes, already when they complete, not only when they are freed.
Now, while the above might have sounded like we have an easy solution in bdrv_reopen(), I see another problem that looks quite tough:
3) bdrv_reopen_queue() is a recursive operation that involves all children that inherited options. If the graph changes between the bdrv_reopen_queue() call and the end of bdrv_reopen_multiple(), the calculated options (and today possibly permissions) don't actually match the graph any more.
The requirement we really have is that between bdrv_reopen_queue() and bdrv_reopen_multiple() no graph changes are made. Calling bdrv_drain_all_begin() in bdrv_reopen_multiple() is too late.
Maybe that actually gives us another relatively simple solution: We need to push up the drain into the callers, and possibly just assert that the nodes are drained in the reopen functions.
Kevin
Quoted 11 lines · Kevin Wolf
Quoted 7 lines
Destroying a paused block job during
bdrv_reopen_multiple()has two consequences:1) The references to the nodes involved in the job are released, possibly destroying some of them. If those nodes were in the reopen queue this would trigger the problem originally described in commit
40840e419be, crashing QEMU.This specific problem could be avoided by making the BDS reference in the reopen queue strong, i.e.
bdrv_ref()inbdrv_reopen_queue_child()andbdrv_unref()only at the end ofbdrv_reopen_multiple().
That is correct.
Quoted 4 lines
Quoted 2 lines
2) At the end of
bdrv_reopen_multiple(),bdrv_drain_all_end()would not be doing all necessarybdrv_parent_drained_end()calls.If I understand correctly, you don't have a reproducer here.
That's unfortunately not correct.
You can use the very test case that I mentioned in the commit message:
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.html
With that one, QEMU master crashes easily because of problem (1). If I hold strong references in the reopen queue as you mentioned, the test case hangs because of problem (2).
Quoted 5 lines
It's certainly not a full solution because keeping a reference to a block job does not prevent it from completing, but only from being freed. Most block jobs do graph modifications, including dropping the references to nodes, already when they complete, not only when they are freed.
Yes but the block job itself holds additional references (thanks to block_job_add_bdrv()).
Quoted 9 lines
Now, while the above might have sounded like we have an easy solution in
bdrv_reopen(), I see another problem that looks quite tough:3)
bdrv_reopen_queue()is a recursive operation that involves all children that inherited options. If the graph changes between thebdrv_reopen_queue()call and the end ofbdrv_reopen_multiple(), the calculated options (and today possibly permissions) don't actually match the graph any more.
Quoted 3 lines
The requirement we really have is that between
bdrv_reopen_queue()andbdrv_reopen_multiple()no graph changes are made. Callingbdrv_drain_all_begin()inbdrv_reopen_multiple()is too late.
Yes I agree that that should be the requirement.
Berto
Am 30.11.2017 um 15:35 hat Alberto Garcia geschrieben:
Quoted 23 lines · Kevin Wolf
Quoted 4 lines
Quoted 4 lines
Destroying a paused block job during
bdrv_reopen_multiple()has two consequences:Quoted 8 lines
Quoted 4 lines
1) The references to the nodes involved in the job are released, possibly destroying some of them. If those nodes were in the reopen queue this would trigger the problem originally described in commit
40840e419be, crashing QEMU.This specific problem could be avoided by making the BDS reference in the reopen queue strong, i.e.
bdrv_ref()inbdrv_reopen_queue_child()andbdrv_unref()only at the end ofbdrv_reopen_multiple().That is correct.
Quoted 4 lines
Quoted 2 lines
2) At the end of
bdrv_reopen_multiple(),bdrv_drain_all_end()would not be doing all necessarybdrv_parent_drained_end()calls.If I understand correctly, you don't have a reproducer here.
That's unfortunately not correct.
Quoted 7 lines
You can use the very test case that I mentioned in the commit message:
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.htmlWith that one, QEMU master crashes easily because of problem (1). If I hold strong references in the reopen queue as you mentioned, the test case hangs because of problem (2).
Ok, thanks. I'll try to play with this a bit myself later.
Quoted 8 lines
Quoted 5 lines
It's certainly not a full solution because keeping a reference to a block job does not prevent it from completing, but only from being freed. Most block jobs do graph modifications, including dropping the references to nodes, already when they complete, not only when they are freed.
Yes but the block job itself holds additional references (thanks to
block_job_add_bdrv()).
Mirror and commit call block_job_remove_all_bdrv() during their completion. So yes, it does help for streaming, but not for all block jobs.
Kevin
Quoted 26 lines · Kevin Wolf
Am 30.11.2017 um 15:35 hat Alberto Garcia geschrieben:
Quoted 25 lines · Kevin Wolf
Quoted 4 lines
Quoted 4 lines
Destroying a paused block job during
bdrv_reopen_multiple()has two consequences:Quoted 8 lines
Quoted 4 lines
1) The references to the nodes involved in the job are released, possibly destroying some of them. If those nodes were in the reopen queue this would trigger the problem originally described in commit
40840e419be, crashing QEMU.This specific problem could be avoided by making the BDS reference in the reopen queue strong, i.e.
bdrv_ref()inbdrv_reopen_queue_child()andbdrv_unref()only at the end ofbdrv_reopen_multiple().That is correct.
Quoted 4 lines
Quoted 2 lines
2) At the end of
bdrv_reopen_multiple(),bdrv_drain_all_end()would not be doing all necessarybdrv_parent_drained_end()calls.If I understand correctly, you don't have a reproducer here.
That's unfortunately not correct.
You can use the very test case that I mentioned in the commit message:
Quoted 8 lines
Quoted 5 lines
https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00934.htmlWith that one, QEMU master crashes easily because of problem (1). If I hold strong references in the reopen queue as you mentioned, the test case hangs because of problem (2).
Ok, thanks. I'll try to play with this a bit myself later.
Another data point: I'm able to reproduce that crash, by both increasing STREAM_BUFFER_SIZE as mentioned, and using the new test case, on -rc3.
Quoted 14 lines
Quoted 8 lines
Quoted 5 lines
It's certainly not a full solution because keeping a reference to a block job does not prevent it from completing, but only from being freed. Most block jobs do graph modifications, including dropping the references to nodes, already when they complete, not only when they are freed.
Yes but the block job itself holds additional references (thanks to
block_job_add_bdrv()).Mirror and commit call
block_job_remove_all_bdrv()during their completion. So yes, it does help for streaming, but not for all block jobs.Kevin