[dovecot-cvs] dovecot/src/lib-storage/index/maildir maildir-save.c,
1.69, 1.70 maildir-storage.h, 1.48, 1.49 maildir-sync.c, 1.76,
1.77 maildir-uidlist.c, 1.50, 1.51 maildir-uidlist.h, 1.16, 1.17
tss-movial at dovecot.org
tss-movial at dovecot.org
Tue May 2 14:11:08 EEST 2006
Update of /var/lib/cvs/dovecot/src/lib-storage/index/maildir
In directory talvi:/tmp/cvs-serv23152
Modified Files:
maildir-save.c maildir-storage.h maildir-sync.c
maildir-uidlist.c maildir-uidlist.h
Log Message:
Adding mail to index while saving it had a race condition. Fixing it
required a bit larger changes. Switched uidlist/index locking order so that
uidlist is now locked first.
Index: maildir-save.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-save.c,v
retrieving revision 1.69
retrieving revision 1.70
diff -u -d -r1.69 -r1.70
--- maildir-save.c 25 Mar 2006 10:54:35 -0000 1.69
+++ maildir-save.c 2 May 2006 11:11:05 -0000 1.70
@@ -47,7 +47,7 @@
time_t received_date;
uint32_t first_seq, seq;
- unsigned int synced:1;
+ unsigned int want_mails:1;
unsigned int failed:1;
unsigned int moving:1;
unsigned int finished:1;
@@ -110,7 +110,11 @@
ctx->newdir = p_strconcat(pool, mbox->path, "/new", NULL);
ctx->curdir = p_strconcat(pool, mbox->path, "/cur", NULL);
- ctx->synced = maildir_sync_is_synced(mbox) > 0;
+ /* we'll do a quick check here to see if maildir is currently in
+ synced state. in that case it's cheap to update index file.
+ this can't be completely trusted because uidlist isn't locked,
+ but if there are some changes we can deal with it. */
+ ctx->want_mails = maildir_sync_is_synced(mbox);
ctx->keywords_buffer = buffer_create_const_data(pool, NULL, 0);
array_create_from_buffer(&ctx->keywords_array, ctx->keywords_buffer,
@@ -124,19 +128,10 @@
struct mail_keywords *keywords, bool want_mail)
{
struct maildir_save_context *ctx = t->save_ctx;
- struct maildir_mailbox *mbox = (struct maildir_mailbox *)t->ictx.ibox;
struct maildir_filename *mf;
- if (!ctx->synced && want_mail) {
- /* we could support adding the missing mails to index, but
- currently there's no need. */
- i_assert(ctx->files == NULL);
-
- if (maildir_storage_sync_force(mbox) < 0)
- ctx->failed = TRUE;
- else
- ctx->synced = TRUE;
- }
+ if (want_mail)
+ ctx->want_mails = TRUE;
/* now, we want to be able to rollback the whole append session,
so we'll just store the name of this temp file and move it later
@@ -162,7 +157,7 @@
sizeof(unsigned int) * keywords->count);
}
- if (ctx->synced) {
+ if (ctx->want_mails) {
/* insert into index */
mail_index_append(ctx->trans, 0, &ctx->seq);
mail_index_update_flags(ctx->trans, ctx->seq,
@@ -430,22 +425,29 @@
i_assert(ctx->output == NULL);
i_assert(ctx->finished);
- /* Start syncing so that keywords_sync_ctx gets set.. */
- ctx->sync_ctx = maildir_sync_index_begin(ctx->mbox);
- if (ctx->sync_ctx == NULL) {
+ if (maildir_uidlist_sync_init(ctx->mbox->uidlist, TRUE,
+ &ctx->uidlist_sync_ctx) <= 0) {
+ /* error or timeout - our transaction is broken */
maildir_transaction_save_rollback(ctx);
return -1;
}
- if (maildir_uidlist_sync_init(ctx->mbox->uidlist, TRUE,
- &ctx->uidlist_sync_ctx) <= 0) {
- /* error or timeout - our transaction is broken */
- maildir_sync_index_abort(ctx->sync_ctx);
+ /* Start syncing so that keywords_sync_ctx gets set.. */
+ if (maildir_sync_index_begin(ctx->mbox, &ctx->sync_ctx) < 0) {
maildir_transaction_save_rollback(ctx);
return -1;
}
- if (ctx->synced) {
+ if (ctx->want_mails) {
+ /* now that uidlist is locked, make sure all the existing mails
+ have been added to index. we don't really look into the
+ maildir, just add all the new mails listed in
+ dovecot-uidlist to index. */
+ if (maildir_sync_index(ctx->sync_ctx, TRUE) < 0) {
+ maildir_transaction_save_rollback(ctx);
+ return -1;
+ }
+
first_uid = maildir_uidlist_get_next_uid(ctx->mbox->uidlist);
mail_index_append_assign_uids(ctx->trans, first_uid, &last_uid);
}
@@ -476,35 +478,27 @@
t_pop();
}
+ if (maildir_sync_index_finish(&ctx->sync_ctx, ret < 0) < 0)
+ ret = -1;
+
if (ret < 0) {
/* unlink the files we just moved in an attempt to rollback
the transaction. uidlist is still locked, so at least other
Dovecot instances haven't yet seen the files. */
maildir_transaction_unlink_copied_files(ctx, mf);
- }
- if (maildir_uidlist_sync_deinit(ctx->uidlist_sync_ctx) < 0)
- ret = -1;
- ctx->uidlist_sync_ctx = NULL;
-
- if (ret < 0) {
/* returning failure finishes the save_context */
maildir_transaction_save_rollback(ctx);
}
+
return ret;
}
void maildir_transaction_save_commit_post(struct maildir_save_context *ctx)
{
- /* since we've allocated more UIDs, the index must be kept locked
- from that time until the changes are written to transaction log.
- keeping the syncing open until here we also keep the lock open.
-
- if the transaction log writer itself had to grab the lock, it'd
- mean that there's a chance for another process to start maildir
- sync and write the same UIDs twice for the transaction log. */
- maildir_sync_index_abort(ctx->sync_ctx);
-
+ /* uidlist locks the syncing. don't release it until save's transaction
+ has been written to disk. */
+ (void)maildir_uidlist_sync_deinit(&ctx->uidlist_sync_ctx);
pool_unref(ctx->pool);
}
@@ -542,6 +536,11 @@
if (hardlinks)
maildir_transaction_unlink_copied_files(ctx, NULL);
+ if (ctx->uidlist_sync_ctx != NULL)
+ (void)maildir_uidlist_sync_deinit(&ctx->uidlist_sync_ctx);
+ if (ctx->sync_ctx != NULL)
+ (void)maildir_sync_index_finish(&ctx->sync_ctx, TRUE);
+
t_pop();
pool_unref(ctx->pool);
Index: maildir-storage.h
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-storage.h,v
retrieving revision 1.48
retrieving revision 1.49
diff -u -d -r1.48 -r1.49
--- maildir-storage.h 1 Apr 2006 09:31:15 -0000 1.48
+++ maildir-storage.h 2 May 2006 11:11:05 -0000 1.49
@@ -46,6 +46,7 @@
struct maildir_save_context;
struct maildir_copy_context;
struct maildir_keywords_sync_ctx;
+struct maildir_index_sync_context;
struct maildir_storage {
struct index_storage storage;
@@ -103,11 +104,12 @@
maildir_storage_sync_init(struct mailbox *box, enum mailbox_sync_flags flags);
int maildir_storage_sync_force(struct maildir_mailbox *mbox);
-struct maildir_index_sync_context *
-maildir_sync_index_begin(struct maildir_mailbox *mbox);
-void maildir_sync_index_abort(struct maildir_index_sync_context *sync_ctx);
-int maildir_sync_index_finish(struct maildir_index_sync_context *sync_ctx,
- bool partial);
+int maildir_sync_index_begin(struct maildir_mailbox *mbox,
+ struct maildir_index_sync_context **ctx_r);
+int maildir_sync_index(struct maildir_index_sync_context *sync_ctx,
+ bool partial);
+int maildir_sync_index_finish(struct maildir_index_sync_context **sync_ctx,
+ bool failed);
struct mailbox_transaction_context *
maildir_transaction_begin(struct mailbox *box,
Index: maildir-sync.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-sync.c,v
retrieving revision 1.76
retrieving revision 1.77
diff -u -d -r1.76 -r1.77
--- maildir-sync.c 17 Apr 2006 17:31:54 -0000 1.76
+++ maildir-sync.c 2 May 2006 11:11:05 -0000 1.77
@@ -606,9 +606,9 @@
static void maildir_sync_deinit(struct maildir_sync_context *ctx)
{
if (ctx->uidlist_sync_ctx != NULL)
- (void)maildir_uidlist_sync_deinit(ctx->uidlist_sync_ctx);
+ (void)maildir_uidlist_sync_deinit(&ctx->uidlist_sync_ctx);
if (ctx->index_sync_ctx != NULL)
- maildir_sync_index_abort(ctx->index_sync_ctx);
+ (void)maildir_sync_index_finish(&ctx->index_sync_ctx, TRUE);
}
static int maildir_fix_duplicate(struct maildir_mailbox *mbox, const char *dir,
@@ -823,36 +823,77 @@
return 0;
}
-struct maildir_index_sync_context *
-maildir_sync_index_begin(struct maildir_mailbox *mbox)
+int maildir_sync_index_begin(struct maildir_mailbox *mbox,
+ struct maildir_index_sync_context **ctx_r)
{
- struct maildir_index_sync_context *sync_ctx;
-
- sync_ctx = i_new(struct maildir_index_sync_context, 1);
- sync_ctx->mbox = mbox;
+ struct maildir_index_sync_context *ctx;
+ struct mail_index_sync_ctx *sync_ctx;
+ struct mail_index_view *view;
- if (mail_index_sync_begin(mbox->ibox.index, &sync_ctx->sync_ctx,
- &sync_ctx->view, (uint32_t)-1, (uoff_t)-1,
+ if (mail_index_sync_begin(mbox->ibox.index, &sync_ctx, &view,
+ (uint32_t)-1, (uoff_t)-1,
FALSE, FALSE) <= 0) {
mail_storage_set_index_error(&mbox->ibox);
- i_free(sync_ctx);
- return NULL;
+ return -1;
}
- sync_ctx->keywords_sync_ctx =
+ ctx = i_new(struct maildir_index_sync_context, 1);
+ ctx->mbox = mbox;
+ ctx->sync_ctx = sync_ctx;
+ ctx->view = view;
+ ctx->keywords_sync_ctx =
maildir_keywords_sync_init(mbox->keywords, mbox->ibox.index);
- return sync_ctx;
+ *ctx_r = ctx;
+ return 0;
}
-void maildir_sync_index_abort(struct maildir_index_sync_context *sync_ctx)
+int maildir_sync_index_finish(struct maildir_index_sync_context **_sync_ctx,
+ bool failed)
{
- mail_index_sync_rollback(&sync_ctx->sync_ctx);
+ struct maildir_index_sync_context *sync_ctx = *_sync_ctx;
+ struct maildir_mailbox *mbox = sync_ctx->mbox;
+ uint32_t seq;
+ uoff_t offset;
+ int ret = failed ? -1 : 0;
+
+ *_sync_ctx = NULL;
+
+ if (sync_ctx->trans != NULL) {
+ if (ret < 0)
+ mail_index_transaction_rollback(&sync_ctx->trans);
+ else {
+ if (mail_index_transaction_commit(&sync_ctx->trans,
+ &seq, &offset) < 0)
+ ret = -1;
+ else if (seq != 0) {
+ mbox->ibox.commit_log_file_seq = seq;
+ mbox->ibox.commit_log_file_offset = offset;
+ }
+ }
+ }
+ if (ret < 0)
+ mail_index_sync_rollback(&sync_ctx->sync_ctx);
+ else {
+ if (mail_index_sync_commit(&sync_ctx->sync_ctx) < 0)
+ ret = -1;
+ else {
+ mbox->ibox.commit_log_file_seq = 0;
+ mbox->ibox.commit_log_file_offset = 0;
+ }
+ }
+
+ if (ret < 0)
+ mail_storage_set_index_error(&mbox->ibox);
+
maildir_keywords_sync_deinit(sync_ctx->keywords_sync_ctx);
+ sync_ctx->keywords_sync_ctx = NULL;
+
i_free(sync_ctx);
+ return ret;
}
-int maildir_sync_index_finish(struct maildir_index_sync_context *sync_ctx,
- bool partial)
+int maildir_sync_index(struct maildir_index_sync_context *sync_ctx,
+ bool partial)
{
struct maildir_mailbox *mbox = sync_ctx->mbox;
struct mail_index_view *view = sync_ctx->view;
@@ -874,9 +915,6 @@
i_assert(maildir_uidlist_is_locked(sync_ctx->mbox->uidlist));
- trans = mail_index_transaction_begin(view, FALSE, TRUE);
- sync_ctx->trans = trans;
-
hdr = mail_index_get_header(view);
uid_validity = maildir_uidlist_get_uid_validity(mbox->uidlist);
if (uid_validity != hdr->uid_validity &&
@@ -891,6 +929,9 @@
return -1;
}
+ sync_ctx->trans = trans =
+ mail_index_transaction_begin(sync_ctx->view, FALSE, TRUE);
+
seq = 0;
ARRAY_CREATE(&keywords, pool_datastack_create(),
unsigned int, MAILDIR_MAX_KEYWORDS);
@@ -1149,33 +1190,6 @@
&uid_validity, sizeof(uid_validity), TRUE);
}
- if (ret < 0) {
- mail_index_transaction_rollback(&trans);
- mail_index_sync_rollback(&sync_ctx->sync_ctx);
- } else {
- uint32_t seq;
- uoff_t offset;
-
- if (mail_index_transaction_commit(&trans, &seq, &offset) < 0)
- ret = -1;
- else if (seq != 0) {
- mbox->ibox.commit_log_file_seq = seq;
- mbox->ibox.commit_log_file_offset = offset;
- }
- if (mail_index_sync_commit(&sync_ctx->sync_ctx) < 0)
- ret = -1;
- }
- maildir_keywords_sync_deinit(sync_ctx->keywords_sync_ctx);
- sync_ctx->keywords_sync_ctx = NULL;
-
- if (ret == 0) {
- mbox->ibox.commit_log_file_seq = 0;
- mbox->ibox.commit_log_file_offset = 0;
- } else {
- mail_storage_set_index_error(&mbox->ibox);
- }
-
- i_free(sync_ctx);
return ret < 0 ? -1 : (full_rescan ? 0 : 1);
}
@@ -1206,7 +1220,7 @@
committing changes to maildir but a file was lost (maybe renamed).
So, we're going to need two locks. One for index and one for
- uidlist. To avoid deadlocking do the index lock first.
+ uidlist. To avoid deadlocking do the uidlist lock always first.
uidlist is needed only for figuring out UIDs for newly seen files,
so theoretically we wouldn't need to lock it unless there are new
@@ -1244,12 +1258,6 @@
problem rarely happens except under high amount of modifications.
*/
- if (!ctx->mbox->syncing_commit) {
- ctx->index_sync_ctx = maildir_sync_index_begin(ctx->mbox);
- if (ctx->index_sync_ctx == NULL)
- return -1;
- }
-
ctx->partial = !cur_changed;
ret = maildir_uidlist_sync_init(ctx->mbox->uidlist, ctx->partial,
&ctx->uidlist_sync_ctx);
@@ -1260,6 +1268,12 @@
return ret;
}
+ if (!ctx->mbox->syncing_commit) {
+ if (maildir_sync_index_begin(ctx->mbox,
+ &ctx->index_sync_ctx) < 0)
+ return -1;
+ }
+
if (new_changed || cur_changed) {
/* if we're going to check cur/ dir our current logic requires
that new/ dir is checked as well. it's a good idea anyway. */
@@ -1285,22 +1299,20 @@
files getting lost, so this function might be called
re-entrantly. FIXME: and that breaks in
maildir_uidlist_sync_deinit() */
- ret = maildir_sync_index_finish(ctx->index_sync_ctx,
- ctx->partial);
- if (ret < 0) {
- ctx->index_sync_ctx = NULL;
+ ret = maildir_sync_index(ctx->index_sync_ctx, ctx->partial);
+ if (maildir_sync_index_finish(&ctx->index_sync_ctx,
+ ret < 0) < 0)
+ return -1;
+
+ if (ret < 0)
return -1;
- }
if (ret == 0)
full_rescan = TRUE;
i_assert(maildir_uidlist_is_locked(ctx->mbox->uidlist));
- ctx->index_sync_ctx = NULL;
}
- ret = maildir_uidlist_sync_deinit(ctx->uidlist_sync_ctx);
- ctx->uidlist_sync_ctx = NULL;
-
+ ret = maildir_uidlist_sync_deinit(&ctx->uidlist_sync_ctx);
return ret < 0 ? -1 : (full_rescan ? 0 : 1);
}
Index: maildir-uidlist.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-uidlist.c,v
retrieving revision 1.50
retrieving revision 1.51
diff -u -d -r1.50 -r1.51
--- maildir-uidlist.c 25 Apr 2006 13:40:21 -0000 1.50
+++ maildir-uidlist.c 2 May 2006 11:11:05 -0000 1.51
@@ -907,11 +907,14 @@
ctx->uidlist->initial_sync = TRUE;
}
-int maildir_uidlist_sync_deinit(struct maildir_uidlist_sync_ctx *ctx)
+int maildir_uidlist_sync_deinit(struct maildir_uidlist_sync_ctx **_ctx)
{
+ struct maildir_uidlist_sync_ctx *ctx = *_ctx;
bool unlocked = FALSE;
int ret = ctx->failed ? -1 : 0;
+ *_ctx = NULL;
+
if (!ctx->finished)
maildir_uidlist_sync_finish(ctx);
Index: maildir-uidlist.h
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-uidlist.h,v
retrieving revision 1.16
retrieving revision 1.17
diff -u -d -r1.16 -r1.17
--- maildir-uidlist.h 13 Jan 2006 20:26:35 -0000 1.16
+++ maildir-uidlist.h 2 May 2006 11:11:05 -0000 1.17
@@ -50,7 +50,7 @@
const char *filename,
enum maildir_uidlist_rec_flag flags);
void maildir_uidlist_sync_finish(struct maildir_uidlist_sync_ctx *ctx);
-int maildir_uidlist_sync_deinit(struct maildir_uidlist_sync_ctx *ctx);
+int maildir_uidlist_sync_deinit(struct maildir_uidlist_sync_ctx **ctx);
void maildir_uidlist_add_flags(struct maildir_uidlist *uidlist,
const char *filename,
More information about the dovecot-cvs
mailing list