From 2b722b756864b14b6299a8b24067a37e12aabfc4 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Tue, 27 Jan 2015 19:39:04 +0100 Subject: [SERVER] Fix automatic proxying to use supplied connection; fix race condition in uplink_init --- src/config.h | 2 +- src/server/globals.h | 2 +- src/server/image.c | 30 ++++++++++++++++-------------- src/server/image.h | 2 +- src/server/threadpool.c | 2 -- src/server/uplink.c | 44 ++++++++++++++++++++++++++------------------ 6 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/config.h b/src/config.h index 6f8c33e..271e4bc 100644 --- a/src/config.h +++ b/src/config.h @@ -41,7 +41,7 @@ #define SERVER_RTT_DELAY_MAX 45 #define SERVER_RTT_DELAY_FAILED 180 -#define SERVER_REMOTE_IMAGE_CHECK_CACHETIME 600 // 10 minutes +#define SERVER_REMOTE_IMAGE_CHECK_CACHETIME 120 // 2 minutes #define SERVER_MAX_PROXY_IMAGE_SIZE 100000000000LL // 100GB // +++++ Network +++++ // Default port diff --git a/src/server/globals.h b/src/server/globals.h index 1edf44f..2ea1c43 100644 --- a/src/server/globals.h +++ b/src/server/globals.h @@ -107,7 +107,7 @@ struct _dnbd3_image int rid; // revision of image int users; // clients currently using this image time_t atime; // last access time - bool working; // true if image exists and completeness is == 100% or a working upstream proxy is connected + volatile bool working; // true if image exists and completeness is == 100% or a working upstream proxy is connected pthread_spinlock_t lock; }; diff --git a/src/server/image.c b/src/server/image.c index 27b737e..d8c7113 100644 --- a/src/server/image.c +++ b/src/server/image.c @@ -40,7 +40,7 @@ typedef struct time_t deadline; } imagecache; static imagecache remoteCloneCache[CACHELEN]; -static int remoteCloneCacheIndex = -1; +static int remoteCloneCacheIndex = 0; // ########################################## @@ -232,7 +232,7 @@ bool image_saveCacheMap(dnbd3_image_t *image) * point... * Locks on: _images_lock, _images[].lock */ -dnbd3_image_t* image_get(char *name, uint16_t revision) +dnbd3_image_t* image_get(char *name, uint16_t revision, bool checkIfWorking) { int i; dnbd3_image_t *candidate = NULL; @@ -265,6 +265,8 @@ dnbd3_image_t* image_get(char *name, uint16_t revision) candidate->users++; spin_unlock( &candidate->lock ); + if ( !checkIfWorking ) return candidate; + // Found, see if it works struct stat st; if ( candidate->working && stat( candidate->path, &st ) < 0 ) { @@ -420,16 +422,18 @@ static dnbd3_image_t* image_free(dnbd3_image_t *image) // image_saveCacheMap( image ); uplink_shutdown( image ); + spin_lock( &image->lock ); free( image->cache_map ); free( image->crc32 ); free( image->path ); free( image->lower_name ); + spin_unlock( &image->lock ); if ( image->cacheFd != -1 ) { close( image->cacheFd ); } spin_destroy( &image->lock ); // - memset( image, 0, sizeof(dnbd3_image_t) ); + memset( image, 0, sizeof(*image) ); free( image ); return NULL ; } @@ -557,7 +561,7 @@ static bool image_load(char *base, char *path, int withUplink) strtolower( imgName ); // Get pointer to already existing image if possible - existing = image_get( imgName, revision ); + existing = image_get( imgName, revision, true ); // ### Now load the actual image related data ### fdImage = open( path, O_RDONLY ); @@ -874,19 +878,15 @@ bool image_create(char *image, int revision, uint64_t size) */ dnbd3_image_t* image_getOrClone(char *name, uint16_t revision) { - if ( !_isProxy ) return image_get( name, revision ); + if ( !_isProxy ) return image_get( name, revision, true ); int i; const size_t len = strlen( name ); // Sanity check if ( len == 0 || name[len - 1] == '/' || name[0] == '/' ) return NULL ; // Already existing locally? - dnbd3_image_t *image = image_get( name, revision ); + dnbd3_image_t *image = image_get( name, revision, true ); if ( image != NULL ) return image; // Doesn't exist, try remote if not already tried it recently - if ( remoteCloneCacheIndex == -1 ) { - remoteCloneCacheIndex = 0; - memset( remoteCloneCache, 0, sizeof(remoteCloneCache) ); - } const time_t now = time( NULL ); char *cmpname = name; @@ -897,10 +897,10 @@ dnbd3_image_t* image_getOrClone(char *name, uint16_t revision) || remoteCloneCache[i].deadline < now || strcmp( cmpname, remoteCloneCache[i].name ) != 0 ) continue; pthread_mutex_unlock( &remoteCloneLock ); // Was recently checked... - return image_get( name, revision ); + return image_get( name, revision, true ); } // Re-check to prevent two clients at the same time triggering this - image = image_get( name, revision ); + image = image_get( name, revision, true ); if ( image != NULL ) { pthread_mutex_unlock( &remoteCloneLock ); return image; @@ -940,11 +940,13 @@ dnbd3_image_t* image_getOrClone(char *name, uint16_t revision) } pthread_mutex_unlock( &remoteCloneLock ); // If everything worked out, this call should now actually return the image - image = image_get( name, remoteRid ); + image = image_get( name, remoteRid, false ); if ( image != NULL && uplinkSock != -1 && uplinkServer != NULL ) { // If so, init the uplink and pass it the socket uplink_init( image, uplinkSock, uplinkServer ); - image->working = true; + i = 0; + while ( !image->working && ++i < 100 ) + usleep( 1000 ); } else if ( uplinkSock >= 0 ) { close( uplinkSock ); } diff --git a/src/server/image.h b/src/server/image.h index 664225c..aca3788 100644 --- a/src/server/image.h +++ b/src/server/image.h @@ -18,7 +18,7 @@ void image_saveAllCacheMaps(); bool image_saveCacheMap(dnbd3_image_t *image); -dnbd3_image_t* image_get(char *name, uint16_t revision); +dnbd3_image_t* image_get(char *name, uint16_t revision, bool checkIfWorking); dnbd3_image_t* image_getOrClone(char *name, uint16_t revision); diff --git a/src/server/threadpool.c b/src/server/threadpool.c index fd90944..2d621c5 100644 --- a/src/server/threadpool.c +++ b/src/server/threadpool.c @@ -72,7 +72,6 @@ bool threadpool_run(void *(*startRoutine)(void *), void *arg) free( entry ); return false; } - printf( "[DEBUG] Thread created!\n" ); } entry->next = NULL; entry->startRoutine = startRoutine; @@ -125,7 +124,6 @@ static void *threadpool_worker(void *entryPtr) } signal_close( entry->signalFd ); free( entry ); - printf(" [DEBUG] Thread killed!\n" ); return NULL; } diff --git a/src/server/uplink.c b/src/server/uplink.c index cc82fe0..fdc4b27 100644 --- a/src/server/uplink.c +++ b/src/server/uplink.c @@ -43,7 +43,8 @@ bool uplink_init(dnbd3_image_t *image, int sock, dnbd3_host_t *host) assert( image != NULL ); spin_lock( &image->lock ); if ( image->uplink != NULL ) { - goto failure; + spin_unlock( &image->lock ); + return true; // There's already an uplink, so should we consider this success or failure? } if ( image->cache_map == NULL ) { memlogf( "[WARNING] Uplink was requested for image %s, but it is already complete", image->lower_name ); @@ -66,47 +67,49 @@ bool uplink_init(dnbd3_image_t *image, int sock, dnbd3_host_t *host) link->recvBufferLen = 0; link->shutdown = false; spin_init( &link->queueLock, PTHREAD_PROCESS_PRIVATE ); - if ( 0 != thread_create( &(link->thread), NULL, &uplink_mainloop, (void *)(uintptr_t)link ) ) { - memlogf( "[ERROR] Could not start thread for new client." ); + if ( 0 != thread_create( &(link->thread), NULL, &uplink_mainloop, (void *)link ) ) { + memlogf( "[ERROR] Could not start thread for new uplink." ); goto failure; } spin_unlock( &image->lock ); return true; failure: ; - if ( link != NULL ) free( link ); - link = image->uplink = NULL; + if ( link != NULL ) { + free( link ); + link = image->uplink = NULL; + } spin_unlock( &image->lock ); return false; } /** * Locks on image.lock, uplink.lock - * Sets image->uplink to NULL while locked, so - * calling it multiple times, even concurrently, will + * Calling it multiple times, even concurrently, will * not break anything. */ void uplink_shutdown(dnbd3_image_t *image) { + bool join = false; + pthread_t thread; assert( image != NULL ); spin_lock( &image->lock ); - if ( image->uplink == NULL || image->uplink->shutdown ) { + if ( image->uplink == NULL ) { spin_unlock( &image->lock ); return; } dnbd3_connection_t * const uplink = image->uplink; spin_lock( &uplink->queueLock ); - if ( uplink->shutdown ) { - spin_unlock( &uplink->queueLock ); - spin_unlock( &image->lock ); - return; + if ( !uplink->shutdown ) { + uplink->shutdown = true; + signal_call( uplink->signal ); + thread = uplink->thread; + join = true; } - image->uplink = NULL; - uplink->shutdown = true; - signal_call( uplink->signal ); - pthread_t thread = uplink->thread; spin_unlock( &uplink->queueLock ); spin_unlock( &image->lock ); - thread_join( thread, NULL ); + if ( join ) thread_join( thread, NULL ); + while ( image->uplink != NULL ) + usleep( 10000 ); } /** @@ -140,6 +143,11 @@ bool uplink_request(dnbd3_client_t *client, uint64_t handle, uint64_t start, uin return false; } dnbd3_connection_t * const uplink = client->image->uplink; + if ( uplink->shutdown ) { + spin_unlock( &client->image->lock ); + printf( "[DEBUG] Uplink request for image with uplink shutting down (%s)\n", client->image->lower_name ); + return false; + } // Check if the client is the same host as the uplink. If so assume this is a circular proxy chain if ( isSameAddress( &uplink->currentServer, &client->host ) ) { spin_unlock( &client->image->lock ); @@ -253,7 +261,7 @@ static void* uplink_mainloop(void *data) link->image->working = true; buffer[0] = '@'; if ( host_to_string( &link->currentServer, buffer + 1, sizeof(buffer) - 1 ) ) { - printf( "[DEBUG] Now connected to %s\n", buffer + 1 ); + printf( "[DEBUG] (Uplink %s) Now connected to %s\n", link->image->lower_name, buffer + 1 ); setThreadName( buffer ); } // Re-send all pending requests -- cgit v1.2.3-55-g7522