From: Timo Sirainen Date: Tue, 7 Oct 2014 16:07:16 +0000 (+0300) Subject: lib-index: Delay unlocking cache compression until changes to transaction log are... X-Git-Tag: 2.2.14~45 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0f833d18af36bf307cd69f235eb3fb779245b478;p=thirdparty%2Fdovecot%2Fcore.git lib-index: Delay unlocking cache compression until changes to transaction log are committed. This should fix race condition with two processes compressing the file at the same time with same file_seq and becoming confused. --- diff --git a/src/lib-index/mail-cache-compress.c b/src/lib-index/mail-cache-compress.c index fff286c2aa..af5fece2c0 100644 --- a/src/lib-index/mail-cache-compress.c +++ b/src/lib-index/mail-cache-compress.c @@ -10,6 +10,7 @@ #include "file-set-size.h" #include "mail-cache-private.h" +#include #include struct mail_cache_copy_context { @@ -23,6 +24,10 @@ struct mail_cache_copy_context { bool new_msg; }; +struct mail_cache_compress_lock { + struct dotlock *dotlock; +}; + static void mail_cache_merge_bitmask(struct mail_cache_copy_context *ctx, const struct mail_cache_iterate_field *field) @@ -315,6 +320,56 @@ mail_cache_copy(struct mail_cache *cache, struct mail_index_transaction *trans, return 0; } +static int +mail_cache_compress_write(struct mail_cache *cache, + struct mail_index_transaction *trans, + int fd, const char *temp_path, bool *unlock) +{ + struct stat st; + uint32_t file_seq, old_offset; + ARRAY_TYPE(uint32_t) ext_offsets; + const uint32_t *offsets; + unsigned int i, count; + + if (mail_cache_copy(cache, trans, fd, &file_seq, &ext_offsets) < 0) + return -1; + + if (fstat(fd, &st) < 0) { + mail_cache_set_syscall_error(cache, "fstat()"); + array_free(&ext_offsets); + return -1; + } + if (rename(temp_path, cache->filepath) < 0) { + mail_cache_set_syscall_error(cache, "rename()"); + array_free(&ext_offsets); + return -1; + } + + /* once we're sure that the compression was successful, + update the offsets */ + mail_index_ext_reset(trans, cache->ext_id, file_seq, TRUE); + offsets = array_get(&ext_offsets, &count); + for (i = 0; i < count; i++) { + if (offsets[i] != 0) { + mail_index_update_ext(trans, i + 1, cache->ext_id, + &offsets[i], &old_offset); + } + } + array_free(&ext_offsets); + + if (*unlock) { + (void)mail_cache_unlock(cache); + *unlock = FALSE; + } + + mail_cache_file_close(cache); + cache->fd = fd; + cache->st_ino = st.st_ino; + cache->st_dev = st.st_dev; + cache->field_header_write_pending = FALSE; + return 0; +} + static int mail_cache_compress_has_file_changed(struct mail_cache *cache) { struct mail_cache_header hdr; @@ -338,7 +393,8 @@ static int mail_cache_compress_has_file_changed(struct mail_cache *cache) if (ret == 0) return 0; if (cache->need_compress_file_seq == 0) { - /* previously it didn't exist */ + /* previously it didn't exist or it + was unusable and was just unlinked */ return 1; } return hdr.file_seq != cache->need_compress_file_seq; @@ -349,97 +405,63 @@ static int mail_cache_compress_has_file_changed(struct mail_cache *cache) } } +static int mail_cache_compress_dotlock(struct mail_cache *cache, + struct dotlock **dotlock_r) +{ + if (file_dotlock_create(&cache->dotlock_settings, cache->filepath, + DOTLOCK_CREATE_FLAG_NONBLOCK, dotlock_r) <= 0) { + if (errno != EAGAIN) + mail_cache_set_syscall_error(cache, "file_dotlock_open()"); + return -1; + } + return 0; +} + static int mail_cache_compress_locked(struct mail_cache *cache, struct mail_index_transaction *trans, - bool *unlock) + bool *unlock, struct dotlock **dotlock_r) { - struct dotlock *dotlock; - struct stat st; - mode_t old_mask; - uint32_t file_seq, old_offset; - ARRAY_TYPE(uint32_t) ext_offsets; - const uint32_t *offsets; + const char *temp_path; const void *data; - unsigned int i, count; int fd, ret; - old_mask = umask(cache->index->mode ^ 0666); - fd = file_dotlock_open(&cache->dotlock_settings, cache->filepath, - DOTLOCK_CREATE_FLAG_NONBLOCK, &dotlock); - umask(old_mask); + /* There are two possible locking situations here: + a) Cache is locked against any modifications. + b) Cache doesn't exist or is unusable. There's no lock. + Because the cache lock itself is unreliable, we'll be using a + separate dotlock to guard against two processes compressing the + cache at the same time. */ - if (fd == -1) { - if (errno != EAGAIN) - mail_cache_set_syscall_error(cache, "file_dotlock_open()"); + if (mail_cache_compress_dotlock(cache, dotlock_r) < 0) return -1; - } - + /* we've locked the cache compression now. if somebody else had just + recreated the cache, reopen the cache and return success. */ if ((ret = mail_cache_compress_has_file_changed(cache)) != 0) { if (ret < 0) return -1; /* was just compressed, forget this */ cache->need_compress_file_seq = 0; - file_dotlock_delete(&dotlock); + file_dotlock_delete(dotlock_r); if (*unlock) { (void)mail_cache_unlock(cache); *unlock = FALSE; } - return mail_cache_reopen(cache); - } - - mail_index_fchown(cache->index, fd, - file_dotlock_get_lock_path(dotlock)); - - if (mail_cache_copy(cache, trans, fd, &file_seq, &ext_offsets) < 0) { - /* the fields may have been updated in memory already. - reverse those changes by re-reading them from file. */ - if (mail_cache_header_fields_read(cache) < 0) - return -1; - file_dotlock_delete(&dotlock); - return -1; + return mail_cache_reopen(cache) < 0 ? -1 : 0; } - if (fstat(fd, &st) < 0) { - mail_cache_set_syscall_error(cache, "fstat()"); - file_dotlock_delete(&dotlock); + /* we want to recreate the cache. write it first to a temporary file */ + fd = mail_index_create_tmp_file(cache->index, cache->filepath, &temp_path); + if (fd == -1) return -1; - } - - if (file_dotlock_replace(&dotlock, - DOTLOCK_REPLACE_FLAG_DONT_CLOSE_FD) < 0) { - mail_cache_set_syscall_error(cache, - "file_dotlock_replace()"); + if (mail_cache_compress_write(cache, trans, fd, temp_path, unlock) < 0) { i_close_fd(&fd); - array_free(&ext_offsets); + if (unlink(temp_path) < 0) + i_error("unlink(%s) failed: %m", temp_path); return -1; } - - /* once we're sure that the compression was successful, - update the offsets */ - mail_index_ext_reset(trans, cache->ext_id, file_seq, TRUE); - offsets = array_get(&ext_offsets, &count); - for (i = 0; i < count; i++) { - if (offsets[i] != 0) { - mail_index_update_ext(trans, i + 1, cache->ext_id, - &offsets[i], &old_offset); - } - } - array_free(&ext_offsets); - - if (*unlock) { - (void)mail_cache_unlock(cache); - *unlock = FALSE; - } - - mail_cache_file_close(cache); - cache->fd = fd; - cache->st_ino = st.st_ino; - cache->st_dev = st.st_dev; - cache->field_header_write_pending = FALSE; - if (cache->file_cache != NULL) file_cache_set_fd(cache->file_cache, cache->fd); @@ -453,15 +475,21 @@ static int mail_cache_compress_locked(struct mail_cache *cache, } int mail_cache_compress(struct mail_cache *cache, - struct mail_index_transaction *trans) + struct mail_index_transaction *trans, + struct mail_cache_compress_lock **lock_r) { + struct dotlock *dotlock = NULL; bool unlock = FALSE; int ret; i_assert(!cache->compressing); - if (MAIL_INDEX_IS_IN_MEMORY(cache->index) || cache->index->readonly) + *lock_r = NULL; + + if (MAIL_INDEX_IS_IN_MEMORY(cache->index) || cache->index->readonly) { + *lock_r = i_new(struct mail_cache_compress_lock, 1); return 0; + } /* compression isn't very efficient with small read()s */ if (cache->map_with_read) { @@ -483,9 +511,10 @@ int mail_cache_compress(struct mail_cache *cache, } else { switch (mail_cache_try_lock(cache)) { case -1: + /* already locked or some other error */ return -1; case 0: - /* couldn't lock, either it's broken or doesn't exist. + /* cache is broken or doesn't exist. just start creating it. */ break; default: @@ -494,15 +523,36 @@ int mail_cache_compress(struct mail_cache *cache, } } cache->compressing = TRUE; - ret = mail_cache_compress_locked(cache, trans, &unlock); + ret = mail_cache_compress_locked(cache, trans, &unlock, &dotlock); cache->compressing = FALSE; if (unlock) { if (mail_cache_unlock(cache) < 0) ret = -1; } + if (ret < 0) { + if (dotlock != NULL) + file_dotlock_delete(&dotlock); + /* the fields may have been updated in memory already. + reverse those changes by re-reading them from file. */ + (void)mail_cache_header_fields_read(cache); + } else { + *lock_r = i_new(struct mail_cache_compress_lock, 1); + (*lock_r)->dotlock = dotlock; + } return ret; } +void mail_cache_compress_unlock(struct mail_cache_compress_lock **_lock) +{ + struct mail_cache_compress_lock *lock = *_lock; + + *_lock = NULL; + + if (lock->dotlock != NULL) + file_dotlock_delete(&lock->dotlock); + i_free(lock); +} + bool mail_cache_need_compress(struct mail_cache *cache) { return cache->need_compress_file_seq != 0 && diff --git a/src/lib-index/mail-cache-transaction.c b/src/lib-index/mail-cache-transaction.c index e72a895a80..d0ffef8048 100644 --- a/src/lib-index/mail-cache-transaction.c +++ b/src/lib-index/mail-cache-transaction.c @@ -168,6 +168,7 @@ mail_cache_transaction_compress(struct mail_cache_transaction_ctx *ctx) struct mail_cache *cache = ctx->cache; struct mail_index_view *view; struct mail_index_transaction *trans; + struct mail_cache_compress_lock *lock; int ret; ctx->tried_compression = TRUE; @@ -178,11 +179,13 @@ mail_cache_transaction_compress(struct mail_cache_transaction_ctx *ctx) view = mail_index_view_open(cache->index); trans = mail_index_transaction_begin(view, MAIL_INDEX_TRANSACTION_FLAG_EXTERNAL); - if (mail_cache_compress(cache, trans) < 0) { + if (mail_cache_compress(cache, trans, &lock) < 0) { mail_index_transaction_rollback(&trans); ret = -1; } else { ret = mail_index_transaction_commit(&trans); + if (lock != NULL) + mail_cache_compress_unlock(&lock); } mail_index_view_close(&view); mail_cache_transaction_reset(ctx); diff --git a/src/lib-index/mail-cache.h b/src/lib-index/mail-cache.h index 324159ffa6..4b39dbbe81 100644 --- a/src/lib-index/mail-cache.h +++ b/src/lib-index/mail-cache.h @@ -8,6 +8,7 @@ struct mail_cache; struct mail_cache_view; struct mail_cache_transaction_ctx; +struct mail_cache_compress_lock; enum mail_cache_decision_type { /* Not needed currently */ @@ -64,9 +65,15 @@ mail_cache_register_get_list(struct mail_cache *cache, pool_t pool, /* Returns TRUE if cache should be compressed. */ bool mail_cache_need_compress(struct mail_cache *cache); -/* Compress cache file. Offsets are updated to given transaction. */ +/* Compress cache file. Offsets are updated to given transaction. The cache + compression lock should be kept until the transaction is committed. + mail_cache_compress_unlock() needs to be called afterwards. The lock doesn't + prevent updates to the cache while it's held, it only prevents another cache + compression. */ int mail_cache_compress(struct mail_cache *cache, - struct mail_index_transaction *trans); + struct mail_index_transaction *trans, + struct mail_cache_compress_lock **lock_r); +void mail_cache_compress_unlock(struct mail_cache_compress_lock **lock); /* Returns TRUE if there is at least something in the cache. */ bool mail_cache_exists(struct mail_cache *cache); /* Open and read cache header. Returns 0 if ok, -1 if error/corrupted. */ diff --git a/src/lib-index/mail-index-sync.c b/src/lib-index/mail-index-sync.c index afea7a3e28..1e98d323a5 100644 --- a/src/lib-index/mail-index-sync.c +++ b/src/lib-index/mail-index-sync.c @@ -6,7 +6,7 @@ #include "mail-index-sync-private.h" #include "mail-index-transaction-private.h" #include "mail-transaction-log-private.h" -#include "mail-cache.h" +#include "mail-cache-private.h" #include #include @@ -784,9 +784,10 @@ int mail_index_sync_commit(struct mail_index_sync_ctx **_ctx) { struct mail_index_sync_ctx *ctx = *_ctx; struct mail_index *index = ctx->index; + struct mail_cache_compress_lock *cache_lock = NULL; uint32_t next_uid; bool want_rotate, index_undeleted, delete_index; - int ret = 0; + int ret = 0, ret2; index_undeleted = ctx->ext_trans->index_undeleted; delete_index = index->index_delete_requested && !index_undeleted && @@ -807,7 +808,8 @@ int mail_index_sync_commit(struct mail_index_sync_ctx **_ctx) /* if cache compression fails, we don't really care. the cache offsets are updated only if the compression was successful. */ - (void)mail_cache_compress(index->cache, ctx->ext_trans); + (void)mail_cache_compress(index->cache, ctx->ext_trans, + &cache_lock); } if ((ctx->flags & MAIL_INDEX_SYNC_FLAG_DROP_RECENT) != 0) { @@ -820,7 +822,10 @@ int mail_index_sync_commit(struct mail_index_sync_ctx **_ctx) } } - if (mail_index_transaction_commit(&ctx->ext_trans) < 0) { + ret2 = mail_index_transaction_commit(&ctx->ext_trans); + if (cache_lock != NULL) + mail_cache_compress_unlock(&cache_lock); + if (ret2 < 0) { mail_index_sync_end(&ctx); return -1; } diff --git a/src/lib-storage/index/index-rebuild.c b/src/lib-storage/index/index-rebuild.c index f98bb6b5d3..909f9d3f07 100644 --- a/src/lib-storage/index/index-rebuild.c +++ b/src/lib-storage/index/index-rebuild.c @@ -189,11 +189,19 @@ void index_index_rebuild_deinit(struct index_rebuild_context **_ctx, index_rebuild_generate_uidvalidity_t *cb) { struct index_rebuild_context *ctx = *_ctx; + struct mail_cache_compress_lock *lock = NULL; *_ctx = NULL; /* initialize cache file with the old field decisions */ - (void)mail_cache_compress(ctx->box->cache, ctx->trans); + (void)mail_cache_compress(ctx->box->cache, ctx->trans, &lock); + if (lock != NULL) { + /* FIXME: this is a bit too early. ideally we should return it + from this function and unlock only after the transaction is + committed, but it would be an API change and this rebuilding + isn't happening normally anyway. */ + mail_cache_compress_unlock(&lock); + } index_rebuild_header(ctx, cb); if (ctx->backup_index != NULL) { mail_index_view_close(&ctx->backup_view);