From 56bef0d90f0f8a50dbe62f77e941f6428e8af2d7 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Fri, 15 Jan 2016 19:50:21 +0100 Subject: [PATCH 1/3] Fix an invalid assumption about allocated parity In the copy detection it's possible to need to copy from a file wihout parity. This could result in the parity position not found. --- Makefile.am | 11 ++++++++++ cmdline/elem.c | 8 ++------ cmdline/elem.h | 1 + cmdline/scan.c | 56 +++++++++++++++++++++++++++++--------------------- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/Makefile.am b/Makefile.am index 389b54cc..b295c75b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -303,6 +303,17 @@ endif $(TESTENV) ./snapraid$(EXEEXT) $(CHECKFLAGS) -c $(CONF) sync -l ">&1" rm -r bench/disk1/TEST* $(TESTENV) ./snapraid$(EXEEXT) $(CHECKFLAGS) -c $(CONF) -E sync +# Now enforce copy detection of a file without parity +# Create a file in a high number disk + head -c 8192 /dev/urandom > bench/disk6/STEP1 + $(TESTENV) ./snapraid$(EXEEXT) $(CHECKFLAGS) -c $(CONF) sync +# Create two new copies. The one in disk1 will copy +# the hash from the one in disk6, and the one in disk2 +# will copy the hash from the one in disk1, that +# doesn't have parity + cp -a bench/disk6/STEP1 bench/disk1/STEP1 + cp -a bench/disk6/STEP1 bench/disk2/STEP1 + $(TESTENV) ./snapraid$(EXEEXT) $(CHECKFLAGS) -c $(CONF) sync # Now create, duplicate, move and partial sync head -c 8192 /dev/zero > bench/disk1/INVA1 head -c 8192 /dev/zero > bench/disk1/INVA2 diff --git a/cmdline/elem.c b/cmdline/elem.c index 2f8caf61..b3185c97 100644 --- a/cmdline/elem.c +++ b/cmdline/elem.c @@ -1160,12 +1160,8 @@ block_off_t fs_file2par_get_ts(struct snapraid_disk* disk, struct snapraid_chunk struct snapraid_chunk* chunk; 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/%u' in disk '%s'\n", file->sub, file_pos, file->blockmax, disk->name); - os_abort(); - /* LCOV_EXCL_STOP */ - } + if (!chunk) + return POS_INVALID; return chunk->parity_pos + (file_pos - chunk->file_pos); } diff --git a/cmdline/elem.h b/cmdline/elem.h index 9264ba9b..972342e4 100644 --- a/cmdline/elem.h +++ b/cmdline/elem.h @@ -941,6 +941,7 @@ block_off_t fs_file2par_get_ts(struct snapraid_disk* disk, struct snapraid_chunk /** * Get the parity position from the file position. + * Return POS_INVALID if no parity is allocated. * \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) diff --git a/cmdline/scan.c b/cmdline/scan.c index 6cb403bd..5a2ef66e 100644 --- a/cmdline/scan.c +++ b/cmdline/scan.c @@ -378,19 +378,24 @@ static int file_is_full_invalid_parity_and_stable(struct snapraid_state* state, return 0; /* - * Get the parity position, but only after checking - * that the file has invalid parity. - * This is not really required, as we are sure - * that 'kept" files are mapped into parity. + * Get the parity position. + * + * Note that here we expect to always have mapped + * parity, because kept files always have it. + * + * Anyway, checking for POS_INVALID doesn't hurt. */ parity_pos = fs_file2par_get(disk, file, i); - /* get block specific info */ - info = info_get(&state->infoarr, parity_pos); + /* if it's not mapped, it cannot have rehash */ + if (parity_pos != POS_INVALID) { + /* get block specific info */ + info = info_get(&state->infoarr, parity_pos); - /* if rehash fails */ - if (info_get_rehash(info)) - return 0; + /* if rehash fails */ + if (info_get_rehash(info)) + return 0; + } } return 1; @@ -419,25 +424,30 @@ static int file_is_full_hashed_and_stable(struct snapraid_state* state, struct s return 0; /* - * Get the parity position, but only AFTER checking - * that the file has an updated hash. - * This is required because not hashed files may be - * not yet mapped into parity. + * Get the parity position. * - * This happens when a new file is in the delayed insertion - * list, and scan finds a copy of it, and it tries to copy - * the hash info. - * As just allocated files have only CHG blocks, checking that - * the hash is updated is enough to exclude such cases. + * Note that it's possible to have files + * not mapped into the parity, even if they + * have a valid hash. + * + * This happens for example, for 'copied' files + * that have REP blocks, but not yet mapped. + * + * If there are multiple copies, it's also possible + * that such files are used as 'source' to copy + * hashes, and then to get them inside this function. */ parity_pos = fs_file2par_get(disk, file, i); - /* get block specific info */ - info = info_get(&state->infoarr, parity_pos); + /* if it's not mapped, it cannot have rehash */ + if (parity_pos != POS_INVALID) { + /* get block specific info */ + info = info_get(&state->infoarr, parity_pos); - /* exclude blocks needing a rehash */ - if (info_get_rehash(info)) - return 0; + /* exclude blocks needing a rehash */ + if (info_get_rehash(info)) + return 0; + } } return 1; From 3194ea653fae586f6d69c3aad55b5a7b5c7fcb25 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Fri, 15 Jan 2016 19:57:02 +0100 Subject: [PATCH 2/3] Update HISTORY --- HISTORY | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/HISTORY b/HISTORY index 95d633bb..c8981a3c 100644 --- a/HISTORY +++ b/HISTORY @@ -1,6 +1,18 @@ SnapRAID HISTORY ================ +9.3 2016/01 +=========== + * Fixes an invalid assumption in the copy detection mechanism that + could result in an internal inconsistency, and with the impossibility + to run the 'sync' and 'diff' commands. + This was triggered by a very specific pattern of identical files. + At least three of them, with one already in the parity, and at a + higher disk number than the others that should be instead new ones. + This had no bad effect, if not preventing the 'sync' command to run. + A workaround was to just run 'sync' one time with the -N, --force-nocopy + option to disable the copy detection. + 9.2 2016/01 =========== * Fixes support for symlinks pointing to an empty target. Before they @@ -277,7 +289,7 @@ SnapRAID HISTORY * For files with changes only in some blocks, updates the parity only for blocks that really are changed. This improves the performance in sync for modified files. - * Added a new "list" command to see the stored list of files. + * Added a new "list" command to see the stored list of files. * Removed the detailed list of errors from the screen output. To get it you must explicitly use the -l, --log option. It's now too detailed for the screen, because it contains a lot @@ -454,7 +466,7 @@ SnapRAID HISTORY * Fixed very long paths in Windows. We now always use the special '\\?' prefix to remove the 260 chars limitation. * If a file is excluded, it prints explicitly which attribute caused the - exclusion. + exclusion. * Automatically excludes also the temporary copy of content file, the one with the ".tmp" extension. * Avoid Windows to go in automatic sleep mode when running. From eaa89473b47b6543939ec7e9ec11a29c14699973 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Fri, 15 Jan 2016 20:05:38 +0100 Subject: [PATCH 3/3] Keep different the functions that may not find the parity --- cmdline/elem.c | 2 +- cmdline/elem.h | 29 ++++++++++++++++++++++++++++- cmdline/scan.c | 4 ++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/cmdline/elem.c b/cmdline/elem.c index b3185c97..5dc6af18 100644 --- a/cmdline/elem.c +++ b/cmdline/elem.c @@ -1155,7 +1155,7 @@ struct snapraid_file* fs_par2file_get_ts(struct snapraid_disk* disk, struct snap return chunk->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_maybe_ts(struct snapraid_disk* disk, struct snapraid_chunk** fs_last, struct snapraid_file* file, block_off_t file_pos) { struct snapraid_chunk* chunk; diff --git a/cmdline/elem.h b/cmdline/elem.h index 972342e4..c50c9052 100644 --- a/cmdline/elem.h +++ b/cmdline/elem.h @@ -937,13 +937,40 @@ static inline struct snapraid_file* fs_par2file_get(struct snapraid_disk* disk, /** * 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); +block_off_t fs_file2par_maybe_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. * Return POS_INVALID if no parity is allocated. * \note This function is NOT thread-safe as it uses the the disk cache. */ +static inline block_off_t fs_file2par_maybe(struct snapraid_disk* disk, struct snapraid_file* file, block_off_t file_pos) +{ + return fs_file2par_maybe_ts(disk, &disk->fs_last, file, file_pos); +} + +/** + * Like fs_file2par_get() but thread-safe. + */ +static inline 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 ret; + + ret = fs_file2par_maybe_ts(disk, fs_last, file, file_pos); + if (ret == POS_INVALID) { + /* 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); + os_abort(); + /* LCOV_EXCL_STOP */ + } + + return ret; +} + +/** + * 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); diff --git a/cmdline/scan.c b/cmdline/scan.c index 5a2ef66e..c3eeff1f 100644 --- a/cmdline/scan.c +++ b/cmdline/scan.c @@ -385,7 +385,7 @@ static int file_is_full_invalid_parity_and_stable(struct snapraid_state* state, * * Anyway, checking for POS_INVALID doesn't hurt. */ - parity_pos = fs_file2par_get(disk, file, i); + parity_pos = fs_file2par_maybe(disk, file, i); /* if it's not mapped, it cannot have rehash */ if (parity_pos != POS_INVALID) { @@ -437,7 +437,7 @@ static int file_is_full_hashed_and_stable(struct snapraid_state* state, struct s * that such files are used as 'source' to copy * hashes, and then to get them inside this function. */ - parity_pos = fs_file2par_get(disk, file, i); + parity_pos = fs_file2par_maybe(disk, file, i); /* if it's not mapped, it cannot have rehash */ if (parity_pos != POS_INVALID) {