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

[Kevin Wolf]

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.

Kevin

signature.asc
Description: PGP signature