Mercurial > dovecot > original-hg > dovecot-1.2

changeset 4238:3c8b191b0019 HEAD

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
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.
author Timo Sirainen <timo.sirainen@movial.fi>
date Tue, 02 May 2006 14:11:05 +0300
parents 20e6d554d3fe
children edd923a44ef7
files src/lib-storage/index/maildir/maildir-save.c src/lib-storage/index/maildir/maildir-storage.h src/lib-storage/index/maildir/maildir-sync.c src/lib-storage/index/maildir/maildir-uidlist.c src/lib-storage/index/maildir/maildir-uidlist.h
diffstat 5 files changed, 124 insertions(+), 108 deletions(-) [+]
line wrap: on
line diff
--- a/src/lib-storage/index/maildir/maildir-save.c	Tue May 02 14:04:30 2006 +0300
+++ b/src/lib-storage/index/maildir/maildir-save.c	Tue May 02 14:11:05 2006 +0300
@@ -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);
--- a/src/lib-storage/index/maildir/maildir-storage.h	Tue May 02 14:04:30 2006 +0300
+++ b/src/lib-storage/index/maildir/maildir-storage.h	Tue May 02 14:11:05 2006 +0300
@@ -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,
--- a/src/lib-storage/index/maildir/maildir-sync.c	Tue May 02 14:04:30 2006 +0300
+++ b/src/lib-storage/index/maildir/maildir-sync.c	Tue May 02 14:11:05 2006 +0300
@@ -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;
+	struct maildir_index_sync_context *ctx;
+	struct mail_index_sync_ctx *sync_ctx;
+	struct mail_index_view *view;
 
-	sync_ctx = i_new(struct maildir_index_sync_context, 1);
-	sync_ctx->mbox = mbox;
-
-	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);
 }
 
--- a/src/lib-storage/index/maildir/maildir-uidlist.c	Tue May 02 14:04:30 2006 +0300
+++ b/src/lib-storage/index/maildir/maildir-uidlist.c	Tue May 02 14:11:05 2006 +0300
@@ -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);
 
--- a/src/lib-storage/index/maildir/maildir-uidlist.h	Tue May 02 14:04:30 2006 +0300
+++ b/src/lib-storage/index/maildir/maildir-uidlist.h	Tue May 02 14:11:05 2006 +0300
@@ -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,