From efdd6139682f182c80c85741460353a6ed5811bf Mon Sep 17 00:00:00 2001 From: nick Date: Wed, 14 Aug 2013 16:09:55 +0100 Subject: [PATCH] listen: Turn off CLIENT_MAX_WAIT_SECS The idea behind this feature was to avoid the client thread in a listen server getting stuck forever if the mirroring thread in the source died. However, it breaks any sane implementation of max_Bps in that thread, and there are lingering concerns over how it might operate under normal conditions anyway. Specifically, if iterating over the bitmap takes a long time, or even just reading the requisite 8MB from the disc in order to send it, then the 5-second timeout could be hit, causing mirroring to fail unnecessarily. --- src/client.c | 15 ++++++++++----- src/client.h | 4 ++++ tests/acceptance/test_dest_error_handling.rb | 6 ++++-- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/client.c b/src/client.c index fbc55d7..5653a9b 100644 --- a/src/client.c +++ b/src/client.c @@ -192,14 +192,19 @@ int client_read_request( struct client * client , struct nbd_request *out_reques struct nbd_request_raw request_raw; fd_set fds; - struct timeval tv = {CLIENT_MAX_WAIT_SECS, 0}; - struct timeval * ptv; + struct timeval * ptv = NULL; int fd_count; - /* We want a timeout if this is an inbound migration, but not - * otherwise + /* We want a timeout if this is an inbound migration, but not otherwise. + * This is compile-time selectable, as it will break mirror max_bps */ - ptv = server_is_in_control( client->serve ) ? NULL : &tv; +#ifdef HAS_LISTEN_TIMEOUT + struct timeval tv = {CLIENT_MAX_WAIT_SECS, 0}; + + if ( !server_is_in_control( client->serve ) ) { + ptv = &tv; + } +#endif FD_ZERO(&fds); FD_SET(client->socket, &fds); diff --git a/src/client.h b/src/client.h index 0f0ae0c..04b1d05 100644 --- a/src/client.h +++ b/src/client.h @@ -4,6 +4,8 @@ #include #include +#ifdef HAS_LISTEN_TIMEOUT + /** CLIENT_MAX_WAIT_SECS * This is the length of time an inbound migration will wait for a fresh * write before assuming the source has Gone Away. Note: it is *not* @@ -12,6 +14,8 @@ */ #define CLIENT_MAX_WAIT_SECS 5 +#endif + /** CLIENT_HANDLER_TIMEOUT * This is the length of time (in seconds) any request can be outstanding for. * If we spend longer than this in a request, the whole server is killed. diff --git a/tests/acceptance/test_dest_error_handling.rb b/tests/acceptance/test_dest_error_handling.rb index e358868..b4171ee 100644 --- a/tests/acceptance/test_dest_error_handling.rb +++ b/tests/acceptance/test_dest_error_handling.rb @@ -21,12 +21,13 @@ class TestDestErrorHandling < Test::Unit::TestCase assert_no_control end - +=begin + # This is disabled while CLIENT_MAX_WAIT_SECS is removed def test_hello_goes_astray_causes_timeout_error run_fake( "source/hang_after_hello" ) assert_no_control end - +=end def test_sigterm_has_bad_exit_status @env.nbd1.can_die(1) @@ -99,3 +100,4 @@ class TestDestErrorHandling < Test::Unit::TestCase end end # class TestDestErrorHandling +