| Message ID | 20230510203601.418015-9-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Honour graph read lock even in the main thread | expand |
On Wed, May 10, 2023 at 10:36:01PM +0200, Kevin Wolf wrote: > > There are some conditions under which we don't actually need to do > anything for taking a reader lock: Writing the graph is only possible > from the main context while holding the BQL. So if a reader is running > in the main context under the BQL and knows that it won't be interrupted > until the next writer runs, we don't actually need to do anything. > > This is the case if the reader code neither has a nested event loop > (this is forbidden anyway while you hold the lock) nor is a coroutine > (because a writer could run when the coroutine has yielded). > > These conditions are exactly what bdrv_graph_rdlock_main_loop() asserts. > They are not fulfilled in bdrv_graph_co_rdlock(), which always runs in a > coroutine. > > This deletes the shortcuts in bdrv_graph_co_rdlock() that skip taking > the reader lock in the main thread. > > Reported-by: Fiona Ebner <f.ebner@proxmox.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/graph-lock.c | 10 ---------- > 1 file changed, 10 deletions(-) Bug fix by deletion is always fun. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/block/graph-lock.c b/block/graph-lock.c index 377884c3a9..9c42bd9799 100644 --- a/block/graph-lock.c +++ b/block/graph-lock.c @@ -162,11 +162,6 @@ void coroutine_fn bdrv_graph_co_rdlock(void) BdrvGraphRWlock *bdrv_graph; bdrv_graph = qemu_get_current_aio_context()->bdrv_graph; - /* Do not lock if in main thread */ - if (qemu_in_main_thread()) { - return; - } - for (;;) { qatomic_set(&bdrv_graph->reader_count, bdrv_graph->reader_count + 1); @@ -230,11 +225,6 @@ void coroutine_fn bdrv_graph_co_rdunlock(void) BdrvGraphRWlock *bdrv_graph; bdrv_graph = qemu_get_current_aio_context()->bdrv_graph; - /* Do not lock if in main thread */ - if (qemu_in_main_thread()) { - return; - } - qatomic_store_release(&bdrv_graph->reader_count, bdrv_graph->reader_count - 1); /* make sure writer sees reader_count before we check has_writer */
There are some conditions under which we don't actually need to do anything for taking a reader lock: Writing the graph is only possible from the main context while holding the BQL. So if a reader is running in the main context under the BQL and knows that it won't be interrupted until the next writer runs, we don't actually need to do anything. This is the case if the reader code neither has a nested event loop (this is forbidden anyway while you hold the lock) nor is a coroutine (because a writer could run when the coroutine has yielded). These conditions are exactly what bdrv_graph_rdlock_main_loop() asserts. They are not fulfilled in bdrv_graph_co_rdlock(), which always runs in a coroutine. This deletes the shortcuts in bdrv_graph_co_rdlock() that skip taking the reader lock in the main thread. Reported-by: Fiona Ebner <f.ebner@proxmox.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/graph-lock.c | 10 ---------- 1 file changed, 10 deletions(-)