From 8ff39bb9bf3a1a67b5140ed844c8c2de0973174e Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Fri, 30 Oct 2015 19:06:24 +0100 Subject: [PATCH] Fix thread operations using thread-safe functions We have a common disk cache, that we cannot share during multithread operations. Intead of using a mutex to access the cache, we use a local thread cache. --- cmdline/elem.c | 42 +++++++++++++++++++++--------------------- cmdline/elem.h | 39 ++++++++++++++++++++++++++++++++++----- cmdline/state.c | 34 +++++++++++++++++++++------------- 3 files changed, 76 insertions(+), 39 deletions(-) diff --git a/cmdline/elem.c b/cmdline/elem.c index b1e036b2..20672f5b 100644 --- a/cmdline/elem.c +++ b/cmdline/elem.c @@ -1029,16 +1029,16 @@ 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(struct snapraid_disk* disk, block_off_t parity_pos) +static struct snapraid_chunk* fs_par2chunk_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos) { struct snapraid_chunk* chunk; /* check if the last accessed chunk matches */ - if (disk->fs_last - && parity_pos >= disk->fs_last->parity_pos - && parity_pos < disk->fs_last->parity_pos + disk->fs_last->count + if (*fs_last + && parity_pos >= (*fs_last)->parity_pos + && parity_pos < (*fs_last)->parity_pos + (*fs_last)->count ) { - chunk = disk->fs_last; + chunk = *fs_last; } else { struct chunk_parity_inside arg = { parity_pos }; chunk = tommy_tree_search_compare(&disk->fs_parity, chunk_parity_inside_compare, &arg); @@ -1048,7 +1048,7 @@ static struct snapraid_chunk* fs_par2chunk_get(struct snapraid_disk* disk, block return 0; /* store the last accessed chunk */ - disk->fs_last = chunk; + *fs_last = chunk; return chunk; } @@ -1084,17 +1084,17 @@ 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(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos) +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) { struct snapraid_chunk* chunk; /* check if the last accessed chunk matches */ - if (disk->fs_last - && file == disk->fs_last->file - && file_pos >= disk->fs_last->file_pos - && file_pos < disk->fs_last->file_pos + disk->fs_last->count + if (*fs_last + && file == (*fs_last)->file + && file_pos >= (*fs_last)->file_pos + && file_pos < (*fs_last)->file_pos + (*fs_last)->count ) { - chunk = disk->fs_last; + 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); @@ -1104,16 +1104,16 @@ static struct snapraid_chunk* fs_file2chunk_get(struct snapraid_disk* disk, stru return 0; /* store the last accessed chunk */ - disk->fs_last = chunk; + *fs_last = chunk; return chunk; } -struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t parity_pos, block_off_t* file_pos) +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_chunk* chunk; - chunk = fs_par2chunk_get(disk, parity_pos); + chunk = fs_par2chunk_get_ts(disk, fs_last, parity_pos); if (!chunk) return 0; @@ -1123,11 +1123,11 @@ struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t pa return chunk->file; } -block_off_t fs_file2par_get(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos) +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_chunk* chunk; - chunk = fs_file2chunk_get(disk, file, file_pos); + chunk = fs_file2chunk_get_ts(disk, fs_last, file, file_pos); if (!chunk) { /* LCOV_EXCL_START */ log_fatal("Internal inconsistency when resolving file '%s' at position '%u' in disk '%s'\n", file->sub, file_pos, disk->name); @@ -1146,7 +1146,7 @@ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snap if (file_pos > 0) { /* search an existing chunk for the previous file_pos */ - chunk = fs_file2chunk_get(disk, file, file_pos - 1); + chunk = fs_file2chunk_get_ts(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 */ @@ -1190,7 +1190,7 @@ 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(disk, parity_pos); + chunk = fs_par2chunk_get_ts(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); @@ -1251,12 +1251,12 @@ void fs_deallocate(struct snapraid_disk* disk, block_off_t parity_pos) disk->fs_last = second_chunk; } -struct snapraid_block* fs_par2block_get(struct snapraid_disk* disk, block_off_t parity_pos) +struct snapraid_block* fs_par2block_get_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t parity_pos) { struct snapraid_file* file; block_off_t file_pos; - file = fs_par2file_get(disk, parity_pos, &file_pos); + file = fs_par2file_get_ts(disk, fs_last, parity_pos, &file_pos); if (!file) return BLOCK_EMPTY; diff --git a/cmdline/elem.h b/cmdline/elem.h index f297f737..ae81445a 100644 --- a/cmdline/elem.h +++ b/cmdline/elem.h @@ -321,9 +321,7 @@ struct snapraid_disk { char dir[PATH_MAX]; /**< Mount point of the disk. It always terminates with /. */ char smartctl[PATH_MAX]; /**< Custom command for smartctl. Empty means auto. */ uint64_t device; /**< Device identifier. */ - uint64_t tick; /**< Usage time of the disk. */ - block_off_t total_blocks; /**< Number of total blocks. */ block_off_t free_blocks; /**< Number of free blocks at the last sync. */ @@ -899,6 +897,8 @@ int fs_check(struct snapraid_disk* disk); * * After this call you can use the par2file/par2block operations * to query the relation. + * + * \note This function is NOT thread-safe as it uses the the disk cache. + */ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snapraid_file* file, block_off_t file_pos); @@ -907,6 +907,8 @@ void fs_allocate(struct snapraid_disk* disk, block_off_t parity_pos, struct snap * * After this call the par2file/par2block operations * won't find anymore the parity association. + * + * \note This function is NOT thread-safe as it uses the the disk cache. */ void fs_deallocate(struct snapraid_disk* disk, block_off_t pos); @@ -920,22 +922,49 @@ static inline struct snapraid_block* fs_file2block_get(struct snapraid_file* fil return &file->blockvec[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. */ -struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, block_off_t parity_pos, block_off_t* file_pos); +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); /** * Get the parity position from the file position. + * \note This function is NOT thread-safe as it uses the the disk cache. */ -block_off_t fs_file2par_get(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos); +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); /** * 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. */ -struct snapraid_block* fs_par2block_get(struct snapraid_disk* disk, block_off_t parity_pos); +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); +} /** * Allocate a disk mapping. diff --git a/cmdline/state.c b/cmdline/state.c index 59cf476e..8870266f 100644 --- a/cmdline/state.c +++ b/cmdline/state.c @@ -1464,7 +1464,7 @@ static void state_content_check(struct snapraid_state* state, const char* path) * * Note that position with only DELETED blocks are discarged. */ -static int position_is_required(struct snapraid_state* state, block_off_t pos) +static int fs_position_is_required(struct snapraid_state* state, block_off_t pos) { tommy_node* i; @@ -1493,7 +1493,7 @@ static int position_is_required(struct snapraid_state* state, block_off_t pos) * Note also that not requiring an info block, doesn't mean that if present it * can be discarded. */ -static int info_is_required(struct snapraid_state* state, block_off_t pos) +static int fs_info_is_required(struct snapraid_state* state, block_off_t pos) { tommy_node* i; @@ -1510,7 +1510,7 @@ static int info_is_required(struct snapraid_state* state, block_off_t pos) return 0; } -static void position_clear_deleted(struct snapraid_state* state, block_off_t pos) +static void fs_position_clear_deleted(struct snapraid_state* state, block_off_t pos) { tommy_node* i; @@ -1530,9 +1530,9 @@ static void position_clear_deleted(struct snapraid_state* state, block_off_t pos /** * Check if a block position in a disk is deleted. */ -static int is_block_deleted(struct snapraid_disk* disk, block_off_t pos) +static int fs_is_block_deleted_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, block_off_t pos) { - struct snapraid_block* block = fs_par2block_get(disk, pos); + struct snapraid_block* block = fs_par2block_get_ts(disk, fs_last, pos); return block_state_get(block) == BLOCK_STATE_DELETED; } @@ -1948,7 +1948,7 @@ static void state_read_content(struct snapraid_state* state, const char* path, S info_set(&state->infoarr, v_pos, info); /* ensure that an info is present only for used positions */ - if (info_is_required(state, v_pos)) { + if (fs_info_is_required(state, v_pos)) { if (!info) { /* LCOV_EXCL_START */ decoding_error(path, f); @@ -2653,6 +2653,14 @@ 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) @@ -2693,7 +2701,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(disk, file, begin); + block_off_t v_pos = fs_file2par_get_ts(disk, &fs_last, file, begin); uint32_t v_count; block_off_t end; @@ -2703,7 +2711,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(disk, file, end)) + if (v_pos + (end - begin) != fs_file2par_get_ts(disk, &fs_last, file, end)) break; ++end; } @@ -2808,12 +2816,12 @@ static void* state_write_thread(void* arg) int is_deleted; block_off_t end; - is_deleted = is_block_deleted(disk, begin); + is_deleted = fs_is_block_deleted_ts(disk, &fs_last, begin); /* find the end of run of blocks */ end = begin + 1; while (end < blockmax - && is_deleted == is_block_deleted(disk, end) + && is_deleted == fs_is_block_deleted_ts(disk, &fs_last, end) ) { ++end; } @@ -2826,7 +2834,7 @@ static void* state_write_thread(void* arg) /* write all the hash */ while (begin < end) { - struct snapraid_block* block = fs_par2block_get(disk, begin); + struct snapraid_block* block = fs_par2block_get_ts(disk, &fs_last, begin); swrite(block->hash, HASH_SIZE, f); @@ -2997,7 +3005,7 @@ static void state_write_content(struct snapraid_state* state, uint32_t* out_crc) info_has_rehash = 0; /* if there is a rehash info */ for (idx = 0; idx < blockmax; ++idx) { /* if the position is used */ - if (position_is_required(state, idx)) { + if (fs_position_is_required(state, idx)) { snapraid_info info = info_get(&state->infoarr, idx); /* only if there is some info to store */ @@ -3015,7 +3023,7 @@ static void state_write_content(struct snapraid_state* state, uint32_t* out_crc) info_set(&state->infoarr, idx, 0); /* and clear any deleted blocks */ - position_clear_deleted(state, idx); + fs_position_clear_deleted(state, idx); } }