| Message ID | 20210419085541.22310-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block layer thread-safety, continued | expand |
On Mon, Apr 19, 2021 at 10:55:35AM +0200, Emanuele Giuseppe Esposito wrote: > For simplicity, use bdrv_drained_begin/end to avoid concurrent > writes to the write threshold, or reading it while it is being set. > qmp_block_set_write_threshold is protected by the BQL. > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/write-threshold.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/block/write-threshold.c b/block/write-threshold.c > index 63040fa4f2..77c74bdaa7 100644 > --- a/block/write-threshold.c > +++ b/block/write-threshold.c > @@ -111,7 +111,6 @@ void qmp_block_set_write_threshold(const char *node_name, > Error **errp) > { > BlockDriverState *bs; > - AioContext *aio_context; > > bs = bdrv_find_node(node_name); > if (!bs) { > @@ -119,10 +118,8 @@ void qmp_block_set_write_threshold(const char *node_name, > return; > } > > - aio_context = bdrv_get_aio_context(bs); > - aio_context_acquire(aio_context); > - > + /* Avoid a concurrent write_threshold_disable. */ > + bdrv_drained_begin(bs); Is there documentation that says it's safe to call bdrv_drained_begin(bs) without the AioContext acquired? AIO_WAIT_WHILE() contradicts this: The caller's thread must be the IOThread that owns @ctx or the main loop thread (with @ctx acquired exactly once). ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
On 05/05/21 10:55, Stefan Hajnoczi wrote: >> @@ -119,10 +118,8 @@ void qmp_block_set_write_threshold(const char *node_name, >> return; >> } >> >> - aio_context = bdrv_get_aio_context(bs); >> - aio_context_acquire(aio_context); >> - >> + /* Avoid a concurrent write_threshold_disable. */ >> + bdrv_drained_begin(bs); > > Is there documentation that says it's safe to call > bdrv_drained_begin(bs) without the AioContext acquired? AIO_WAIT_WHILE() > contradicts this: > > The caller's thread must be the IOThread that owns @ctx or the main loop > thread (with @ctx acquired exactly once). > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Well, I cannot remember why this patch was correct at the time I was working on them. Patches 7/8 on the other hand were okay because back then bdrv_reopen_multiple() called bdrv_drain_all_begin(). Overall, what survives of this series is patches 5 and 6, plus Vladimir's take on the write threshold code. Anyway, things have gotten _more_ broken in the meanwhile, and this is probably what causes the deadlocks that Emanuele has seen with the remainder of the branch. Since this patch: commit aa1361d54aac43094b98024b8b6c804eb6e41661 Author: Kevin Wolf <kwolf@redhat.com> Date: Fri Aug 17 18:54:18 2018 +0200 block: Add missing locking in bdrv_co_drain_bh_cb() bdrv_do_drained_begin/end() assume that they are called with the AioContext lock of bs held. If we call drain functions from a coroutine with the AioContext lock held, we yield and schedule a BH to move out of coroutine context. This means that the lock for the home context of the coroutine is released and must be re-acquired in the bottom half. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> AIO_WAIT_WHILE is going down a path that does not do the release/acquire of the AioContext, which can and will cause deadlocks when the main thread tries to acquire the AioContext and the I/O thread is in bdrv_co_drain. The message that introduced it does not help very much (https://mail.gnu.org/archive/html/qemu-devel/2018-09/msg01687.html) but I think this must be reverted. The next steps however should have less problems with bitrot, in particular the snapshots have changed very little. Block job code is very different but it is all in the main thread. Paolo
diff --git a/block/write-threshold.c b/block/write-threshold.c index 63040fa4f2..77c74bdaa7 100644 --- a/block/write-threshold.c +++ b/block/write-threshold.c @@ -111,7 +111,6 @@ void qmp_block_set_write_threshold(const char *node_name, Error **errp) { BlockDriverState *bs; - AioContext *aio_context; bs = bdrv_find_node(node_name); if (!bs) { @@ -119,10 +118,8 @@ void qmp_block_set_write_threshold(const char *node_name, return; } - aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); - + /* Avoid a concurrent write_threshold_disable. */ + bdrv_drained_begin(bs); bdrv_write_threshold_set(bs, threshold_bytes); - - aio_context_release(aio_context); + bdrv_drained_end(bs); }