Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
On 6/10/19 8:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
>> it, and now I don't see any reason for it. We store this information anyway
>> for the whole life of the driver..
>>
>> So, if I'm going to refactor it, I have a question: is there a reason for
>> layered BDRVNBDState:
>>
>> typedef struct BDRVNBDState {
>> NBDClientSession client;
>>
>> /* For nbd_refresh_filename() */
>> SocketAddress *saddr;
>> char *export, *tlscredsid;
>> } BDRVNBDState;
>>
>> and use nbd_get_client_session everywhere instead of simple converting void
>> *opaque
>> to State pointer like in qcow2 for example?
>>
>> The only reason I can imagine is not to share the whole BDRVNBDState, and
>> keep
>> things we are using only in nbd.c to be available only in nbd.c.. But I've
>> put
>> saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I
>> think
>> it's a good moment to flatten and share BDRVNBDState and drop
>> nbd_get_client_session.
>>
>
> And if we are here, what is exact purpose of splitting nbd-client.c from
> nbd.c?
I see no strong reasons to keep the separation. If a single large file
is easier to maintain than an arbitrary split between two files, I'm
open to the idea of a patch that undoes the split.
> or need of defining driver callbacks in header file, instead of keeping them
> together
> with driver struct definition and static (like other block drivers)...
>
>
> And names of these two files always confused me.. nbd.c is nbd client block
> driver, and
> driver is defined here.. But we have nbd-client.c which defines nbd client
> driver too.
> And we also have nbd/client.c (OK, this split I understand of course, but it
> increases
> confusion anyway).
I'm also toying with the idea of writing block/nbd.c to utilize the
relatively-new libnbd library, to see if there are any differences in
behavior by offloading NBD access to a 3rd-party library. But that's
further down the road.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
10.06.2019 15:38, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 6:17, Eric Blake wrote:
>>> +typedef struct NBDConnection {
>>> + BlockDriverState *bs;
>>> + SocketAddress *saddr;
>>> + const char *export;
>>> + QCryptoTLSCreds *tlscreds;
>>> + const char *hostname;
>>> + const char *x_dirty_bitmap;
>>> +} NBDConnection;
>> Can we put this type in a header, and use it instead of passing lots of
>> individual parameters to nbd_client_connect()? Probably as a separate
>> pre-requisite cleanup patch.
>>
>
>
> Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
> it, and now I don't see any reason for it. We store this information anyway
> for the whole life of the driver..
>
> So, if I'm going to refactor it, I have a question: is there a reason for
> layered BDRVNBDState:
>
> typedef struct BDRVNBDState {
> NBDClientSession client;
>
> /* For nbd_refresh_filename() */
> SocketAddress *saddr;
> char *export, *tlscredsid;
> } BDRVNBDState;
>
> and use nbd_get_client_session everywhere instead of simple converting void
> *opaque
> to State pointer like in qcow2 for example?
>
> The only reason I can imagine is not to share the whole BDRVNBDState, and keep
> things we are using only in nbd.c to be available only in nbd.c.. But I've put
> saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I
> think
> it's a good moment to flatten and share BDRVNBDState and drop
> nbd_get_client_session.
>
And if we are here, what is exact purpose of splitting nbd-client.c from nbd.c?
I see, it was long ago:
commit 2302c1cafb13df23938b098d9c6595de52ec2577
Author: Marc-André Lureau
Date: Sun Dec 1 22:23:41 2013 +0100
Split nbd block client code
But still, it doesn't explain..
And all this leads to noop wrappers like this
static int nbd_co_flush(BlockDriverState *bs)
{
return nbd_client_co_flush(bs);
}
static void nbd_detach_aio_context(BlockDriverState *bs)
{
nbd_client_detach_aio_context(bs);
}
static void nbd_attach_aio_context(BlockDriverState *bs,
AioContext *new_context)
{
nbd_client_attach_aio_context(bs, new_context);
}
or need of defining driver callbacks in header file, instead of keeping them
together
with driver struct definition and static (like other block drivers)...
And names of these two files always confused me.. nbd.c is nbd client block
driver, and
driver is defined here.. But we have nbd-client.c which defines nbd client
driver too.
And we also have nbd/client.c (OK, this split I understand of course, but it
increases
confusion anyway).
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
07.06.2019 6:17, Eric Blake wrote:
>> +typedef struct NBDConnection {
>> +BlockDriverState *bs;
>> +SocketAddress *saddr;
>> +const char *export;
>> +QCryptoTLSCreds *tlscreds;
>> +const char *hostname;
>> +const char *x_dirty_bitmap;
>> +} NBDConnection;
> Can we put this type in a header, and use it instead of passing lots of
> individual parameters to nbd_client_connect()? Probably as a separate
> pre-requisite cleanup patch.
>
Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
it, and now I don't see any reason for it. We store this information anyway
for the whole life of the driver..
So, if I'm going to refactor it, I have a question: is there a reason for
layered BDRVNBDState:
typedef struct BDRVNBDState {
NBDClientSession client;
/* For nbd_refresh_filename() */
SocketAddress *saddr;
char *export, *tlscredsid;
} BDRVNBDState;
and use nbd_get_client_session everywhere instead of simple converting void
*opaque
to State pointer like in qcow2 for example?
The only reason I can imagine is not to share the whole BDRVNBDState, and keep
things we are using only in nbd.c to be available only in nbd.c.. But I've put
saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I think
it's a good moment to flatten and share BDRVNBDState and drop
nbd_get_client_session.
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
Am 07.06.2019 um 16:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.06.2019 16:26, Kevin Wolf wrote:
> > Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 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 * 10UL;
>
> Do we have a #define constant for nanoseconds in a second to make this
> more legible than counting '0's?
>
> > +uint64_t timeout = 10UL; /* 1 sec */
> > +uint64_t max_timeout = 160UL; /* 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(>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
> >> 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.
> >
> > Ah, right, I see. I think it's fine to add a second point where we
> > decrease the counter (though I would add a comment telling why we do
> > this) as long as the right conditions are met.
> >
> > The right conditions are probably something like: Once drained, we
> > guarantee that we don't call any callbacks until the drained section
> > ends. In nbd_read_eof() this is true because we can't get an answer if
> > we had no request running.
> >
> > Or actually... This assumes a nice compliant server that doesn't just
> > arbitrarily send unexpected messages. Is the existing code broken if we
> > connect to a malicious server?
> >
> >> 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);
> >> }
> >> }
> >
> > This should make sure that we don't call any callbacks before the drain
> > section has ended.
> >
> > We probably still have a problem because nbd_client_attach_aio_context()
> > reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
> > an assertion failure if co->scheduled is set. So this needs to use a
> > version that can cancel a sleep.
> >
> > I see that your patch currently simply ignores attaching a new context,
> > but then the coroutine stays in the old AioContext. Did I miss some
> > additional code that moves it to the new context somehow or will it just
> > stay where it was if the coroutine happens to be reconnecting when the
> > AioContext was supposed to change?
>
>
> Hmm. Do you mean "in latter case we do nothing" in
>
>void nbd_client_attach_aio_context(BlockDriverState *bs,
> AioContext *new_context)
>{
>NBDClientSession *client = nbd_get_client_session(bs);
>
>/*
> * client->connection_co is either yielded from nbd_receive_reply or
> from
> * nbd_reconnect_loop(), in latter case
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
07.06.2019 16:26, Kevin Wolf wrote:
> Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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 * 10UL;
Do we have a #define constant for nanoseconds in a second to make this
more legible than counting '0's?
> +uint64_t timeout = 10UL; /* 1 sec */
> +uint64_t max_timeout = 160UL; /* 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(>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
>> 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.
>
> Ah, right, I see. I think it's fine to add a second point where we
> decrease the counter (though I would add a comment telling why we do
> this) as long as the right conditions are met.
>
> The right conditions are probably something like: Once drained, we
> guarantee that we don't call any callbacks until the drained section
> ends. In nbd_read_eof() this is true because we can't get an answer if
> we had no request running.
>
> Or actually... This assumes a nice compliant server that doesn't just
> arbitrarily send unexpected messages. Is the existing code broken if we
> connect to a malicious server?
>
>> 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);
>> }
>> }
>
> This should make sure that we don't call any callbacks before the drain
> section has ended.
>
> We probably still have a problem because nbd_client_attach_aio_context()
> reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
> an assertion failure if co->scheduled is set. So this needs to use a
> version that can cancel a sleep.
>
> I see that your patch currently simply ignores attaching a new context,
> but then the coroutine stays in the old AioContext. Did I miss some
> additional code that moves it to the new context somehow or will it just
> stay where it was if the coroutine happens to be reconnecting when the
> AioContext was supposed to change?
Hmm. Do you mean "in latter case we do nothing" in
void nbd_client_attach_aio_context(BlockDriverState *bs,
AioContext *new_context)
{
NBDClientSession *client = nbd_get_client_session(bs);
/*
* client->connection_co is either yielded from nbd_receive_reply or from
* nbd_reconnect_loop(), in latter case we do nothing
*/
if (client->state == NBD_CLIENT_CONNECTED) {
qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc),
new_context);
bdrv_inc_in_flight(bs);
/*
* Need to wait here for the BH to run because the BH must run while
the
* node is
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
Am 07.06.2019 um 14:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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 * 10UL;
> >>
> >> Do we have a #define constant for nanoseconds in a second to make this
> >> more legible than counting '0's?
> >>
> >>> +uint64_t timeout = 10UL; /* 1 sec */
> >>> +uint64_t max_timeout = 160UL; /* 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(>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
> 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.
Ah, right, I see. I think it's fine to add a second point where we
decrease the counter (though I would add a comment telling why we do
this) as long as the right conditions are met.
The right conditions are probably something like: Once drained, we
guarantee that we don't call any callbacks until the drained section
ends. In nbd_read_eof() this is true because we can't get an answer if
we had no request running.
Or actually... This assumes a nice compliant server that doesn't just
arbitrarily send unexpected messages. Is the existing code broken if we
connect to a malicious server?
> 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);
> }
> }
This should make sure that we don't call any callbacks before the drain
section has ended.
We probably still have a problem because nbd_client_attach_aio_context()
reenters the coroutine with qemu_aio_coroutine_enter(), which will cause
an assertion failure if co->scheduled is set. So this needs to use a
version that can cancel a sleep.
I see that your patch currently simply ignores attaching a new context,
but then the coroutine stays in the old AioContext. Did I miss some
additional code that moves it to the new context somehow or will it just
stay where it was if the coroutine happens to be reconnecting when the
AioContext was supposed to change?
Kevin
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
07.06.2019 6:17, Eric Blake wrote:
> On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Implement reconnect. To achieve this:
>>
>> 1. add new modes:
>> connecting-wait: means, that reconnecting is in progress, and there
>> were small number of reconnect attempts, so all requests are
>> waiting for the connection.
>> connecting-nowait: reconnecting is in progress, there were a lot of
>> attempts of reconnect, all requests will return errors.
>>
>> two old modes are used too:
>> connected: normal state
>> quit: exiting after fatal error or on close
>>
>> Possible transitions are:
>>
>> * -> quit
>> connecting-* -> connected
>> connecting-wait -> connecting-nowait (transition is done after
>>reconnect-delay seconds in connecting-wait mode)
>> connected -> connecting-wait
>>
>> 2. Implement reconnect in connection_co. So, in connecting-* mode,
>> connection_co, tries to reconnect unlimited times.
>>
>> 3. Retry nbd queries on channel error, if we are in connecting-wait
>> state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>
> Does this also mean that we can start queuing up guest I/O even before
> the first time connected is reached?
No, we decided that it's simpler and clearer way to keep first connect to
be synchronous in nbd_open.
>
>> block/nbd-client.h | 7 +
>> block/nbd-client.c | 336 +++--
>> 2 files changed, 273 insertions(+), 70 deletions(-)
>>
>
>> +++ b/block/nbd-client.c
>> @@ -1,6 +1,7 @@
>> /*
>>* QEMU Block driver for NBD
>>*
>> + * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>
> Adding copyright is fine - you are indeed adding a non-trivial amount of
> code to this file. Adding "All rights reserved" is questionable, in part
> because it no longer has legal status (see this recent nbdkit patch
> https://github.com/libguestfs/nbdkit/commit/952ffe0fc7 for example).
>
> Furthermore, I really cringe when I see it mixed with GPL, because the
> GPL works by explicitly stating that you are NOT reserving all rights,
> but are rather granting specific permissions to recipients. However, as
> this file is BSD licensed, and the various family tree of BSD licenses
> have (often due to copy-and-paste) used this phrase in the past, I'm not
> going to reject the patch because of the phrase, even though I can
> definitely ask if you can remove it.
Hmm, I think it's not a problem to drop "All rights reserved".
>
>> @@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState
>> *bs)
>> {
>> NBDClientSession *client = nbd_get_client_session(bs);
>>
>> -assert(client->ioc);
>> -
>> -/* finish any pending coroutines */
>> -qio_channel_shutdown(client->ioc,
>> - QIO_CHANNEL_SHUTDOWN_BOTH,
>> - NULL);
>> +if (client->state == NBD_CLIENT_CONNECTED) {
>> +/* finish any pending coroutines */
>> +assert(client->ioc);
>> +qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>> +}
>> +client->state = NBD_CLIENT_QUIT;
>> +if (client->connection_co) {
>> +qemu_co_sleep_wake(client->connection_co);
>> +}
>> BDRV_POLL_WHILE(bs, client->connection_co);
>
> So you are using the qemu_co_sleep_wake code as a way to in effect
> cancel any ongoing sleep. I'm still not sure if there is already another
> way to achieve the same effect, perhaps by re-entering the coroutine?
It marked as scheduled by qemu_co_sleep_ns and can't be entered, due to this
code
in qemu_aio_coroutine_enter:
128 if (scheduled) {
129 fprintf(stderr,
130 "%s: Co-routine was already scheduled in '%s'\n",
131 __func__, scheduled);
132 abort();
133 }
>
>> +typedef struct NBDConnection {
>> +BlockDriverState *bs;
>> +SocketAddress *saddr;
>> +const char *export;
>> +QCryptoTLSCreds *tlscreds;
>> +const char *hostname;
>> +const char *x_dirty_bitmap;
>> +} NBDConnection;
>
> Can we put this type in a header, and use it instead of passing lots of
> individual parameters to nbd_client_connect()? Probably as a separate
> pre-requisite cleanup patch.
Will try
>
>> +
>> +static bool nbd_client_connecting(NBDClientSession *client)
>> +{
>> +return client->state == NBD_CLIENT_CONNECTING_WAIT ||
>> + client->state == NBD_CLIENT_CONNECTING_NOWAIT;
>> +}
>
> I don't know what style we prefer to use here. If my returns split
> across lines, I tend to go with either 4-space indent instead of 7, or
> to use () so that the second line is indented to the column after (; but
> this is aesthetics and so I'm not going to change what you have.
Not a problem for me to change, if you dislike)
>
>> +
>> +static bool nbd_client_connecting_wait(NBDClientSession *client)
>> +{
>>
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
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 * 10UL;
>>
>> Do we have a #define constant for nanoseconds in a second to make this
>> more legible than counting '0's?
>>
>>> +uint64_t timeout = 10UL; /* 1 sec */
>>> +uint64_t max_timeout = 160UL; /* 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(>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
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
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
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 * 10UL;
>
> Do we have a #define constant for nanoseconds in a second to make this
> more legible than counting '0's?
>
> > +uint64_t timeout = 10UL; /* 1 sec */
> > +uint64_t max_timeout = 160UL; /* 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(>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
Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect
On 4/11/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> Implement reconnect. To achieve this:
>
> 1. add new modes:
>connecting-wait: means, that reconnecting is in progress, and there
> were small number of reconnect attempts, so all requests are
> waiting for the connection.
>connecting-nowait: reconnecting is in progress, there were a lot of
> attempts of reconnect, all requests will return errors.
>
>two old modes are used too:
>connected: normal state
>quit: exiting after fatal error or on close
>
> Possible transitions are:
>
>* -> quit
>connecting-* -> connected
>connecting-wait -> connecting-nowait (transition is done after
> reconnect-delay seconds in connecting-wait mode)
>connected -> connecting-wait
>
> 2. Implement reconnect in connection_co. So, in connecting-* mode,
> connection_co, tries to reconnect unlimited times.
>
> 3. Retry nbd queries on channel error, if we are in connecting-wait
> state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
Does this also mean that we can start queuing up guest I/O even before
the first time connected is reached?
> block/nbd-client.h | 7 +
> block/nbd-client.c | 336 +++--
> 2 files changed, 273 insertions(+), 70 deletions(-)
>
> +++ b/block/nbd-client.c
> @@ -1,6 +1,7 @@
> /*
> * QEMU Block driver for NBD
> *
> + * Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
Adding copyright is fine - you are indeed adding a non-trivial amount of
code to this file. Adding "All rights reserved" is questionable, in part
because it no longer has legal status (see this recent nbdkit patch
https://github.com/libguestfs/nbdkit/commit/952ffe0fc7 for example).
Furthermore, I really cringe when I see it mixed with GPL, because the
GPL works by explicitly stating that you are NOT reserving all rights,
but are rather granting specific permissions to recipients. However, as
this file is BSD licensed, and the various family tree of BSD licenses
have (often due to copy-and-paste) used this phrase in the past, I'm not
going to reject the patch because of the phrase, even though I can
definitely ask if you can remove it.
> @@ -59,24 +77,133 @@ static void nbd_teardown_connection(BlockDriverState *bs)
> {
> NBDClientSession *client = nbd_get_client_session(bs);
>
> -assert(client->ioc);
> -
> -/* finish any pending coroutines */
> -qio_channel_shutdown(client->ioc,
> - QIO_CHANNEL_SHUTDOWN_BOTH,
> - NULL);
> +if (client->state == NBD_CLIENT_CONNECTED) {
> +/* finish any pending coroutines */
> +assert(client->ioc);
> +qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +client->state = NBD_CLIENT_QUIT;
> +if (client->connection_co) {
> +qemu_co_sleep_wake(client->connection_co);
> +}
> BDRV_POLL_WHILE(bs, client->connection_co);
So you are using the qemu_co_sleep_wake code as a way to in effect
cancel any ongoing sleep. I'm still not sure if there is already another
way to achieve the same effect, perhaps by re-entering the coroutine?
> +typedef struct NBDConnection {
> +BlockDriverState *bs;
> +SocketAddress *saddr;
> +const char *export;
> +QCryptoTLSCreds *tlscreds;
> +const char *hostname;
> +const char *x_dirty_bitmap;
> +} NBDConnection;
Can we put this type in a header, and use it instead of passing lots of
individual parameters to nbd_client_connect()? Probably as a separate
pre-requisite cleanup patch.
> +
> +static bool nbd_client_connecting(NBDClientSession *client)
> +{
> +return client->state == NBD_CLIENT_CONNECTING_WAIT ||
> + client->state == NBD_CLIENT_CONNECTING_NOWAIT;
> +}
I don't know what style we prefer to use here. If my returns split
across lines, I tend to go with either 4-space indent instead of 7, or
to use () so that the second line is indented to the column after (; but
this is aesthetics and so I'm not going to change what you have.
> +
> +static bool nbd_client_connecting_wait(NBDClientSession *client)
> +{
> +return client->state == NBD_CLIENT_CONNECTING_WAIT;
> +}
> +
> +static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
> +{
> +NBDClientSession *s = nbd_get_client_session(con->bs);
> +Error *local_err = NULL;
> +
> +if (!nbd_client_connecting(s)) {
> +return;
> +}
> +assert(nbd_client_connecting(s));
> +
> +/* Wait completion of all in-flight requests */
Wait for completion
> +
> +qemu_co_mutex_lock(>send_mutex);
>
> -nbd_client_detach_aio_context(bs);
> -object_unref(OBJECT(client->sioc));
> -client->sioc = NULL;
> -object_unref(OBJECT(client->ioc));
> -client->ioc = NULL;
> +while (s->in_flight > 0) {
> +qemu_co_mutex_unlock(>send_mutex);
> +nbd_recv_coroutines_wake_all(s);
> +
