Re: fsync in glib/gio

[Alexander Larsson <alexl redhat com>]

On Fri, 2009-03-13 at 18:45 +0100, Alexander Larsson wrote:
> One compromise we could make it to only fsync in the case we're actually
> overwriting an existing file. This would mean that we don't risk loosing
> both the old and the new version of the file, you only lose "new" files.
> This case is far less common so the performance aspects are not as bad,
> and its also get rids of the worst failure mode.

Attached is a patch that does this. It adds a
G_FILE_CREATE_SYNC_ON_CLOSE flag, and turns it on by default when
handling an overwrite. The same thing happens in g_file_set_contents().

I think we need to do this, at the minimum, because even if there are
some ext4 patches to fix up this issue they can be disabled, and from
what I understand XFS and btrfs have similar issues.

The question is, do we want this or the full patch?

Index: gio/glocalfileoutputstream.c
===================================================================
--- gio/glocalfileoutputstream.c	(revision 7974)
+++ gio/glocalfileoutputstream.c	(working copy)
@@ -69,6 +69,7 @@ struct _GLocalFileOutputStreamPrivate {
   char *original_filename;
   char *backup_filename;
   char *etag;
+  gboolean sync_on_close;
   int fd;
 };
 
@@ -81,7 +82,7 @@ static gboolean   g_local_file_output_st
 							   GCancellable       *cancellable,
 							   GError            **error);
 static GFileInfo *g_local_file_output_stream_query_info   (GFileOutputStream  *stream,
-							   char               *attributes,
+							   const char         *attributes,
 							   GCancellable       *cancellable,
 							   GError            **error);
 static char *     g_local_file_output_stream_get_etag     (GFileOutputStream  *stream);
@@ -190,6 +191,22 @@ g_local_file_output_stream_close (GOutpu
 
   file = G_LOCAL_FILE_OUTPUT_STREAM (stream);
 
+#ifdef HAVE_FSYNC
+  if (file->priv->sync_on_close)
+    {
+      if (fsync (file->priv->fd) != 0)
+	{
+          int errsv = errno;
+	  
+	  g_set_error (error, G_IO_ERROR,
+		       g_io_error_from_errno (errno),
+		       _("Error writing to file: %s"),
+		       g_strerror (errsv));
+	  goto err_out;
+	}
+    }
+#endif
+ 
 #ifdef G_OS_WIN32
 
   /* Must close before renaming on Windows, so just do the close first
@@ -459,7 +476,7 @@ g_local_file_output_stream_truncate (GFi
 
 static GFileInfo *
 g_local_file_output_stream_query_info (GFileOutputStream  *stream,
-				       char               *attributes,
+				       const char         *attributes,
 				       GCancellable       *cancellable,
 				       GError            **error)
 {
@@ -517,6 +534,8 @@ _g_local_file_output_stream_create  (con
   
   stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
   stream->priv->fd = fd;
+  if (flags & G_FILE_CREATE_SYNC_ON_CLOSE)
+    stream->priv->sync_on_close = TRUE;
   return G_FILE_OUTPUT_STREAM (stream);
 }
 
@@ -562,6 +581,8 @@ _g_local_file_output_stream_append  (con
   
   stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
   stream->priv->fd = fd;
+  if (flags & G_FILE_CREATE_SYNC_ON_CLOSE)
+    stream->priv->sync_on_close = TRUE;
   
   return G_FILE_OUTPUT_STREAM (stream);
 }
@@ -992,6 +1013,7 @@ _g_local_file_output_stream_replace (con
 
   if (fd == -1 && errno == EEXIST)
     {
+      flags |= G_FILE_CREATE_SYNC_ON_CLOSE;
       /* The file already exists */
       fd = handle_overwrite_open (filename, etag, create_backup, &temp_file,
 				  flags, cancellable, error);
@@ -1022,6 +1044,8 @@ _g_local_file_output_stream_replace (con
  
   stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
   stream->priv->fd = fd;
+  if (flags & G_FILE_CREATE_SYNC_ON_CLOSE)
+    stream->priv->sync_on_close = TRUE;
   stream->priv->tmp_filename = temp_file;
   if (create_backup)
     stream->priv->backup_filename = create_backup_filename (filename);
Index: gio/gioenums.h
===================================================================
--- gio/gioenums.h	(revision 7974)
+++ gio/gioenums.h	(working copy)
@@ -164,13 +164,17 @@ typedef enum {
  *    You can think of it as "unlink destination" before
  *    writing to it, although the implementation may not
  *    be exactly like that. Since 2.20
+ * @G_FILE_CREATE_SYNC_ON_CLOSE: If possible, try to ensure
+ *    that all data is on disk before returning. For local
+ *    files this means calling fsync() before close.
  *
  * Flags used when an operation may create a file.
  */
 typedef enum {
   G_FILE_CREATE_NONE    = 0,
   G_FILE_CREATE_PRIVATE = (1 << 0),
-  G_FILE_CREATE_REPLACE_DESTINATION = (1 << 1)
+  G_FILE_CREATE_REPLACE_DESTINATION = (1 << 1),
+  G_FILE_CREATE_SYNC_ON_CLOSE = (1 << 2)
 } GFileCreateFlags;
 
 
Index: configure.in
===================================================================
--- configure.in	(revision 7974)
+++ configure.in	(working copy)
@@ -563,6 +563,7 @@ AC_CHECK_FUNCS(mmap)
 AC_CHECK_FUNCS(posix_memalign)
 AC_CHECK_FUNCS(memalign)
 AC_CHECK_FUNCS(valloc)
+AC_CHECK_FUNCS(fsync)
 
 AC_CHECK_FUNCS(atexit on_exit)
 
Index: glib/gfileutils.c
===================================================================
--- glib/gfileutils.c	(revision 7974)
+++ glib/gfileutils.c	(working copy)
@@ -869,6 +869,7 @@ static gchar *
 write_to_temp_file (const gchar  *contents,
 		    gssize        length,
 		    const gchar  *template,
+		    gboolean      sync_on_close,
 		    GError      **err)
 {
   gchar *tmp_name;
@@ -942,11 +943,47 @@ write_to_temp_file (const gchar  *conten
 	  goto out;
 	}
     }
-   
+
+  errno = 0;
+  if (fflush (file) != 0)
+    { 
+      save_errno = errno;
+      
+      g_set_error (err,
+		   G_FILE_ERROR,
+		   g_file_error_from_errno (save_errno),
+		   _("Failed to write file '%s': fflush() failed: %s"),
+		   display_name, 
+		   g_strerror (save_errno));
+
+      g_unlink (tmp_name);
+      
+      goto out;
+    }
+  
+#ifdef HAVE_FSYNC
+  errno = 0;
+  if (sync_on_close && fsync (fileno (file)) != 0)
+    { 
+      save_errno = errno;
+      
+      g_set_error (err,
+		   G_FILE_ERROR,
+		   g_file_error_from_errno (save_errno),
+		   _("Failed to write file '%s': fsync() failed: %s"),
+		   display_name, 
+		   g_strerror (save_errno));
+
+      g_unlink (tmp_name);
+      
+      goto out;
+    }
+#endif
+  
   errno = 0;
   if (fclose (file) == EOF)
     { 
-      save_errno = 0;
+      save_errno = errno;
       
       g_set_error (err,
 		   G_FILE_ERROR,
@@ -1027,7 +1064,9 @@ g_file_set_contents (const gchar  *filen
   if (length == -1)
     length = strlen (contents);
 
-  tmp_filename = write_to_temp_file (contents, length, filename, error);
+  tmp_filename = write_to_temp_file (contents, length, filename,
+				     g_file_test (filename, G_FILE_TEST_EXISTS),
+				     error);
   
   if (!tmp_filename)
     {