From bae89f2509a2ab5d6703b544c7ff643ea25077e5 Mon Sep 17 00:00:00 2001
From: phk <phk@d4fa192b-c00b-0410-8231-f00ffab90ce4>
Date: Mon, 26 May 2008 10:13:52 +0000
Subject: [PATCH] Be more consistent about sockets and blocking/non-blocking
 mode:

Accept sockets are non-blocking, to avoid races where the client closes
before we get to accept it.  (Spotted by: "chen xiaoyong")

Unfortunately, that means that session sockets inherit non-blocking mode,
which is the opposite of what we want in the worker thread but correct
for the acceptor thread.

We prefer to have the extra syscalls in the worker thread, that complicates
things a little bit.

Use ioctl(FIONBIO) instead of fcntl(2) which is surprisingly expensive.


git-svn-id: svn+ssh://projects.linpro.no/svn/varnish/trunk@2639 d4fa192b-c00b-0410-8231-f00ffab90ce4
---
 varnish-cache/bin/varnishd/cache_acceptor.c |  5 +++++
 varnish-cache/bin/varnishd/cache_center.c   | 10 +++++++++
 varnish-cache/bin/varnishd/mgt_child.c      |  6 +++++
 varnish-cache/bin/varnishd/tcp.c            | 25 +++++++++++----------
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/varnish-cache/bin/varnishd/cache_acceptor.c b/varnish-cache/bin/varnishd/cache_acceptor.c
index 6394cee0..28e18bcd 100644
--- a/varnish-cache/bin/varnishd/cache_acceptor.c
+++ b/varnish-cache/bin/varnishd/cache_acceptor.c
@@ -270,6 +270,11 @@ vca_return_session(struct sess *sp)
 	AZ(sp->obj);
 	AZ(sp->vcl);
 	assert(sp->fd >= 0);
+	/*
+	 * Set nonblocking in the worker-thread, before passing to the
+	 * acceptor thread, to reduce syscall density of the latter.
+	 */
+	TCP_nonblocking(sp->fd);
 	if (vca_act->pass == NULL)
 		assert(sizeof sp == write(vca_pipes[1], &sp, sizeof sp));
 	else
diff --git a/varnish-cache/bin/varnishd/cache_center.c b/varnish-cache/bin/varnishd/cache_center.c
index 2ae2dcbc..1f859c21 100644
--- a/varnish-cache/bin/varnishd/cache_center.c
+++ b/varnish-cache/bin/varnishd/cache_center.c
@@ -906,6 +906,16 @@ CNT_Session(struct sess *sp)
 	w = sp->wrk;
 	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
 
+	/*
+	 * Whenever we come in from the acceptor we need to set blocking
+	 * mode, but there is no point in setting it when we come from
+	 * ESI or when a parked sessions returns.
+	 * It would be simpler to do this in the acceptor, but we'd rather
+	 * do the syscall in the worker thread.
+ 	 */
+	if (sp->step == STP_FIRST || sp->step == STP_START)
+		TCP_blocking(sp->fd);
+
 	for (done = 0; !done; ) {
 		assert(sp->wrk == w);
 		/*
diff --git a/varnish-cache/bin/varnishd/mgt_child.c b/varnish-cache/bin/varnishd/mgt_child.c
index 717833b7..7b88d304 100644
--- a/varnish-cache/bin/varnishd/mgt_child.c
+++ b/varnish-cache/bin/varnishd/mgt_child.c
@@ -150,6 +150,12 @@ open_sockets(void)
 			free(ls);
 			continue;
 		}
+		/*
+		 * Set nonblocking mode to avoid a race where a client
+		 * closes before we call accept(2) and nobody else are in
+		 * the listen queue to release us.
+		 */
+		TCP_nonblocking(ls->sock);
 		TCP_filter_http(ls->sock);
 		good++;
 	}
diff --git a/varnish-cache/bin/varnishd/tcp.c b/varnish-cache/bin/varnishd/tcp.c
index 0d1d7d62..f0c9a966 100644
--- a/varnish-cache/bin/varnishd/tcp.c
+++ b/varnish-cache/bin/varnishd/tcp.c
@@ -37,7 +37,7 @@
 #include <netinet/in.h>
 
 #include <errno.h>
-#include <fcntl.h>
+#include <sys/ioctl.h>
 #include <netdb.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -112,18 +112,22 @@ TCP_filter_http(int sock)
 #endif
 }
 
-/*--------------------------------------------------------------------*/
+/*--------------------------------------------------------------------
+ * Functions for controlling NONBLOCK mode.
+ * 
+ * We use FIONBIO because it is cheaper than fcntl(2), which requires
+ * us to do two syscalls, one to get and one to set, the latter of
+ * which mucks about a bit before it ends up calling ioctl(FIONBIO),
+ * at least on FreeBSD.
+ */
 
 void
 TCP_blocking(int sock)
 {
 	int i;
 
-	i = fcntl(sock, F_GETFL);
-	assert(i != -1);
-	i &= ~O_NONBLOCK;
-	i = fcntl(sock, F_SETFL, i);
-	assert(i != -1);
+	i = 0;
+	AZ(ioctl(sock, FIONBIO, &i));
 }
 
 void
@@ -131,9 +135,6 @@ TCP_nonblocking(int sock)
 {
 	int i;
 
-	i = fcntl(sock, F_GETFL);
-	assert(i != -1);
-	i |= O_NONBLOCK;
-	i = fcntl(sock, F_SETFL, i);
-	assert(i != -1);
+	i = 1;
+	AZ(ioctl(sock, FIONBIO, &i));
 }
-- 
2.39.5