[PATCH libnbd] lib: Kill subprocess in nbd_close.
This is a simple patch which stops nbd_close from waiting too long for a server subprocess to shut down. I wanted to send SIGHUP because the server will be able to catch it and do a clean shutdown if that is required. Is another signal better? Is it right to send a signal here? Rich.
$ time nbdsh -c 'h.connect_command (["nbdkit", "-s", "null", "size=512", "--filter=delay", "delay-read=10"]); b = nbd.aio_buffer(1); h.aio_pread (b, 0); del (h)' real 0m10.499s user 0m0.065s sys 0m0.023s With this patch the elapsed time is near instantaneous. --- lib/handle.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/handle.c b/lib/handle.c index 1fe4467..111cfc5 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -124,8 +124,10 @@ nbd_close (struct nbd_handle *h) freeaddrinfo (h->result); if (h->sock) h->sock->ops->close (h->sock); - if (h->pid >= 0) /* XXX kill it? */ + if (h->pid >= 0) { + kill (h->pid, SIGHUP); waitpid (h->pid, NULL, 0); + } free (h->export_name); free (h->tls_certificates); -- 2.22.0
On 7/25/19 12:43 PM, Richard W.M. Jones wrote:
$ time nbdsh -c 'h.connect_command (["nbdkit", "-s", "null", "size=512", "--filter=delay", "delay-read=10"]); b = nbd.aio_buffer(1); h.aio_pread (b, 0); del (h)' real 0m10.499s user 0m0.065s sys 0m0.023s
With this patch the elapsed time is near instantaneous. --- lib/handle.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/handle.c b/lib/handle.c index 1fe4467..111cfc5 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -124,8 +124,10 @@ nbd_close (struct nbd_handle *h) freeaddrinfo (h->result); if (h->sock) h->sock->ops->close (h->sock);
Note that this patch is trying to speed up a slow waitpid() issued _after_ we call h->sock->ops->close(). You'd think that after we close() our end, that the remote end doing a read() would see an immediate EOF, and that would be enough for the server to shut down quickly instead of waiting for the delay to expire and finally notice during its write() that we are no longer reading. However, POSIX says that when sockets are involved, a close() on our end does not trigger an immediate EOF on the other side's read end, but rather may start an asynchronous timer to try to finish the TCP connection gracefully to flush any pending traffic, where both directions have to agree that the connection is dead before either direction sees EOF/EPIPE. And that's where the shutdown(2) API comes in handy: it forces the kernel to realize that one or both directions of traffic are no longer needed, making this asynchronous handling in close() much faster. At least nbdkit is known to react favorably to a shutdown() request - see nbdkit commit 430f8141. We may want to add h->sock->ops->shutdown() (especially if we want to handle SHUT_WR through adding more states for more graceful shutdown paths, including when TLS is involved), but even in the short-term, something as simple as adding shutdown(h->fd, SHUT_RDWR) immediately before the s->sock->ops->close() would be helpful. That said, I think we want a two-pronged approach - adding a use of shutdown(), but also playing with signal:
- if (h->pid >= 0) /* XXX kill it? */
We're only doing this for connect_command. We must NOT kill TCP or Unix servers, as those are pre-existing and presumably must stick around for further clients; but when we spawned the server, it is indeed likely that the server should not continue to exist after our handle goes away - it is unlikely that a client would ever expect the server they just created to continue to exist after the handle to serve other clients. So this makes sense.
+ if (h->pid >= 0) { + kill (h->pid, SIGHUP);
I don't know if SIGTERM is any better, but think SIGHUP is okay for now. Also, I don't know if we want to put a time limit on waiting for the server, and follow up with an even harder SIGKILL if it hasn't gracefully exited. Maybe it should be a knob where the user can call nbd_set_FOO to tune the knob for their desired cleanup behavior (whether for which signal to send, or whether to follow up with SIGKILL, or even how long to wait)? Or are we just reinventing timeout(1) from coreutils? So for now this seems okay.
waitpid (h->pid, NULL, 0); + }
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 7/25/19 1:28 PM, Eric Blake wrote:
Note that this patch is trying to speed up a slow waitpid() issued _after_ we call h->sock->ops->close(). You'd think that after we close() our end, that the remote end doing a read() would see an immediate EOF, and that would be enough for the server to shut down quickly instead of waiting for the delay to expire and finally notice during its write() that we are no longer reading. However, POSIX says that when sockets are involved, a close() on our end does not trigger an immediate EOF on the other side's read end, but rather may start an asynchronous timer to try to finish the TCP connection gracefully to flush any pending traffic, where both directions have to agree that the connection is dead before either direction sees EOF/EPIPE. And that's where the shutdown(2) API comes in handy: it forces the kernel to realize that one or both directions of traffic are no longer needed, making this asynchronous handling in close() much faster. At least nbdkit is known to react favorably to a shutdown() request - see nbdkit commit 430f8141.
We may want to add h->sock->ops->shutdown() (especially if we want to handle SHUT_WR through adding more states for more graceful shutdown paths, including when TLS is involved), but even in the short-term, something as simple as adding shutdown(h->fd, SHUT_RDWR) immediately before the s->sock->ops->close() would be helpful.
That effect of wanting to use shutdown() is still real, but may not be the only thing at play here. As we discussed on IRC, adding shutdown() alone was not enough to make this specific nbdkit invocation get to exit() any faster; the waitpid() was still stalling without a kill(). So we've identified a second issue: nbdkit is refusing to exit() until the plugin and all filters unload, but the filter cannot unload until it has no pending transactions, but the pending transaction is stuck in a "non-interruptible" usleep() (since all the signal handlers installed by nbdkit use SA_RESTART, the usleep() doesn't exit early due to EINTR; it takes the external SIGHUP to force nbdkit to die without waiting for the filter to clean up). So the thought here is that maybe nbdkit should be patched to add an nbdkit_usleep() function, which returns 0 if the full sleep happened, or returns -1 if the sleep is interrupted early in one thread due to another thread noticing EOF on the read()s from the client. The delay filter may be the only code to make use of nbdkit_usleep, but having an easy way to recognize that the client has gone away and thus waiting out the rest of the sleep is pointless will make nbdkit much more responsive to a client close()ing the connection. I'll work on the nbdkit patch. Finally, calling kill() unconditionally in nbd_close() may or may not be the nicest, so we have the idea of adding a new API (maybe named nbd_kill) which the user can request which signal to send to the underlying command pid, and control how long it waits before trying a second signal attempt, all prior to calling nbd_close(). Rich will tackle something along those lines. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones