[PATCH v2 15/20] insert: fsync new directory after rename

[Mark Walters]

On Sun, 25 Nov 2012, Peter Wang <novalazy at gmail.com> wrote:
> After moving the file from the 'tmp' to the 'new' directory,
> fsync on the 'new' directory for durability.
> ---
>  notmuch-insert.c | 39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index f09c579..831b322 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -143,10 +143,10 @@ maildir_create_folder (void *ctx, const char *dir)
>  /* Open a unique file in the Maildir 'tmp' directory.
>   * Returns the file descriptor on success, or -1 on failure.
>   * On success, file paths into the 'tmp' and 'new' directories are returned
> - * via tmppath and newpath. */
> + * via tmppath and newpath, and the path of the 'new' directory in newdir. */

I found this comment very difficult to understand since it looks like
the 'new' directory gets returned twice. Perhaps something like

"On success, file paths for the message in the 'tmp' and 'new'
directories are returned via tmppath and newpath, and the path of the
'new' directory itself in newdir."


Best wishes

Mark

>  static int
>  maildir_open_tmp_file (void *ctx, const char *dir,
> -		       char **tmppath, char **newpath)
> +		       char **tmppath, char **newpath, char **newdir)
>  {
>      pid_t pid;
>      char hostname[256];
> @@ -183,8 +183,9 @@ maildir_open_tmp_file (void *ctx, const char *dir,
>  	return -1;
>      }
>  
> +    *newdir = talloc_asprintf (ctx, "%s/new", dir);
>      *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
> -    if (! *newpath) {
> +    if (! *newdir || ! *newpath) {
>  	fprintf (stderr, "Out of memory\n");
>  	close (fd);
>  	unlink (*tmppath);
> @@ -204,14 +205,31 @@ maildir_open_tmp_file (void *ctx, const char *dir,
>   * http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery
>   */
>  static notmuch_bool_t
> -maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
> +maildir_move_tmp_to_new (const char *tmppath, const char *newpath,
> +		         const char *newdir)
>  {
> +    notmuch_bool_t ret;
> +    int fd;
> +
>      if (rename (tmppath, newpath) != 0) {
>  	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
>  	return FALSE;
>      }
>  
> -    return TRUE;
> +    /* Sync the 'new' directory after rename for durability. */
> +    ret = TRUE;
> +    fd = open (newdir, O_RDONLY);
> +    if (fd == -1) {
> +	fprintf (stderr, "Error: open() dir failed: %s\n", strerror (errno));
> +	ret = FALSE;
> +    }
> +    if (ret && fsync (fd) != 0) {
> +	fprintf (stderr, "Error: fsync() dir failed: %s\n", strerror (errno));
> +	ret = FALSE;
> +    }
> +    if (fd != -1)
> +	close (fd);
> +    return ret;
>  }
>  
>  /* Copy the contents of fdin into fdout. */
> @@ -307,6 +325,12 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
>  
>      notmuch_message_thaw (message);
>  
> +    /* notmuch_message_tags_to_maildir_flags may rename the message file
> +     * once more, and does so without fsyncing the directory afterwards.
> +     * rename() is atomic so after a crash the file should appear under
> +     * the old or new name. notmuch new should be able to rename the file
> +     * again if required, so another fsync is not required, I think.
> +     */
>      notmuch_message_tags_to_maildir_flags (message);
>  
>      notmuch_message_destroy (message);
> @@ -321,10 +345,11 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
>  {
>      char *tmppath;
>      char *newpath;
> +    char *newdir;
>      int fdout;
>      notmuch_bool_t ret;
>  
> -    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath);
> +    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
>      if (fdout < 0) {
>  	return FALSE;
>      }
> @@ -335,7 +360,7 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
>      }
>      close (fdout);
>      if (ret) {
> -	ret = maildir_move_tmp_to_new (tmppath, newpath);
> +	ret = maildir_move_tmp_to_new (tmppath, newpath, newdir);
>      }
>      if (!ret) {
>  	unlink (tmppath);
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch