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

[Wouter Verhelst <wouter@...3...>]

On Fri, Aug 04, 2006 at 02:15:14PM -0400, Mike Snitzer wrote:
> 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.

I concur.

> 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?

Eric mailed me in private that the needed fix is really what is done in
the 2.7 branch in r105, which made it in only after I released the 2.7.3
that Eric is using. For some reason that hadn't made it in 2.8 or trunk
yet, so I've just commited a fix for that to both the 2.8 branch and
trunk, too.

> Wouter, what are your thoughts?

Your patch looks okay, and this is something I wanted to do for some
time now, so I'll incorporate it some time soon. I have no time anymore
now, however, have to catch my train. Kick me if I forget (which is not
totally unlikely, knowing me).

I'll probably release 2.8.6 after that happens.

> 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;
> +		}
>  	}
>  }
>  

> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys -- and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Nbd-general mailing list
> Nbd-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general


-- 
Fun will now commence
  -- Seven Of Nine, "Ashes to Ashes", stardate 53679.4