git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/3] core.fsync: introduce granular fsync control

From
Patrick Steinhardt <ps@pks.im>
Date
Dec 7, 2021, 11:53 UTC
Message-ID
<Ya9LPOseu8geBi4v@ncase>
In-Reply-To
<ff80a94bf9add8a6fabcd5146e5177edf5e35e49.1638845211.git.gitgitgadget@gmail.com>
On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wrote:
> From: Neeraj Singh <neerajsi@microsoft.com>
[snip]
Show 33 quoted lines
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index c23d01de7dc..c32534c13b4 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1286,7 +1286,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
>  			    nr_objects - nr_objects_initial);
>  		stop_progress_msg(&progress, msg.buf);
>  		strbuf_release(&msg);
> -		finalize_hashfile(f, tail_hash, 0);
> +		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
>  		hashcpy(read_hash, pack_hash);
>  		fixup_pack_header_footer(output_fd, pack_hash,
>  					 curr_pack, nr_objects,
> @@ -1508,7 +1508,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  	if (!from_stdin) {
>  		close(input_fd);
>  	} else {
> -		fsync_or_die(output_fd, curr_pack_name);
> +		fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
>  		err = close(output_fd);
>  		if (err)
>  			die_errno(_("error while closing pack file"));
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 857be7826f3..916c55d6ce9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1204,11 +1204,13 @@ static void write_pack_file(void)
>  		 * If so, rewrite it like in fast-import
>  		 */
>  		if (pack_to_stdout) {
> -			finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
> +			finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
> +					  CSUM_HASH_IN_STREAM | CSUM_CLOSE);

It doesn't have any effect here given that we don't sync at all when writing to stdout, but I wonder whether we should set up the component correctly regardless of that such that it makes for a less confusing read.

[snip]
Show 100 quoted lines
> diff --git a/config.c b/config.c
> index c3410b8a868..29c867aab03 100644
> --- a/config.c
> +++ b/config.c
> @@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *value)
>  	return -1;
>  }
>  
> +static const struct fsync_component_entry {
> +	const char *name;
> +	enum fsync_component component_bits;
> +} fsync_component_table[] = {
> +	{ "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
> +	{ "pack", FSYNC_COMPONENT_PACK },
> +	{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
> +	{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
> +	{ "objects", FSYNC_COMPONENTS_OBJECTS },
> +	{ "default", FSYNC_COMPONENTS_DEFAULT },
> +	{ "all", FSYNC_COMPONENTS_ALL },
> +};
> +
> +static enum fsync_component parse_fsync_components(const char *var, const char *string)
> +{
> +	enum fsync_component output = 0;
> +
> +	if (!strcmp(string, "none"))
> +		return output;
> +
> +	while (string) {
> +		int i;
> +		size_t len;
> +		const char *ep;
> +		int negated = 0;
> +		int found = 0;
> +
> +		string = string + strspn(string, ", \t\n\r");
> +		ep = strchrnul(string, ',');
> +		len = ep - string;
> +
> +		if (*string == '-') {
> +			negated = 1;
> +			string++;
> +			len--;
> +			if (!len)
> +				warning(_("invalid value for variable %s"), var);
> +		}
> +
> +		if (!len)
> +			break;
> +
> +		for (i = 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
> +			const struct fsync_component_entry *entry = &fsync_component_table[i];
> +
> +			if (strncmp(entry->name, string, len))
> +				continue;
> +
> +			found = 1;
> +			if (negated)
> +				output &= ~entry->component_bits;
> +			else
> +				output |= entry->component_bits;
> +		}
> +
> +		if (!found) {
> +			char *component = xstrndup(string, len);
> +			warning(_("unknown %s value '%s'"), var, component);
> +			free(component);
> +		}
> +
> +		string = ep;
> +	}
> +
> +	return output;
> +}
> +
>  int git_parse_maybe_bool(const char *value)
>  {
>  	int v = git_parse_maybe_bool_text(value);
> @@ -1490,6 +1557,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "core.fsync")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		fsync_components = parse_fsync_components(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.fsyncmethod")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> @@ -1503,7 +1577,7 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	if (!strcmp(var, "core.fsyncobjectfiles")) {
> -		fsync_object_files = git_config_bool(var, value);
> +		warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
>  		return 0;
>  	}

Shouldn't we continue to support this for now such that users can migrate from the old, deprecated value first before we start to ignore it?

Patrick
Show 152 quoted lines
> diff --git a/csum-file.c b/csum-file.c
> index 26e8a6df44e..59ef3398ca2 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -58,7 +58,8 @@ static void free_hashfile(struct hashfile *f)
>  	free(f);
>  }
>  
> -int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags)
> +int finalize_hashfile(struct hashfile *f, unsigned char *result,
> +		      enum fsync_component component, unsigned int flags)
>  {
>  	int fd;
>  
> @@ -69,7 +70,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl
>  	if (flags & CSUM_HASH_IN_STREAM)
>  		flush(f, f->buffer, the_hash_algo->rawsz);
>  	if (flags & CSUM_FSYNC)
> -		fsync_or_die(f->fd, f->name);
> +		fsync_component_or_die(component, f->fd, f->name);
>  	if (flags & CSUM_CLOSE) {
>  		if (close(f->fd))
>  			die_errno("%s: sha1 file error on close", f->name);
> diff --git a/csum-file.h b/csum-file.h
> index 291215b34eb..0d29f528fbc 100644
> --- a/csum-file.h
> +++ b/csum-file.h
> @@ -1,6 +1,7 @@
>  #ifndef CSUM_FILE_H
>  #define CSUM_FILE_H
>  
> +#include "cache.h"
>  #include "hash.h"
>  
>  struct progress;
> @@ -38,7 +39,7 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);
>  struct hashfile *hashfd(int fd, const char *name);
>  struct hashfile *hashfd_check(const char *name);
>  struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp);
> -int finalize_hashfile(struct hashfile *, unsigned char *, unsigned int);
> +int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);
>  void hashwrite(struct hashfile *, const void *, unsigned int);
>  void hashflush(struct hashfile *f);
>  void crc32_begin(struct hashfile *);
> diff --git a/environment.c b/environment.c
> index f9140e842cf..09905adecf9 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -42,6 +42,7 @@ const char *git_hooks_path;
>  int zlib_compression_level = Z_BEST_SPEED;
>  int pack_compression_level = Z_DEFAULT_COMPRESSION;
>  enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
> +enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
>  size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
>  size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
>  size_t delta_base_cache_limit = 96 * 1024 * 1024;
> diff --git a/midx.c b/midx.c
> index 837b46b2af5..882f91f7d57 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1406,7 +1406,8 @@ static int write_midx_internal(const char *object_dir,
>  	write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);
>  	write_chunkfile(cf, &ctx);
>  
> -	finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);
> +	finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,
> +			  CSUM_FSYNC | CSUM_HASH_IN_STREAM);
>  	free_chunkfile(cf);
>  
>  	if (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))
> diff --git a/object-file.c b/object-file.c
> index eb972cdccd2..9d9c4a39e85 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1809,8 +1809,7 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,
>  /* Finalize a file on disk, and close it. */
>  static void close_loose_object(int fd)
>  {
> -	if (fsync_object_files)
> -		fsync_or_die(fd, "loose object file");
> +	fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd, "loose object file");
>  	if (close(fd) != 0)
>  		die_errno(_("error when closing loose object file"));
>  }
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 9c55c1531e1..c16e43d1669 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -719,7 +719,8 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
>  	if (options & BITMAP_OPT_HASH_CACHE)
>  		write_hash_cache(f, index, index_nr);
>  
> -	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
> +	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
> +			  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
>  
>  	if (adjust_shared_perm(tmp_file.buf))
>  		die_errno("unable to make temporary bitmap file readable");
> diff --git a/pack-write.c b/pack-write.c
> index a5846f3a346..51812cb1299 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -159,9 +159,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
>  	}
>  
>  	hashwrite(f, sha1, the_hash_algo->rawsz);
> -	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
> -				    ((opts->flags & WRITE_IDX_VERIFY)
> -				    ? 0 : CSUM_FSYNC));
> +	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
> +			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
> +			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
>  	return index_name;
>  }
>  
> @@ -281,8 +281,9 @@ const char *write_rev_file_order(const char *rev_name,
>  	if (rev_name && adjust_shared_perm(rev_name) < 0)
>  		die(_("failed to make %s readable"), rev_name);
>  
> -	finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_CLOSE |
> -				    ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
> +	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
> +			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
> +			  ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
>  
>  	return rev_name;
>  }
> @@ -390,7 +391,7 @@ void fixup_pack_header_footer(int pack_fd,
>  		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
>  	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
>  	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
> -	fsync_or_die(pack_fd, pack_name);
> +	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
>  }
>  
>  char *index_pack_lockfile(int ip_out, int *is_well_formed)
> diff --git a/read-cache.c b/read-cache.c
> index f3986596623..f3539681f49 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3060,7 +3060,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  			return -1;
>  	}
>  
> -	finalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);
> +	finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_NONE, CSUM_HASH_IN_STREAM);
>  	if (close_tempfile_gently(tempfile)) {
>  		error(_("could not close '%s'"), get_tempfile_path(tempfile));
>  		return -1;
> -- 
> gitgitgadget
>
Previous: Neeraj Singh via GitGitGadgetNext: Neeraj Singh
Message 13 of 122 in “A design for future-proofing fsync() configuration”
  1. 0/2 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Dec 4, 2021
  2. 1/2 fsync: add writeout-only mode for fsyncing repo dataNeeraj Singh via GitGitGadget, Dec 4, 2021
  3. Neeraj SinghDec 6, 2021
  4. 2/2 core.fsync: introduce granular fsync controlNeeraj Singh via GitGitGadget, Dec 4, 2021
  5. 0/3 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Dec 7, 2021
  6. 1/3 core.fsyncmethod: add writeout-only modeNeeraj Singh via GitGitGadget, Dec 7, 2021
  7. Patrick SteinhardtDec 7, 2021
  8. Ævar Arnfjörð BjarmasonDec 7, 2021
  9. Neeraj SinghDec 7, 2021
  10. Ævar Arnfjörð BjarmasonDec 7, 2021
  11. Neeraj SinghDec 7, 2021
  12. 2/3 core.fsync: introduce granular fsync controlNeeraj Singh via GitGitGadget, Dec 7, 2021
  13. Patrick SteinhardtDec 7, 2021
  14. Neeraj SinghDec 7, 2021
  15. Ævar Arnfjörð BjarmasonDec 7, 2021
  16. Neeraj SinghDec 7, 2021
  17. Ævar Arnfjörð BjarmasonDec 8, 2021
  18. Neeraj SinghDec 9, 2021
  19. Junio C HamanoDec 9, 2021
  20. Ævar Arnfjörð BjarmasonDec 9, 2021
  21. Neeraj SinghDec 9, 2021
  22. Neeraj SinghJan 18, 2022
  23. Ævar Arnfjörð BjarmasonJan 19, 2022
  24. Ævar Arnfjörð BjarmasonJan 19, 2022
  25. Neeraj SinghJan 28, 2022
  26. 3/3 core.fsync: new option to harden the indexNeeraj Singh via GitGitGadget, Dec 7, 2021
  27. Patrick SteinhardtDec 7, 2021
  28. Neeraj SinghDec 8, 2021
  29. 0/4 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Dec 9, 2021
  30. 1/4 core.fsyncmethod: add writeout-only modeNeeraj Singh via GitGitGadget, Dec 9, 2021
  31. 2/4 core.fsync: introduce granular fsync controlNeeraj Singh via GitGitGadget, Dec 9, 2021
  32. 3/4 core.fsync: new option to harden the indexNeeraj Singh via GitGitGadget, Dec 9, 2021
  33. 4/4 core.fsync: add a `derived-metadata` aggregate optionNeeraj Singh via GitGitGadget, Dec 9, 2021
  34. Neeraj SinghJan 8, 2022
  35. rsbecker@nexbridge.comJan 9, 2022
  36. Neeraj SinghJan 10, 2022
  37. 0/4 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Feb 1, 2022
  38. 1/4 core.fsyncmethod: add writeout-only modeNeeraj Singh via GitGitGadget, Feb 1, 2022
  39. 2/4 core.fsync: introduce granular fsync controlNeeraj Singh via GitGitGadget, Feb 1, 2022
  40. Junio C HamanoFeb 2, 2022
  41. Junio C HamanoFeb 2, 2022
  42. Neeraj SinghFeb 11, 2022
  43. Junio C HamanoFeb 11, 2022
  44. Neeraj SinghFeb 11, 2022
  45. Junio C HamanoFeb 11, 2022
  46. rsbecker@nexbridge.comFeb 12, 2022
  47. Patrick SteinhardtFeb 14, 2022
  48. Junio C HamanoFeb 14, 2022
  49. Patrick SteinhardtMar 9, 2022
  50. Ævar Arnfjörð BjarmasonMar 9, 2022
  51. Junio C HamanoMar 9, 2022
  52. Patrick SteinhardtMar 10, 2022
  53. Junio C HamanoMar 10, 2022
  54. Neeraj SinghMar 9, 2022
  55. Neeraj SinghFeb 11, 2022
  56. 3/4 core.fsync: new option to harden the indexNeeraj Singh via GitGitGadget, Feb 1, 2022
  57. 4/4 core.fsync: add a `derived-metadata` aggregate optionNeeraj Singh via GitGitGadget, Feb 1, 2022
  58. 0/5 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Mar 9, 2022
  59. 1/5 wrapper: move inclusion of CSPRNG headers the wrapper.c fileNeeraj Singh via GitGitGadget, Mar 9, 2022
  60. Junio C HamanoMar 9, 2022
  61. Neeraj SinghMar 10, 2022
  62. brian m. carlsonMar 10, 2022
  63. Neeraj SinghMar 10, 2022
  64. 2/5 core.fsyncmethod: add writeout-only modeNeeraj Singh via GitGitGadget, Mar 9, 2022
  65. Junio C HamanoMar 9, 2022
  66. 3/5 core.fsync: introduce granular fsync controlNeeraj Singh via GitGitGadget, Mar 9, 2022
  67. Junio C HamanoMar 10, 2022
  68. Neeraj SinghMar 10, 2022
  69. Junio C HamanoMar 10, 2022
  70. Neeraj SinghMar 10, 2022
  71. Junio C HamanoMar 10, 2022
  72. Junio C HamanoMar 10, 2022
  73. Neeraj SinghMar 10, 2022
  74. Junio C HamanoMar 10, 2022
  75. Johannes SchindelinMar 10, 2022
  76. Junio C HamanoMar 10, 2022
  77. 4/5 core.fsync: new option to harden the indexNeeraj Singh via GitGitGadget, Mar 9, 2022
  78. 5/5 core.fsync: documentation and user-friendly aggregate optionsNeeraj Singh via GitGitGadget, Mar 9, 2022
  79. Future-proofed syncing of refsPatrick Steinhardt, Mar 10, 2022
  80. 6/8 core.fsync: add `fsync_component()` wrapper which doesn't diePatrick Steinhardt, Mar 10, 2022
  81. Junio C HamanoMar 10, 2022
  82. Neeraj SinghMar 10, 2022
  83. 7/8 core.fsync: new option to harden loose referencesPatrick Steinhardt, Mar 10, 2022
  84. Junio C HamanoMar 10, 2022
  85. Neeraj SinghMar 10, 2022
  86. Neeraj SinghMar 10, 2022
  87. Junio C HamanoMar 11, 2022
  88. Patrick SteinhardtMar 11, 2022
  89. Ævar Arnfjörð BjarmasonMar 11, 2022
  90. 8/8 core.fsync: new option to harden packed referencesPatrick Steinhardt, Mar 10, 2022
  91. Junio C HamanoMar 10, 2022
  92. Patrick SteinhardtMar 11, 2022
  93. 0/6 A design for future-proofing fsync() configurationNeeraj K. Singh via GitGitGadget, Mar 10, 2022
  94. 1/6 wrapper: make inclusion of Windows csprng header tightly scopedNeeraj Singh via GitGitGadget, Mar 10, 2022
  95. 3/6 core.fsync: introduce granular fsync control infrastructureNeeraj Singh via GitGitGadget, Mar 10, 2022
  96. 4/6 core.fsync: add configuration parsingNeeraj Singh via GitGitGadget, Mar 10, 2022
  97. Jiang XinMar 28, 2022
  98. Neeraj SinghMar 28, 2022
  99. 5/6 core.fsync: new option to harden the indexNeeraj Singh via GitGitGadget, Mar 10, 2022
  100. 2/6 core.fsyncmethod: add writeout-only modeNeeraj Singh via GitGitGadget, Mar 10, 2022
  101. 6/6 core.fsync: documentation and user-friendly aggregate optionsNeeraj Singh via GitGitGadget, Mar 10, 2022
  102. core.fsync: documentation and user-friendly aggregate optionsNeeraj Singh, Mar 15, 2022
  103. Junio C HamanoMar 15, 2022
  104. Neeraj SinghMar 15, 2022
  105. do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)Ævar Arnfjörð Bjarmason, Mar 23, 2022
  106. Neeraj SinghMar 25, 2022
  107. Ævar Arnfjörð BjarmasonMar 26, 2022
  108. Junio C HamanoMar 26, 2022
  109. Neeraj SinghMar 26, 2022
  110. Ævar Arnfjörð BjarmasonMar 26, 2022
  111. Neeraj SinghMar 27, 2022
  112. Ævar Arnfjörð BjarmasonMar 27, 2022
  113. Patrick SteinhardtMar 28, 2022
  114. Ævar Arnfjörð BjarmasonMar 28, 2022
  115. Neeraj SinghMar 28, 2022
  116. Neeraj SinghMar 30, 2022
  117. Junio C HamanoMar 10, 2022
  118. Neeraj SinghMar 11, 2022
  119. Neeraj SinghMar 11, 2022
  120. Junio C HamanoMar 13, 2022
  121. core.fsync: new option to harden referencesPatrick Steinhardt, Mar 11, 2022
  122. SZEDER GáborMar 25, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.