07.06.2019 11:06, Kevin Wolf wrote: > Am 07.06.2019 um 05:17 hat Eric Blake geschrieben: >> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote: >>> +static coroutine_fn void nbd_reconnect_loop(NBDConnection *con) >>> +{ >>> + NBDClientSession *s = nbd_get_client_session(con->bs); >>> + uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); >>> + uint64_t delay_ns = s->reconnect_delay * 1000000000UL; >> >> Do we have a #define constant for nanoseconds in a second to make this >> more legible than counting '0's? >> >>> + uint64_t timeout = 1000000000UL; /* 1 sec */ >>> + uint64_t max_timeout = 16000000000UL; /* 16 sec */ >> >> 1 * constant, 16 * constant >> >>> + >>> + nbd_reconnect_attempt(con); >>> + >>> + while (nbd_client_connecting(s)) { >>> + if (s->state == NBD_CLIENT_CONNECTING_WAIT && >>> + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > >>> delay_ns) >>> + { >>> + s->state = NBD_CLIENT_CONNECTING_NOWAIT; >>> + qemu_co_queue_restart_all(&s->free_sema); >>> + } >>> + >>> + bdrv_dec_in_flight(con->bs); >>> + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout); >> >> Another place where I'd like someone more familiar with coroutines to >> also have a look. > > What's the exact question you'd like me to answer? > > But anyway, bdrv_dec/inc_in_flight() around the sleep looks wrong to me. > Either the operation must be waited for in drain, then you can't > decrease the counter even during the sleep. Or drain doesn't have to > consider it, then why is the counter even increased in the first place? > > The way it currently is, drain can return assuming that there are no > requests, but after the timeout the request automatically comes back > while the drain caller assumes that there is no more activity. This > doesn't look right. >
Hmm. This ind/dec around all lifetime of connection coroutine you added in commit 5ad81b4946baf948c65cf7e1436d9b74803c1280 Author: Kevin Wolf <kw...@redhat.com> Date: Fri Feb 15 16:53:51 2019 +0100 nbd: Restrict connection_co reentrance And now I try to connect in endless loop, with qemu_co_sleep_ns() inside. I need to get a change to bdrv_drain to complete, so I have to sometimes drop this in_flight request. But I understand your point. Will the following work better? bdrv_dec_in_flight(con->bs); qemu_co_sleep_ns(...); while (s->drained) { s->wait_drained_end = true; qemu_coroutine_yield(); } bdrv_inc_in_flight(con->bs); ... .drained_begin() { s->drained = true; } .drained_end() { if (s->wait_drained_end) { s->wait_drained_end = false; aio_co_wake(s->connection_co); } } -- Best regards, Vladimir