During .close, the nbd plugin waits for pthread_join() to collect the reader thread, ensuring we have read any remaining replies from the server prior to giving up control. However, after we have sent NBD_CMD_DISC, we end up in a blocking read() that won't progress until the server hangs up its end (the NBD protocol says the server won't reply to NBD_CMD_DISC, but it may still be flushing replies to other pending requests). But if the server is likewise stuck in a read() to see if we (erroneously) send anything after NBD_CMD_DISC (which is the case when nbdkit is the remote server, using parallel threads where the next read() is started before processing the command that was just read off the wire), then our NBD plugin and the remote NBD server are deadlocked with both sides trying to read from the other.
Prior to commit 9e6d990f, this deadlock was (typically?) not visible in the testsuite: after the client quits, nbdkit code was unloading the nbd plugin without waiting for .close to complete, at which point process exit() breaks the deadlock (unloading the .so while code is still running might also explain some harder-to-reproduce crashes that also sometimes plagued the testsuite). But now that an ongoing .close inhibits an early unload (which is a GOOD thing), the deadlock is hanging the testsuite on tests involving the nbd plugin when the server doesn't immediately hang up after NBD_CMD_DISC. Using nonblocking reads with a poll() loop that handles both reads and writes from a single thread could probably solve the issue (as then, nbd_close wouldn't have to wait to join a thread stuck on blocking I/O); but refactoring the code to support nonblocking I/O is rather invasive. Instead, we can use shutdown() to inform the remote server that we will not be writing any more data to the socket; if the remote server is blocked on read(), it will now see an immediate EOF rather than waiting forever for something we will never write, and can proceed on its cleanup paths to eventually close its end, so that our NBD reader will likewise complete quickly and let us leave nbd_close(); then the .so can be unloaded with no plugin code running. Note that the deadlock also goes away if the server hangs up automatically after flushing all prior pending requests when NBD_CMD_DISC is received, and that nbdkit will be patched along those lines in the next patch. But while that fixes our testsuite in our own setup, it's better to fix our plugin to not be reliant on the server behavior, since we can't guarantee that all other servers behave the same way as nbdkit. Signed-off-by: Eric Blake <[email protected]> --- plugins/nbd/nbd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index a4a1f12..b9e72bc 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -568,8 +568,10 @@ nbd_close (void *handle) { struct handle *h = handle; - if (!h->dead) + if (!h->dead) { nbd_request_raw (h, 0, NBD_CMD_DISC, 0, 0, 0, NULL); + shutdown (h->fd, SHUT_WR); + } close (h->fd); if ((errno = pthread_join (h->reader, NULL))) nbdkit_debug ("failed to join reader thread: %m"); -- 2.14.3 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
