This is an implementation of an extensible configuration mechanism for fsyncing persistent components of a repo. The main goals are to separate the "what" to sync from the "how". There are now two settings: core.fsync - Control the 'what', including the index. core.fsyncMethod - Control the 'how'. Currently we support writeout-only and full fsync. Syncing of refs can be layered on top of core.fsync. And batch mode will be layered on core.fsyncMethod. core.fsyncObjectfiles is removed and will issue a deprecation warning if it's seen. I'd like to get agreement on this direction before submitting batch mode to the list. The batch mode series is available to view at https://github.com/gitgitgadget/git/pull/1134 Please see [1], [2], and [3] for discussions that led to this series. After this change, new persistent data files added to the repo will need to be added to the fsync_component enum and documented in the Documentation/config/core.txt text. V4 changes: * Rebase onto master at b23dac905bd. * Add a comment to write_pack_file indicating why we don't fsync when writing to stdout. * I kept the configuration schema as-is rather than switching to multi-value. The thinking here is that a stateless last-one-wins config schema (comma separated) will make it easier to achieve some holistic self-consistent fsync configuration for a particular repo. V3 changes: * Remove relative path from git-compat-util.h include [4]. * Updated newly added warning texts to have more context for localization [4]. * Fixed tab spacing in enum fsync_action * Moved the fsync looping out to a helper and do it consistently. [4] * Changed commit description to use camelCase for config names. [5] * Add an optional fourth patch with derived-metadata so that the user can exclude a forward-compatible set of things that should be recomputable given existing data. V2 changes: * Updated the documentation for core.fsyncmethod to be less certain. writeout-only probably does not do the right thing on Linux. * Split out the core.fsync=index change into its own commit. * Rename REPO_COMPONENT to FSYNC_COMPONENT. This is really specific to fsyncing, so the name should reflect that. * Re-add missing Makefile change for SYNC_FILE_RANGE. * Tested writeout-only mode, index syncing, and general config settings. [1] https://lore.kernel.org/git/211110.86r1bogg27.gmgdl@xxxxxxxxxxxxxxxxxxx/ [2] https://lore.kernel.org/git/dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@xxxxxx/ [3] https://lore.kernel.org/git/pull.1076.git.git.1629856292.gitgitgadget@xxxxxxxxx/ [4] https://lore.kernel.org/git/CANQDOdf8C4-haK9=Q_J4Cid8bQALnmGDm=SvatRbaVf+tkzqLw@xxxxxxxxxxxxxx/ [5] https://lore.kernel.org/git/211207.861r2opplg.gmgdl@xxxxxxxxxxxxxxxxxxx/ Neeraj Singh (4): core.fsyncmethod: add writeout-only mode core.fsync: introduce granular fsync control core.fsync: new option to harden the index core.fsync: add a `derived-metadata` aggregate option Documentation/config/core.txt | 35 ++++++++--- Makefile | 6 ++ builtin/fast-import.c | 2 +- builtin/index-pack.c | 4 +- builtin/pack-objects.c | 24 +++++--- bulk-checkin.c | 5 +- cache.h | 49 +++++++++++++++- commit-graph.c | 3 +- compat/mingw.h | 3 + compat/win32/flush.c | 28 +++++++++ config.c | 90 ++++++++++++++++++++++++++++- config.mak.uname | 3 + configure.ac | 8 +++ contrib/buildsystems/CMakeLists.txt | 3 +- csum-file.c | 5 +- csum-file.h | 3 +- environment.c | 3 +- git-compat-util.h | 24 ++++++++ midx.c | 3 +- object-file.c | 3 +- pack-bitmap-write.c | 3 +- pack-write.c | 13 +++-- read-cache.c | 19 ++++-- wrapper.c | 64 ++++++++++++++++++++ write-or-die.c | 11 ++-- 25 files changed, 367 insertions(+), 47 deletions(-) create mode 100644 compat/win32/flush.c base-commit: b23dac905bde28da47543484320db16312c87551 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1093%2Fneerajsi-msft%2Fns%2Fcore-fsync-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1093/neerajsi-msft/ns/core-fsync-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1093 Range-diff vs v3: 1: 15edfe51509 ! 1: 51a218d100d core.fsyncmethod: add writeout-only mode @@ Makefile: ifdef HAVE_CLOCK_MONOTONIC endif ## cache.h ## -@@ cache.h: extern int read_replace_refs; - extern char *git_replace_ref_base; +@@ cache.h: extern char *git_replace_ref_base; extern int fsync_object_files; + extern int use_fsync; + +enum fsync_method { + FSYNC_METHOD_FSYNC, @@ compat/win32/flush.c (new) + +#define FLUSH_FLAGS_FILE_DATA_ONLY 1 + -+ DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NtFlushBuffersFileEx, ++ DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, NtFlushBuffersFileEx, + HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize, + PIO_STATUS_BLOCK IoStatusBlock); + @@ contrib/buildsystems/CMakeLists.txt: if(CMAKE_SYSTEM_NAME STREQUAL "Windows") compat/nedmalloc/nedmalloc.c compat/strdup.c) ## environment.c ## -@@ environment.c: const char *git_attributes_file; - const char *git_hooks_path; - int zlib_compression_level = Z_BEST_SPEED; +@@ environment.c: int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; --int fsync_object_files; + int fsync_object_files; + int use_fsync = -1; +enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; @@ wrapper.c: int xmkstemp_mode(char *filename_template, int mode) int err; ## write-or-die.c ## -@@ write-or-die.c: void fprintf_or_die(FILE *f, const char *fmt, ...) - - void fsync_or_die(int fd, const char *msg) - { +@@ write-or-die.c: void fsync_or_die(int fd, const char *msg) + use_fsync = git_env_bool("GIT_TEST_FSYNC", 1); + if (!use_fsync) + return; - while (fsync(fd) < 0) { - if (errno != EINTR) - die_errno("fsync error on '%s'", msg); - } ++ + if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY && + git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) + return; 2: 080be1a6f64 ! 2: 7a164ba9571 core.fsync: introduce granular fsync control @@ builtin/index-pack.c: static void final(const char *final_pack_name, const char ## builtin/pack-objects.c ## @@ builtin/pack-objects.c: static void write_pack_file(void) - * If so, rewrite it like in fast-import - */ + display_progress(progress_state, written); + } + +- /* +- * Did we write the wrong # entries in the header? +- * If so, rewrite it like in fast-import +- */ if (pack_to_stdout) { - finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE); ++ /* ++ * We never fsync when writing to stdout since we may ++ * not be writing to an actual pack file. For instance, ++ * the upload-pack code passes a pipe here. Calling ++ * fsync on a pipe results in unnecessary ++ * synchronization with the reader on some platforms. ++ */ + finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE, + CSUM_HASH_IN_STREAM | CSUM_CLOSE); } else if (nr_written == nr_remaining) { @@ builtin/pack-objects.c: static void write_pack_file(void) + CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); } else { - int fd = finalize_hashfile(f, hash, 0); ++ /* ++ * If we wrote the wrong number of entries in the ++ * header, rewrite it like in fast-import. ++ */ ++ + int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0); fixup_pack_header_footer(fd, hash, pack_tmp_name, nr_written, hash, offset); @@ cache.h: void reset_shared_repository(void); extern char *git_replace_ref_base; -extern int fsync_object_files; +-extern int use_fsync; +/* + * These values are used to help identify parts of a repository to fsync. + * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the + * repository and so shouldn't be fsynced. + */ +enum fsync_component { -+ FSYNC_COMPONENT_NONE = 0, ++ FSYNC_COMPONENT_NONE, + FSYNC_COMPONENT_LOOSE_OBJECT = 1 << 0, + FSYNC_COMPONENT_PACK = 1 << 1, + FSYNC_COMPONENT_PACK_METADATA = 1 << 2, @@ cache.h: void reset_shared_repository(void); enum fsync_method { FSYNC_METHOD_FSYNC, +@@ cache.h: enum fsync_method { + }; + + extern enum fsync_method fsync_method; ++extern int use_fsync; + extern int core_preload_index; + extern int precomposed_unicode; + extern int protect_hfs; @@ cache.h: int copy_file_with_time(const char *dst, const char *src, int mode); void write_or_die(int fd, const void *buf, size_t count); void fsync_or_die(int fd, const char *); @@ csum-file.h: int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint void crc32_begin(struct hashfile *); ## environment.c ## -@@ environment.c: const char *git_hooks_path; +@@ environment.c: const char *git_attributes_file; + const char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; +-int fsync_object_files; + int use_fsync = -1; 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; @@ midx.c: static int write_midx_internal(const char *object_dir, ## object-file.c ## @@ object-file.c: 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 (!the_repository->objects->odb->will_destroy) { +- 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")); - } ## pack-bitmap-write.c ## @@ pack-bitmap-write.c: void bitmap_writer_finish(struct pack_idx_entry **index, 3: 2207950beba = 3: f217dba77a1 core.fsync: new option to harden the index 4: a830d177d4c = 4: 5c22a41c1f3 core.fsync: add a `derived-metadata` aggregate option -- gitgitgadget
