Use correct constant name.

This alters the meaning of NBD_MAX_SIZE to be the actual max request size
we'll accept over nbd.  Previously it was *nearly* the max size we'd
accept depending on the size of the struct.

Makefile

@@ -12,11 +12,6 @@ else
 	CFLAGS_EXTRA=-O2
 endif
 
-NO_MSYNC=1
-ifdef NO_MSYNC
-	CFLAGS_EXTRA += -DNO_MSYNC
-endif
-
 CFLAGS_EXTRA  += -fPIC --std=gnu99
 LDFLAGS_EXTRA += -Wl,--relax,--gc-sections -L$(LIB) -Wl,-rpath-link,$(LIB)
 

debian/changelog

@@ -1,7 +1,6 @@
 flexnbd (0.1.8) UNRELEASED; urgency=medium
 
-  * Set TCP keepalive on sockets so broken connections are reaped (#33, !33,
+  * Set TCP keepalive on sockets so broken connections are reaped (#33, !33)
-    !36)
   * Add a context to logs to make debugging problems easier (#34, !34)
 
  -- James Carter <james.carter@bytemark.co.uk>  Thu, 11 Jan 2018 10:05:35 +0000

src/common/ioutil.c

@@ -73,6 +73,7 @@ int build_allocation_map(struct bitset * allocation_map, int fd)
 	return 1;
 }
 
+
 int open_and_mmap(const char* filename, int* out_fd, uint64_t *out_size, void **out_map)
 {
 	/* 
@@ -100,7 +101,7 @@ int open_and_mmap(const char* filename, int* out_fd, uint64_t *out_size, void **
 		*out_size = size;
 	}
 
-	if (0) {
+	if (out_map) {
 		*out_map = mmap64(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED,
 		  *out_fd, 0);
 		if (((long) *out_map) == -1) {
@@ -348,36 +349,3 @@ ssize_t iobuf_write( int fd, struct iobuf *iobuf )
 
 	return count;
 }
-
-struct iommap *iommap_alloc(int fd, off64_t from, uint64_t len) {
-
-	off66_t mmap_from = from & ~((off64_t) getpagesize() - 1);
-        uint64_t mmap_len = len + (from - mmap_from);
-        void *mmap_buf = NULL;
-
-	// TODO: Check the error code from mmap64
-	if(mmap_len)
-		mmap_buf = mmap64(NULL, mmap_len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, mmap_from);
-
-	struct iommap *im = xmalloc(sizeof(struct iommap ));
-
-	im->mmap_buf = mmap_buf;
-	im->mmap_len = mmap_len;
-	im->buf = (char *) mmap_buf + from - mmap_from;
-	debug("mmapped file from %ld:%d", mmap_from, mmap_len);
-	return im;
-}
-
-void iommap_sync(struct iommap *im) {
-        if (im->mmap_len)
-		msync(im->mmap_buf, im->mmap_len, MS_SYNC | MS_INVALIDATE);
-	return;
-}
-
-void iommap_free(struct iommap *im) {
-        if (im->mmap_len)
-		munmap(im->mmap_buf, im->mmap_len);
-
-	free(im);
-}
-

src/common/ioutil.h

@@ -11,20 +11,7 @@ struct iobuf {
 ssize_t iobuf_read( int fd, struct iobuf* iobuf, size_t default_size );
 ssize_t iobuf_write( int fd, struct iobuf* iobuf );
 
-#include <inttypes.h>
-
-struct iommap {
-	char *buf;
-	char *mmap_buf;
-	uint64_t mmap_len;
-};
-
-struct iommap *iommap_alloc(int fd, off64_t from, uint64_t len);
-void   iommap_sync(struct iommap *im);
-void   iommap_free(struct iommap *im);
-
 #include "serve.h"
-
 struct bitset; /* don't need whole of bitset.h here */
 
 /** Scan the file opened in ''fd'', set bits in ''allocation_map''  that

src/proxy/proxy.c

@@ -76,8 +76,12 @@ struct proxier* proxy_create(
 	}
 
 	out->init.buf = xmalloc( sizeof( struct nbd_init_raw ) );
-	out->req.buf  = xmalloc( NBD_MAX_SIZE );
+
-	out->rsp.buf  = xmalloc( NBD_MAX_SIZE );
+	/* Add on the request / reply size to our malloc to accommodate both
+	 * the struct and the data
+	 */
+	out->req.buf  = xmalloc( NBD_MAX_SIZE + NBD_REQUEST_SIZE );
+	out->rsp.buf  = xmalloc( NBD_MAX_SIZE + NBD_REPLY_SIZE );
 
 	log_context = xmalloc( strlen(s_upstream_address) + strlen(s_upstream_port) + 2 );
 	sprintf(log_context, "%s:%s", s_upstream_address, s_upstream_port);
@@ -440,15 +444,18 @@ int proxy_read_from_downstream( struct proxier *proxy, int state )
 			return EXIT;
 		}
 
-		/* Simple validations */
+		/* Simple validations -- the request / reply size have already
+		 * been taken into account in the xmalloc, so no need to worry
+		 * about them here
+		 */
 		if ( ( request->type & REQUEST_MASK ) == REQUEST_READ ) {
-			if (request->len > ( NBD_MAX_SIZE - NBD_REPLY_SIZE ) ) {
+			if ( request->len > NBD_MAX_SIZE ) {
 				warn( "NBD read request size %"PRIu32" too large", request->len );
 				return EXIT;
 			}
 		}
 		if ( (request->type & REQUEST_MASK ) == REQUEST_WRITE ) {
-			if (request->len > ( NBD_MAX_SIZE - NBD_REQUEST_SIZE ) ) {
+			if ( request->len > NBD_MAX_SIZE ) {
 				warn( "NBD write request size %"PRIu32" too large", request->len );
 				return EXIT;
 			}

src/server/client.c

@@ -118,7 +118,6 @@ void write_not_zeroes(struct client* client, uint64_t from, uint64_t len)
 		 */
 
 		uint64_t run = bitset_run_count(map, from, len);
-		struct iommap *iommap = iommap_alloc(client->fileno, from, len); 
 
 		debug("write_not_zeroes: from=%ld, len=%d, run=%d", from, len, run);
 
@@ -156,7 +155,7 @@ void write_not_zeroes(struct client* client, uint64_t from, uint64_t len)
 		if (bitset_is_set_at(map, from)) {
 			debug("writing the lot: from=%ld, run=%d", from, run);
 			/* already allocated, just write it all */
-			DO_READ(iommap->buf, run);
+			DO_READ(client->mapped + from, run);
 			/* We know from our earlier call to  bitset_run_count that the
 			 * bitset is all-1s at this point, but we need to dirty it for the
 			 * sake of the event stream - the actual bytes have changed, and we
@@ -187,7 +186,7 @@ void write_not_zeroes(struct client* client, uint64_t from, uint64_t len)
 					(0 == memcmp( zerobuffer, zerobuffer+1, blockrun-1 ));
 
 				if ( !all_zeros ) {
-					memcpy(iommap->buf, zerobuffer, blockrun);
+					memcpy(client->mapped+from, zerobuffer, blockrun);
 					bitset_set_range(map, from, blockrun);
 					/* at this point we could choose to
 					 * short-cut the rest of the write for
@@ -206,8 +205,6 @@ void write_not_zeroes(struct client* client, uint64_t from, uint64_t len)
 				from += blockrun;
 			}
 		}
-		iommap_sync(iommap);
-		iommap_free(iommap);
 	}
 }
 
@@ -237,10 +234,6 @@ int client_read_request( struct client * client , struct nbd_request *out_reques
 				debug( "Connection reset while"
 						" reading request" );
 				return 0;
-			case ETIMEDOUT:
-				debug( "Connection timed out while"
-						" reading request" );
-				return 0;
 			default:
 				/* FIXME: I've seen this happen, but I
 				 * couldn't reproduce it so I'm leaving
@@ -452,28 +445,22 @@ void client_reply_to_read( struct client* client, struct nbd_request request )
 void client_reply_to_write( struct client* client, struct nbd_request request )
 {
 	debug("request write from=%"PRIu64", len=%"PRIu32", handle=0x%08X", request.from, request.len, request.handle);
-	// TODO: Just write directly for now.  Not (yet) convinced my changes later on work.
+	if (client->serve->allocation_map_built) {
-	// if (client->serve->allocation_map_built) {
-	if (0) {
 		write_not_zeroes( client, request.from, request.len );
 	}
 	else {
 		debug("No allocation map, writing directly.");
 		/* If we get cut off partway through reading this data:
 		 * */
-		struct iommap *iommap = iommap_alloc(client->fileno, request.from, request.len); 
-
 		ERROR_IF_NEGATIVE(
 			readloop( client->socket,
-				iommap->buf,
+				client->mapped + request.from,
 				request.len),
 			"reading write data failed from=%ld, len=%d",
 			request.from,
 			request.len
 		);
 
-		iommap_sync(iommap);
-		iommap_free(iommap);
 		/* the allocation_map is shared between client threads, and may be
 		 * being built. We need to reflect the write in it, as it may be in
 		 * a position the builder has already gone over.
@@ -481,6 +468,19 @@ void client_reply_to_write( struct client* client, struct nbd_request request )
 		bitset_set_range(client->serve->allocation_map, request.from, request.len);
 	}
 
+	if (1) /* not sure whether this is necessary... */
+	{
+		/* multiple of 4K page size */
+		uint64_t from_rounded = request.from & (!0xfff);
+		uint64_t len_rounded = request.len + (request.from - from_rounded);
+
+		FATAL_IF_NEGATIVE(
+			msync( client->mapped + from_rounded,
+				len_rounded,
+				MS_SYNC | MS_INVALIDATE),
+			"msync failed %ld %ld", request.from, request.len
+		);
+	}
 	client_write_reply( client, &request, 0);
 }
 
@@ -646,6 +646,9 @@ void client_cleanup(struct client* client,
 		debug("Closed client socket fd %d", client->socket);
 		client->socket = -1;
 	}
+	if (client->mapped) {
+		munmap(client->mapped, client->serve->size);
+	}
 	if (client->fileno) {
 		FATAL_IF_NEGATIVE( close(client->fileno),
 			"Error closing file %d",
@@ -661,20 +664,25 @@ void client_cleanup(struct client* client,
 void* client_serve(void* client_uncast)
 {
 	struct client* client = (struct client*) client_uncast;
-	void** a = NULL;
 
 	error_set_handler((cleanup_handler*) client_cleanup, client);
+
 	info("client: mmaping file");
 	FATAL_IF_NEGATIVE(
 		open_and_mmap(
 			client->serve->filename,
 			&client->fileno,
 			NULL,
-			a
+			(void**) &client->mapped
 		),
 		"Couldn't open/mmap file %s: %s", client->serve->filename, strerror( errno )
 	);
 
+	FATAL_IF_NEGATIVE(
+		madvise( client->mapped, client->serve->size, MADV_RANDOM ),
+		SHOW_ERRNO( "Failed to madvise() %s", client->serve->filename )
+	);
+
 	debug( "Opened client file fd %d", client->fileno);
 	debug("client: sending hello");
 	client_send_hello(client);

src/server/client.h

@@ -29,6 +29,7 @@ struct client {
 	int     socket;
 
 	int     fileno;
+	char*   mapped;
 
 	struct self_pipe * stop_signal;