Am 14.05.25 um 21:54 schrieb Kevin Wolf: > Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben: >> In bdrv_graph_wrlock() there is a comment that it uses >> bdrv_drain_all_begin_nopoll() to make sure that constantly arriving >> new I/O doesn't cause starvation. The changes from this series are at >> odds with that, but there doesn't seem to be any (new) test failures. > > I don't see why they are at odds with it? Draining an already drained > node isn't a problem, it just increases the counter without doing > anything else.
What I mean is: the introduction of calls to bdrv_drain_all_begin() before bdrv_drain_all_begin_nopoll() could introduce potential for starvation when there is constantly arriving new I/O. Or is this not true? >> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h >> index 2c26c72108..f291ccbc97 100644 >> --- a/include/block/graph-lock.h >> +++ b/include/block/graph-lock.h >> @@ -108,17 +108,21 @@ void unregister_aiocontext(AioContext *ctx); >> * >> * The wrlock can only be taken from the main loop, with BQL held, as only >> the >> * main loop is allowed to modify the graph. >> + * >> + * @drain whether bdrv_drain_all_begin() should be called before taking the >> lock >> */ >> void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA >> -bdrv_graph_wrlock(void); >> +bdrv_graph_wrlock(bool drain); > > I would prefer having two separate functions instead of a bool > parameter. > > bdrv_graph_wrlock() could stay as it is, and bdrv_graph_wrlock_drained() > could be the convenience wrapper that drains first. Will do! >> /* >> * bdrv_graph_wrunlock: >> * Write finished, reset global has_writer to 0 and restart >> * all readers that are waiting. >> + * >> + * @drain whether bdrv_drain_all_end() should be called after releasing the >> lock >> */ >> void no_coroutine_fn TSA_RELEASE(graph_lock) TSA_NO_TSA >> -bdrv_graph_wrunlock(void); >> +bdrv_graph_wrunlock(bool drain); > > Here I would prefer to only keep the old bdrv_graph_wrunlock() without > a parameter. Can we just remember @drain from bdrv_graph_wrlock() in a > global variable? This would prevent callers from mismatching lock and > unlock variants (which TSA wouldn't be able to catch). Okay. Best Regards, Fiona
