qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC] nbd: decouple reconnect from drain


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [RFC] nbd: decouple reconnect from drain
Date: Mon, 15 Mar 2021 17:07:28 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]