core.fsync: new option to harden references

When writing both loose and packed references to disk we first create a
lockfile, write the updated values into that lockfile, and on commit we
rename the file into place. According to filesystem developers, this
behaviour is broken because applications should always sync data to disk
before doing the final rename to ensure data consistency [1][2][3]. If
applications fail to do this correctly, a hard crash of the machine can
easily result in corrupted on-disk data.

This kind of corruption can in fact be easily observed with Git when the
machine hard-resets shortly after writing references to disk. On
machines with ext4, this will likely lead to the "empty files" problem:
the file has been renamed, but its data has not been synced to disk. The
result is that the reference is corrupt, and in the worst case this can
lead to data loss.

Implement a new option to harden references so that users and admins can
avoid this scenario by syncing locked loose and packed references to
disk before we rename them into place.

[1]: https://thunk.org/tytso/blog/2009/03/15/dont-fear-the-fsync/
[2]: https://btrfs.wiki.kernel.org/index.php/FAQ (What are the crash guarantees of overwrite-by-rename)
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/ext4.rst (see auto_da_alloc)

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 9a3ad71e9e7ee9b105f11e6e680613c80256127e..9da3e5d88f6a26b0ff36413ddfdfbd745f6e6897 100644 (file)
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -575,6 +575,7 @@ but risks losing recent work in the event of an unclean system shutdown.
 * `index` hardens the index when it is modified.
 * `objects` is an aggregate option that is equivalent to
   `loose-object,pack`.
+* `reference` hardens references modified in the repo.
 * `derived-metadata` is an aggregate option that is equivalent to
   `pack-metadata,commit-graph`.
 * `committed` is an aggregate option that is currently equivalent to

diff --git a/cache.h b/cache.h
index cde0900d0523dc227b95219b57f24a0db0089d31..033e5b07792650bdc951b67af4c8be2dc31e0e8d 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1005,6 +1005,7 @@ enum fsync_component {
        FSYNC_COMPONENT_PACK_METADATA           = 1 << 2,
        FSYNC_COMPONENT_COMMIT_GRAPH            = 1 << 3,
        FSYNC_COMPONENT_INDEX                   = 1 << 4,
+       FSYNC_COMPONENT_REFERENCE               = 1 << 5,
 };
 
 #define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \
@@ -1017,7 +1018,8 @@ enum fsync_component {
                                  FSYNC_COMPONENTS_DERIVED_METADATA | \
                                  ~FSYNC_COMPONENT_LOOSE_OBJECT)
 
-#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS)
+#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS | \
+                                   FSYNC_COMPONENT_REFERENCE)
 
 #define FSYNC_COMPONENTS_ADDED (FSYNC_COMPONENTS_COMMITTED | \
                                FSYNC_COMPONENT_INDEX)
@@ -1026,7 +1028,8 @@ enum fsync_component {
                              FSYNC_COMPONENT_PACK | \
                              FSYNC_COMPONENT_PACK_METADATA | \
                              FSYNC_COMPONENT_COMMIT_GRAPH | \
-                             FSYNC_COMPONENT_INDEX)
+                             FSYNC_COMPONENT_INDEX | \
+                             FSYNC_COMPONENT_REFERENCE)
 
 /*
  * A bitmask indicating which components of the repo should be fsynced.

diff --git a/config.c b/config.c
index eb75f653382b16d3dc45ec7241bae67be3bd91eb..3c9b6b589ab581ca2711f82922633d3e07ab63d1 100644 (file)
--- a/config.c
+++ b/config.c
@@ -1333,6 +1333,7 @@ static const struct fsync_component_name {
        { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
        { "index", FSYNC_COMPONENT_INDEX },
        { "objects", FSYNC_COMPONENTS_OBJECTS },
+       { "reference", FSYNC_COMPONENT_REFERENCE },
        { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA },
        { "committed", FSYNC_COMPONENTS_COMMITTED },
        { "added", FSYNC_COMPONENTS_ADDED },

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f59589d6cce4594e323ce12fdcde98f2dccd95ee..6521ee8af500893852858dc2486bb1b9fd0cc74b 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1787,6 +1787,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
        fd = get_lock_file_fd(&lock->lk);
        if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
            write_in_full(fd, &term, 1) < 0 ||
+           fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&lock->lk)) < 0 ||
            close_ref_gently(lock) < 0) {
                strbuf_addf(err,
                            "couldn't write '%s'", get_lock_file_path(&lock->lk));

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 27dd8c3922b7039323bbfcfdf7f3b77c65ef1681..9d704ccd3e630fd2af44765159a61c140e48d785 100644 (file)
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1262,7 +1262,8 @@ static int write_with_updates(struct packed_ref_store *refs,
                goto error;
        }
 
-       if (close_tempfile_gently(refs->tempfile)) {
+       if (fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||
+           close_tempfile_gently(refs->tempfile)) {
                strbuf_addf(err, "error closing file %s: %s",
                            get_tempfile_path(refs->tempfile),
                            strerror(errno));