Re: [RFC] nbd: decouple reconnect from drain

[Vladimir Sementsov-Ogievskiy]

15.03.2021 08:36, Roman Kagan wrote:
> On Fri, Mar 12, 2021 at 03:35:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 10.03.2021 12:32, Roman Kagan wrote:
> > > NBD connect coroutine takes an extra in_flight reference as if it's a
> > > request handler.  This prevents drain from completion until the
> > > connection coroutine is releases the reference.
> > >
> > > When NBD is configured to reconnect, however, this appears to be fatal
> > > to the reconnection idea: the drain procedure wants the reconnection to
> > > be suspended, but this is only possible if the in-flight requests are
> > > canceled.
> >
> > As I remember from our conversation, the problem is not that we don't
> > reconnect during drained section, but exactly that we cancel requests
> > on drained begins starting from 8c517de24a8a1dcbeb.
>
> Well, I'd put it the other way around: the problem is that we don't
> reconnect during the drained section, so the requests can't be
> successfully completed if the connection breaks: they can either stall
> forever (before 8c517de24a8a1dcbeb) or be aborted (after
> 8c517de24a8a1dcbeb).

The sense of the drained section is that we don't have any inflight requests 
during drained section.
So, all requests must be finished on drained_begin()..

We can continue reconnect process during drained section. But requests should 
be completed on drained_begin() anyway.

> > This is not a problem in scenarios when reconnect is rare case and
> > failed request is acceptable. But if we have bad connection and
> > requests should often wait for reconnect (so, it may be considered as
> > a kind of "latency") then really, cancelling the waiting requests on
> > any drain() kills the reconnect feature.
>
> In our experience the primary purpose of reconnect is not to withstand
> poor network links, but about being able to restart the NBD server
> without disrupting the guest operation.  So failing the requests during
> the server maintenance window is never acceptable, no matter how rare it
> is (and in our practice it isn't).
>
> > > Fix this by making the connection coroutine stop messing with the
> > > in-flight counter.  Instead, certain care is taken to properly move the
> > > reconnection stuff from one aio_context to another in
> > > .bdrv_{attach,detach}_aio_context callbacks.
> > >
> > > Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance")
> > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > ---
> > > This patch passes the regular make check but fails some extra iotests,
> > > in particular 277.  It obviously lacks more robust interaction with the
> > > connection thread (which in general is fairly complex and hard to reason
> > > about), and perhaps has some other drawbacks, so I'll work on this
> > > further, but I'd appreciate some feedback on whether the idea is sound.
> >
> > In general I like the idea. The logic around drain in nbd is
> > overcomplicated. And I never liked the fact that nbd_read_eof() does
> > dec-inc inflight section. Some notes:
> >
> > 1. I hope, the patch can be divided into several ones, as there are
> > several things done:
> >
> > - removing use of in_flight counter introduced by 5ad81b4946
> > - do reconnect during drained section
> > - stop cancelling requests on .drained_begin
>
> I've split the (somewhat extended) patch into a series of 7, but not
> exactly as you suggested.  In particular, I can't just stop aborting the
> requests on .drained_begin as this would reintroduce the deadlock, so I
> just remove the whole .drained_begin/end in a single patch.
>
> > 2. 5ad81b4946 was needed to make nbd_client_attach_aio_context()
> > reenter connection_co only in one (or two) possible places, not on any
> > yield.. And I don't see how it is achieved now.. This should be
> > described in commit msg..
>
> My problem is that I failed to figure out why it was necessary in the
> first place, so I think I don't achieve this with the series.

nbd_client_attach_aio_context() reenters connection_co. So we must be sure,
that on any yield() of connection_co it's safe to enter from
nbd_client_attach_aio_context(). Or somehow prevent enetering on unsafe
yields. So does 5ad81b4946: it prevents entering connection_co from any place
except two, where we decrease inflight counter. (amd we exploit the fact that
nbd_client_attach_aio_context() is called inside drained section.

> > 3. About cancelling requests on drained_begin. The behavior was
> > introduced by 8c517de24a8a1dcbeb, to fix a deadlock. So, if now the
> > deadlock is fixed another way, let's change the logic (don't cancel
> > requests) in a separate patch, and note 8c517de24a8a1dcbeb commit and
> > the commit that fixes deadlock the other way in the commit message.
>
> As I mentioned earlier I did a patch that removed the root cause; a
> separate patch removing just the request cancelation didn't look
> justified.  I did mention the commits in the log.
>
> Thanks for the review!  I'm now on to submitting a non-RFC version.
> Roman.


--
Best regards,
Vladimir