@matheusbsbs / matheus-bagliano / commits / bc22d845c4

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>

Patrick Steinhardt committed Mar 11, 2022 at 10:58 UTC bc22d845c4328f5bd896d019b3729f776ad4be4c
5 files changed +10 -3
Documentation/config/core.txt
+1
@@ -575,6 +575,7 @@ but risks losing recent work in the event of an unclean system shutdown.
575 * `index` hardens the index when it is modified.
576 * `objects` is an aggregate option that is equivalent to
577 `loose-object,pack`.
578 +* `reference` hardens references modified in the repo.
579 * `derived-metadata` is an aggregate option that is equivalent to
580 `pack-metadata,commit-graph`.
581 * `committed` is an aggregate option that is currently equivalent to
cache.h
+5 -2
@@ -1005,6 +1005,7 @@ enum fsync_component {
1005 FSYNC_COMPONENT_PACK_METADATA = 1 << 2,
1006 FSYNC_COMPONENT_COMMIT_GRAPH = 1 << 3,
1007 FSYNC_COMPONENT_INDEX = 1 << 4,
1008 + FSYNC_COMPONENT_REFERENCE = 1 << 5,
1009 };
1010
1011 #define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \
@@ -1017,7 +1018,8 @@ enum fsync_component {
1018 FSYNC_COMPONENTS_DERIVED_METADATA | \
1019 ~FSYNC_COMPONENT_LOOSE_OBJECT)
1020
1020 -#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS)
1021 +#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS | \
1022 + FSYNC_COMPONENT_REFERENCE)
1023
1024 #define FSYNC_COMPONENTS_ADDED (FSYNC_COMPONENTS_COMMITTED | \
1025 FSYNC_COMPONENT_INDEX)
@@ -1026,7 +1028,8 @@ enum fsync_component {
1028 FSYNC_COMPONENT_PACK | \
1029 FSYNC_COMPONENT_PACK_METADATA | \
1030 FSYNC_COMPONENT_COMMIT_GRAPH | \
1029 - FSYNC_COMPONENT_INDEX)
1031 + FSYNC_COMPONENT_INDEX | \
1032 + FSYNC_COMPONENT_REFERENCE)
1033
1034 /*
1035 * A bitmask indicating which components of the repo should be fsynced.
config.c
+1
@@ -1333,6 +1333,7 @@ static const struct fsync_component_name {
1333 { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
1334 { "index", FSYNC_COMPONENT_INDEX },
1335 { "objects", FSYNC_COMPONENTS_OBJECTS },
1336 + { "reference", FSYNC_COMPONENT_REFERENCE },
1337 { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA },
1338 { "committed", FSYNC_COMPONENTS_COMMITTED },
1339 { "added", FSYNC_COMPONENTS_ADDED },
refs/files-backend.c
+1
@@ -1787,6 +1787,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
1787 fd = get_lock_file_fd(&lock->lk);
1788 if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
1789 write_in_full(fd, &term, 1) < 0 ||
1790 + fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&lock->lk)) < 0 ||
1791 close_ref_gently(lock) < 0) {
1792 strbuf_addf(err,
1793 "couldn't write '%s'", get_lock_file_path(&lock->lk));
refs/packed-backend.c
+2 -1
@@ -1262,7 +1262,8 @@ static int write_with_updates(struct packed_ref_store *refs,
1262 goto error;
1263 }
1264
1265 - if (close_tempfile_gently(refs->tempfile)) {
1265 + if (fsync_component(FSYNC_COMPONENT_REFERENCE, get_tempfile_fd(refs->tempfile)) ||
1266 + close_tempfile_gently(refs->tempfile)) {
1267 strbuf_addf(err, "error closing file %s: %s",
1268 get_tempfile_path(refs->tempfile),
1269 strerror(errno));