From 56b60fab4bc942483434c524a660a98212bb1b58 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 6 Feb 2017 10:36:46 -0800 Subject: [PATCH] Improve history's save_internal_via_appending Allow retrying, fix an issue where we trip over our own changes by thinking the file has changed when we are responsible for changing it, and improve some commenting --- src/history.cpp | 65 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 14 deletions(-) diff --git a/src/history.cpp b/src/history.cpp index 87ee1e663..36ff91e21 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -51,7 +51,15 @@ #define HISTORY_SAVE_MAX (1024 * 256) // Default buffer size for flushing to the history file. -#define HISTORY_OUTPUT_BUFFER_SIZE (16 * 1024) +#define HISTORY_OUTPUT_BUFFER_SIZE (64 * 1024) + +// The file access mode we use for creating history files +static constexpr int history_file_mode = 0644; + +// How many times we retry to save +// Saving may fail if the file is modified in between our opening +// the file and taking the lock +static constexpr int max_save_tries = 1024; namespace { @@ -1314,6 +1322,8 @@ bool history_t::save_internal_via_rewrite() { return ok; } +// Function called to save our unwritten history file by appending to the existing history file +// Returns true on success, false on failure. bool history_t::save_internal_via_appending() { // This must be called while locked. ASSERT_IS_LOCKED(lock); @@ -1331,12 +1341,17 @@ bool history_t::save_internal_via_appending() { signal_block(); - // Open the file. - int out_fd = wopen_cloexec(history_path, O_WRONLY | O_APPEND); - if (out_fd >= 0) { - // Check to see if the file changed. - if (file_id_for_fd(out_fd) != mmap_file_id) file_changed = true; - + // We are going to open the file, lock it, append to it, and then close it + // After locking it, we need to stat the file at the path; if there is a new file there, it means + // the file was replaced and we have to try again + // Limit our max tries so we don't do this forever + int history_fd = -1; + for (int i=0; i < max_save_tries; i++) { + int fd = wopen_cloexec(history_path, O_WRONLY | O_APPEND); + if (fd < 0) { + // can't open, we're hosed + break; + } // Exclusive lock on the entire file. This is released when we close the file (below). This // may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that // we may get interleaved history items, which is considered better than no history, or @@ -1344,8 +1359,23 @@ bool history_t::save_internal_via_appending() { // by writing with O_APPEND. // // Simulate a failing lock in chaos_mode - if (!chaos_mode) history_file_lock(out_fd, LOCK_EX); + if (!chaos_mode) history_file_lock(fd, LOCK_EX); + const file_id_t file_id = file_id_for_fd(fd); + if (file_id_for_path(history_path) != file_id) { + // The file has changed, we're going to retry + close(fd); + } else { + // File IDs match, so the file we opened is still at that path + // We're going to use this fd + if (file_id != this->mmap_file_id) { + file_changed = true; + } + history_fd = fd; + break; + } + } + if (history_fd >= 0) { // We (hopefully successfully) took the exclusive lock. Append to the file. // Note that this is sketchy for a few reasons: // - Another shell may have appended its own items with a later timestamp, so our file may @@ -1366,13 +1396,15 @@ bool history_t::save_internal_via_appending() { // So far so good. Write all items at or after first_unwritten_new_item_index. Note that we // write even a pending item - pending items are ignored by history within the command // itself, but should still be written to the file. + // TODO: consider filling the buffer ahead of time, so we can just lock, splat, and unlock? bool errored = false; - history_output_buffer_t buffer; + // Use a small buffer size for appending, we usually only have 1 item + history_output_buffer_t buffer(64); while (first_unwritten_new_item_index < new_items.size()) { const history_item_t &item = new_items.at(first_unwritten_new_item_index); append_yaml_to_buffer(item.str(), item.timestamp(), item.get_required_paths(), &buffer); if (buffer.output_size() >= HISTORY_OUTPUT_BUFFER_SIZE) { - errored = !buffer.flush_to_fd(out_fd); + errored = !buffer.flush_to_fd(history_fd); if (errored) break; } @@ -1380,12 +1412,17 @@ bool history_t::save_internal_via_appending() { first_unwritten_new_item_index++; } - if (!errored && buffer.flush_to_fd(out_fd)) { + if (!errored && buffer.flush_to_fd(history_fd)) { ok = true; } - if (!chaos_mode) history_file_lock(out_fd, LOCK_UN); - close(out_fd); + // Since we just modified the file, update our mmap_file_id to match its current state + // Otherwise we'll think the file has been changed by someone else the next time we go to write + // We don't update the mapping since we only appended to the file, and everything we appended + // remains in our new_items + this->mmap_file_id = file_id_for_fd(history_fd); + + close(history_fd); } signal_unblock(); @@ -1553,7 +1590,7 @@ void history_t::populate_from_config_path() { // destination file descriptor, since it destroys the name and the file. this->clear(); - int dst_fd = wopen_cloexec(new_file, O_WRONLY | O_CREAT, 0644); + int dst_fd = wopen_cloexec(new_file, O_WRONLY | O_CREAT, history_file_mode); char buf[BUFSIZ]; ssize_t size; while ((size = read(src_fd, buf, BUFSIZ)) > 0) {