|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH 3/4] io/channel-socket: implement non-blocking connect |
Date: | Wed, 22 Jul 2020 14:00:25 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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? But seems simpler to use a thread than move to async interfaces everywhere.
+ if (socket_check(sock, &local_err) < 0) { + qio_channel_socket_close(QIO_CHANNEL(ioc), NULL); + continue; + } + } + + break; + } + + freeaddrinfo(infos); + + error_propagate(errp, local_err); + return sock; +}Regards, Daniel
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |