[dovecot-cvs] dovecot/src/lib-storage/index/maildir maildir-save.c, 1.42, 1.43 maildir-storage.h, 1.30, 1.31 maildir-sync.c, 1.42, 1.43

[cras at dovecot.org]

Update of /var/lib/cvs/dovecot/src/lib-storage/index/maildir
In directory talvi:/tmp/cvs-serv28400/lib-storage/index/maildir

Modified Files:
	maildir-save.c maildir-storage.h maildir-sync.c 
Log Message:
Always protect maildir syncing with uidlist lock. Before we only tried to
do it and if it failed, we went ahead and just didn't sync any new messages.
That however could have caused files to be lost temporarily due to race
conditions between readdir() and rename().



Index: maildir-save.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-save.c,v
retrieving revision 1.42
retrieving revision 1.43
diff -u -d -r1.42 -r1.43
--- maildir-save.c	17 Oct 2004 14:35:01 -0000	1.42
+++ maildir-save.c	25 Oct 2004 17:12:51 -0000	1.43
@@ -308,6 +308,7 @@
 
 int maildir_transaction_save_commit_pre(struct maildir_save_context *ctx)
 {
+	struct maildir_index_sync_context *sync_ctx;
 	struct maildir_filename *mf;
 	uint32_t first_uid, last_uid;
 	enum maildir_uidlist_rec_flag flags;
@@ -316,14 +317,21 @@
 
 	i_assert(ctx->output == NULL);
 
+	sync_ctx = maildir_sync_index_begin(ctx->ibox);
+	if (sync_ctx == NULL) {
+		maildir_save_commit_abort(ctx, ctx->files);
+		return -1;
+	}
+
 	ret = maildir_uidlist_lock(ctx->ibox->uidlist);
 	if (ret <= 0) {
 		/* error or timeout - our transaction is broken */
+		maildir_sync_index_abort(sync_ctx);
 		maildir_save_commit_abort(ctx, ctx->files);
 		return -1;
 	}
 
-	if (maildir_sync_index(ctx->ibox, TRUE) < 0) {
+	if (maildir_sync_index_finish(sync_ctx, TRUE) < 0) {
 		maildir_save_commit_abort(ctx, ctx->files);
 		return -1;
 	}

Index: maildir-storage.h
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-storage.h,v
retrieving revision 1.30
retrieving revision 1.31
diff -u -d -r1.30 -r1.31
--- maildir-storage.h	17 Oct 2004 14:35:01 -0000	1.30
+++ maildir-storage.h	25 Oct 2004 17:12:51 -0000	1.31
@@ -42,7 +42,12 @@
 struct mailbox_sync_context *
 maildir_storage_sync_init(struct mailbox *box, enum mailbox_sync_flags flags);
 int maildir_storage_sync_force(struct index_mailbox *ibox);
-int maildir_sync_index(struct index_mailbox *ibox, int partial);
+
+struct maildir_index_sync_context *
+maildir_sync_index_begin(struct index_mailbox *ibox);
+void maildir_sync_index_abort(struct maildir_index_sync_context *sync_ctx);
+int maildir_sync_index_finish(struct maildir_index_sync_context *sync_ctx,
+			      int partial);
 
 struct mailbox_transaction_context *
 maildir_transaction_begin(struct mailbox *box, int hide);

Index: maildir-sync.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib-storage/index/maildir/maildir-sync.c,v
retrieving revision 1.42
retrieving revision 1.43
diff -u -d -r1.42 -r1.43
--- maildir-sync.c	24 Oct 2004 00:22:11 -0000	1.42
+++ maildir-sync.c	25 Oct 2004 17:12:51 -0000	1.43
@@ -193,7 +193,8 @@
 	const char *new_dir, *cur_dir;
 	int partial;
 
-        struct maildir_uidlist_sync_ctx *uidlist_sync_ctx;
+	struct maildir_uidlist_sync_ctx *uidlist_sync_ctx;
+        struct maildir_index_sync_context *index_sync_ctx;
 };
 
 struct maildir_index_sync_context {
@@ -380,6 +381,8 @@
 {
 	if (ctx->uidlist_sync_ctx != NULL)
 		(void)maildir_uidlist_sync_deinit(ctx->uidlist_sync_ctx);
+	if (ctx->index_sync_ctx != NULL)
+		maildir_sync_index_abort(ctx->index_sync_ctx);
 }
 
 static int maildir_fix_duplicate(struct index_mailbox *ibox, const char *dir,
@@ -573,12 +576,36 @@
 	return 0;
 }
 
-int maildir_sync_index(struct index_mailbox *ibox, int partial)
+struct maildir_index_sync_context *
+maildir_sync_index_begin(struct index_mailbox *ibox)
 {
-	struct maildir_index_sync_context sync_ctx;
+	struct maildir_index_sync_context *sync_ctx;
+
+	sync_ctx = i_new(struct maildir_index_sync_context, 1);
+	sync_ctx->ibox = ibox;
+
+	if (mail_index_sync_begin(ibox->index, &sync_ctx->sync_ctx,
+				  &sync_ctx->view, (uint32_t)-1, (uoff_t)-1,
+				  FALSE, FALSE) <= 0) {
+		mail_storage_set_index_error(ibox);
+		return NULL;
+	}
+	return sync_ctx;
+}
+
+void maildir_sync_index_abort(struct maildir_index_sync_context *sync_ctx)
+{
+	mail_index_sync_rollback(sync_ctx->sync_ctx);
+	i_free(sync_ctx);
+}
+
+int maildir_sync_index_finish(struct maildir_index_sync_context *sync_ctx,
+			      int partial)
+{
+	struct index_mailbox *ibox = sync_ctx->ibox;
+	struct mail_index_view *view = sync_ctx->view;
 	struct maildir_uidlist_iter_ctx *iter;
 	struct mail_index_transaction *trans;
-	struct mail_index_view *view;
 	const struct mail_index_header *hdr;
 	const struct mail_index_record *rec;
 	uint32_t seq, uid;
@@ -589,21 +616,10 @@
 	uint32_t uid_validity, next_uid;
 	int ret;
 
-	memset(&sync_ctx, 0, sizeof(sync_ctx));
-	sync_ctx.ibox = ibox;
-
-	if (mail_index_sync_begin(ibox->index, &sync_ctx.sync_ctx, &view,
-				  (uint32_t)-1, (uoff_t)-1,
-				  FALSE, FALSE) <= 0) {
-		mail_storage_set_index_error(ibox);
-		return -1;
-	}
-	sync_ctx.view = view;
-
 	if (mail_index_get_header(view, &hdr) < 0) {
 		/* view is invalidated */
 		mail_storage_set_index_error(ibox);
-		(void)mail_index_sync_rollback(sync_ctx.sync_ctx);
+                maildir_sync_index_abort(sync_ctx);
 		return -1;
 	}
 
@@ -616,12 +632,12 @@
 			"Maildir %s sync: UIDVALIDITY changed (%u -> %u)",
 			ibox->path, hdr->uid_validity, uid_validity);
 		mail_index_mark_corrupted(ibox->index);
-		(void)mail_index_sync_rollback(sync_ctx.sync_ctx);
+                maildir_sync_index_abort(sync_ctx);
 		return -1;
 	}
 
 	trans = mail_index_transaction_begin(view, FALSE);
-	sync_ctx.trans = trans;
+	sync_ctx->trans = trans;
 
 	seq = 0;
 	iter = maildir_uidlist_iter_init(ibox->uidlist);
@@ -773,9 +789,9 @@
 
 	/* now, sync the index */
         ibox->syncing_commit = TRUE;
-	while ((ret = mail_index_sync_next(sync_ctx.sync_ctx,
-					   &sync_ctx.sync_rec)) > 0) {
-		if (maildir_sync_record(ibox, &sync_ctx) < 0) {
+	while ((ret = mail_index_sync_next(sync_ctx->sync_ctx,
+					   &sync_ctx->sync_rec)) > 0) {
+		if (maildir_sync_record(ibox, sync_ctx) < 0) {
 			ret = -1;
 			break;
 		}
@@ -821,7 +837,7 @@
 
 	if (ret < 0) {
 		mail_index_transaction_rollback(trans);
-		mail_index_sync_rollback(sync_ctx.sync_ctx);
+		mail_index_sync_rollback(sync_ctx->sync_ctx);
 	} else {
 		uint32_t seq;
 		uoff_t offset;
@@ -832,7 +848,7 @@
 			ibox->commit_log_file_seq = seq;
 			ibox->commit_log_file_offset = offset;
 		}
-		if (mail_index_sync_commit(sync_ctx.sync_ctx) < 0)
+		if (mail_index_sync_commit(sync_ctx->sync_ctx) < 0)
 			ret = -1;
 	}
 
@@ -843,6 +859,7 @@
 		mail_storage_set_index_error(ibox);
 	}
 
+	i_free(sync_ctx);
 	return ret;
 }
 
@@ -860,26 +877,62 @@
 		new_changed = cur_changed = TRUE;
 	}
 
-	/* we have to lock uidlist immediately, otherwise there's race
-	   conditions with other processes who might write older maildir
-	   file list into uidlist.
+	/*
+	   Locking, locking, locking.. Wasn't maildir supposed to be lockless?
 
-	   alternative would be to lock it when new files are found, but
-	   the directory scans _must_ be restarted then.
+	   We can get here either as beginning a real maildir sync, or when
+	   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 is needed only for figuring out UIDs for newly seen files,
+	   so theoretically we wouldn't need to lock it unless there are new
+	   files. It has a few problems though, assuming the index lock didn't
+	   already protect it (eg. in-memory indexes):
+
+	   1. Just because you see a new file which doesn't exist in uidlist
+	   file, doesn't mean that the file really exists anymore, or that
+	   your readdir() lists all new files. Meaning that this is possible:
+
+	     A: opendir(), readdir() -> new file ...
+	     -- new files are written to the maildir --
+	     B: opendir(), readdir() -> new file, lock uidlist,
+		readdir() -> another new file, rewrite uidlist, unlock
+	     A: ... lock uidlist, readdir() -> nothing left, rewrite uidlist,
+		unlock
+
+	   The second time running A didn't see the two new files. To handle
+	   this correctly, it must not remove the new unseen files from
+	   uidlist. This is possible to do, but adds extra complexity.
+
+	   2. If another process is rename()ing files while we are
+	   readdir()ing, it's possible that readdir() never lists some files,
+	   causing Dovecot to assume they were expunged. In next sync they
+	   would show up again, but client could have already been notified of
+	   that and they would show up under new UIDs, so the damage is
+	   already done.
+
+	   Both of the problems can be avoided if we simply lock the uidlist
+	   before syncing and keep it until sync is finished. Typically this
+	   would happen in any case, as there is the index lock..
+
+	   The second case is still a problem with external changes though,
+	   because maildir doesn't require any kind of locking. Luckily this
+	   problem rarely happens except under high amount of modifications.
+	*/
 
-	   if we got here through maildir_sync_last_commit(), we can't sync
-	   index as it's already being synced. so, don't try locking uidlist
-	   either, we only want to find new filename for some mail.
-	   */
 	if (!ctx->ibox->syncing_commit) {
-		if ((ret = maildir_uidlist_try_lock(ctx->ibox->uidlist)) < 0)
-			return ret;
-		if (ret == 0 && !forced) {
-			/* we didn't get a lock, don't do syncing unless we
-			   really want to check for expunges or renames. new
-			   files won't be added. */
-			return 0;
-		}
+		ctx->index_sync_ctx = maildir_sync_index_begin(ctx->ibox);
+		if (ctx->index_sync_ctx == NULL)
+			return -1;
+	}
+
+	if ((ret = maildir_uidlist_lock(ctx->ibox->uidlist)) <= 0) {
+		/* failure / timeout. if forced is TRUE, we could still go
+		   forward and check only for renamed files, but is it worth
+		   the trouble? .. */
+		return ret;
 	}
 
 	ctx->partial = !cur_changed;
@@ -896,8 +949,12 @@
 	/* finish uidlist syncing, but keep it still locked */
 	maildir_uidlist_sync_finish(ctx->uidlist_sync_ctx);
 	if (!ctx->ibox->syncing_commit) {
-		if (maildir_sync_index(ctx->ibox, ctx->partial) < 0)
+		if (maildir_sync_index_finish(ctx->index_sync_ctx,
+					      ctx->partial) < 0) {
+			ctx->index_sync_ctx = NULL;
 			return -1;
+		}
+		ctx->index_sync_ctx = NULL;
 	}
 
 	ret = maildir_uidlist_sync_deinit(ctx->uidlist_sync_ctx);