| Message ID | 20210315060611.2989049-7-rvkagan@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/nbd: decouple reconnect from drain | expand |
15.03.2021 09:06, Roman Kagan wrote: > The reconnection logic doesn't need to stop while in a drained section. > Moreover it has to be active during the drained section, as the requests > that were caught in-flight with the connection to the server broken can > only usefully get drained if the connection is restored. Otherwise such > requests can only either stall resulting in a deadlock (before > 8c517de24a), or be aborted defeating the purpose of the reconnection > machinery (after 8c517de24a). > > Since the pieces of the reconnection logic are now properly migrated > from one aio_context to another, it appears safe to just stop messing > with the drained section in the reconnection code. > > Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance") I'd not think that it "fixes" it. Behavior changes.. But 5ad81b4946 didn't introduce any bugs. > Fixes: 8c517de24a ("block/nbd: fix drain dead-lock because of nbd reconnect-delay") And here.. 1. There is an existing problem (unrelated to nbd) in Qemu that long io request which we have to wait for at drained_begin may trigger a dead lock (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html) 2. So, when we have nbd reconnect (and therefore long io requests) we simply trigger this deadlock.. That's why I decided to cancel the requests (assuming they will most probably fail anyway). I agree that nbd driver is wrong place for fixing the problem described in (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html), but if you just revert 8c517de24a, you'll see the deadlock again..
On Mon, Mar 15, 2021 at 11:10:14PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 15.03.2021 09:06, Roman Kagan wrote: > > The reconnection logic doesn't need to stop while in a drained section. > > Moreover it has to be active during the drained section, as the requests > > that were caught in-flight with the connection to the server broken can > > only usefully get drained if the connection is restored. Otherwise such > > requests can only either stall resulting in a deadlock (before > > 8c517de24a), or be aborted defeating the purpose of the reconnection > > machinery (after 8c517de24a). > > > > Since the pieces of the reconnection logic are now properly migrated > > from one aio_context to another, it appears safe to just stop messing > > with the drained section in the reconnection code. > > > > Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance") > > I'd not think that it "fixes" it. Behavior changes.. But 5ad81b4946 > didn't introduce any bugs. I guess you're right. In fact I did reproduce the situation when the drain made no progress exactly becase the only in-flight reference was taken by the connection_co, but it may be due to some intermediate stage of the patch development so I need to do a more thorough analysis to tell if it was triggerable with the original code. > > Fixes: 8c517de24a ("block/nbd: fix drain dead-lock because of nbd reconnect-delay") > > And here.. > > 1. There is an existing problem (unrelated to nbd) in Qemu that long > io request which we have to wait for at drained_begin may trigger a > dead lock > (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html) > > 2. So, when we have nbd reconnect (and therefore long io requests) we > simply trigger this deadlock.. That's why I decided to cancel the > requests (assuming they will most probably fail anyway). > > I agree that nbd driver is wrong place for fixing the problem > described in > (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html), > but if you just revert 8c517de24a, you'll see the deadlock again.. I may have misunderstood that thread, but isn't that deadlock exactly due to the requests being unable to ever drain because the reconnection process is suspended while the drain is in progress? Thanks, Roman.
16.03.2021 19:03, Roman Kagan wrote: > On Mon, Mar 15, 2021 at 11:10:14PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> 15.03.2021 09:06, Roman Kagan wrote: >>> The reconnection logic doesn't need to stop while in a drained section. >>> Moreover it has to be active during the drained section, as the requests >>> that were caught in-flight with the connection to the server broken can >>> only usefully get drained if the connection is restored. Otherwise such >>> requests can only either stall resulting in a deadlock (before >>> 8c517de24a), or be aborted defeating the purpose of the reconnection >>> machinery (after 8c517de24a). >>> >>> Since the pieces of the reconnection logic are now properly migrated >>> from one aio_context to another, it appears safe to just stop messing >>> with the drained section in the reconnection code. >>> >>> Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance") >> >> I'd not think that it "fixes" it. Behavior changes.. But 5ad81b4946 >> didn't introduce any bugs. > > I guess you're right. > > In fact I did reproduce the situation when the drain made no progress > exactly becase the only in-flight reference was taken by the > connection_co, but it may be due to some intermediate stage of the patch > development so I need to do a more thorough analysis to tell if it was > triggerable with the original code. > >>> Fixes: 8c517de24a ("block/nbd: fix drain dead-lock because of nbd reconnect-delay") >> >> And here.. >> >> 1. There is an existing problem (unrelated to nbd) in Qemu that long >> io request which we have to wait for at drained_begin may trigger a >> dead lock >> (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html) >> >> 2. So, when we have nbd reconnect (and therefore long io requests) we >> simply trigger this deadlock.. That's why I decided to cancel the >> requests (assuming they will most probably fail anyway). >> >> I agree that nbd driver is wrong place for fixing the problem >> described in >> (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html), >> but if you just revert 8c517de24a, you'll see the deadlock again.. > > I may have misunderstood that thread, but isn't that deadlock exactly > due to the requests being unable to ever drain because the > reconnection process is suspended while the drain is in progress? > Hmm, I didn't thought about it this way. What you mean is that reconnection is cancelled on drain_begin, so drain_begin will never finish, because it waits for requests, which will never be reconnected. So, you are right it's a deadlock too. But as I remember what is described in 8c517de24a is another deadlock, triggered by "just a long request during drain_begin". And it may be triggered again, if the we'll try to reconnect for several seconds during drained_begin() instead of cancelling requests. Didn't you try the scenario described in 8c517de24a on top of your series?
On Tue, Mar 16, 2021 at 09:09:12PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 16.03.2021 19:03, Roman Kagan wrote: > > On Mon, Mar 15, 2021 at 11:10:14PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > 15.03.2021 09:06, Roman Kagan wrote: > > > > The reconnection logic doesn't need to stop while in a drained section. > > > > Moreover it has to be active during the drained section, as the requests > > > > that were caught in-flight with the connection to the server broken can > > > > only usefully get drained if the connection is restored. Otherwise such > > > > requests can only either stall resulting in a deadlock (before > > > > 8c517de24a), or be aborted defeating the purpose of the reconnection > > > > machinery (after 8c517de24a). > > > > > > > > Since the pieces of the reconnection logic are now properly migrated > > > > from one aio_context to another, it appears safe to just stop messing > > > > with the drained section in the reconnection code. > > > > > > > > Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance") > > > > > > I'd not think that it "fixes" it. Behavior changes.. But 5ad81b4946 > > > didn't introduce any bugs. > > > > I guess you're right. > > > > In fact I did reproduce the situation when the drain made no progress > > exactly becase the only in-flight reference was taken by the > > connection_co, but it may be due to some intermediate stage of the patch > > development so I need to do a more thorough analysis to tell if it was > > triggerable with the original code. > > > > > > Fixes: 8c517de24a ("block/nbd: fix drain dead-lock because of nbd reconnect-delay") > > > > > > And here.. > > > > > > 1. There is an existing problem (unrelated to nbd) in Qemu that long > > > io request which we have to wait for at drained_begin may trigger a > > > dead lock > > > (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html) > > > > > > 2. So, when we have nbd reconnect (and therefore long io requests) we > > > simply trigger this deadlock.. That's why I decided to cancel the > > > requests (assuming they will most probably fail anyway). > > > > > > I agree that nbd driver is wrong place for fixing the problem > > > described in > > > (https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg01339.html), > > > but if you just revert 8c517de24a, you'll see the deadlock again.. > > > > I may have misunderstood that thread, but isn't that deadlock exactly > > due to the requests being unable to ever drain because the > > reconnection process is suspended while the drain is in progress? > > > > > Hmm, I didn't thought about it this way. What you mean is that > reconnection is cancelled on drain_begin, so drain_begin will never > finish, because it waits for requests, which will never be > reconnected. So, you are right it's a deadlock too. Right. > But as I remember what is described in 8c517de24a is another deadlock, > triggered by "just a long request during drain_begin". And it may be > triggered again, if the we'll try to reconnect for several seconds > during drained_begin() instead of cancelling requests. IMO it wasn't a different deadlock, it wass exactly this one: the drain got stuck the way described above with the qemu global mutex taken, so the rest of qemu got stuck too. > Didn't you try the scenario described in 8c517de24a on top of your > series? I've tried it with the current master with just 8c517de24a reverted -- the deadlock reproduced in all several attempts. Then with my series -- the deadlock didn't reproduce in any of my several attempts. More precisely, qemu appeared frozen once the guest timed out the requests and initiated ATA link soft reset, which presumably caused that drain, but, as soon as the reconnect timer expired (resulting in requests completed into the guest with errors) or the connection was restored (resulting in requests completed successfully), it resumed normal operation. So yes, I think this series does address that deadlock. Thanks, Roman.
diff --git a/block/nbd.c b/block/nbd.c index a3eb9b9079..a5a9e4aca5 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -117,8 +117,6 @@ typedef struct BDRVNBDState { Coroutine *connection_co; Coroutine *teardown_co; QemuCoSleepState *connection_co_sleep_ns_state; - bool drained; - bool wait_drained_end; int in_flight; NBDClientState state; int connect_status; @@ -311,12 +309,6 @@ static void nbd_client_attach_aio_context_bh(void *opaque) qemu_mutex_unlock(&thr->mutex); if (s->connection_co) { - /* - * The node is still drained, so we know the coroutine has yielded in - * nbd_read_eof(), the only place where bs->in_flight can reach 0, or - * it is entered for the first time. Both places are safe for entering - * the coroutine. - */ qemu_aio_coroutine_enter(bs->aio_context, s->connection_co); } bdrv_dec_in_flight(bs); @@ -344,37 +336,6 @@ static void nbd_client_attach_aio_context(BlockDriverState *bs, aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs); } -static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs) -{ - BDRVNBDState *s = (BDRVNBDState *)bs->opaque; - - s->drained = true; - if (s->connection_co_sleep_ns_state) { - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); - } - - nbd_co_establish_connection_cancel(bs, false); - - reconnect_delay_timer_del(s); - - if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) { - s->state = NBD_CLIENT_CONNECTING_NOWAIT; - qemu_co_queue_restart_all(&s->free_sema); - } -} - -static void coroutine_fn nbd_client_co_drain_end(BlockDriverState *bs) -{ - BDRVNBDState *s = (BDRVNBDState *)bs->opaque; - - s->drained = false; - if (s->wait_drained_end) { - s->wait_drained_end = false; - aio_co_wake(s->connection_co); - } -} - - static void nbd_teardown_connection(BlockDriverState *bs) { BDRVNBDState *s = (BDRVNBDState *)bs->opaque; @@ -686,16 +647,6 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s) ret = nbd_client_handshake(s->bs, &local_err); - if (s->drained) { - s->wait_drained_end = true; - while (s->drained) { - /* - * We may be entered once from nbd_client_attach_aio_context_bh - * and then from nbd_client_co_drain_end. So here is a loop. - */ - qemu_coroutine_yield(); - } - } bdrv_inc_in_flight(s->bs); out: @@ -724,26 +675,10 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s) nbd_reconnect_attempt(s); while (nbd_client_connecting(s)) { - if (s->drained) { - bdrv_dec_in_flight(s->bs); - s->wait_drained_end = true; - while (s->drained) { - /* - * We may be entered once from nbd_client_attach_aio_context_bh - * and then from nbd_client_co_drain_end. So here is a loop. - */ - qemu_coroutine_yield(); - } - bdrv_inc_in_flight(s->bs); - } else { - qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout, - &s->connection_co_sleep_ns_state); - if (s->drained) { - continue; - } - if (timeout < max_timeout) { - timeout *= 2; - } + qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout, + &s->connection_co_sleep_ns_state); + if (timeout < max_timeout) { + timeout *= 2; } nbd_reconnect_attempt(s); @@ -2548,8 +2483,6 @@ static BlockDriver bdrv_nbd = { .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_client_detach_aio_context, .bdrv_attach_aio_context = nbd_client_attach_aio_context, - .bdrv_co_drain_begin = nbd_client_co_drain_begin, - .bdrv_co_drain_end = nbd_client_co_drain_end, .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, @@ -2577,8 +2510,6 @@ static BlockDriver bdrv_nbd_tcp = { .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_client_detach_aio_context, .bdrv_attach_aio_context = nbd_client_attach_aio_context, - .bdrv_co_drain_begin = nbd_client_co_drain_begin, - .bdrv_co_drain_end = nbd_client_co_drain_end, .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, @@ -2606,8 +2537,6 @@ static BlockDriver bdrv_nbd_unix = { .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_client_detach_aio_context, .bdrv_attach_aio_context = nbd_client_attach_aio_context, - .bdrv_co_drain_begin = nbd_client_co_drain_begin, - .bdrv_co_drain_end = nbd_client_co_drain_end, .bdrv_refresh_filename = nbd_refresh_filename, .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname,
The reconnection logic doesn't need to stop while in a drained section. Moreover it has to be active during the drained section, as the requests that were caught in-flight with the connection to the server broken can only usefully get drained if the connection is restored. Otherwise such requests can only either stall resulting in a deadlock (before 8c517de24a), or be aborted defeating the purpose of the reconnection machinery (after 8c517de24a). Since the pieces of the reconnection logic are now properly migrated from one aio_context to another, it appears safe to just stop messing with the drained section in the reconnection code. Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance") Fixes: 8c517de24a ("block/nbd: fix drain dead-lock because of nbd reconnect-delay") Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> --- block/nbd.c | 79 +++-------------------------------------------------- 1 file changed, 4 insertions(+), 75 deletions(-)