From cbf2040b89cdf6bfff4cd608ed31ff16d5476665 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Fri, 1 Jan 2016 10:00:18 +0100 Subject: [PATCH] Add mutex protection at the filesystem structure In sync we need to modify the filesystem, like to call fs_deallocate(). We need then proper protection. This also removes the need of multiple fs_last cached value. --- cmdline/elem.c | 115 +++++++++++++++++++++++++++++++++++------------ cmdline/elem.h | 47 +++++++------------ cmdline/io.c | 1 - cmdline/io.h | 9 ---- cmdline/parity.c | 4 +- cmdline/scrub.c | 4 +- cmdline/state.c | 24 ++++------ cmdline/sync.c | 4 +- 8 files changed, 117 insertions(+), 91 deletions(-) diff --git a/cmdline/elem.c b/cmdline/elem.c index 2f8caf61..21187320 100644 --- a/cmdline/elem.c +++ b/cmdline/elem.c @@ -737,6 +737,8 @@ struct snapraid_disk* disk_alloc(const char* name, const char* dir, uint64_t dev /* ensure that the dir terminate with "/" if it isn't empty */ pathslash(disk->dir, sizeof(disk->dir)); + pthread_mutex_init(&disk->fs_mutex, 0); + disk->smartctl[0] = 0; disk->device = dev; disk->tick = 0; @@ -778,9 +780,22 @@ void disk_free(struct snapraid_disk* disk) tommy_hashdyn_done(&disk->linkset); tommy_list_foreach(&disk->dirlist, (tommy_foreach_func*)dir_free); tommy_hashdyn_done(&disk->dirset); + + pthread_mutex_destroy(&disk->fs_mutex); + free(disk); } +static inline void fs_lock(struct snapraid_disk* disk) +{ + pthread_mutex_lock(&disk->fs_mutex); +} + +static inline void fs_unlock(struct snapraid_disk* disk) +{ + pthread_mutex_unlock(&disk->fs_mutex); +} + struct chunk_disk_empty { block_off_t blockmax; }; @@ -788,7 +803,7 @@ struct chunk_disk_empty { /** * Compare the chunk if inside the specified blockmax. */ -static int chunk_disk_empty_compare(const void* void_a, const void* void_b) +static int chunk_disk_empty_compare_unlock(const void* void_a, const void* void_b) { const struct chunk_disk_empty* arg_a = void_a; const struct snapraid_chunk* arg_b = void_b; @@ -801,7 +816,7 @@ static int chunk_disk_empty_compare(const void* void_a, const void* void_b) return -1; } -int disk_is_empty(struct snapraid_disk* disk, block_off_t blockmax) +int fs_is_empty(struct snapraid_disk* disk, block_off_t blockmax) { struct chunk_disk_empty arg = { blockmax }; @@ -814,11 +829,16 @@ int disk_is_empty(struct snapraid_disk* disk, block_off_t blockmax) if (!tommy_list_empty(&disk->dirlist)) return 0; + fs_lock(disk); + /* search for any chunk inside blockmax */ - if (tommy_tree_search_compare(&disk->fs_parity, chunk_disk_empty_compare, &arg) != 0) + if (tommy_tree_search_compare(&disk->fs_parity, chunk_disk_empty_compare_unlock, &arg) != 0) { + fs_unlock(disk); return 0; + } /* finally, it's empty */ + fs_unlock(disk); return 1; } @@ -831,7 +851,7 @@ struct chunk_disk_size { * * The maximum parity position is stored as size. */ -static int chunk_disk_size_compare(const void* void_a, const void* void_b) +static int chunk_disk_size_compare_unlock(const void* void_a, const void* void_b) { struct chunk_disk_size* arg_a = (void*)void_a; const struct snapraid_chunk* arg_b = void_b; @@ -844,10 +864,16 @@ static int chunk_disk_size_compare(const void* void_a, const void* void_b) return 1; } -block_off_t disk_size(struct snapraid_disk* disk) +block_off_t fs_size(struct snapraid_disk* disk) { struct chunk_disk_size arg = { 0 }; - tommy_tree_search_compare(&disk->fs_parity, chunk_disk_size_compare, &arg); + + fs_lock(disk); + + tommy_tree_search_compare(&disk->fs_parity, chunk_disk_size_compare_unlock, &arg); + + fs_unlock(disk); + return arg.size; } @@ -856,7 +882,7 @@ struct chunk_check { int result; }; -void chunk_parity_check_foreach(void* void_arg, void* void_obj) +static void chunk_parity_check_foreach_unlock(void* void_arg, void* void_obj) { struct chunk_check* arg = void_arg; const struct snapraid_chunk* obj = void_obj; @@ -906,7 +932,7 @@ void chunk_parity_check_foreach(void* void_arg, void* void_obj) } } -void chunk_file_check_foreach(void* void_arg, void* void_obj) +static void chunk_file_check_foreach_unlock(void* void_arg, void* void_obj) { struct chunk_check* arg = void_arg; const struct snapraid_chunk* obj = void_obj; @@ -1022,13 +1048,17 @@ int fs_check(struct snapraid_disk* disk) /* error count starts from 0 */ arg.result = 0; + fs_lock(disk); + /* check parity sequence */ arg.prev = 0; - tommy_tree_foreach_arg(&disk->fs_parity, chunk_parity_check_foreach, &arg); + tommy_tree_foreach_arg(&disk->fs_parity, chunk_parity_check_foreach_unlock, &arg); /* check file sequence */ arg.prev = 0; - tommy_tree_foreach_arg(&disk->fs_file, chunk_file_check_foreach, &arg); + tommy_tree_foreach_arg(&disk->fs_file, chunk_file_check_foreach_unlock, &arg); + + fs_unlock(disk); if (arg.result != 0) return -1; @@ -1043,7 +1073,7 @@ struct chunk_parity_inside { /** * Compare the chunk if containing the specified parity position. */ -static int chunk_parity_inside_compare(const void* void_a, const void* void_b) +static int chunk_parity_inside_compare_unlock(const void* void_a, const void* void_b) { const struct chunk_parity_inside* arg_a = void_a; const struct snapraid_chunk* arg_b = void_b; @@ -1061,7 +1091,7 @@ static int chunk_parity_inside_compare(const void* void_a, const void* void_b) * The search is optimized for sequential accesses. * \return If not found return 0 */ -static struct snapraid_chunk* fs_par2chunk_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos) +static struct snapraid_chunk* fs_par2chunk_get_unlock(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos) { struct snapraid_chunk* chunk; @@ -1073,7 +1103,7 @@ static struct snapraid_chunk* fs_par2chunk_get_ts(struct snapraid_disk* disk, st chunk = *fs_last; } else { struct chunk_parity_inside arg = { parity_pos }; - chunk = tommy_tree_search_compare(&disk->fs_parity, chunk_parity_inside_compare, &arg); + chunk = tommy_tree_search_compare(&disk->fs_parity, chunk_parity_inside_compare_unlock, &arg); } if (!chunk) @@ -1093,7 +1123,7 @@ struct chunk_file_inside { /** * Compare the chunk if containing the specified file position. */ -static int chunk_file_inside_compare(const void* void_a, const void* void_b) +static int chunk_file_inside_compare_unlock(const void* void_a, const void* void_b) { const struct chunk_file_inside* arg_a = void_a; const struct snapraid_chunk* arg_b = void_b; @@ -1116,7 +1146,7 @@ static int chunk_file_inside_compare(const void* void_a, const void* void_b) * The search is optimized for sequential accesses. * \return If not found return 0 */ -static struct snapraid_chunk* fs_file2chunk_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, struct snapraid_file* file, block_off_t file_pos) +static struct snapraid_chunk* fs_file2chunk_get_unlock(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, struct snapraid_file* file, block_off_t file_pos) { struct snapraid_chunk* chunk; @@ -1129,7 +1159,7 @@ static struct snapraid_chunk* fs_file2chunk_get_ts(struct snapraid_disk* disk, s chunk = *fs_last; } else { struct chunk_file_inside arg = { file, file_pos }; - chunk = tommy_tree_search_compare(&disk->fs_file, chunk_file_inside_compare, &arg); + chunk = tommy_tree_search_compare(&disk->fs_file, chunk_file_inside_compare_unlock, &arg); } if (!chunk) @@ -1141,25 +1171,37 @@ static struct snapraid_chunk* fs_file2chunk_get_ts(struct snapraid_disk* disk, s return chunk; } -struct snapraid_file* fs_par2file_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos, block_off_t* file_pos) +struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t parity_pos, block_off_t* file_pos) { struct snapraid_chunk* chunk; + struct snapraid_file* file; - chunk = fs_par2chunk_get_ts(disk, fs_last, parity_pos); - if (!chunk) + fs_lock(disk); + + chunk = fs_par2chunk_get_unlock(disk, &disk->fs_last, parity_pos); + + if (!chunk) { + fs_unlock(disk); return 0; + } if (file_pos) *file_pos = chunk->file_pos + (parity_pos - chunk->parity_pos); - return chunk->file; + file = chunk->file; + + fs_unlock(disk); + return file; } -block_off_t fs_file2par_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, struct snapraid_file* file, block_off_t file_pos) +block_off_t fs_file2par_get(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos) { struct snapraid_chunk* chunk; + block_off_t ret; - chunk = fs_file2chunk_get_ts(disk, fs_last, file, file_pos); + fs_lock(disk); + + chunk = fs_file2chunk_get_unlock(disk, &disk->fs_last, file, file_pos); if (!chunk) { /* LCOV_EXCL_START */ log_fatal("Internal inconsistency when resolving file '%s' at position '%u/%u' in disk '%s'\n", file->sub, file_pos, file->blockmax, disk->name); @@ -1167,7 +1209,10 @@ block_off_t fs_file2par_get_ts(struct snapraid_disk* disk, struct snapraid_chunk /* LCOV_EXCL_STOP */ } - return chunk->parity_pos + (file_pos - chunk->file_pos); + ret = chunk->parity_pos + (file_pos - chunk->file_pos); + + fs_unlock(disk); + return ret; } void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snapraid_file* file, block_off_t file_pos) @@ -1176,9 +1221,11 @@ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snap struct snapraid_chunk* parity_chunk; struct snapraid_chunk* file_chunk; + fs_lock(disk); + if (file_pos > 0) { /* search an existing chunk for the previous file_pos */ - chunk = fs_file2chunk_get_ts(disk, &disk->fs_last, file, file_pos - 1); + chunk = fs_file2chunk_get_unlock(disk, &disk->fs_last, file, file_pos - 1); if (chunk != 0 && parity_pos == chunk->parity_pos + chunk->count) { /* ensure that we are extending the chunk at the end */ @@ -1192,6 +1239,7 @@ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snap /* extend the existing chunk */ ++chunk->count; + fs_unlock(disk); return; } } @@ -1212,6 +1260,8 @@ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snap /* store the last accessed chunk */ disk->fs_last = chunk; + + fs_unlock(disk); } void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) @@ -1222,7 +1272,9 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) struct snapraid_chunk* file_chunk; block_off_t first_count, second_count; - chunk = fs_par2chunk_get_ts(disk, &disk->fs_last, parity_pos); + fs_lock(disk); + + chunk = fs_par2chunk_get_unlock(disk, &disk->fs_last, parity_pos); if (!chunk) { /* LCOV_EXCL_START */ log_fatal("Internal inconsistency when deallocating parity position '%u' for not existing chunk in disk '%s'\n", parity_pos, disk->name); @@ -1241,6 +1293,8 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) /* clear the last accessed chunk */ disk->fs_last = 0; + + fs_unlock(disk); return; } @@ -1249,12 +1303,16 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) ++chunk->parity_pos; ++chunk->file_pos; --chunk->count; + + fs_unlock(disk); return; } /* if it's at the end of the chunk, shrink the chunk */ if (parity_pos == chunk->parity_pos + chunk->count - 1) { --chunk->count; + + fs_unlock(disk); return; } @@ -1281,6 +1339,8 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) /* store the last accessed chunk */ disk->fs_last = second_chunk; + + fs_unlock(disk); } struct snapraid_block* fs_file2block_get(struct snapraid_file* file, block_off_t file_pos) @@ -1295,13 +1355,12 @@ struct snapraid_block* fs_file2block_get(struct snapraid_file* file, block_off_t return &file->blockvec[file_pos]; } -struct snapraid_block* fs_par2block_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos) +struct snapraid_block* fs_par2block_get(struct snapraid_disk* disk, block_off_t parity_pos) { struct snapraid_file* file; block_off_t file_pos; - file = fs_par2file_get_ts(disk, fs_last, parity_pos, &file_pos); - + file = fs_par2file_get(disk, parity_pos, &file_pos); if (!file) return BLOCK_EMPTY; diff --git a/cmdline/elem.h b/cmdline/elem.h index 9264ba9b..e1a11a2c 100644 --- a/cmdline/elem.h +++ b/cmdline/elem.h @@ -347,6 +347,17 @@ struct snapraid_disk { int mapping_idx; /**< Index in the mapping vector. Used only as buffer when writing the content file. */ int skip_access; /**< If the disk is inaccessible and it should be skipped. */ + /** + * Mutex for protecting the filesystem structure. + * + * Specifically, this protects ::fs_parity, ::fs_file, and ::fs_last, + * meaning that it protectes only chunks. + * + * Files, links and dirs are not protected as they are not expected to + * change during multithread processing. + */ + pthread_mutex_t fs_mutex; + /** * Mapping of chunks in the parity. * Sorted by and by @@ -878,7 +889,7 @@ void disk_free(struct snapraid_disk* disk); /** * Get the size of the disk in blocks. */ -block_off_t disk_size(struct snapraid_disk* disk); +block_off_t fs_size(struct snapraid_disk* disk); /** * Check if a disk is totally empty and can be discarded from the content file. @@ -886,7 +897,7 @@ block_off_t disk_size(struct snapraid_disk* disk); * and without any DELETED block. * The blockmax is used to limit the search of DELETED block up to blockmax. */ -int disk_is_empty(struct snapraid_disk* disk, block_off_t blockmax); +int fs_is_empty(struct snapraid_disk* disk, block_off_t blockmax); /** * Check the file-system for errors. @@ -919,49 +930,23 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t pos); */ struct snapraid_block* fs_file2block_get(struct snapraid_file* file, block_off_t file_pos); -/** - * Like fs_file2par_get() but thread-safe. - */ -struct snapraid_file* fs_par2file_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos, block_off_t* file_pos); - /** * Get the file position from the parity position. * Return 0 if no file is using it. - * \note This function is NOT thread-safe as it uses the the disk cache. */ -static inline struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t parity_pos, block_off_t* file_pos) -{ - return fs_par2file_get_ts(disk, &disk->fs_last, parity_pos, file_pos); -} - -/** - * Like fs_file2par_get() but thread-safe. - */ -block_off_t fs_file2par_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, struct snapraid_file* file, block_off_t file_pos); +struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t parity_pos, block_off_t* file_pos); /** * Get the parity position from the file position. - * \note This function is NOT thread-safe as it uses the the disk cache. */ -static inline block_off_t fs_file2par_get(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos) -{ - return fs_file2par_get_ts(disk, &disk->fs_last, file, file_pos); -} - -/** - * Like fs_par2block_get() but thread-safe. - */ -struct snapraid_block* fs_par2block_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos); +block_off_t fs_file2par_get(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos); /** * Get the block from the parity position. * Return BLOCK_EMPTY==0 if the block is over the end of the disk or not used. * \note This function is NOT thread-safe as it uses the the disk cache. */ -static inline struct snapraid_block* fs_par2block_get(struct snapraid_disk* disk, block_off_t parity_pos) -{ - return fs_par2block_get_ts(disk, &disk->fs_last, parity_pos); -} +struct snapraid_block* fs_par2block_get(struct snapraid_disk* disk, block_off_t parity_pos); /** * Allocate a disk mapping. diff --git a/cmdline/io.c b/cmdline/io.c index c29b58c7..f8a7d422 100644 --- a/cmdline/io.c +++ b/cmdline/io.c @@ -53,7 +53,6 @@ void io_init(struct snapraid_io* io, struct snapraid_state* state, unsigned buff struct snapraid_worker* worker = &io->worker_map[i]; worker->io = io; - worker->fs_last = 0; if (i < handle_max) { worker->handle = &handle_map[i]; diff --git a/cmdline/io.h b/cmdline/io.h index 335bad9e..a15c1d53 100644 --- a/cmdline/io.h +++ b/cmdline/io.h @@ -105,15 +105,6 @@ struct snapraid_worker { * Which buffer base index should be used for destination. */ unsigned buffer_skew; - - /** - * Last chunk we accessed. - * - * It's used to optimize access of sequential blocks. - * We cannot use the one in ::disk because it's used - * by the main thread. - */ - struct snapraid_chunk* fs_last; }; /** diff --git a/cmdline/parity.c b/cmdline/parity.c index 400f0601..92d2db8c 100644 --- a/cmdline/parity.c +++ b/cmdline/parity.c @@ -37,7 +37,7 @@ block_off_t parity_allocated_size(struct snapraid_state* state) struct snapraid_disk* disk = i->data; /* start from the declared size */ - block_off_t block = disk_size(disk); + block_off_t block = fs_size(disk); /* decrease the block until an allocated one, but part of a file */ /* we don't stop at deleted blocks, because we want to have them cleared */ @@ -64,7 +64,7 @@ block_off_t parity_used_size(struct snapraid_state* state) struct snapraid_disk* disk = i->data; /* start from the declared size */ - block_off_t block = disk_size(disk); + block_off_t block = fs_size(disk); /* decrease the block until an used one */ while (block > parity_block && !block_has_file_and_valid_parity(fs_par2block_get(disk, block - 1))) diff --git a/cmdline/scrub.c b/cmdline/scrub.c index 0e06d5f2..135b773f 100644 --- a/cmdline/scrub.c +++ b/cmdline/scrub.c @@ -121,7 +121,7 @@ static void scrub_data_reader(struct snapraid_worker* worker, struct snapraid_ta } /* get the block */ - task->block = fs_par2block_get_ts(disk, &worker->fs_last, blockcur); + task->block = fs_par2block_get(disk, blockcur); /* if the block is not used */ if (!block_has_file(task->block)) { @@ -132,7 +132,7 @@ static void scrub_data_reader(struct snapraid_worker* worker, struct snapraid_ta } /* get the file of this block */ - task->file = fs_par2file_get_ts(disk, &worker->fs_last, blockcur, &task->file_pos); + task->file = fs_par2file_get(disk, blockcur, &task->file_pos); /* if the file is different than the current one, close it */ if (handle->file != 0 && handle->file != task->file) { diff --git a/cmdline/state.c b/cmdline/state.c index a140d122..465f9f89 100644 --- a/cmdline/state.c +++ b/cmdline/state.c @@ -1599,9 +1599,9 @@ static void fs_position_clear_deleted(struct snapraid_state* state, block_off_t /** * Check if a block position in a disk is deleted. */ -static int fs_is_block_deleted_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t pos) +static int fs_is_block_deleted(struct snapraid_disk* disk, block_off_t pos) { - struct snapraid_block* block = fs_par2block_get_ts(disk, fs_last, pos); + struct snapraid_block* block = fs_par2block_get(disk, pos); return block_state_get(block) == BLOCK_STATE_DELETED; } @@ -2735,14 +2735,6 @@ static void* state_write_thread(void* arg) for (i = state->disklist; i != 0; i = i->next) { tommy_node* j; struct snapraid_disk* disk = i->data; - struct snapraid_chunk* fs_last; - - /** - * This is the last accessed chunk in the tree operations. - * Here we are inside a thread, and we cannot use the - * ::disk member to store this information. - */ - fs_last = 0; /* if the disk is not mapped, skip it */ if (disk->mapping_idx < 0) @@ -2783,7 +2775,7 @@ static void* state_write_thread(void* arg) begin = 0; while (begin < file->blockmax) { unsigned v_state = block_state_get(fs_file2block_get(file, begin)); - block_off_t v_pos = fs_file2par_get_ts(disk, &fs_last, file, begin); + block_off_t v_pos = fs_file2par_get(disk, file, begin); uint32_t v_count; block_off_t end; @@ -2793,7 +2785,7 @@ static void* state_write_thread(void* arg) while (end < file->blockmax) { if (v_state != block_state_get(fs_file2block_get(file, end))) break; - if (v_pos + (end - begin) != fs_file2par_get_ts(disk, &fs_last, file, end)) + if (v_pos + (end - begin) != fs_file2par_get(disk,file, end)) break; ++end; } @@ -2898,12 +2890,12 @@ static void* state_write_thread(void* arg) int is_deleted; block_off_t end; - is_deleted = fs_is_block_deleted_ts(disk, &fs_last, begin); + is_deleted = fs_is_block_deleted(disk, begin); /* find the end of run of blocks */ end = begin + 1; while (end < blockmax - && is_deleted == fs_is_block_deleted_ts(disk, &fs_last, end) + && is_deleted == fs_is_block_deleted(disk, end) ) { ++end; } @@ -2916,7 +2908,7 @@ static void* state_write_thread(void* arg) /* write all the hash */ while (begin < end) { - struct snapraid_block* block = fs_par2block_get_ts(disk, &fs_last, begin); + struct snapraid_block* block = fs_par2block_get(disk, begin); swrite(block->hash, HASH_SIZE, f); @@ -3108,7 +3100,7 @@ static void state_write_content(struct snapraid_state* state, uint32_t* out_crc) } /* save the mapping only for not empty disks */ - if (!disk_is_empty(disk, blockmax)) { + if (!fs_is_empty(disk, blockmax)) { /* assign the mapping index used to identify disks */ disk->mapping_idx = mapping_idx; ++mapping_idx; diff --git a/cmdline/sync.c b/cmdline/sync.c index b4840ee2..0d13c9c8 100644 --- a/cmdline/sync.c +++ b/cmdline/sync.c @@ -481,7 +481,7 @@ static void sync_data_reader(struct snapraid_worker* worker, struct snapraid_tas } /* get the block */ - task->block = fs_par2block_get_ts(disk, &worker->fs_last, blockcur); + task->block = fs_par2block_get(disk, blockcur); /* if the block has no file, meanining that it's EMPTY or DELETED, */ /* it doesn't partecipate in the new parity computation */ @@ -493,7 +493,7 @@ static void sync_data_reader(struct snapraid_worker* worker, struct snapraid_tas } /* get the file of this block */ - task->file = fs_par2file_get_ts(disk, &worker->fs_last, blockcur, &task->file_pos); + task->file = fs_par2file_get(disk, blockcur, &task->file_pos); /* if the file is different than the current one, close it */ if (handle->file != 0 && handle->file != task->file) {