Re: [PATCH v2] core.fsync: new option to harden references

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Mar 11, 2022 at 10:58:59AM +0100, Patrick Steinhardt wrote:
> 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.

In 't5541-http-push-smart.sh' there is a test case called 'push 2000
tags over http', which does pretty much what it's title says.  This
patch makes that test case significantly slower.


diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ca50f8b18..d7e94cb791 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -415,7 +415,7 @@ test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '
 	  sort |
 	  sed "s|.*|$sha1 refs/tags/really-long-tag-name-&|" \
 	  >.git/packed-refs &&
-	run_with_limited_cmdline git push --mirror
+	run_with_limited_cmdline /usr/bin/time git push --mirror
 '
 
 test_expect_success GPG 'push with post-receive to inspect certificate' '

Before this patch (bc22d845c4^) 'time' reports:

  3.62user 0.03system 0:03.83elapsed 95%CPU (0avgtext+0avgdata 11904maxresident)k
  0inputs+312outputs (0major+4597minor)pagefaults 0swaps

With this patch (bc22d845c4):

  3.56user 0.04system 0:33.60elapsed 10%CPU (0avgtext+0avgdata 11832maxresident)k
  0inputs+320outputs (0major+4578minor)pagefaults 0swaps

And the total runtime of the whole test script increases from 8-9s to
37-39s.


I wonder whether we should relax the fsync options for this test case.

> 
> [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>
> ---
> 
> Hi,
> 
> here's my updated patch series which implements syncing of refs. It
> applies on top of Neeraj's v6 of "A design for future-proofing fsync()
> configuration".
> 
> I've simplified these patches a bit:
> 
>     - I don't distinguishing between "loose" and "packed" refs anymore.
>       I agree with Junio that it's probably not worth it, but we can
>       still reintroduce the split at a later point without breaking
>       backwards compatibility if the need comes up.
> 
>     - I've simplified the way loose refs are written to disk so that we
>       now sync them when before we close their files. The previous
>       implementation I had was broken because we tried to sync after
>       closing.
> 
> Because this really only changes a few lines of code I've also decided
> to squash together the patches into a single one.
> 
> Patrick
> 
>  Documentation/config/core.txt | 1 +
>  cache.h                       | 7 +++++--
>  config.c                      | 1 +
>  refs/files-backend.c          | 1 +
>  refs/packed-backend.c         | 3 ++-
>  5 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 37105a7be4..812cca7de7 100644
> --- 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 cde0900d05..033e5b0779 100644
> --- 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 eb75f65338..3c9b6b589a 100644
> --- 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 f59589d6cc..6521ee8af5 100644
> --- 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 27dd8c3922..9d704ccd3e 100644
> --- 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));
> -- 
> 2.35.1
>