[SERVER] Refactor uplink/cache handling, improve crc checking

The cacheFd is now moved to the uplink data structure and will
only be handled by the uplink thread.
The integrity checker now supports checking all blocks of an
image. This will be triggered automatically whenever a check for
a single block failed.
Also, if a crc check on startup fails, the image won't be discarded
anymore, but rather a full check will be initiated.
Furthermore, when calling image_updateCacheMap() on an image that
was previously complete, the cache map will now be re-initialized,
and a new uplink connection created.

1 files changed, 73 insertions, 216 deletions

diff --git a/src/server/image.c b/src/server/image.c
index 02a383f..673a269 100644
--- a/src/server/image.c
+++ b/src/server/image.c
@@ -20,12 +20,6 @@
 #define PATHLEN (2000)
 #define NONWORKING_RECHECK_INTERVAL_SECONDS (60)
 
-#ifdef HAVE_FDATASYNC
-#define dnbd3_fdatasync fdatasync
-#else
-#define dnbd3_fdatasync fsync
-#endif
-
 // ##########################################
 
 static dnbd3_image_t *_images[SERVER_MAX_IMAGES];
@@ -49,7 +43,6 @@ static imagecache remoteCloneCache[CACHELEN];
 static bool isForbiddenExtension(const char* name);
 static dnbd3_image_t* image_remove(dnbd3_image_t *image);
 static dnbd3_image_t* image_free(dnbd3_image_t *image);
-static bool image_isHashBlockComplete(const uint8_t * const cacheMap, const uint64_t block, const uint64_t fileSize);
 static bool image_load_all_internal(char *base, char *path);
 static bool image_addToList(dnbd3_image_t *image);
 static bool image_load(char *base, char *path, int withUplink);
@@ -70,43 +63,6 @@ void image_serverStartup()
 }
 
 /**
- * Returns true if the given image is complete.
- * DOES NOT LOCK
- */
-bool image_isComplete(dnbd3_image_t *image)
-{
-	assert( image != NULL );
-	if ( image->working && image->cache_map == NULL ) {
-		return true;
-	}
-	if ( image->virtualFilesize == 0 ) {
-		return false;
-	}
-	bool complete = true;
-	int j;
-	const int map_len_bytes = IMGSIZE_TO_MAPBYTES( image->virtualFilesize );
-	for (j = 0; j < map_len_bytes - 1; ++j) {
-		if ( image->cache_map[j] != 0xFF ) {
-			complete = false;
-			break;
-		}
-	}
-	if ( complete ) // Every block except the last one is complete
-	{ // Last one might need extra treatment if it's not a full byte
-		const int blocks_in_last_byte = (image->virtualFilesize >> 12) & 7;
-		uint8_t last_byte = 0;
-		if ( blocks_in_last_byte == 0 ) {
-			last_byte = 0xFF;
-		} else {
-			for (j = 0; j < blocks_in_last_byte; ++j)
-				last_byte |= (uint8_t)(1 << j);
-		}
-		complete = ((image->cache_map[map_len_bytes - 1] & last_byte) == last_byte);
-	}
-	return complete;
-}
-
-/**
  * Update cache-map of given image for the given byte range
  * start (inclusive) - end (exclusive)
  * Locks on: images[].lock
@@ -125,29 +81,36 @@ void image_updateCachemap(dnbd3_image_t *image, uint64_t start, uint64_t end, co
 		start &= ~(uint64_t)(DNBD3_BLOCK_SIZE - 1);
 		end = (uint64_t)(end + DNBD3_BLOCK_SIZE - 1) & ~(uint64_t)(DNBD3_BLOCK_SIZE - 1);
 	}
-	bool dirty = false;
+	bool setNewBlocks = false;
 	uint64_t pos = start;
 	spin_lock( &image->lock );
 	if ( image->cache_map == NULL ) {
 		// Image seems already complete
-		spin_unlock( &image->lock );
-		logadd( LOG_DEBUG1, "image_updateCachemap with no cache_map: %s", image->path );
-		return;
+		if ( set ) {
+			// This makes no sense
+			spin_unlock( &image->lock );
+			logadd( LOG_DEBUG1, "image_updateCachemap(true) with no cache_map: %s", image->path );
+			return;
+		}
+		// Recreate a cache map, set it to all 1 initially as we assume the image was complete
+		const int byteSize = IMGSIZE_TO_MAPBYTES( image->virtualFilesize );
+		image->cache_map = malloc( byteSize );
+		memset( image->cache_map, 0xff, byteSize );
 	}
 	while ( pos < end ) {
 		const size_t map_y = (int)( pos >> 15 );
 		const int map_x = (int)( (pos >> 12) & 7 ); // mod 8
 		const int bit_mask = 1 << map_x;
 		if ( set ) {
-			if ( (image->cache_map[map_y] & bit_mask) == 0 ) dirty = true;
+			if ( (image->cache_map[map_y] & bit_mask) == 0 ) setNewBlocks = true;
 			image->cache_map[map_y] |= (uint8_t)bit_mask;
 		} else {
 			image->cache_map[map_y] &= (uint8_t)~bit_mask;
 		}
 		pos += DNBD3_BLOCK_SIZE;
 	}
-	if ( dirty && image->crc32 != NULL ) {
-		// If dirty is set, at least one of the blocks was not cached before, so queue all hash blocks
+	if ( setNewBlocks && image->crc32 != NULL ) {
+		// If setNewBlocks is set, at least one of the blocks was not cached before, so queue all hash blocks
 		// for checking, even though this might lead to checking some hash block again, if it was
 		// already complete and the block range spanned at least two hash blocks.
 		// First set start and end to borders of hash blocks
@@ -157,7 +120,7 @@ void image_updateCachemap(dnbd3_image_t *image, uint64_t start, uint64_t end, co
 		while ( pos < end ) {
 			if ( image->cache_map == NULL ) break;
 			const int block = (int)( pos / HASH_BLOCK_SIZE );
-			if ( image_isHashBlockComplete( image->cache_map, block, image->virtualFilesize ) ) {
+			if ( image_isHashBlockComplete( image->cache_map, block, image->realFilesize ) ) {
 				spin_unlock( &image->lock );
 				integrity_check( image, block );
 				spin_lock( &image->lock );
@@ -169,112 +132,54 @@ void image_updateCachemap(dnbd3_image_t *image, uint64_t start, uint64_t end, co
 }
 
 /**
- * Mark image as complete by freeing the cache_map and deleting the map file on disk
+ * Returns true if the given image is complete.
+ * Also frees cache_map and deletes it on disk
+ * if it hasn't been complete before
  * Locks on: image.lock
  */
-void image_markComplete(dnbd3_image_t *image)
+bool image_isComplete(dnbd3_image_t *image)
 {
-	char mapfile[PATHLEN] = "";
 	assert( image != NULL );
 	spin_lock( &image->lock );
-	if ( image->cache_map != NULL ) {
-		free( image->cache_map );
-		image->cache_map = NULL;
-		snprintf( mapfile, PATHLEN, "%s.map", image->path );
-	}
-	spin_unlock( &image->lock );
-	if ( mapfile[0] != '\0' ) {
-		remove( mapfile );
-	}
-}
-
-/**
- * Save cache map of every image
- */
-void image_saveAllCacheMaps()
-{
-	spin_lock( &imageListLock );
-	for (int i = 0; i < _num_images; ++i) {
-		if ( _images[i] == NULL ) continue;
-		dnbd3_image_t * const image = _images[i];
-		spin_lock( &image->lock );
-		image->users++;
-		spin_unlock( &imageListLock );
-		spin_unlock( &image->lock );
-		image_saveCacheMap( image );
-		spin_lock( &imageListLock );
-		spin_lock( &image->lock );
-		image->users--;
+	if ( image->virtualFilesize == 0 ) {
 		spin_unlock( &image->lock );
+		return false;
 	}
-	spin_unlock( &imageListLock );
-}
-
-/**
- * Saves the cache map of the given image.
- * Return true on success.
- * Locks on: imageListLock, image.lock
- */
-bool image_saveCacheMap(dnbd3_image_t *image)
-{
-	if ( image == NULL || image->cache_map == NULL ) return true;
-	image = image_lock( image ); // Again after locking:
-	if ( image == NULL ) return true;
-	spin_lock( &image->lock );
-	// Lock and get a copy of the cache map, as it could be freed by another thread that is just about to
-	// figure out that this image's cache copy is complete
-	if ( image->cache_map == NULL || image->virtualFilesize < DNBD3_BLOCK_SIZE ) {
+	if ( image->cache_map == NULL ) {
 		spin_unlock( &image->lock );
-		image_release( image );
 		return true;
 	}
-	const size_t size = IMGSIZE_TO_MAPBYTES(image->virtualFilesize);
-	uint8_t *map = malloc( size );
-	memcpy( map, image->cache_map, size );
-	// Unlock. Use path and cacheFd without locking. path should never change after initialization of the image,
-	// cacheFd is written to and we don't want to hold a spinlock during I/O
-	spin_unlock( &image->lock );
-	assert( image->path != NULL );
-	char mapfile[strlen( image->path ) + 4 + 1];
-	strcpy( mapfile, image->path );
-	strcat( mapfile, ".map" );
-
-	int fd = open( mapfile, O_WRONLY | O_CREAT, 0644 );
-	if ( fd < 0 ) {
-		const int err = errno;
-		image_release( image );
-		free( map );
-		logadd( LOG_WARNING, "Could not open file to write cache map to disk (errno=%d) file %s", err, mapfile );
-		return false;
-	}
-
-	size_t done = 0;
-	while ( done < size ) {
-		const ssize_t ret = write( fd, map, size - done );
-		if ( ret == -1 ) {
-			if ( errno == EINTR ) continue;
-			logadd( LOG_WARNING, "Could not write cache map (errno=%d) file %s", errno, mapfile );
-			break;
-		}
-		if ( ret <= 0 ) {
-			logadd( LOG_WARNING, "Unexpected return value %d for write() to %s", (int)ret, mapfile );
+	bool complete = true;
+	int j;
+	const int map_len_bytes = IMGSIZE_TO_MAPBYTES( image->virtualFilesize );
+	for (j = 0; j < map_len_bytes - 1; ++j) {
+		if ( image->cache_map[j] != 0xFF ) {
+			complete = false;
 			break;
 		}
-		done += (size_t)ret;
 	}
-	if ( image->cacheFd != -1 ) {
-		if ( dnbd3_fdatasync( image->cacheFd ) == -1 ) {
-			logadd( LOG_ERROR, "fsync() on image file %s failed with errno %d", image->path, errno );
-			logadd( LOG_ERROR, "Bailing out immediately" );
-			exit( 1 );
+	if ( complete ) { // Every block except the last one is complete
+		// Last one might need extra treatment if it's not a full byte
+		const int blocks_in_last_byte = (image->virtualFilesize >> 12) & 7;
+		uint8_t last_byte = 0;
+		if ( blocks_in_last_byte == 0 ) {
+			last_byte = 0xFF;
+		} else {
+			for (j = 0; j < blocks_in_last_byte; ++j)
+				last_byte |= (uint8_t)(1 << j);
 		}
+		complete = ((image->cache_map[map_len_bytes - 1] & last_byte) == last_byte);
 	}
-	if ( dnbd3_fdatasync( fd ) == -1 ) {
-		logadd( LOG_WARNING, "fsync() on image map %s failed with errno %d", mapfile, errno );
+	if ( !complete ) {
+		spin_unlock( &image->lock );
+		return false;
 	}
-	image_release( image ); // Release only after both hit the disk
-	close( fd );
-	free( map );
+	char mapfile[PATHLEN] = "";
+	free( image->cache_map );
+	image->cache_map = NULL;
+	snprintf( mapfile, PATHLEN, "%s.map", image->path );
+	spin_unlock( &image->lock );
+	unlink( mapfile );
 	return true;
 }
 
@@ -435,7 +340,6 @@ dnbd3_image_t* image_get(char *name, uint16_t revision, bool checkIfWorking)
 		img->atime = now;
 		img->masterCrc32 = candidate->masterCrc32;
 		img->readFd = -1;
-		img->cacheFd = -1;
 		img->rid = candidate->rid;
 		img->users = 1;
 		img->working = false;
@@ -467,15 +371,7 @@ dnbd3_image_t* image_get(char *name, uint16_t revision, bool checkIfWorking)
 
 	// Check if image is incomplete, handle
 	if ( candidate->cache_map != NULL ) {
-		// -- Incomplete - rw check
-		if ( candidate->cacheFd == -1 ) { // Make sure file is open for writing
-			image_reopenCacheFd( candidate, false );
-			// It might have failed - still offer proxy mode, we just can't cache
-			if ( candidate->cacheFd == -1 ) {
-				logadd( LOG_WARNING, "Cannot re-open %s for writing - replication disabled", candidate->path );
-			}
-		}
-		if ( candidate->uplink == NULL && candidate->cacheFd != -1 ) {
+		if ( candidate->uplink == NULL ) {
 			uplink_init( candidate, -1, NULL, -1 );
 		}
 	}
@@ -484,49 +380,6 @@ dnbd3_image_t* image_get(char *name, uint16_t revision, bool checkIfWorking)
 }
 
 /**
- * Open the given image's main image file in
- * rw mode, assigning it to the cacheFd struct member.
- *
- * @param force If cacheFd was previously assigned a file descriptor (not == -1),
- * it will be closed first. Otherwise, nothing will happen and true will be returned
- * immediately.
- */
-bool image_reopenCacheFd(dnbd3_image_t *image, const bool force)
-{
-	if ( image->cacheFd != -1 && !force )
-		return true;
-	const int nfd = open( image->path, O_RDWR );
-	if ( nfd == -1 ) {
-		if ( force ) {
-			// Opening new one failed, force is enabled -- close old one
-			spin_lock( &image->lock );
-			int ofd = image->cacheFd;
-			image->cacheFd = -1;
-			spin_unlock( &image->lock );
-			if ( ofd != -1 ) {
-				close( ofd );
-			}
-		}
-		return false;
-	}
-	// Open succeeded, now switch out while holding lock to make it look somewhat atomic
-	spin_lock( &image->lock );
-	int closeFd = -1;
-	if ( force || image->cacheFd == -1 ) {
-		closeFd = image->cacheFd;
-		image->cacheFd = nfd;
-	} else {
-		closeFd = nfd;
-	}
-	spin_unlock( &image->lock );
-	if ( closeFd != -1 ) { // Failed
-		close( closeFd );
-	}
-	return true; // We either replaced the fd in force mode or the old one was -1, or
-	// force was false and cacheFd != -1. Either way we consider that success
-}
-
-/**
  * Lock the image by increasing its users count
  * Returns the image on success, NULL if it is not found in the image list
  * Every call to image_lock() needs to be followed by a call to image_release() at some point.
@@ -636,7 +489,12 @@ void image_killUplinks()
 		if ( _images[i] == NULL ) continue;
 		spin_lock( &_images[i]->lock );
 		if ( _images[i]->uplink != NULL ) {
-			_images[i]->uplink->shutdown = true;
+			spin_lock( &_images[i]->uplink->queueLock );
+			if ( !_images[i]->uplink->shutdown ) {
+				thread_detach( _images[i]->uplink->thread );
+				_images[i]->uplink->shutdown = true;
+			}
+			spin_unlock( &_images[i]->uplink->queueLock );
 			signal_call( _images[i]->uplink->signal );
 		}
 		spin_unlock( &_images[i]->lock );
@@ -741,15 +599,17 @@ static dnbd3_image_t* image_free(dnbd3_image_t *image)
 		logadd( LOG_INFO, "Freeing image %s:%d", image->name, (int)image->rid );
 	}
 	//
-	image_saveCacheMap( image );
 	uplink_shutdown( image );
 	spin_lock( &image->lock );
 	free( image->cache_map );
 	free( image->crc32 );
 	free( image->path );
 	free( image->name );
+	image->cache_map = NULL;
+	image->crc32 = NULL;
+	image->path = NULL;
+	image->name = NULL;
 	spin_unlock( &image->lock );
-	if ( image->cacheFd != -1 ) close( image->cacheFd );
 	if ( image->readFd != -1 ) close( image->readFd );
 	spin_destroy( &image->lock );
 	//
@@ -758,7 +618,7 @@ static dnbd3_image_t* image_free(dnbd3_image_t *image)
 	return NULL ;
 }
 
-static bool image_isHashBlockComplete(const uint8_t * const cacheMap, const uint64_t block, const uint64_t realFilesize)
+bool image_isHashBlockComplete(const uint8_t * const cacheMap, const uint64_t block, const uint64_t realFilesize)
 {
 	if ( cacheMap == NULL ) return true;
 	const uint64_t end = (block + 1) * HASH_BLOCK_SIZE;
@@ -953,6 +813,7 @@ static bool image_load(char *base, char *path, int withUplink)
 	// XXX: Maybe try sha-256 or 512 first if you're paranoid (to be implemented)
 
 	// 2. Load CRC-32 list of image
+	bool doFullCheck = false;
 	uint32_t masterCrc = 0;
 	const int hashBlockCount = IMGSIZE_TO_HASHBLOCKS( virtualFilesize );
 	crc32list = image_loadCrcList( path, virtualFilesize, &masterCrc );
@@ -961,7 +822,7 @@ static bool image_load(char *base, char *path, int withUplink)
 	if ( crc32list != NULL ) {
 		if ( !image_checkRandomBlocks( 4, fdImage, realFilesize, crc32list, cache_map ) ) {
 			logadd( LOG_ERROR, "quick crc32 check of %s failed. Data corruption?", path );
-			goto load_error;
+			doFullCheck = true;
 		}
 	}
 
@@ -1012,7 +873,6 @@ static bool image_load(char *base, char *path, int withUplink)
 	image->rid = (uint16_t)revision;
 	image->users = 0;
 	image->readFd = -1;
-	image->cacheFd = -1;
 	image->working = (image->cache_map == NULL );
 	timing_get( &image->nextCompletenessEstimate );
 	image->completenessEstimate = -1;
@@ -1032,22 +892,13 @@ static bool image_load(char *base, char *path, int withUplink)
 	crc32list = NULL;
 
 	// Get rid of cache map if image is complete
-	if ( image->cache_map != NULL && image_isComplete( image ) ) {
-		image_markComplete( image );
-		image->working = true;
+	if ( image->cache_map != NULL ) {
+		image_isComplete( image );
 	}
 
-	// Image is definitely incomplete, open image file for writing, so we can update the cache
+	// Image is definitely incomplete, initialize uplink worker
 	if ( image->cache_map != NULL ) {
 		image->working = false;
-		image->cacheFd = open( path, O_WRONLY );
-		if ( image->cacheFd < 0 ) {
-			// Proxy mode without disk caching is pointless, bail out
-			image->cacheFd = -1;
-			logadd( LOG_ERROR, "Could not open incomplete image %s for writing!", path );
-			image = image_free( image );
-			goto load_error;
-		}
 		if ( withUplink ) {
 			uplink_init( image, -1, NULL, -1 );
 		}
@@ -1060,11 +911,16 @@ static bool image_load(char *base, char *path, int withUplink)
 		fdImage = -1;
 	} else {
 		logadd( LOG_ERROR, "Image list full: Could not add image %s", path );
-		image->readFd = -1;
+		image->readFd = -1; // Keep fdImage instead, will be closed below
 		image = image_free( image );
 		goto load_error;
 	}
 	logadd( LOG_DEBUG1, "Loaded image '%s:%d'\n", image->name, (int)image->rid );
+	// CRC errors found...
+	if ( doFullCheck ) {
+		logadd( LOG_INFO, "Queueing full CRC32 check for '%s:%d'\n", image->name, (int)image->rid );
+		integrity_check( image, -1 );
+	}
 
 	function_return = true;
 
@@ -1407,7 +1263,7 @@ server_fail: ;
 			while ( !image->working && ++i < 100 )
 				usleep( 2000 );
 		}
-	} else if ( uplinkSock >= 0 ) {
+	} else if ( uplinkSock != -1 ) {
 		close( uplinkSock );
 	}
 	return image;
@@ -1719,8 +1575,9 @@ int image_getCompletenessEstimate(dnbd3_image_t * const image)
 }
 
 /**
- * Check the CRC-32 of the given blocks. The array blocks is of variable length.
+ * Check the CRC-32 of the given blocks. The array "blocks" is of variable length.
  * !! pass -1 as the last block so the function knows when to stop !!
+ * Does NOT check whether block index is within image.
  * Returns true or false
  */
 bool image_checkBlocksCrc32(const int fd, uint32_t *crc32list, const int *blocks, const uint64_t realFilesize)