From 9be2cbaac0cd19ce5518a4f0884c44ed3ba14a60 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Sat, 29 Aug 2026 21:25:49 +0200 Subject: [PATCH] Make .unrecoverable a persistent one-way recovery quarantine --- cmdline/check.c | 164 ++++++++++++++++++++-------------------- cmdline/handle.c | 191 ++++++++++++++++++++++++++++++++++------------- cmdline/handle.h | 18 +++-- 3 files changed, 233 insertions(+), 140 deletions(-) diff --git a/cmdline/check.c b/cmdline/check.c index 83ee654e..65071b09 100644 --- a/cmdline/check.c +++ b/cmdline/check.c @@ -188,16 +188,20 @@ * such data, but the containing file remains marked damaged and is kept under * the .unrecoverable name instead of being reported as successfully recovered. * - * Missing files are created as .unrecoverable from the beginning, and an - * existing .unrecoverable file is reused by later fix runs. This allows a - * recovery to proceed over multiple runs without exposing a partially rebuilt - * file under its final name. + * Missing files are created under their normal names. They are quarantined only + * if recovery produces data that cannot be certified as CURRENT. * - * Promotion to the final file name is a file-level commit operation. It happens - * only after the complete file has been processed successfully, its metadata - * has been restored, and no block remains damaged. A partial -S/-B fix operates - * only at block level and therefore never performs this file-level promotion - * or other finalization. + * A later fix starts a new recovery attempt under the normal filename even if a + * previous .unrecoverable file exists. If the new attempt succeeds, the normal + * file is kept and the previous quarantine is left untouched. If the new attempt + * also becomes unrecoverable, it replaces the previous quarantine. + * + * A quarantined file may be reopened later during the same fix run. In that case + * FILE_IS_DAMAGED identifies it as belonging to the current failed recovery and + * handle_create() continues operating on its .unrecoverable file. + * + * A partial -S/-B fix operates only at block level and therefore never performs + * file-level finalization. */ /****************************************************************************/ @@ -1293,8 +1297,8 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o /* * A partial fix operates only at block level. Reaching the last block of a * file doesn't imply that the whole file was processed, so don't perform any - * file-level finalization, including marking it finished, promoting an - * .unrecoverable file, reporting it recovered, or restoring its timestamp. + * file-level finalization, including marking it finished, reporting its final + * status, or restoring its timestamp. * This is intentional also when an explicit block range happens to cover all * the parity blocks, to keep -S/-B behavior independent of the parity size. */ @@ -1309,35 +1313,6 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o /* if the file is damaged, meaning that a fix failed */ if (file_flag_has(file, FILE_IS_DAMAGED)) { - char path[PATH_MAX]; - char path_to[PATH_MAX]; - - pathprint(path, sizeof(path), "%s%s", disk->dir, file->sub); - pathprint(path_to, sizeof(path_to), "%s%s.unrecoverable", disk->dir, file->sub); - - /* ensure to close the file before renaming */ - ret = handle_close(&handle[j]); - if (ret != 0) { - /* LCOV_EXCL_START */ - log_tag("%s:%" PRIu64 ":%s:%s: Close error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno)); - log_fatal_errno(errno, disk->name); - return -1; - /* LCOV_EXCL_STOP */ - } - - /* a previous recovery is already quarantined */ - if (!was_unrecoverable) { - ret = rename(path, path_to); - if (ret != 0) { - /* LCOV_EXCL_START */ - log_fatal(errno, "Error renaming '%s%s'. %s.\n", disk->dir, file->sub, strerror(errno)); - log_tag("%s:%" PRIu64 ":%s:%s: Rename error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno)); - log_fatal_errno(errno, disk->name); - return -1; - /* LCOV_EXCL_STOP */ - } - } - log_tag("status:unrecoverable:%s:%s\n", disk->name, esc_tag(file->sub)); msg_info("unrecoverable %s\n", fmt_term(disk, file->sub)); @@ -1401,7 +1376,7 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o || collide_file->mtime_sec != file->mtime_sec /* if the mtime is different, the collision is identified */ || collide_file->mtime_nsec != file->mtime_nsec /* same for mtime_nsec */ ) { - /* set the original modification time before promoting/renaming */ + /* set the original modification time */ ret = handle_utime(&handle[j]); if (ret == -1) { /* LCOV_EXCL_START */ @@ -1418,36 +1393,16 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o } /* - * This rename is the file-level recovery commit point. Keep the file - * quarantined until every block and metadata was processed without error; - * promoting only after setting the timestamp ensures the final file is - * never exposed with incomplete data or provisional timestamp. + * If the current recovery attempt was quarantined, keep it under the + * .unrecoverable name and report it as unrecoverable. + * + * A later fix starts a new recovery attempt under the normal filename; + * the quarantined file is never promoted back to the normal name. */ if (was_unrecoverable) { - char path[PATH_MAX]; - char path_from[PATH_MAX]; - - pathprint(path, sizeof(path), "%s%s", disk->dir, file->sub); - pathprint(path_from, sizeof(path_from), "%s%s.unrecoverable", disk->dir, file->sub); - - ret = handle_close(&handle[j]); - if (ret != 0) { - /* LCOV_EXCL_START */ - log_tag("%s:%" PRIu64 ":%s:%s: Close error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno)); - log_fatal_errno(errno, disk->name); - return -1; - /* LCOV_EXCL_STOP */ - } - - ret = rename(path_from, path); - if (ret != 0) { - /* LCOV_EXCL_START */ - log_fatal(errno, "Error renaming '%s'. %s.\n", path_from, strerror(errno)); - log_tag("%s:%" PRIu64 ":%s:%s: Rename error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno)); - log_fatal_errno(errno, disk->name); - return -1; - /* LCOV_EXCL_STOP */ - } + log_tag("status:unrecoverable:%s:%s\n", disk->name, esc_tag(file->sub)); + msg_info("unrecoverable %s\n", fmt_term(disk, file->sub)); + goto close_and_continue; } log_tag("status:recovered:%s:%s\n", disk->name, esc_tag(file->sub)); @@ -2083,8 +2038,28 @@ static int state_check_process(struct snapraid_state* state, int fix, struct sna /* keep track of damaged files */ for (j = 0; j < failed_count; ++j) { - if (failed[j].is_bad) - file_flag_set(failed[j].file, FILE_IS_DAMAGED); + if (!failed[j].is_bad) + continue; + + file_flag_set(failed[j].file, FILE_IS_DAMAGED); + + /* + * Persist the unrecoverable state immediately. + * Previous blocks of this file may already have been + * successfully recovered. + */ + if (fix && !file_flag_has(failed[j].file, FILE_IS_EXCLUDED) && !(state->opt.syncedonly && file_flag_has(failed[j].file, FILE_IS_UNSYNCED))) { + ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode); + if (ret == -1) { + /* LCOV_EXCL_START */ + log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno)); + log_fatal_errno(errno, failed[j].disk->name); + log_fatal(errno, "Stopping at block %" PRIu64 "\n", i); + ++unrecoverable_error; + goto bail; + /* LCOV_EXCL_STOP */ + } + } } } else { /* @@ -2149,32 +2124,56 @@ static int state_check_process(struct snapraid_state* state, int fix, struct sna /* * Write the best recovered candidate even when it is out-of-date. Such data * may still be useful to the user or to a later multistep recovery. - * - * An out-of-date write is not considered a successful fix: the file is marked - * damaged below and file_post() keeps or moves it to .unrecoverable instead - * of promoting it as recovered. */ + /* + * An out-of-date candidate is useful best-effort data, + * but it cannot be certified as the required CURRENT + * contents. Persist this fact before modifying the file + * so that a SIGKILL cannot leave unproven recovered data + * under the normal filename. + */ + if (failed[j].is_outofdate) { + file_flag_set(failed[j].file, FILE_IS_DAMAGED); + + ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode); + if (ret == -1) { + /* LCOV_EXCL_START */ + log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno)); + log_fatal_errno(errno, failed[j].disk->name); + log_fatal(errno, "Stopping at block %" PRIu64 "\n", i); + ++unrecoverable_error; + goto bail; + /* LCOV_EXCL_STOP */ + } + } + ret = handle_write(failed[j].handle, failed[j].file_pos, buffer[failed[j].index], state->block_size); if (ret == -1) { /* LCOV_EXCL_START */ - log_tag("%s:%" PRIu64 ":%s:%s: Write error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno)); - log_fatal_errno(errno, failed[j].disk->name); - log_fatal(errno, "Stopping at block %" PRIu64 "\n", i); + int saved_errno = errno; /* mark the file as damaged */ file_flag_set(failed[j].file, FILE_IS_DAMAGED); + ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode); + if (ret == -1) { + log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno)); + log_fatal_errno(errno, failed[j].disk->name); + } + + errno = saved_errno; + log_tag("%s:%" PRIu64 ":%s:%s: Write error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno)); + log_fatal_errno(errno, failed[j].disk->name); + log_fatal(errno, "Stopping at block %" PRIu64 "\n", i); + ++unrecoverable_error; goto bail; /* LCOV_EXCL_STOP */ } /* if we are not sure that the recovered content is uptodate */ - if (failed[j].is_outofdate) { - /* mark the file as damaged */ - file_flag_set(failed[j].file, FILE_IS_DAMAGED); + if (failed[j].is_outofdate) continue; - } /* * Mark the file as containing some fixes @@ -2993,4 +2992,3 @@ int state_check(struct snapraid_state* state, int fix, block_off_t blockstart, b return 0; } - diff --git a/cmdline/handle.c b/cmdline/handle.c index ff5fdfda..fedd5e64 100644 --- a/cmdline/handle.c +++ b/cmdline/handle.c @@ -14,6 +14,8 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in { int ret; int flags; + char path[PATH_MAX]; + char path_unrecoverable[PATH_MAX]; /* if it's the same file, and already opened, nothing to do */ if (handle->file == file && handle->f != -1) { @@ -21,9 +23,11 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in } advise_init(&handle->advise, mode); - pathprint(handle->path, sizeof(handle->path), "%s%s", handle->disk->dir, file->sub); - ret = mkancestor(handle->path); + pathprint(path, sizeof(path), "%s%s", handle->disk->dir, file->sub); + pathprint(path_unrecoverable, sizeof(path_unrecoverable), "%s.unrecoverable", path); + + ret = mkancestor(path); if (ret != 0) { /* LCOV_EXCL_START */ return -1; @@ -42,63 +46,61 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in */ flags = O_BINARY | O_NOFOLLOW | advise_flags(&handle->advise); - /* open for read write */ - handle->f = open(handle->path, flags | O_RDWR); - - /* if failed for missing write permission */ - if (handle->f == -1 && (errno == EACCES || errno == EROFS)) { - int saved_errno = errno; - /* open for read-only */ - handle->f = open(handle->path, flags | O_RDONLY); - if (handle->f != -1) { - handle->readonly_errno = saved_errno; - } - } - - /* if failed for missing file */ - if (handle->f == -1 && errno == ENOENT) { - char path_unrecoverable[PATH_MAX]; - - /* check if exists a .unrecoverable copy */ - pathprint(path_unrecoverable, sizeof(path_unrecoverable), "%s.unrecoverable", handle->path); - - /* - * Open a previous recovery directly as .unrecoverable, keeping it - * quarantined until the whole file is processed successfully. Promoting - * it now could expose an incomplete or unverified file if this fix aborts - * or fails again. Reuse it for a multistep fix, preserving readable CHG - * blocks because their current hash is not available. - */ + /* + * If this file was already quarantined during the current fix run, + * continue operating on its .unrecoverable copy. + * + * FILE_IS_DAMAGED is runtime state and is not carried to a later fix, + * allowing a later recovery attempt to start again from the normal name. + */ + if (file_flag_has(file, FILE_IS_DAMAGED)) { handle->f = open(path_unrecoverable, flags | O_RDWR); + + /* if failed for missing write permission */ + if (handle->f == -1 && (errno == EACCES || errno == EROFS)) { + int saved_errno = errno; + /* open for read-only */ + handle->f = open(path_unrecoverable, flags | O_RDONLY); + if (handle->f != -1) { + handle->readonly_errno = saved_errno; + } + } + if (handle->f != -1) { handle->is_unrecoverable = 1; - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); - } else if (errno == ENOENT) { - /* - * Create missing file as .unrecoverable so that an interruption or - * crash (e.g. SIGKILL) during recovery never leaves a partially recovered - * file visible under its final name. - */ - handle->f = open(path_unrecoverable, flags | O_RDWR | O_CREAT | O_EXCL, 0600); + } + + pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); + } else { + /* + * This is a new recovery attempt. Always use the normal filename. + * A .unrecoverable file left by an earlier fix does not prevent a + * new attempt from reconstructing the normal file. + */ + handle->f = open(path, flags | O_RDWR); + + /* if failed for missing write permission */ + if (handle->f == -1 && (errno == EACCES || errno == EROFS)) { + int saved_errno = errno; + /* open for read-only */ + handle->f = open(path, flags | O_RDONLY); if (handle->f != -1) { - handle->is_unrecoverable = 1; - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); + handle->readonly_errno = saved_errno; + } + } + + /* if failed for missing file */ + if (handle->f == -1 && errno == ENOENT) { + handle->f = open(path, flags | O_RDWR | O_CREAT | O_EXCL, 0600); + if (handle->f != -1) { + handle->created = 1; } else if (errno == EEXIST) { /* if created concurrently, reopen safely */ - handle->f = open(path_unrecoverable, flags | O_RDWR); - if (handle->f != -1) { - handle->is_unrecoverable = 1; - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); - } else { - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); - } - } else { - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); + handle->f = open(path, flags | O_RDWR); } - } else { - /* report the path that failed to open */ - pathcpy(handle->path, sizeof(handle->path), path_unrecoverable); } + + pathcpy(handle->path, sizeof(handle->path), path); } if (handle->f == -1) { @@ -137,6 +139,92 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in return 0; } +/** + * Move an opened file to its persistent .unrecoverable name. + */ +int handle_quarantine(struct snapraid_handle* handle, struct snapraid_file* file, int mode) +{ + char path[PATH_MAX]; + char path_to[PATH_MAX]; + int ret; + + (void)mode; + + /* already persistently quarantined */ + if (handle->is_unrecoverable) + return 0; + + /* mark this recovery attempt as quarantined */ + file_flag_set(file, FILE_IS_DAMAGED); + + pathprint(path, sizeof(path), "%s%s", handle->disk->dir, file->sub); + pathprint(path_to, sizeof(path_to), "%s%s.unrecoverable", handle->disk->dir, file->sub); + +#ifdef _WIN32 + /* + * Windows may not allow renaming an opened file. Close it before + * replacing the previous quarantine and renaming the current file. + */ + ret = handle_close(handle); + if (ret != 0) + return -1; + + /* + * Windows rename() doesn't replace an existing destination. + * Remove a previous failed recovery before installing the current one. + */ + ret = remove(path_to); + if (ret != 0 && errno != ENOENT) { + /* LCOV_EXCL_START */ + log_error(errno, "Error removing file '%s'. %s.\n", path_to, strerror(errno)); + return -1; + /* LCOV_EXCL_STOP */ + } + + ret = rename(path, path_to); + if (ret != 0) { + /* LCOV_EXCL_START */ + log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno)); + return -1; + /* LCOV_EXCL_STOP */ + } + + /* + * FILE_IS_DAMAGED is set, so handle_create() reopens the + * .unrecoverable file belonging to the current recovery attempt. + */ + ret = handle_create(handle, file, mode); + if (ret != 0) + return -1; + + if (!handle->is_unrecoverable) { + /* LCOV_EXCL_START */ + errno = ENXIO; + log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno)); + return -1; + /* LCOV_EXCL_STOP */ + } +#else + /* + * POSIX rename() atomically replaces an existing destination file. + * The opened descriptor continues to reference the renamed inode, so + * no close/reopen is required. + */ + ret = rename(path, path_to); + if (ret != 0) { + /* LCOV_EXCL_START */ + log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno)); + return -1; + /* LCOV_EXCL_STOP */ + } + + handle->is_unrecoverable = 1; + pathcpy(handle->path, sizeof(handle->path), path_to); +#endif + + return 0; +} + int handle_truncate(struct snapraid_handle* handle, struct snapraid_file* file) { int ret; @@ -490,4 +578,3 @@ struct snapraid_handle* handle_mapping(struct snapraid_state* state, unsigned* h *handlemax = size; return handle; } - diff --git a/cmdline/handle.h b/cmdline/handle.h index cc35ee3b..4a6c533d 100644 --- a/cmdline/handle.h +++ b/cmdline/handle.h @@ -36,16 +36,25 @@ struct snapraid_handle { }; /** - * Create a file. - * The file is created if missing, and opened with write access. - * If the file is created, the handle->created is set. - * If a previous .unrecoverable file is opened, the handle->is_unrecoverable is set. + * Create or reopen a recovery file. + * A new recovery attempt always uses the normal filename, even if a previous .unrecoverable file exists. + * If the file is already marked FILE_IS_DAMAGED in the current run, its .unrecoverable file is reopened instead. + * If the normal file is created, the handle->created is set. + * If the .unrecoverable file is opened, the handle->is_unrecoverable is set. * The initial size of the file is stored in the file->st struct. * If the file cannot be opened for write access, it's opened with read-only access. * The read-only access works only if the file has already the correct size and doesn't need to be modified. */ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, int mode); +/** + * Move an opened file to its persistent .unrecoverable name. + * A previous .unrecoverable from an older recovery attempt is replaced. + * If already quarantined in the current run, do nothing. + * On error, log the failure and return -1 with errno set. + */ +int handle_quarantine(struct snapraid_handle* handle, struct snapraid_file* file, int mode); + /** * Truncate a file if required. */ @@ -86,4 +95,3 @@ int handle_utime(struct snapraid_handle* handle); struct snapraid_handle* handle_mapping(struct snapraid_state* state, unsigned* diskmax); #endif -