Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
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