Re: [Nbd] nbd-client hangs on negotiation - possible NBD bug

["Mike Snitzer" <snitzer@...17...>]

On 8/1/06, Wouter Verhelst <wouter@...3...> wrote:
> On Mon, Jul 31, 2006 at 03:57:12PM -0700, Eric Tessler wrote:
> >   We now know that the server side is blocking on the accept() call to
> >   the socket - even though the client is connecting to the server, the
> >   accept() call on the server side hangs.
>
> I see.
>
> One thing I've been meaning to do is to convert the current situation
> (where the server blocks in accept() until a client connects) to some
> select() based thing. I didn't do this yet since there's nothing else
> the main server needs to do but to wait for clients to connect and fork
> off servers to handle them, but I don't know whether it's actually a
> good idea to keep it this way.

After looking at the problem a bit closer, I don't think its a good
idea for the server to use a blocking socket with accept() as the
mechanism to wait for a client connection.  As such I wrote the
attached patch that enables the nbd-server to use a non-blocking
accept() that is guarded by a select() like was suggested.

Eric, any chance you could try nbd-2.8.5 with this patch in your environment?
Wouter, what are your thoughts?

regards,
Mike

diff -u nbd-2.8.5.orig/nbd-server.c nbd-2.8.5/nbd-server.c
--- nbd-2.8.5.orig/nbd-server.c	2006-06-07 02:29:10.000000000 -0400
+++ nbd-2.8.5/nbd-server.c	2006-08-04 13:39:10.000000000 -0400
@@ -58,6 +58,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
+#include <sys/select.h>		/* select */
 #include <sys/wait.h>		/* wait */
 #ifdef HAVE_SYS_IOCTL_H
 #include <sys/ioctl.h>
@@ -900,7 +901,7 @@
 	struct sockaddr_in addrin;
 	struct sigaction sa;
 	int addrinlen = sizeof(addrin);
-	int fhandle;
+	int fhandle, sock_flags;
 #ifndef sun
 	int yes=1;
 #else
@@ -938,6 +939,14 @@
 		err("setsockopt SO_KEEPALIVE");
 	}
 
+	/* make the listening socket non-blocking */
+	if ((sock_flags = fcntl(serve->socket, F_GETFL, 0)) == -1) {
+		err("fcntl F_GETFL");
+	}
+	if (fcntl(serve->socket, F_SETFL, sock_flags | O_NONBLOCK) == -1) {
+		err("fcntl F_SETFL O_NONBLOCK");
+	}
+	
 	DEBUG("Waiting for connections... bind, ");
 	addrin.sin_family = AF_INET;
 	addrin.sin_port = htons(serve->port);
@@ -969,47 +978,65 @@
 int serveloop(SERVER* serve) {
 	struct sockaddr_in addrin;
 	socklen_t addrinlen=sizeof(addrin);
+	
+	int max_fd = serve->socket;
+	fd_set read_fds;	
+	FD_ZERO(&read_fds);
+	FD_SET(serve->socket, &read_fds);
+	
 	for(;;) {
-		CLIENT *client;
-		int net;
-		pid_t *pid;
-
-		DEBUG("accept, ");
-		if ((net = accept(serve->socket, (struct sockaddr *) &addrin, &addrinlen)) < 0) {
-			msg2(LOG_ERR,"accept: %m");
+		DEBUG("select, ");
+		/* use to select to tell us when a connection is ready to be accepted */
+		if (select(max_fd+1, &read_fds, NULL, NULL, NULL) <= 0) {
+			if (errno == EINTR)
+				continue;
+			msg2(LOG_ERR,"select: %m");
 			continue;
 		}
 
-		client = g_malloc(sizeof(CLIENT));
-		client->server=serve;
-		client->exportsize=OFFT_MAX;
-		client->net=net;
-		set_peername(net, client);
-		if (!authorized_client(client)) {
-			msg2(LOG_INFO,"Unauthorized client") ;
-			close(net) ;
-			continue ;
-		}
-		msg2(LOG_INFO,"Authorized client") ;
-		pid=g_malloc(sizeof(pid_t));
+		if (FD_ISSET(serve->socket, &read_fds)) {
+			/* accept the new client connection */
+			CLIENT *client;
+			int net;
+			pid_t *pid;			
+			
+			DEBUG("accept, ");
+			if ((net = accept(serve->socket, (struct sockaddr *) &addrin, &addrinlen)) < 0) {
+				msg2(LOG_ERR,"accept: %m");
+				continue;
+			}
+
+			client = g_malloc(sizeof(CLIENT));
+			client->server=serve;
+			client->exportsize=OFFT_MAX;
+			client->net=net;
+			set_peername(net, client);
+			if (!authorized_client(client)) {
+				msg2(LOG_INFO,"Unauthorized client") ;
+				close(net) ;
+				continue ;
+			}
+			msg2(LOG_INFO,"Authorized client") ;
+			pid=g_malloc(sizeof(pid_t));
 #ifndef NOFORK
-		if ((*pid=fork())<0) {
-			msg3(LOG_INFO,"Could not fork (%s)",strerror(errno)) ;
-			close(net) ;
-			continue ;
-		}
-		if (*pid>0) { /* parent */
-			close(net);
-			g_hash_table_insert(children, pid, pid);
-			continue;
-		}
-		/* child */
-		g_hash_table_destroy(children);
-		close(serve->socket) ;
+			if ((*pid=fork())<0) {
+				msg3(LOG_INFO,"Could not fork (%s)",strerror(errno)) ;
+				close(net) ;
+				continue ;
+			}
+			if (*pid>0) { /* parent */
+				close(net);
+				g_hash_table_insert(children, pid, pid);
+				continue;
+			}
+			/* child */
+			g_hash_table_destroy(children);
+			close(serve->socket) ;
 #endif // NOFORK
-		msg2(LOG_INFO,"Starting to serve") ;
-		serveconnection(client);
-		return 0;
+			msg2(LOG_INFO,"Starting to serve") ;
+			serveconnection(client);
+			return 0;
+		}
 	}
 }