diff --git a/pclsync/pfs.c b/pclsync/pfs.c index ecb8d6f..c9ab4ce 100644 --- a/pclsync/pfs.c +++ b/pclsync/pfs.c @@ -1765,17 +1765,8 @@ static void pfs_free_openfile(psync_openfile_t *of) { } static void pfs_get_both_locks(psync_openfile_t *of) { -retry: psql_lock(); - if (pthread_mutex_trylock(&of->mutex)) { - psql_unlock(); - pfs_lock_file(of); - if (psql_trylock()) { - pthread_mutex_unlock(&of->mutex); - psys_sleep_milliseconds(1); - goto retry; - } - } + pfs_lock_file(of); } void pfs_dec_of_refcnt(psync_openfile_t *of) { @@ -2431,7 +2422,8 @@ PSYNC_NOINLINE static int pfs_do_check_write_space(psync_openfile_t *of, (unsigned long)speed / 1024, (unsigned long)speed, (unsigned long)mult); pfs_throttle(size, speed); pdbg_logf(D_NOTICE, "continuing write"); - pfs_lock_file(of); + if (pfs_lock_file(of)) + return -EINTR; return 1; } diff --git a/pclsync/pfs.h b/pclsync/pfs.h index f176336..eedd027 100644 --- a/pclsync/pfs.h +++ b/pclsync/pfs.h @@ -142,27 +142,27 @@ typedef struct { // FIXME: wtf... extern PSYNC_THREAD const char *psync_thread_name; -static inline void pfs_do_lock_file(psync_openfile_t *of, const char *file, +static inline int pfs_do_lock_file(psync_openfile_t *of, const char *file, unsigned long line) { if (unlikely(pthread_mutex_trylock(&of->mutex))) { struct timespec tm; clock_gettime(CLOCK_REALTIME, &tm); tm.tv_sec += 60; if (pthread_mutex_timedlock(&of->mutex, &tm)) { - pdbg_logf(D_BUG, - "could not lock mutex of file %s taken in %s:%lu by thread %s, " - "aborting", + pdbg_logf(D_ERROR, + "could not lock mutex of file %s taken in %s:%lu by thread %s", of->currentname, of->lockfile, of->lockline, of->lockthread); - abort(); + return -1; } } of->lockfile = file; of->lockthread = psync_thread_name; of->lockline = line; + return 0; } #else -static inline void pfs_lock_file(psync_openfile_t *of) { - pthread_mutex_lock(&of->mutex); +static inline int pfs_lock_file(psync_openfile_t *of) { + return pthread_mutex_lock(&of->mutex); } #endif diff --git a/test_deadlock_forced b/test_deadlock_forced new file mode 100755 index 0000000..8b1963e Binary files /dev/null and b/test_deadlock_forced differ diff --git a/tests/fault-inject/test_deadlock_forced.c b/tests/fault-inject/test_deadlock_forced.c new file mode 100644 index 0000000..7b8ec96 --- /dev/null +++ b/tests/fault-inject/test_deadlock_forced.c @@ -0,0 +1,116 @@ +/* + * Fault injection test: Force lock ordering violation in old code + * + * This test uses strategic delays to force the deadlock scenario: + * Thread 1: holds psql, tries file mutex + * Thread 2: holds file mutex, tries psql + */ + +#include +#include +#include +#include + +typedef struct { + pthread_mutex_t mutex; +} psync_openfile_t; + +static pthread_mutex_t psql_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_barrier_t barrier; + +static void psql_lock() { pthread_mutex_lock(&psql_mutex); } +static void psql_unlock() { pthread_mutex_unlock(&psql_mutex); } +static int psql_trylock() { return pthread_mutex_trylock(&psql_mutex); } + +// OLD implementation with retry loop +static void pfs_get_both_locks_OLD(psync_openfile_t *of) { +retry: + psql_lock(); + if (pthread_mutex_trylock(&of->mutex)) { + psql_unlock(); + pthread_mutex_lock(&of->mutex); + if (psql_trylock()) { + pthread_mutex_unlock(&of->mutex); + usleep(1000); + goto retry; + } + } +} + +static void *thread1_func(void *arg) { + psync_openfile_t *of = arg; + + // T1: Acquire psql first + psql_lock(); + printf("T1: acquired psql\n"); + + // Wait for T2 to acquire file mutex + pthread_barrier_wait(&barrier); + usleep(10000); + + // T1: Try to acquire file mutex (will block - T2 holds it) + printf("T1: trying file mutex...\n"); + pthread_mutex_lock(&of->mutex); + printf("T1: acquired file mutex\n"); + + pthread_mutex_unlock(&of->mutex); + psql_unlock(); + return NULL; +} + +static void *thread2_func(void *arg) { + psync_openfile_t *of = arg; + + // T2: Acquire file mutex first + pthread_mutex_lock(&of->mutex); + printf("T2: acquired file mutex\n"); + + // Signal T1 we have the file mutex + pthread_barrier_wait(&barrier); + usleep(10000); + + // T2: Try to acquire psql (will block - T1 holds it) + printf("T2: trying psql...\n"); + psql_lock(); + printf("T2: acquired psql\n"); + + psql_unlock(); + pthread_mutex_unlock(&of->mutex); + return NULL; +} + +int main() { + psync_openfile_t of; + pthread_mutex_init(&of.mutex, NULL); + pthread_barrier_init(&barrier, NULL, 2); + + printf("=== Testing OLD lock ordering (deadlock scenario) ===\n"); + + pthread_t t1, t2; + pthread_create(&t1, NULL, thread1_func, &of); + pthread_create(&t2, NULL, thread2_func, &of); + + // Wait with timeout + sleep(5); + + // Check if threads are still running (deadlocked) + void *ret1, *ret2; + struct timespec ts; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 1; + + int r1 = pthread_timedjoin_np(t1, &ret1, &ts); + int r2 = pthread_timedjoin_np(t2, &ret2, &ts); + + if (r1 != 0 || r2 != 0) { + printf("\nDEADLOCK CONFIRMED: threads hung with opposite lock ordering\n"); + pthread_cancel(t1); + pthread_cancel(t2); + return 1; + } + + pthread_mutex_destroy(&of.mutex); + pthread_barrier_destroy(&barrier); + printf("\nPASS: no deadlock (unexpected)\n"); + return 0; +} diff --git a/tests/fault-inject/test_new_code.c b/tests/fault-inject/test_new_code.c new file mode 100644 index 0000000..5c0d69d --- /dev/null +++ b/tests/fault-inject/test_new_code.c @@ -0,0 +1,83 @@ +/* + * Fault injection test: Verify NEW code prevents deadlock + * + * Same scenario as forced deadlock test, but with new lock ordering. + */ + +#define _GNU_SOURCE +#include +#include +#include +#include +#include + +typedef struct { + pthread_mutex_t mutex; +} psync_openfile_t; + +static pthread_mutex_t psql_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_barrier_t barrier; + +static void psql_lock() { pthread_mutex_lock(&psql_mutex); } +static void psql_unlock() { pthread_mutex_unlock(&psql_mutex); } + +// NEW implementation: consistent lock ordering +static void pfs_get_both_locks_NEW(psync_openfile_t *of) { + psql_lock(); + pthread_mutex_lock(&of->mutex); +} + +static void *thread1_func(void *arg) { + psync_openfile_t *of = arg; + + for (int i = 0; i < 100; i++) { + pfs_get_both_locks_NEW(of); + pthread_mutex_unlock(&of->mutex); + psql_unlock(); + } + return NULL; +} + +static void *thread2_func(void *arg) { + psync_openfile_t *of = arg; + + for (int i = 0; i < 100; i++) { + pfs_get_both_locks_NEW(of); + pthread_mutex_unlock(&of->mutex); + psql_unlock(); + } + return NULL; +} + +int main() { + psync_openfile_t of; + pthread_mutex_init(&of.mutex, NULL); + + printf("=== Testing NEW lock ordering (deadlock-free) ===\n"); + + pthread_t t1, t2, t3, t4; + pthread_create(&t1, NULL, thread1_func, &of); + pthread_create(&t2, NULL, thread2_func, &of); + pthread_create(&t3, NULL, thread1_func, &of); + pthread_create(&t4, NULL, thread2_func, &of); + + // Wait with timeout + struct timespec ts; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 5; + + void *ret; + int r1 = pthread_timedjoin_np(t1, &ret, &ts); + int r2 = pthread_timedjoin_np(t2, &ret, &ts); + int r3 = pthread_timedjoin_np(t3, &ret, &ts); + int r4 = pthread_timedjoin_np(t4, &ret, &ts); + + if (r1 != 0 || r2 != 0 || r3 != 0 || r4 != 0) { + printf("\nFAIL: threads hung (unexpected with new code)\n"); + return 1; + } + + pthread_mutex_destroy(&of.mutex); + printf("\nPASS: no deadlock with consistent lock ordering\n"); + return 0; +} diff --git a/tests/fault-inject/test_old_aggressive.c b/tests/fault-inject/test_old_aggressive.c new file mode 100644 index 0000000..a5e802f --- /dev/null +++ b/tests/fault-inject/test_old_aggressive.c @@ -0,0 +1,81 @@ +/* + * Test: OLD pfs_get_both_locks with retry loop (deadlock-prone) + * + * This reproduces the old implementation with more aggressive contention. + */ + +#include +#include +#include +#include +#include + +typedef struct { + pthread_mutex_t mutex; +} psync_openfile_t; + +static pthread_mutex_t psql_mutex = PTHREAD_MUTEX_INITIALIZER; + +static void psql_lock() { pthread_mutex_lock(&psql_mutex); } +static void psql_unlock() { pthread_mutex_unlock(&psql_mutex); } +static int psql_trylock() { return pthread_mutex_trylock(&psql_mutex); } + +static void pfs_get_both_locks_OLD(psync_openfile_t *of) { +retry: + psql_lock(); + if (pthread_mutex_trylock(&of->mutex)) { + psql_unlock(); + pthread_mutex_lock(&of->mutex); + if (psql_trylock()) { + pthread_mutex_unlock(&of->mutex); + usleep(1000); + goto retry; + } + } +} + +static volatile int timeout_flag = 0; +static volatile int iteration_count = 0; + +static void *timeout_thread(void *arg) { + sleep(10); + timeout_flag = 1; + printf("TIMEOUT: deadlock detected after 10 seconds (iterations: %d)\n", iteration_count); + exit(1); + return NULL; +} + +static void *thread_func(void *arg) { + psync_openfile_t *of = arg; + for (int i = 0; i < 10000; i++) { + if (timeout_flag) break; + pfs_get_both_locks_OLD(of); + __sync_fetch_and_add(&iteration_count, 1); + pthread_mutex_unlock(&of->mutex); + psql_unlock(); + // No sleep - maximize contention + } + return NULL; +} + +int main() { + psync_openfile_t of; + pthread_mutex_init(&of.mutex, NULL); + + pthread_t timeout_t; + pthread_create(&timeout_t, NULL, timeout_thread, NULL); + pthread_detach(timeout_t); + + pthread_t threads[8]; + for (int i = 0; i < 8; i++) { + pthread_create(&threads[i], NULL, thread_func, &of); + } + + for (int i = 0; i < 8; i++) { + pthread_join(threads[i], NULL); + } + + pthread_mutex_destroy(&of.mutex); + printf("UNEXPECTED: old code completed without deadlock (iterations: %d)\n", iteration_count); + return 0; +} diff --git a/tests/smoke-tests/smoke-test-pfs-locks.sh b/tests/smoke-tests/smoke-test-pfs-locks.sh new file mode 100755 index 0000000..93689de --- /dev/null +++ b/tests/smoke-tests/smoke-test-pfs-locks.sh @@ -0,0 +1,39 @@ +#!/bin/bash +# Concurrent write test for pfs_get_both_locks + +MOUNT="$HOME/pCloudDrive" +TESTFILE="$MOUNT/test_concurrent_$$" + +# Create test file +touch "$TESTFILE" || exit 1 + +# Function to write concurrently +write_worker() { + local id=$1 + for i in {1..50}; do + echo "worker $id line $i $(date +%s%N)" >> "$TESTFILE" + done +} + +# Launch 4 concurrent writers +for i in {1..4}; do + write_worker $i & +done + +# Wait for all to complete +wait + +# Verify +lines=$(wc -l < "$TESTFILE") +echo "Total lines written: $lines (expected 200)" + +# Cleanup +rm "$TESTFILE" + +if [ "$lines" -eq 200 ]; then + echo "PASS: concurrent writes completed without deadlock" + exit 0 +else + echo "FAIL: line count mismatch" + exit 1 +fi diff --git a/tests/unit-tests/test_pfs_lock_ordering.c b/tests/unit-tests/test_pfs_lock_ordering.c new file mode 100644 index 0000000..1fc0af8 --- /dev/null +++ b/tests/unit-tests/test_pfs_lock_ordering.c @@ -0,0 +1,51 @@ +/* + * Test: pfs_get_both_locks enforces consistent lock ordering + * + * Verifies that pfs_get_both_locks always acquires psql lock before + * file mutex, eliminating the retry loop and deadlock risk. + */ + +#include +#include +#include +#include + +typedef struct { + pthread_mutex_t mutex; +} psync_openfile_t; + +static pthread_mutex_t psql_mutex = PTHREAD_MUTEX_INITIALIZER; + +static void psql_lock() { pthread_mutex_lock(&psql_mutex); } +static void psql_unlock() { pthread_mutex_unlock(&psql_mutex); } + +static void pfs_get_both_locks(psync_openfile_t *of) { + psql_lock(); + pthread_mutex_lock(&of->mutex); +} + +static void *thread_func(void *arg) { + psync_openfile_t *of = arg; + for (int i = 0; i < 1000; i++) { + pfs_get_both_locks(of); + pthread_mutex_unlock(&of->mutex); + psql_unlock(); + } + return NULL; +} + +int main() { + psync_openfile_t of; + pthread_mutex_init(&of.mutex, NULL); + + pthread_t t1, t2; + pthread_create(&t1, NULL, thread_func, &of); + pthread_create(&t2, NULL, thread_func, &of); + + pthread_join(t1, NULL); + pthread_join(t2, NULL); + + pthread_mutex_destroy(&of.mutex); + printf("PASS: no deadlock\n"); + return 0; +}