Re: [PATCH v2 for-7.1 4/9] nbd: keep send_mutex/free_sema handling outside nbd_co_do_establish_connection

[Paolo Bonzini]

On 4/16/22 13:54, Vladimir Sementsov-Ogievskiy wrote:
> > @@ -187,9 +187,6 @@ static void reconnect_delay_timer_cb(void *opaque)
> >       if (qatomic_load_acquire(&s->state) == 
> > NBD_CLIENT_CONNECTING_WAIT) {
> >           s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> >           nbd_co_establish_connection_cancel(s->conn);
> > -        while (qemu_co_enter_next(&s->free_sema, NULL)) {
> > -            /* Resume all queued requests */
> > -        }
> >       }
> >       reconnect_delay_timer_del(s);
>
> Seems, removing the timer is not needed here too. We do 
> nbd_co_establish_connection_cancel(), and timer will be removed in 
> nbd_reconnect_attempt anyway.
>
> More over, we don't need to keep timer in the state at all: it should 
> become local thing for nbd_reconnect_attempt. A kind of call 
> nbd_co_do_establish_connection() with timeout. That could be refactored 
> later, using same way as in 4-5 patches of my "[PATCH v4 0/7] 
> copy-before-write: on-cbw-error and cbw-timeout" series.

Yes, good point.

> > nbd_reconnect_attempt(BDRVNBDState *s)
> >           s->ioc = NULL;
> >       }
> > -    nbd_co_do_establish_connection(s->bs, NULL);
> > +    qemu_co_mutex_unlock(&s->send_mutex);
> > +    nbd_co_do_establish_connection(s->bs, blocking, NULL);
> > +    qemu_co_mutex_lock(&s->send_mutex);
>
>
> Hmm. So, that breaks a critical section.
>
> We can do it because in_flight=1 and we are not connected => all other 
> requests will wait in the queue.
>
> Still. during nbd_co_do_establish_connection() state may become 
> CONNECTED. That theoretically means that parallel requests may start.
>
> Is it bad? Seems not.. Bad thing that comes into my mind is that 
> parallel request fails, and go to reconnect, and start own timer, but we 
> remove it now after locking the mutex back. But that's not possible as 
> parallel request should wait for in-flight=0 to start own 
> reconnect-attempt. And that is not possible, as we keep our in-flight 
> point.

Yes that was even intended. :>  Synchronizing on in_flight == 0 before 
reconnecting is the important bit that simplifies a lot of things.

> > @@ -2049,7 +2046,6 @@ static void 
> > nbd_cancel_in_flight(BlockDriverState *bs)
> >       if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
> >           s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> > -        qemu_co_queue_restart_all(&s->free_sema);
>
> As I understand, we always have one request running (or no requests at 
> all, but that's OK too) and it will wake all others (altogether, or they 
> will wake each other in a chain). So, we don't need to wake them here.

Exactly, the "let each coroutine wake up the next one" pattern can be 
generalized to the error cases because the wakeups cascade until 
in_flight becomes 0.

Paolo

> >       }
> >       nbd_co_establish_connection_cancel(s->conn);