From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: kwolf@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org,
mreitz@redhat.com, kraxel@redhat.com, den@openvz.org
Subject: Re: [PATCH 3/4] io/channel-socket: implement non-blocking connect
Date: Wed, 22 Jul 2020 15:43:54 +0300 [thread overview]
Message-ID: <cd5058fe-20d4-615e-8489-a30ac54d57de@virtuozzo.com> (raw)
In-Reply-To: <20200722112104.GC2324845@redhat.com>
22.07.2020 14:21, Daniel P. Berrangé wrote:
> On Wed, Jul 22, 2020 at 02:00:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 20.07.2020 21:29, Daniel P. Berrangé wrote:
>>> On Mon, Jul 20, 2020 at 09:07:14PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Utilize new socket API to make a non-blocking connect for inet sockets.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>> include/io/channel-socket.h | 14 +++++++
>>>> io/channel-socket.c | 74 +++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 88 insertions(+)
>>>>
>>>> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
>>>> index 777ff5954e..82e868bc02 100644
>>>> --- a/include/io/channel-socket.h
>>>> +++ b/include/io/channel-socket.h
>>>> @@ -94,6 +94,20 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
>>>> SocketAddress *addr,
>>>> Error **errp);
>>>> +/**
>>>> + * qio_channel_socket_connect_non_blocking_sync:
>>>> + * @ioc: the socket channel object
>>>> + * @addr: the address to connect to
>>>> + * @errp: pointer to a NULL-initialized error object
>>>> + *
>>>> + * Attempt to connect to the address @addr using non-blocking mode of
>>>> + * the socket. Function is synchronous, but being called from
>>>> + * coroutine context will yield during connect operation.
>>>> + */
>>>> +int qio_channel_socket_connect_non_blocking_sync(QIOChannelSocket *ioc,
>>>> + SocketAddress *addr,
>>>> + Error **errp);
>>>> +
>>>> /**
>>>> * qio_channel_socket_connect_async:
>>>> * @ioc: the socket channel object
>>>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>>>> index e1b4667087..076de7578a 100644
>>>> --- a/io/channel-socket.c
>>>> +++ b/io/channel-socket.c
>>>> @@ -22,6 +22,7 @@
>>>> #include "qapi/error.h"
>>>> #include "qapi/qapi-visit-sockets.h"
>>>> #include "qemu/module.h"
>>>> +#include "qemu/sockets.h"
>>>> #include "io/channel-socket.h"
>>>> #include "io/channel-watch.h"
>>>> #include "trace.h"
>>>> @@ -29,6 +30,8 @@
>>>> #define SOCKET_MAX_FDS 16
>>>> +static int qio_channel_socket_close(QIOChannel *ioc, Error **errp);
>>>> +
>>>> SocketAddress *
>>>> qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
>>>> Error **errp)
>>>> @@ -157,6 +160,77 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
>>>> return 0;
>>>> }
>>>> +static int qio_channel_inet_connect_non_blocking_sync(QIOChannelSocket *ioc,
>>>> + InetSocketAddress *addr, Error **errp)
>>>> +{
>>>> + Error *local_err = NULL;
>>>> + struct addrinfo *infos, *info;
>>>> + int sock = -1;
>>>> +
>>>> + infos = inet_parse_connect_saddr(addr, errp);
>>>> + if (!infos) {
>>>> + return -1;
>>>> + }
>>>
>>> This call is blocking since it calls getaddrinfo whose design
>>> offers no ability todo non-blocking DNS lookups. Given this
>>> call, ...
>>
>> Oh, that's bad, thanks for taking a look on that early stage!
>>
>>>
>>>> +
>>>> + for (info = infos; info != NULL; info = info->ai_next) {
>>>> + bool in_progress;
>>>> +
>>>> + error_free(local_err);
>>>> + local_err = NULL;
>>>> +
>>>> + sock = inet_connect_addr(addr, info, false, &in_progress, &local_err);
>>>> + if (sock < 0) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + if (qio_channel_socket_set_fd(ioc, sock, &local_err) < 0) {
>>>> + close(sock);
>>>> + continue;
>>>> + }
>>>> +
>>>> + if (in_progress) {
>>>> + if (qemu_in_coroutine()) {
>>>> + qio_channel_yield(QIO_CHANNEL(ioc), G_IO_OUT);
>>>> + } else {
>>>> + qio_channel_wait(QIO_CHANNEL(ioc), G_IO_OUT);
>>>> + }
>>>
>>> ...this is offering false assurances of being non-blocking.
>>>
>>> If we don't want the current thread to be blocked then we
>>> need to be using the existing qio_channel_socket_connect_async
>>> method or similar. It uses a throw away background thread to
>>> run the connection attempt, and then reports completion back
>>> later, thus avoiding the getaddrinfo design flaw for the callers.
>>>
>>> I explicitly didn't want to add an method like the impl in this
>>> patch, because getaddrinfo dooms it and we already had bugs in
>>> the pre-QIOChannel code where QEMU thought it was non-blocking
>>> but wasn't due to getaddrinfo lookups.
>>>
>>>
>>> IIUC, the main appeal of this method is that the non-blocking
>>> nature is hidden from the caller who can continue to treat it
>>> as a synchronous call and have the coroutine magic happen in
>>> behind the scenes.
>>>
>>> IOW, What's needed is a simple way to run the operation in a
>>> thread, and sleep for completion while having the coroutine
>>> yield.
>>>
>>> I think this could likely be achieved with QIOTask with an
>>> alternate impl of the qio_task_wait_thread() method that is
>>> friendly to coroutines instead of being based on pthread
>>> condition variable waits.
>>
>> The most simple thing is just run qio_channel_socket_connect_sync in
>> a thread with help of thread_pool_submit_co() which is coroutine-friendly.
>> And this don't need any changes in io/channel.
>>
>> Actually, I've started with such design, but decided that better use
>> non-blocking connect to not deal with cancelling the connecting thread
>> on shutdown.
>>
>> I think, I'll resend based on thread_pool_submit_co().
>>
>> ===
>>
>> Hmm, there is async getaddrinfo_a function.. What do you think of it?
>
> It isn't portable, glibc only.
>
>> But seems simpler to use a thread than move to async interfaces everywhere.
>
>
Hmm.. Still, on shutdown, how to cancel this connect and getaddrinfo ? I'm not sure
how much time may getaddrinfo take, but connect can take about a minute. It's not really
good to wait for it on shutdown.
So, it may make sense to split connect, and keep it true-async. And call getaddrinfo in a thread
(or just keep it synchronous).
--
Best regards,
Vladimir
next prev parent reply other threads:[~2020-07-22 12:59 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-20 18:07 [PATCH for-5.1? 0/4] non-blocking connect Vladimir Sementsov-Ogievskiy
2020-07-20 18:07 ` [PATCH 1/4] qemu-sockets: refactor inet_connect_addr Vladimir Sementsov-Ogievskiy
2020-07-20 18:07 ` [PATCH 2/4] qemu-sockets: implement non-blocking connect interface Vladimir Sementsov-Ogievskiy
2020-07-20 18:07 ` [PATCH 3/4] io/channel-socket: implement non-blocking connect Vladimir Sementsov-Ogievskiy
2020-07-20 18:29 ` Daniel P. Berrangé
2020-07-22 11:00 ` Vladimir Sementsov-Ogievskiy
2020-07-22 11:21 ` Daniel P. Berrangé
2020-07-22 12:43 ` Vladimir Sementsov-Ogievskiy [this message]
2020-07-22 12:53 ` Daniel P. Berrangé
2020-07-22 13:47 ` Vladimir Sementsov-Ogievskiy
2020-07-22 15:04 ` Vladimir Sementsov-Ogievskiy
2020-07-22 15:21 ` Daniel P. Berrangé
2020-07-22 15:40 ` Vladimir Sementsov-Ogievskiy
2020-07-22 15:43 ` Daniel P. Berrangé
2020-07-22 15:56 ` Vladimir Sementsov-Ogievskiy
2020-07-20 18:07 ` [PATCH 4/4] block/nbd: use non-blocking connect: fix vm hang on connect() Vladimir Sementsov-Ogievskiy
2020-07-23 19:35 ` [PATCH for-5.1? 0/4] non-blocking connect Eric Blake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cd5058fe-20d4-615e-8489-a30ac54d57de@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=berrange@redhat.com \
--cc=den@openvz.org \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).