From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1g70fz-0002FP-Q7 for qemu-devel@nongnu.org; Mon, 01 Oct 2018 12:04:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1g70fy-0005no-5A for qemu-devel@nongnu.org; Mon, 01 Oct 2018 12:04:03 -0400 Date: Mon, 1 Oct 2018 18:03:48 +0200 From: Kevin Wolf Message-ID: <20181001160348.GB4445@localhost.localdomain> References: <20181001143218.17069-1-kwolf@redhat.com> <9a0e30f2-90d0-11ba-b5db-3e2df18590cc@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9a0e30f2-90d0-11ba-b5db-3e2df18590cc@redhat.com> Subject: Re: [Qemu-devel] [PATCH] test-replication: Lock AioContext around blk_unref() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-block@nongnu.org, mreitz@redhat.com, peter.maydell@linaro.org, qemu-devel@nongnu.org Am 01.10.2018 um 17:40 hat Paolo Bonzini geschrieben: > On 01/10/2018 16:32, Kevin Wolf wrote: > > Recently, the test case has started failing because some job related > > functions want to drop the AioContext lock even though it hasn't been > > taken: > > > > (gdb) bt > > #0 0x00007f51c067c9fb in raise () from /lib64/libc.so.6 > > #1 0x00007f51c067e77d in abort () from /lib64/libc.so.6 > > #2 0x0000558c9d5dde7b in error_exit (err=, msg=msg@entry=0x558c9d6fe120 <__func__.18373> "qemu_mutex_unlock_impl") at util/qemu-thread-posix.c:36 > > #3 0x0000558c9d6b5263 in qemu_mutex_unlock_impl (mutex=mutex@entry=0x558c9f3999a0, file=file@entry=0x558c9d6fd36f "util/async.c", line=line@entry=516) at util/qemu-thread-posix.c:96 > > #4 0x0000558c9d6b0565 in aio_context_release (ctx=ctx@entry=0x558c9f399940) at util/async.c:516 > > #5 0x0000558c9d5eb3da in job_completed_txn_abort (job=0x558c9f68e640) at job.c:738 > > #6 0x0000558c9d5eb227 in job_finish_sync (job=0x558c9f68e640, finish=finish@entry=0x558c9d5eb8d0 , errp=errp@entry=0x0) at job.c:986 > > #7 0x0000558c9d5eb8ee in job_cancel_sync (job=) at job.c:941 > > #8 0x0000558c9d64d853 in replication_close (bs=) at block/replication.c:148 > > #9 0x0000558c9d5e5c9f in bdrv_close (bs=0x558c9f41b020) at block.c:3420 > > #10 bdrv_delete (bs=0x558c9f41b020) at block.c:3629 > > #11 bdrv_unref (bs=0x558c9f41b020) at block.c:4685 > > #12 0x0000558c9d62a3f3 in blk_remove_bs (blk=blk@entry=0x558c9f42a7c0) at block/block-backend.c:783 > > #13 0x0000558c9d62a667 in blk_delete (blk=0x558c9f42a7c0) at block/block-backend.c:402 > > #14 blk_unref (blk=0x558c9f42a7c0) at block/block-backend.c:457 > > #15 0x0000558c9d5dfcea in test_secondary_stop () at tests/test-replication.c:478 > > #16 0x00007f51c1f13178 in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0 > > #17 0x00007f51c1f1337b in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0 > > #18 0x00007f51c1f1337b in g_test_run_suite_internal () from /lib64/libglib-2.0.so.0 > > #19 0x00007f51c1f13552 in g_test_run_suite () from /lib64/libglib-2.0.so.0 > > #20 0x00007f51c1f13571 in g_test_run () from /lib64/libglib-2.0.so.0 > > #21 0x0000558c9d5de31f in main (argc=, argv=) at tests/test-replication.c:581 > > > > It is yet unclear whether this should really be considered a bug in the > > test case or whether blk_unref() should work for callers that haven't > > taken the AioContext lock, but in order to fix the build tests quickly, > > just take the AioContext lock around blk_unref(). > > Given the backtrace, I think bdrv_close should be taking the AioContext > lock instead of blockdev_close_all_bdrv_states. Conversely, that would mean that calling bdrv_unref() with the AioContext lock held is a bug (because close callbacks can involve AIO_WAIT_WHILE()). I'm not sure if that's very practical. Of course, there will probably be a lot of callers to fix either way after we define whether to hold the lock for bdrv_unref() or not. Either you need to add locking to the places where it's missing or you need to drop the locks in all other places. I was leaning towards requiring the lock for bdrv_unref() (and therefore blk_unref()). Kevin