After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 637544 - Skip fsync() on btrfs
Skip fsync() on btrfs
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-12-19 00:19 UTC by Allison Karlitskaya (desrt)
Modified: 2010-12-21 01:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] Skip fsync() on btrfs (1.43 KB, patch)
2010-12-19 00:21 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2010-12-19 00:19:52 UTC
Currently, g_file_set_contents() calls fsync() before calling rename().  We do this to ensure that the new data has actually made it to disk before we replace the old file.

On btrfs, we don't need to do that (as implemented on Linux, at least).

(quoting here for permanence)

"""
   What are the crash guarantees of overwrite-by-rename?

   Overwriting an existing file using a rename is atomic. That means that
   either the old content of the file is there or the new content. A
   sequence like this:

     echo "oldcontent" > file

     # make sure oldcontent is on disk
     sync

     echo "newcontent" > file.tmp
     mv -f file.tmp file

     # *crash*

   Will give either

    1. file contains "newcontent"; file.tmp does not exist

    2. file contains "oldcontent"; file.tmp may contain "newcontent", be
       zero-length or not exists at all.
"""

https://btrfs.wiki.kernel.org/index.php/FAQ#What_are_the_crash_guarantees_of_overwrite-by-rename.3F

I've done a bit of experimentation that indicates exactly what you'd probably expect: skipping the fsync() is a huge performance win.

On my system, this program:

"""
   #include <glib.h>

   int
   main (void)
   {
     gint i;

     for (i = 0; i < 1000; i++)
       {
         gchar buffer[10];

         g_sprintf (buffer, "%d", i);
         g_file_set_contents ("myfile", buffer, -1, NULL);
       }

     return 0;
   }
"""

goes from 48 seconds to 0.3 seconds.

Another case: dconf currently completely replaces the database on every write and does so using g_file_set_contents().  Not calling fsync() reduces the time taken to do 1 dconf write from 64 milliseconds to 3.5ms, round trip.
Comment 1 Allison Karlitskaya (desrt) 2010-12-19 00:21:36 UTC
Created attachment 176691 [details] [review]
[PATCH] Skip fsync() on btrfs

The patch intentionally applies itself only on Linux -- I have no information about the btrfs implementations on other systems (if they exist at all), so it's safer not to make assumptions.
Comment 2 Allison Karlitskaya (desrt) 2010-12-19 07:20:43 UTC
The existing code for g_file_set_contents() only does the fsync in the case that the destination file already exists.  This is good for ensuring that we don't overwrite an existing file with invalid data but it doesn't do much to help us with avoiding creating a new zero-byte file.

The patch here does not improve (or worsen) the situation and the btrfs wiki specifically mentions that btrfs provides no additional guarantees with respect to this case (a rename that doesn't replace another file).

If we decide that this is something that we want to fix for the general case then the patch here should also be modified to check for this case.
Comment 3 Allison Karlitskaya (desrt) 2010-12-19 07:23:41 UTC
commit d20a188b1250ab3cf211d684429127d99378e886
Author: Alexander Larsson <alexl@redhat.com>
Date:   Tue Aug 11 20:22:51 2009 +0200

    Only fsync if the existing file is > 0 bytes
    
    This means we don't sync in the case where we created an (empty)
    temp file and now replace it with the data.
    
    This fixes (among other things) the performance of trashing files.
Comment 4 Colin Walters 2010-12-19 16:04:55 UTC
See also
https://bugzilla.gnome.org/show_bug.cgi?id=575555