A design for future-proofing fsync() configuration
Subsystems (2)
v6 cover A design for future-proofing fsync() configuration
In reply to Neeraj K. Singh via GitGitGadgetThis 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. Once this series reaches 'seen', I'll submit ns/batched-fsync to introduce batch mode. Please see
https://github.com/gitgitgadget/git/pull/1134.
core.fsyncObjectfiles is removed and we 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
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.
V6 changes:
* Move only the windows csprng includes into wrapper.c rather than all of them. This fixes the specific build issue due to broken Windows headers.
[6]
* Split the configuration parsing of core.fsync from the mechanism to focus the review.
* Incorporate Patrick's patch at [7] into the core.fsync mechanism patch.
* Pick the stricter one of core.fsyncObjectFiles and (fsync_components & FSYNC_COMPONENT_LOOSE_OBJECTS), to respect the older setting.
* Issue a deprecation warning but keep parsing and honoring core.fsyncObjectFiles.
* Change configuration parsing of core.fsync to always start with the platform default. none resets to the empty set. The comma separated list implies a set without regards to ordering now. This follows Junio's suggestion in [8].
* Change the documentation of the core.fsync option to reflect the way the new parsing code works.
* The patch 7 and 8 of Patrick's series at [7] can be cherry-picked after being applied to ns/core-fsyncmethod.
V5 changes:
* Rebase onto main at c2162907e9
* Add a patch to move CSPRNG platform includes to wrapper.c. This avoids build errors in compat/win32/flush.c and other files.
* Move the documentation and aggregate options to the final patch in the series.
* Define new aggregate options and guidance in line with Junio's suggestion to present the user with 'levels of safety' rather than a morass of detailed options.
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/[email protected]/
[2]
https://lore.kernel.org/git/dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@pks.im/
[3]
https://lore.kernel.org/git/[email protected]/
[4]
https://lore.kernel.org/git/CANQDOdf8C4-haK9=Q_J4Cid8bQALnmGDm=SvatRbaVf+tkzqLw@mail.gmail.com/
[5] https://lore.kernel.org/git/[email protected]/
[6]
https://lore.kernel.org/git/CANQDOdfZbOHZQt9Ah0t1AamTO2T7Gq0tmWX1jLqL6njE0LF6DA@mail.gmail.com/
[7]
https://lore.kernel.org/git/50e39f698a7c0cc06d3bc060e6dbc539ea693241.1646905589.git.ps@pks.im/
[8] https://lore.kernel.org/git/[email protected]/
Neeraj Singh (6):
wrapper: make inclusion of Windows csprng header tightly scoped core.fsyncmethod: add writeout-only mode
core.fsync: introduce granular fsync control infrastructure core.fsync: add configuration parsing
core.fsync: new option to harden the index core.fsync: documentation and user-friendly aggregate options
Documentation/config/core.txt | 58 ++++++++++++++++-- Makefile | 6 ++ builtin/fast-import.c | 2 +- builtin/index-pack.c | 4 +- builtin/pack-objects.c | 24 +++++--- bulk-checkin.c | 5 +- cache.h | 48 +++++++++++++++ commit-graph.c | 3 +- compat/mingw.h | 3 + compat/win32/flush.c | 28 +++++++++ compat/winansi.c | 5 -- config.c | 94 +++++++++++++++++++++++++++++ config.mak.uname | 3 + configure.ac | 8 +++ contrib/buildsystems/CMakeLists.txt | 16 +++-- csum-file.c | 5 +- csum-file.h | 3 +- environment.c | 4 +- git-compat-util.h | 30 +++++++-- midx.c | 3 +- object-file.c | 13 ++-- pack-bitmap-write.c | 3 +- pack-write.c | 13 ++-- read-cache.c | 19 ++++-- wrapper.c | 71 ++++++++++++++++++++++ write-or-die.c | 33 ++++++++-- 26 files changed, 444 insertions(+), 60 deletions(-) create mode 100644 compat/win32/flush.c
base-commit: c2162907e9aa884bdb70208389cb99b181620d51
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1093%2Fneerajsi-msft%2Fns%2Fcore-fsync-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1093/neerajsi-msft/ns/core-fsync-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1093
Range-diff vs v5:
1: 685b1db8880 ! 1: 825079b6aa1 wrapper: move inclusion of CSPRNG headers the wrapper.c file
@@ Metadata
Author: Neeraj Singh <[email protected]>
## Commit message ##
- wrapper: move inclusion of CSPRNG headers the wrapper.c file
+ wrapper: make inclusion of Windows csprng header tightly scoped
Including NTSecAPI.h in git-compat-util.h causes build errors in any
- other file that includes winternl.h. That file was included in order to
+ other file that includes winternl.h. NTSecAPI.h was included in order to
get access to the RtlGenRandom cryptographically secure PRNG. This
- change scopes the inclusion of all PRNG headers to just the wrapper.c
- file, which is the only place it is really needed.
+ change scopes the inclusion of ntsecapi.h to wrapper.c, which is the only
+ place that it's actually needed.
+
+ The build breakage is due to the definition of UNICODE_STRING in
+ NtSecApi.h:
+ #ifndef _NTDEF_
+ typedef LSA_UNICODE_STRING UNICODE_STRING, *PUNICODE_STRING;
+ typedef LSA_STRING STRING, *PSTRING ;
+ #endif
+
+ LsaLookup.h:
+ typedef struct _LSA_UNICODE_STRING {
+ USHORT Length;
+ USHORT MaximumLength;
+ #ifdef MIDL_PASS
+ [size_is(MaximumLength/2), length_is(Length/2)]
+ #endif // MIDL_PASS
+ PWSTR Buffer;
+ } LSA_UNICODE_STRING, *PLSA_UNICODE_STRING;
+
+ winternl.h also defines UNICODE_STRING:
+ typedef struct _UNICODE_STRING {
+ USHORT Length;
+ USHORT MaximumLength;
+ PWSTR Buffer;
+ } UNICODE_STRING;
+ typedef UNICODE_STRING *PUNICODE_STRING;
+
+ Both definitions have equivalent layouts. Apparently these internal
+ Windows headers aren't designed to be included together. This is
+ an oversight in the headers and does not represent an incompatibility
+ between the APIs.
@@ git-compat-util.h
#endif
#include <unistd.h>
-@@
- #else
- #include <stdint.h>
- #endif
--#ifdef HAVE_ARC4RANDOM_LIBBSD
--#include <bsd/stdlib.h>
--#endif
--#ifdef HAVE_GETRANDOM
--#include <sys/random.h>
--#endif
- #ifdef NO_INTPTR_T
- /*
- * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
## wrapper.c ##
@@
@@ wrapper.c
+#include <NTSecAPI.h>
+#undef SystemFunction036
+#endif
-+
-+#ifdef HAVE_ARC4RANDOM_LIBBSD
-+#include <bsd/stdlib.h>
-+#endif
-+#ifdef HAVE_GETRANDOM
-+#include <sys/random.h>
-+#endif
+
static int memory_limit_check(size_t size, int gentle) {
2: da8cfc10bb4 = 2: a41bd4a06af core.fsyncmethod: add writeout-only mode
3: e31886717b4 ! 3: 64e2bdcdfd9 core.fsync: introduce granular fsync control
@@ Metadata
Author: Neeraj Singh <[email protected]>
## Commit message ##
- core.fsync: introduce granular fsync control
+ core.fsync: introduce granular fsync control infrastructure
- This commit introduces the `core.fsync` configuration
- knob which can be used to control how components of the
- repository are made durable on disk.
+ This commit introduces the infrastructure for the core.fsync
+ configuration knob. The repository components we want to sync
+ are identified by flags so that we can turn on or off syncing
+ for specific components.
- This setting allows future extensibility of the list of
- syncable components:
- * We issue a warning rather than an error for unrecognized
- components, so new configs can be used with old Git versions.
- * We support negation, so users can choose one of the aggregate
- options and then remove components that they don't want.
- Aggregate options are defined in a later patch in this series.
+ If core.fsyncObjectFiles is set and the core.fsync configuration
+ also includes FSYNC_COMPONENT_LOOSE_OBJECT, we will fsync any
+ loose objects. This picks the strictest data integrity behavior
+ if core.fsync and core.fsyncObjectFiles are set to conflicting values.
- This also supports the common request of doing absolutely no
- fysncing with the `core.fsync=none` value, which is expected
- to make the test suite faster.
+ This change introduces the currently unused fsync_component
+ helper, which will be used by a later patch that adds fsyncing to
+ the refs backend.
- Complete documentation for the new setting is included in a later patch
- in the series so that it can be reviewed in final form.
+ Actual configuration and documentation of the fsync components
+ list are in other patches in the series to separate review of
+ the underlying mechanism from the policy of how it's configured.
+ Helped-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Neeraj Singh <[email protected]>
- ## Documentation/config/core.txt ##
-@@ Documentation/config/core.txt: core.fsyncMethod::
- filesystem and storage hardware, data added to the repository may not be
- durable in the event of a system crash. This is the default mode on macOS.
-
--core.fsyncObjectFiles::
-- This boolean will enable 'fsync()' when writing object files.
--+
--This is a total waste of time and effort on a filesystem that orders
--data writes properly, but can be useful for filesystems that do not use
--journalling (traditional UNIX filesystems) or that only journal metadata
--and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
--
- core.preloadIndex::
- Enable parallel index preload for operations like 'git diff'
- +
-
## builtin/fast-import.c ##
@@ builtin/fast-import.c: static void end_packfile(void)
struct tag *t;
@@ cache.h: void reset_shared_repository(void);
extern int read_replace_refs;
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
@@ cache.h: void reset_shared_repository(void);
+ * A bitmask indicating which components of the repo should be fsynced.
+ */
+extern enum fsync_component fsync_components;
+ extern int fsync_object_files;
+ extern int use_fsync;
- 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 *);
++int fsync_component(enum fsync_component component, int fd);
++void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
-+static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
-+{
-+ if (fsync_components & component)
-+ fsync_or_die(fd, msg);
-+}
-+
ssize_t read_in_full(int fd, void *buf, size_t count);
ssize_t write_in_full(int fd, const void *buf, size_t count);
- ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
## commit-graph.c ##
@@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_context *ctx)
@@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con
if (ctx->split) {
- ## config.c ##
-@@ config.c: 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 },
-+};
-+
-+static enum fsync_component parse_fsync_components(const char *var, const char *string)
-+{
-+ enum fsync_component output = 0;
-+
-+ if (!strcmp(string, "none"))
-+ return FSYNC_COMPONENT_NONE;
-+
-+ 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(_("ignoring unknown core.fsync component '%s'"), component);
-+ free(component);
-+ }
-+
-+ string = ep;
-+ }
-+
-+ return output;
-+}
-+
- int git_parse_maybe_bool(const char *value)
- {
- int v = git_parse_maybe_bool_text(value);
-@@ config.c: 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);
-@@ config.c: 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;
- }
-
-
## csum-file.c ##
@@ csum-file.c: static void free_hashfile(struct hashfile *f)
free(f);
@@ csum-file.h: int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint
void crc32_begin(struct hashfile *);
## environment.c ##
-@@ 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;
+@@ environment.c: 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;
@@ 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 (!the_repository->objects->odb->will_destroy) {
+- 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 (the_repository->objects->odb->will_destroy)
++ goto out;
++ if (fsync_object_files > 0)
++ fsync_or_die(fd, "loose object file");
++ else
++ fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,
++ "loose object file");
++
++out:
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,
@@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf
if (close_tempfile_gently(tempfile)) {
error(_("could not close '%s'"), get_tempfile_path(tempfile));
return -1;
+
+ ## 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)
++static int maybe_fsync(int fd)
+ {
+ if (use_fsync < 0)
+ use_fsync = git_env_bool("GIT_TEST_FSYNC", 1);
+ if (!use_fsync)
+- return;
++ return 0;
+
+ if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY &&
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0)
+- return;
++ return 0;
++
++ return git_fsync(fd, FSYNC_HARDWARE_FLUSH);
++}
+
+- if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
++void fsync_or_die(int fd, const char *msg)
++{
++ if (maybe_fsync(fd) < 0)
+ die_errno("fsync error on '%s'", msg);
+ }
+
++int fsync_component(enum fsync_component component, int fd)
++{
++ if (fsync_components & component)
++ return maybe_fsync(fd);
++ return 0;
++}
++
++void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
++{
++ if (fsync_components & component)
++ fsync_or_die(fd, msg);
++}
++
+ void write_or_die(int fd, const void *buf, size_t count)
+ {
+ if (write_in_full(fd, buf, count) < 0) {
-: ----------- > 4: 6adc8dc1385 core.fsync: add configuration parsing
4: 9da808ba743 ! 5: 757f6d0bbd2 core.fsync: new option to harden the index
@@ cache.h: enum fsync_component {
#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
## config.c ##
-@@ config.c: static const struct fsync_component_entry {
+@@ config.c: static const struct fsync_component_name {
{ "pack", FSYNC_COMPONENT_PACK },
{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
{ "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, 5: 2d71346b10e ! 6: 7e4cc6e10a5 core.fsync: documentation and user-friendly aggregate options
@@ Documentation/config/core.txt: core.whitespace::
errors. The default tab width is 8. Allowed values are 1 to 63.
+core.fsync::
-+ A comma-separated list of parts of the repository which should be
-+ hardened via the core.fsyncMethod when created or modified. You can
-+ disable hardening of any component by prefixing it with a '-'. Later
-+ items take precedence over earlier ones in the comma-separated list.
-+ For example, core.fsync=all,-pack-metadata means "harden everything
-+ except pack metadata." Items that are not hardened may be lost in the
-+ event of an unclean system shutdown. Unless you have special
-+ requirements, it is recommended that you leave this option as default
-+ or pick one of committed, added, or all.
++ A comma-separated list of components of the repository that
++ should be hardened via the core.fsyncMethod when created or
++ modified. You can disable hardening of any component by
++ prefixing it with a '-'. Items that are not hardened may be
++ lost in the event of an unclean system shutdown. Unless you
++ have special requirements, it is recommended that you leave
++ this option empty or pick one of committed, added,
++ or all.
++
-+* none disables fsync completely. This value must be specified alone. ++When this configuration is encountered, the set of components starts with ++the platform default value, disabled components are removed, and additional ++components are added. none resets the state so that the platform default ++is ignored.
+++
++The empty string resets the fsync configuration to the platform ++default. The platform default on most platform is equivalent to ++core.fsync=committed,-loose-object, which has good performance, ++but risks losing recent work in the event of an unclean system shutdown.
+++
++* none clears the set of fsynced components.
+* loose-object hardens objects added to the repo in loose-object form. +* pack hardens objects added to the repo in packfile form.
+* pack-metadata hardens packfile bitmaps and indexes.
@@ Documentation/config/core.txt: core.whitespace::
+ loose-object,pack.
+* derived-metadata is an aggregate option that is equivalent to
+ pack-metadata,commit-graph.
-+* default is an aggregate option that is equivalent to
-+ objects,derived-metadata,-loose-object. This mode is enabled by default. -+ It has good performance, but risks losing recent work if the system shuts -+ down uncleanly, since commits, trees, and blobs in loose-object form may be -+ lost.
+* committed is an aggregate option that is currently equivalent to
-+ objects. This mode sacrifices some performance to ensure that all work ++ objects. This mode sacrifices some performance to ensure that work
+ that is committed to the repository with git commit or similar commands
-+ is preserved.
++ is hardened.
+* added is an aggregate option that is currently equivalent to
+ committed,index. This mode sacrifices additional performance to
+ ensure that the results of commands like git add and similar operations
-+ are preserved.
++ are hardened.
+* all is an aggregate option that syncs all individual components above. +
core.fsyncMethod::
@@ cache.h: enum fsync_component {
* A bitmask indicating which components of the repo should be fsynced.
## config.c ##
-@@ config.c: static const struct fsync_component_entry { +@@ config.c: static const struct fsync_component_name {
{ "pack-metadata", FSYNC_COMPONENT_PACK_METADATA }, { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, { "index", FSYNC_COMPONENT_INDEX },
+ { "objects", FSYNC_COMPONENTS_OBJECTS },
+ { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA },
-+ { "default", FSYNC_COMPONENTS_DEFAULT },
+ { "committed", FSYNC_COMPONENTS_COMMITTED },
+ { "added", FSYNC_COMPONENTS_ADDED },
+ { "all", FSYNC_COMPONENTS_ALL },
Signature · 1 line
gitgitgadget
PATCH v6 1/6 wrapper: make inclusion of Windows csprng header tightly scoped
From: Neeraj Singh <[email protected]>
Including NTSecAPI.h in git-compat-util.h causes build errors in any other file that includes winternl.h. NTSecAPI.h was included in order to get access to the RtlGenRandom cryptographically secure PRNG. This change scopes the inclusion of ntsecapi.h to wrapper.c, which is the only place that it's actually needed.
The build breakage is due to the definition of UNICODE_STRING in NtSecApi.h:
#ifndef _NTDEF_
typedef LSA_UNICODE_STRING UNICODE_STRING, *PUNICODE_STRING;
typedef LSA_STRING STRING, *PSTRING ;
#endifLsaLookup.h:
typedef struct _LSA_UNICODE_STRING {
USHORT Length;
USHORT MaximumLength;
#ifdef MIDL_PASS
[size_is(MaximumLength/2), length_is(Length/2)]
#endif // MIDL_PASS
PWSTR Buffer;
} LSA_UNICODE_STRING, *PLSA_UNICODE_STRING;winternl.h also defines UNICODE_STRING:
typedef struct _UNICODE_STRING {
USHORT Length;
USHORT MaximumLength;
PWSTR Buffer;
} UNICODE_STRING;
typedef UNICODE_STRING *PUNICODE_STRING;Both definitions have equivalent layouts. Apparently these internal Windows headers aren't designed to be included together. This is an oversight in the headers and does not represent an incompatibility between the APIs.
--- compat/winansi.c | 5 ----- git-compat-util.h | 6 ------ wrapper.c | 7 +++++++ 3 files changed, 7 insertions(+), 11 deletions(-)
compat/winansi.c +0 -5
@@ -4,11 +4,6 @@#undef NOGDI -/* -* Including the appropriate header file for RtlGenRandom causes MSVC to see a -* redefinition of types in an incompatible way when including headers below. -*/ -#undef HAVE_RTLGENRANDOM#include "../git-compat-util.h" #include <wingdi.h> #include <winreg.h>
git-compat-util.h +0 -6
@@ -197,12 +197,6 @@#endif #include <windows.h> #define GIT_WINDOWS_NATIVE -#ifdef HAVE_RTLGENRANDOM -/* This is required to get access to RtlGenRandom. */ -#define SystemFunction036 NTAPI SystemFunction036 -#include <NTSecAPI.h> -#undef SystemFunction036 -#endif #endif #include <unistd.h>
wrapper.c +7 -0
@@ -4,6 +4,13 @@#include "cache.h" #include "config.h" +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include <NTSecAPI.h> +#undef SystemFunction036 +#endif +static int memory_limit_check (size_t size,int gentle) {static size_t limit =0 ;
Signature · 1 line
gitgitgadget
PATCH v6 2/6 core.fsyncmethod: add writeout-only mode
In reply to Neeraj K. Singh via GitGitGadgetFrom: Neeraj Singh <[email protected]>
This commit introduces the core.fsyncMethod configuration knob, which can currently be set to fsync or writeout-only.
The new writeout-only mode attempts to tell the operating system to flush its in-memory page cache to the storage hardware without issuing a CACHE_FLUSH command to the storage controller.
Writeout-only fsync is significantly faster than a vanilla fsync on common hardware, since data is written to a disk-side cache rather than all the way to a durable medium. Later changes in this patch series will take advantage of this primitive to implement batching of hardware flushes.
When git_fsync is called with FSYNC_WRITEOUT_ONLY, it may fail and the caller is expected to do an ordinary fsync as needed.
On Apple platforms, the fsync system call does not issue a CACHE_FLUSH directive to the storage controller. This change updates fsync to do fcntl(F_FULLFSYNC) to make fsync actually durable. We maintain parity with existing behavior on Apple platforms by setting the default value of the new core.fsyncMethod option.
--- Documentation/config/core.txt | 9 ++++ Makefile | 6 +++ cache.h | 7 ++++ compat/mingw.h | 3 ++ compat/win32/flush.c | 28 +++++++++++++ config.c | 12 ++++++ config.mak.uname | 3 ++ configure.ac | 8 ++++ contrib/buildsystems/CMakeLists.txt | 16 ++++++-- environment.c | 1 + git-compat-util.h | 24 +++++++++++ wrapper.c | 64 +++++++++++++++++++++++++++++ write-or-die.c | 11 +++-- 13 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 compat/win32/flush.c
Documentation/config/core.txt +9 -0
@@ -547,6 +547,15 @@ core.whitespace:: is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent` errors. The default tab width is 8. Allowed values are 1 to 63. +core.fsyncMethod:: + A value indicating the strategy Git will use to harden repository data + using fsync and related primitives. ++ +* `fsync` uses the fsync() system call or platform equivalents. +* `writeout-only` issues pagecache writeback requests, but depending on the + filesystem and storage hardware, data added to the repository may not be + durable in the event of a system crash. This is the default mode on macOS. + core.fsyncObjectFiles:: This boolean will enable 'fsync()' when writing object files. +
Makefile +6 -0
@@ -411,6 +411,8 @@ all:: # # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC. # +# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range. +# # Define NEEDS_LIBRT if your platform requires linking with librt (glibc version # before 2.17) for clock_gettime and CLOCK_MONOTONIC. #
@@ -1897,6 +1899,10 @@ ifdef HAVE_CLOCK_MONOTONIC BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC endif +ifdef HAVE_SYNC_FILE_RANGE + BASIC_CFLAGS += -DHAVE_SYNC_FILE_RANGE +endif + ifdef NEEDS_LIBRT EXTLIBS += -lrt endif
cache.h +7 -0
@@ -995,6 +995,13 @@ extern char *git_replace_ref_base;extern int fsync_object_files;extern int use_fsync; + +enum fsync_method { +FSYNC_METHOD_FSYNC , +FSYNC_METHOD_WRITEOUT_ONLY +}; + +extern enum fsync_method fsync_method;extern int core_preload_index;extern int precomposed_unicode;extern int protect_hfs;
compat/mingw.h +3 -0
@@ -329,6 +329,9 @@ int mingw_getpagesize(void);#define getpagesize mingw_getpagesize#endif +int win32_fsync_no_flush (int fd); +#define fsync_no_flush win32_fsync_no_flush +struct rlimit {unsigned int rlim_cur; };
compat/win32/flush.c new file +28 -0
@@ -0,0 +1,28 @@ +#include "git-compat-util.h" +#include <winternl.h> +#include "lazyload.h" + +int win32_fsync_no_flush (int fd) +{ +IO_STATUS_BLOCK io_status; + +#define FLUSH_FLAGS_FILE_DATA_ONLY 1 + +DECLARE_PROC_ADDR (ntdll.dll,NTSTATUS ,NTAPI , NtFlushBuffersFileEx, + HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize, + PIO_STATUS_BLOCK IoStatusBlock); + +if (!INIT_PROC_ADDR (NtFlushBuffersFileEx)) { + errno =ENOSYS ; +return -1 ; + } + +memset (&io_status,0 ,sizeof (io_status)); +if (NtFlushBuffersFileEx ((HANDLE )_get_osfhandle (fd),FLUSH_FLAGS_FILE_DATA_ONLY , +NULL ,0 , &io_status)) { + errno =EINVAL ; +return -1 ; + } + +return 0 ; +}
config.c +12 -0
@@ -1600,6 +1600,18 @@ static int git_default_core_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"core.fsyncmethod" )) { +if (!value) +return config_error_nonbool (var); +if (!strcmp (value,"fsync" )) + fsync_method =FSYNC_METHOD_FSYNC ; +else if (!strcmp (value,"writeout-only" )) + fsync_method =FSYNC_METHOD_WRITEOUT_ONLY ; +else +warning (_ ("ignoring unknown core.fsyncMethod value '%s'" ), value); + + } +if (!strcmp (var,"core.fsyncobjectfiles" )) { fsync_object_files =git_config_bool (var, value);return 0 ;
config.mak.uname +3 -0
@@ -57,6 +57,7 @@ ifeq ($(uname_S),Linux) HAVE_CLOCK_MONOTONIC = YesPlease # -lrt is needed for clock_gettime on glibc <= 2.16 NEEDS_LIBRT = YesPlease + HAVE_SYNC_FILE_RANGE = YesPlease HAVE_GETDELIM = YesPlease FREAD_READS_DIRECTORIES = UnfortunatelyYes BASIC_CFLAGS += -DHAVE_SYSINFO
@@ -463,6 +464,7 @@ endif CFLAGS = BASIC_CFLAGS = -nologo -I. -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE COMPAT_OBJS = compat/msvc.o compat/winansi.o \ + compat/win32/flush.o \ compat/win32/path-utils.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/trace2_win32_process_info.o \
@@ -640,6 +642,7 @@ ifeq ($(uname_S),MINGW) COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/trace2_win32_process_info.o \ + compat/win32/flush.o \ compat/win32/path-utils.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/dirent.o
configure.ac +8 -0
@@ -1082,6 +1082,14 @@ AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC], [AC_MSG_RESULT([no]) HAVE_CLOCK_MONOTONIC=]) GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC]) + +# +# Define HAVE_SYNC_FILE_RANGE=YesPlease if sync_file_range is available. +GIT_CHECK_FUNC(sync_file_range, + [HAVE_SYNC_FILE_RANGE=YesPlease], + [HAVE_SYNC_FILE_RANGE]) +GIT_CONF_SUBST([HAVE_SYNC_FILE_RANGE]) + # # Define NO_SETITIMER if you don't have setitimer. GIT_CHECK_FUNC(setitimer,
contrib/buildsystems/CMakeLists.txt +12 -4
@@ -261,10 +261,18 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM) - list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c - compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c - compat/win32/trace2_win32_process_info.c compat/win32/dirent.c - compat/nedmalloc/nedmalloc.c compat/strdup.c) + list(APPEND compat_SOURCES + compat/mingw.c + compat/winansi.c + compat/win32/flush.c + compat/win32/path-utils.c + compat/win32/pthread.c + compat/win32mmap.c + compat/win32/syslog.c + compat/win32/trace2_win32_process_info.c + compat/win32/dirent.c + compat/nedmalloc/nedmalloc.c + compat/strdup.c) set(NO_UNIX_SOCKETS 1) elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
environment.c +1 -0
@@ -44,6 +44,7 @@ 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 ;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 ;
git-compat-util.h +24 -0
@@ -1271,6 +1271,30 @@ __attribute__((format (printf, 1, 2))) NORETURNvoid BUG (const char *fmt, ...);#endif +#ifdef __APPLE__ +#define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY +#else +#define FSYNC_METHOD_DEFAULT FSYNC_METHOD_FSYNC +#endif + +enum fsync_action { +FSYNC_WRITEOUT_ONLY , +FSYNC_HARDWARE_FLUSH +}; + +/* +* Issues an fsync against the specified file according to the specified mode. +* +* FSYNC_WRITEOUT_ONLY attempts to use interfaces available on some operating +* systems to flush the OS cache without issuing a flush command to the storage +* controller. If those interfaces are unavailable, the function fails with +* ENOSYS. +* +* FSYNC_HARDWARE_FLUSH does an OS writeout and hardware flush to ensure that +* changes are durable. It is not expected to fail. +*/ +int git_fsync (int fd,enum fsync_action action); +/* * Preserves errno, prints a message, but gives no warning for ENOENT. * Returns 0 on success, which includes trying to unlink an object that does
wrapper.c +64 -0
70 lines not on this page · raw message
write-or-die.c +7 -4
@@ -62,10 +62,13 @@ 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 ; + +if (git_fsync (fd,FSYNC_HARDWARE_FLUSH ) <0 ) +die_errno ("fsync error on '%s'" , msg); }void write_or_die (int fd,const void *buf,size_t count)
Signature · 1 line
gitgitgadget
PATCH v6 3/6 core.fsync: introduce granular fsync control infrastructure
In reply to Neeraj K. Singh via GitGitGadgetFrom: Neeraj Singh <[email protected]>
This commit introduces the infrastructure for the core.fsync configuration knob. The repository components we want to sync are identified by flags so that we can turn on or off syncing for specific components.
If core.fsyncObjectFiles is set and the core.fsync configuration also includes FSYNC_COMPONENT_LOOSE_OBJECT, we will fsync any loose objects. This picks the strictest data integrity behavior if core.fsync and core.fsyncObjectFiles are set to conflicting values.
This change introduces the currently unused fsync_component helper, which will be used by a later patch that adds fsyncing to the refs backend.
Actual configuration and documentation of the fsync components list are in other patches in the series to separate review of the underlying mechanism from the policy of how it's configured.
--- builtin/fast-import.c | 2 +- builtin/index-pack.c | 4 ++-- builtin/pack-objects.c | 24 +++++++++++++++++------- bulk-checkin.c | 5 +++-- cache.h | 23 +++++++++++++++++++++++ commit-graph.c | 3 ++- csum-file.c | 5 +++-- csum-file.h | 3 ++- environment.c | 1 + midx.c | 3 ++- object-file.c | 13 +++++++++---- pack-bitmap-write.c | 3 ++- pack-write.c | 13 +++++++------ read-cache.c | 2 +- write-or-die.c | 26 ++++++++++++++++++++++---- 15 files changed, 97 insertions(+), 33 deletions(-)
builtin/fast-import.c +1 -1
@@ -865,7 +865,7 @@ static void end_packfile(void)struct tag *t;close_pack_windows (pack_data); -finalize_hashfile (pack_file, cur_pack_oid.hash,0 ); +finalize_hashfile (pack_file, cur_pack_oid.hash,FSYNC_COMPONENT_PACK ,0 );fixup_pack_header_footer (pack_data->pack_fd, pack_data->hash, pack_data->pack_name, object_count, cur_pack_oid.hash, pack_size);
builtin/index-pack.c +2 -2
@@ -1290,7 +1290,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,
@@ -1512,7 +1512,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" ));
builtin/pack-objects.c +17 -7
@@ -1199,16 +1199,26 @@ static void write_pack_file(void)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) { -finalize_hashfile (f, hash,CSUM_HASH_IN_STREAM |CSUM_FSYNC |CSUM_CLOSE ); +finalize_hashfile (f, hash,FSYNC_COMPONENT_PACK , +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);close (fd);
bulk-checkin.c +3 -2
@@ -53,9 +53,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)unlink (state->pack_tmp_name); goto clear_exit; }else if (state->nr_written ==1 ) { -finalize_hashfile (state->f, hash,CSUM_HASH_IN_STREAM |CSUM_FSYNC |CSUM_CLOSE ); +finalize_hashfile (state->f, hash,FSYNC_COMPONENT_PACK , +CSUM_HASH_IN_STREAM |CSUM_FSYNC |CSUM_CLOSE ); }else { -int fd =finalize_hashfile (state->f, hash,0 ); +int fd =finalize_hashfile (state->f, hash,FSYNC_COMPONENT_PACK ,0 );fixup_pack_header_footer (fd, hash, state->pack_tmp_name, state->nr_written, hash, state->offset);
cache.h +23 -0
@@ -993,6 +993,27 @@ void reset_shared_repository(void);extern int read_replace_refs;extern char *git_replace_ref_base; +/* +* 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 , +FSYNC_COMPONENT_LOOSE_OBJECT =1 <<0 , +FSYNC_COMPONENT_PACK =1 <<1 , +FSYNC_COMPONENT_PACK_METADATA =1 <<2 , +FSYNC_COMPONENT_COMMIT_GRAPH =1 <<3 , +}; + +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \ + FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH) + +/* +* A bitmask indicating which components of the repo should be fsynced. +*/ +extern enum fsync_component fsync_components;extern int fsync_object_files;extern int use_fsync;
@@ -1707,6 +1728,8 @@ 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 *); +int fsync_component (enum fsync_component component,int fd); +void fsync_component_or_die (enum fsync_component component,int fd,const char *msg);ssize_t read_in_full (int fd,void *buf,size_t count);ssize_t write_in_full (int fd,const void *buf,size_t count);
commit-graph.c +2 -1
@@ -1942,7 +1942,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) }close_commit_graph (ctx->r->objects); -finalize_hashfile (f, file_hash,CSUM_HASH_IN_STREAM |CSUM_FSYNC ); +finalize_hashfile (f, file_hash,FSYNC_COMPONENT_COMMIT_GRAPH , +CSUM_HASH_IN_STREAM |CSUM_FSYNC );free_chunkfile (cf);if (ctx->split) {
csum-file.c +3 -2
@@ -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 flif (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);
csum-file.h +2 -1
@@ -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 *);
environment.c +1 -0
@@ -45,6 +45,7 @@ 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 ;size_t packed_git_limit =DEFAULT_PACKED_GIT_LIMIT ;size_t delta_base_cache_limit =96 *1024 *1024 ;
midx.c +2 -1
@@ -1438,7 +1438,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 &&
object-file.c +9 -4
@@ -1849,11 +1849,16 @@ 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 (!the_repository->objects->odb->will_destroy) { -if (fsync_object_files) -fsync_or_die (fd,"loose object file" ); - } +if (the_repository->objects->odb->will_destroy) + goto out; +if (fsync_object_files >0 ) +fsync_or_die (fd,"loose object file" ); +else +fsync_component_or_die (FSYNC_COMPONENT_LOOSE_OBJECT , fd, +"loose object file" ); + +out:if (close (fd) !=0 )die_errno (_ ("error when closing loose object file" )); }
pack-bitmap-write.c +2 -1
@@ -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" );
pack-write.c +7 -6
@@ -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)
read-cache.c +1 -1
@@ -3089,7 +3089,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 ;
write-or-die.c +22 -4
@@ -56,21 +56,39 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) } } -void fsync_or_die (int fd,const char *msg) +static int maybe_fsync (int fd) {if (use_fsync <0 ) use_fsync =git_env_bool ("GIT_TEST_FSYNC" ,1 );if (!use_fsync) -return ; +return 0 ;if (fsync_method ==FSYNC_METHOD_WRITEOUT_ONLY &&git_fsync (fd,FSYNC_WRITEOUT_ONLY ) >=0 ) -return ; +return 0 ; + +return git_fsync (fd,FSYNC_HARDWARE_FLUSH ); +} -if (git_fsync (fd,FSYNC_HARDWARE_FLUSH ) <0 ) +void fsync_or_die (int fd,const char *msg) +{ +if (maybe_fsync (fd) <0 )die_errno ("fsync error on '%s'" , msg); } +int fsync_component (enum fsync_component component,int fd) +{ +if (fsync_components & component) +return maybe_fsync (fd); +return 0 ; +} + +void fsync_component_or_die (enum fsync_component component,int fd,const char *msg) +{ +if (fsync_components & component) +fsync_or_die (fd, msg); +} +void write_or_die (int fd,const void *buf,size_t count) {if (write_in_full (fd, buf, count) <0 ) {
Signature · 1 line
gitgitgadget
PATCH v6 4/6 core.fsync: add configuration parsing
In reply to Neeraj K. Singh via GitGitGadgetFrom: Neeraj Singh <[email protected]>
This change introduces code to parse the core.fsync setting and configure the fsync_components variable.
core.fsync is configured as a comma-separated list of component names to sync. Each time a core.fsync variable is encountered in the configuration heirarchy, we start off with a clean state with the platform default value. Passing 'none' resets the value to indicate nothing will be synced. We gather all negative and positive entries from the comma separated list and then compute the new value by removing all the negative entries and adding all of the positive entries.
We issue a warning for components that are not recognized so that the configuration code is compatible with configs from future versions of Git with more repo components.
Complete documentation for the new setting is included in a later patch in the series so that it can be reviewed once in final form.
--- Documentation/config/core.txt | 9 +++-- config.c | 76 +++++++++++++++++++++++++++++++++++ environment.c | 2 +- 3 files changed, 82 insertions(+), 5 deletions(-)
Documentation/config/core.txt +5 -4
@@ -558,11 +558,12 @@ core.fsyncMethod:: core.fsyncObjectFiles:: This boolean will enable 'fsync()' when writing object files. + This setting is deprecated. Use core.fsync instead. + -This is a total waste of time and effort on a filesystem that orders -data writes properly, but can be useful for filesystems that do not use -journalling (traditional UNIX filesystems) or that only journal metadata -and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback"). +This setting affects data added to the Git repository in loose-object +form. When set to true, Git will issue an fsync or similar system call +to flush caches so that loose-objects remain consistent in the face +of a unclean system shutdown. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
config.c +76 -0
94 lines not on this page · raw message
environment.c +1 -1
@@ -42,7 +42,7 @@ 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 fsync_object_files =-1 ;int use_fsync =-1 ;enum fsync_method fsync_method =FSYNC_METHOD_DEFAULT ;enum fsync_component fsync_components =FSYNC_COMPONENTS_DEFAULT ;
Signature · 1 line
gitgitgadget
Re: [PATCH v6 4/6] core.fsync: add configuration parsing
config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1691
Quoted 6 lines · Neeraj Singh via GitGitGadget
config.c +2
-0@@ -1613,6 +1687,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsyncobjectfiles" )) { +if (fsync_object_files <0 ) +warning (_ ("core.fsyncobjectfiles is deprecated; use core.fsync instead" ));
s/core.fsyncobjectfiles/core.fsyncObjectFiles/ to use bumpyCaps for config variable in documentation.
Re: [PATCH v6 4/6] core.fsync: add configuration parsing
Quoted 6 lines · Jiang Xin
Quoted 3 lines · Neeraj Singh via GitGitGadget
(quoted diff) +0
-0@@ -1613,6 +1687,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }
Quoted 6 lines
config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1691
Quoted 3 lines
config.c +2
-0if (!strcmp (var,"core.fsyncobjectfiles" )) { +if (fsync_object_files <0 ) +warning (_ ("core.fsyncobjectfiles is deprecated; use core.fsync instead" ));s/core.fsyncobjectfiles/core.fsyncObjectFiles/ to use bumpyCaps for config variable in documentation.
THanks for pointing this out. I'll fix it.
PATCH v6 5/6 core.fsync: new option to harden the index
In reply to Neeraj K. Singh via GitGitGadgetFrom: Neeraj Singh <[email protected]>
This commit introduces the new ability for the user to harden the index. In the event of a system crash, the index must be durable for the user to actually find a file that has been added to the repo and then deleted from the working tree.
We use the presence of the COMMIT_LOCK flag and absence of the alternate_index_output as a proxy for determining whether we're updating the persistent index of the repo or some temporary index. We don't sync these temporary indexes.
--- cache.h | 1 + config.c | 1 + read-cache.c | 19 +++++++++++++------ 3 files changed, 15 insertions(+), 6 deletions(-)
cache.h +1 -0
@@ -1004,6 +1004,7 @@ enum fsync_component {FSYNC_COMPONENT_PACK =1 <<1 ,FSYNC_COMPONENT_PACK_METADATA =1 <<2 ,FSYNC_COMPONENT_COMMIT_GRAPH =1 <<3 , + FSYNC_COMPONENT_INDEX = 1 << 4, };#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \
config.c +1 -0
@@ -1331,6 +1331,7 @@ static const struct fsync_component_name { { "pack", FSYNC_COMPONENT_PACK }, { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA }, { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, + { "index", FSYNC_COMPONENT_INDEX }, };static enum fsync_component parse_fsync_components (const char *var,const char *string)
read-cache.c +13 -6
@@ -2842,7 +2842,7 @@ static int record_ieot(void)* rely on it. */ static int do_write_index (struct index_state *istate,struct tempfile *tempfile, -int strip_extensions) +int strip_extensions,unsigned flags) {uint64_t start =getnanotime ();struct hashfile *f;
@@ -2856,6 +2856,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,struct strbuf previous_name_buf =STRBUF_INIT , *previous_name;int drop_cache_tree = istate->drop_cache_tree;off_t offset; +int csum_fsync_flag;int ieot_entries =1 ;struct index_entry_offset_table *ieot =NULL ;int nr, nr_threads;
@@ -3089,7 +3090,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,return -1 ; } -finalize_hashfile (f, istate->oid.hash,FSYNC_COMPONENT_NONE ,CSUM_HASH_IN_STREAM ); + csum_fsync_flag =0 ; +if (!alternate_index_output && (flags &COMMIT_LOCK )) + csum_fsync_flag =CSUM_FSYNC ; + +finalize_hashfile (f, istate->oid.hash,FSYNC_COMPONENT_INDEX , +CSUM_HASH_IN_STREAM | csum_fsync_flag); +if (close_tempfile_gently (tempfile)) {error (_ ("could not close '%s'" ),get_tempfile_path (tempfile));return -1 ;
@@ -3144,7 +3151,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l*/ trace2_region_enter_printf ("index" ,"do_write_index" , the_repository,"%s" ,get_lock_file_path (lock)); - ret =do_write_index (istate, lock->tempfile,0 ); + ret =do_write_index (istate, lock->tempfile,0 , flags);trace2_region_leave_printf ("index" ,"do_write_index" , the_repository,"%s" ,get_lock_file_path (lock));
@@ -3238,7 +3245,7 @@ static int clean_shared_index_files(const char *current_hex) }static int write_shared_index (struct index_state *istate, -struct tempfile **temp) +struct tempfile **temp,unsigned flags) {struct split_index *si = istate->split_index;int ret, was_full = !istate->sparse_index;
@@ -3248,7 +3255,7 @@ static int write_shared_index(struct index_state *istate,trace2_region_enter_printf ("index" ,"shared/do_write_index" , the_repository,"%s" ,get_tempfile_path (*temp)); - ret =do_write_index (si->base, *temp,1 ); + ret =do_write_index (si->base, *temp,1 , flags);trace2_region_leave_printf ("index" ,"shared/do_write_index" , the_repository,"%s" ,get_tempfile_path (*temp));
@@ -3357,7 +3364,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, ret =do_write_locked_index (istate, lock, flags); goto out; } - ret =write_shared_index (istate, &temp); + ret =write_shared_index (istate, &temp, flags); saved_errno = errno;if (is_tempfile_active (temp))
Signature · 1 line
gitgitgadget
PATCH v6 6/6 core.fsync: documentation and user-friendly aggregate options
In reply to Neeraj K. Singh via GitGitGadgetFrom: Neeraj Singh <[email protected]>
This commit adds aggregate options for the core.fsync setting that are more user-friendly. These options are specified in terms of 'levels of safety', indicating which Git operations are considered to be sync points for durability.
The new documentation is also included here in its entirety for ease of review.
--- Documentation/config/core.txt | 40 +++++++++++++++++++++++++++++++++++ cache.h | 23 +++++++++++++++++--- config.c | 5 +++++ 3 files changed, 65 insertions(+), 3 deletions(-)
Documentation/config/core.txt +40 -0
@@ -547,6 +547,46 @@ core.whitespace:: is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent` errors. The default tab width is 8. Allowed values are 1 to 63. +core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The platform default on most platform is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commands + is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.
cache.h +20 -3
@@ -1007,9 +1007,26 @@ enum fsync_component { FSYNC_COMPONENT_INDEX = 1 << 4, }; -#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \ - FSYNC_COMPONENT_PACK_METADATA | \ - FSYNC_COMPONENT_COMMIT_GRAPH) +#define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \ + FSYNC_COMPONENT_PACK) + +#define FSYNC_COMPONENTS_DERIVED_METADATA (FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH) + +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENTS_OBJECTS | \ + FSYNC_COMPONENTS_DERIVED_METADATA | \ + ~FSYNC_COMPONENT_LOOSE_OBJECT) + +#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS) + +#define FSYNC_COMPONENTS_ADDED (FSYNC_COMPONENTS_COMMITTED | \ + FSYNC_COMPONENT_INDEX) + +#define FSYNC_COMPONENTS_ALL (FSYNC_COMPONENT_LOOSE_OBJECT | \ + FSYNC_COMPONENT_PACK | \ + FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH | \ + FSYNC_COMPONENT_INDEX)/* * A bitmask indicating which components of the repo should be fsynced.
config.c +5 -0
@@ -1332,6 +1332,11 @@ static const struct fsync_component_name { { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA }, { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, { "index", FSYNC_COMPONENT_INDEX }, + { "objects", FSYNC_COMPONENTS_OBJECTS }, + { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA }, + { "committed", FSYNC_COMPONENTS_COMMITTED }, + { "added", FSYNC_COMPONENTS_ADDED }, + { "all", FSYNC_COMPONENTS_ALL }, };static enum fsync_component parse_fsync_components (const char *var,const char *string)
Signature · 1 line
gitgitgadget
PATCH v7 1/1 core.fsync: documentation and user-friendly aggregate options
This commit adds aggregate options for the core.fsync setting that are more user-friendly. These options are specified in terms of 'levels of safety', indicating which Git operations are considered to be sync points for durability.
The new documentation is also included here in its entirety for ease of review.
---
This revision fixes a grammatical mistake in the core.fsync documentation.
Documentation/config/core.txt | 40 +++++++++++++++++++++++++++++++++++ cache.h | 23 +++++++++++++++++--- config.c | 5 +++++ 3 files changed, 65 insertions(+), 3 deletions(-)
Documentation/config/core.txt +40 -0
@@ -547,6 +547,46 @@ core.whitespace:: is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent` errors. The default tab width is 8. Allowed values are 1 to 63. +core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commands + is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.
cache.h +20 -3
@@ -1007,9 +1007,26 @@ enum fsync_component { FSYNC_COMPONENT_INDEX = 1 << 4, }; -#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENT_PACK | \ - FSYNC_COMPONENT_PACK_METADATA | \ - FSYNC_COMPONENT_COMMIT_GRAPH) +#define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \ + FSYNC_COMPONENT_PACK) + +#define FSYNC_COMPONENTS_DERIVED_METADATA (FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH) + +#define FSYNC_COMPONENTS_DEFAULT (FSYNC_COMPONENTS_OBJECTS | \ + FSYNC_COMPONENTS_DERIVED_METADATA | \ + ~FSYNC_COMPONENT_LOOSE_OBJECT) + +#define FSYNC_COMPONENTS_COMMITTED (FSYNC_COMPONENTS_OBJECTS) + +#define FSYNC_COMPONENTS_ADDED (FSYNC_COMPONENTS_COMMITTED | \ + FSYNC_COMPONENT_INDEX) + +#define FSYNC_COMPONENTS_ALL (FSYNC_COMPONENT_LOOSE_OBJECT | \ + FSYNC_COMPONENT_PACK | \ + FSYNC_COMPONENT_PACK_METADATA | \ + FSYNC_COMPONENT_COMMIT_GRAPH | \ + FSYNC_COMPONENT_INDEX)/* * A bitmask indicating which components of the repo should be fsynced.
config.c +5 -0
@@ -1332,6 +1332,11 @@ static const struct fsync_component_name { { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA }, { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, { "index", FSYNC_COMPONENT_INDEX }, + { "objects", FSYNC_COMPONENTS_OBJECTS }, + { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA }, + { "committed", FSYNC_COMPONENTS_COMMITTED }, + { "added", FSYNC_COMPONENTS_ADDED }, + { "all", FSYNC_COMPONENTS_ALL }, };static enum fsync_component parse_fsync_components (const char *var,const char *string)
Signature · 1 line
2.33.0.334.g55a40fc8fd
Re: [PATCH v7] core.fsync: documentation and user-friendly aggregate options
Quoted 11 lines · Neeraj Singh
This commit adds aggregate options for the core.fsync setting that are more user-friendly. These options are specified in terms of 'levels of safety', indicating which Git operations are considered to be sync points for durability.
The new documentation is also included here in its entirety for ease of review.
Signed-off-by Neeraj Singh <[email protected]>---
This revision fixes a grammatical mistake in the core.fsync documentation.
Is this meant to be [PATCH v7 6/6] where 1-5/6 of v7 are supposed to be identical to their counterparts of v6 and therefor not sent? Not complaining, just double-checking, as I do not want to assume and end up missing the final updates to the other 5.
In the meantime, I'd assume that it is the case, and will fix the author ident (you sent this from your gmail address) before replacing the last bit.
The only change from the previous round is "the platform default on most platform" -> "the default on most platforms", which looks sensible.
Thanks.
1: 39f4b94c2c ! 1: dfeab99d23 core.fsync: documentation and user-friendly aggregate options
@@
## Metadata ##
-Author: Neeraj Singh <[email protected]>
+Author: Neeraj Singh <[email protected]>
## Commit message ##
core.fsync: documentation and user-friendly aggregate options
@@ Documentation/config/core.txt: core.whitespace::
+is ignored.
++
+The empty string resets the fsync configuration to the platform
-+default. The platform default on most platform is equivalent to ++default. The default on most platforms is equivalent to
+core.fsync=committed,-loose-object, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++
Thanks.
Re: [PATCH v7] core.fsync: documentation and user-friendly aggregate options
Quoted 24 lines · Junio C Hamano
Quoted 11 lines · Neeraj Singh
This commit adds aggregate options for the core.fsync setting that are more user-friendly. These options are specified in terms of 'levels of safety', indicating which Git operations are considered to be sync points for durability.
The new documentation is also included here in its entirety for ease of review.
Signed-off-by Neeraj Singh <[email protected]>---
This revision fixes a grammatical mistake in the core.fsync documentation.Is this meant to be [PATCH v7 6/6] where 1-5/6 of v7 are supposed to be identical to their counterparts of v6 and therefor not sent? Not complaining, just double-checking, as I do not want to assume and end up missing the final updates to the other 5.
In the meantime, I'd assume that it is the case, and will fix the author ident (you sent this from your gmail address) before replacing the last bit.
Quoted 5 lines
The only change from the previous round is "the platform default on most platform" -> "the default on most platforms", which looks sensible.
Thanks.
Yes, that's right. I was trying out git-send-email for the first
time. I didn't want to spam the list with a whole new series where
only one patch had a change.
Quoted 20 lines
1:
39f4b94c2c! 1:dfeab99d23core.fsync: documentation and user-friendly aggregate options
@@
## Metadata ##
-Author: Neeraj Singh <[email protected]>
+Author: Neeraj Singh <[email protected]>## Commit message ##
core.fsync: documentation and user-friendly aggregate options
@@Documentation/config/core.txt: core.whitespace::
+is ignored.
++
+The empty string resets the fsync configuration to the platform
-+default. The platform default on most platform is equivalent to ++default. The default on most platforms is equivalent to
+core.fsync=committed,-loose-object, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++Thanks.
do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
In reply to Neeraj SinghOn Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the wildmatch() based thing you didn't go for.
But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commands
Review
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.
On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.
I.e. I think we should just do our default fsync() of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.
But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
If someone really needs this level of detail they can LD_PRELOAD something to have fsync intercept fd's and paths, and act appropriately.
Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" up fsync() operations across these configuration boundaries, which this level of configuration would seem to preclude.
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call fsync() on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a single fsync() for all of PACKs, their metadata files, commit-graph etc., and then rename() things in-place as appropriate afterwards.
Documentation/config/core.txt +26 -40
@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be
@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination with -`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the old
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 62 lines · Ævar Arnfjörð Bjarmason
On Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the
wildmatch()based thing you didn't go for.But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commandsReview
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.
Quoted 8 lines
I.e. I think we should just do our default
fsync()of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
Quoted 9 lines
If someone really needs this level of detail they can
LD_PRELOADsomething to have fsync intercept fd's and paths, and act appropriately.Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" upfsync()operations across these configuration boundaries, which this level of configuration would seem to preclude.
Quoted 5 lines
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call
fsync()on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/
99.9% of users don't care and won't look. The ones who do look deeper and understand the issues have source code and access to this ML discussion to understand why this works this way.
Quoted 90 lines
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a singlefsync()for all of PACKs, their metadata files, commit-graph etc., and thenrename()things in-place as appropriate afterwards.Documentation/config/core.txt +24
-38@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be
Documentation/config/core.txt backed up by any standard (e.g. POSIX), but worked in practice on some · line 673
Quoted 10 lines
Documentation/config/core.txt +2
-2@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination with -`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the old
I'm at the point where I don't want to endlessly revisit this discussion.
-Neeraj
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 73 lines · Neeraj Singh
Quoted 62 lines · Ævar Arnfjörð Bjarmason
On Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the
wildmatch()based thing you didn't go for.But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commandsReview
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.Quoted 8 lines
I.e. I think we should just do our default
fsync()of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
Quoted 5 lines
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
I'm prettty sure (but Patrick knows more) that GitLab's plan for this is to keep it at whatever the safest setting is, presumably GitHub's as well (but I don't know at all on that front).
Quoted 15 lines
Quoted 9 lines
If someone really needs this level of detail they can
LD_PRELOADsomething to have fsync intercept fd's and paths, and act appropriately.Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" upfsync()operations across these configuration boundaries, which this level of configuration would seem to preclude.Quoted 5 lines
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call
fsync()on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/
Quoted 3 lines
99.9% of users don't care and won't look. The ones who do look deeper and understand the issues have source code and access to this ML discussion to understand why this works this way.
Exactly, so we can hopefully have a simpler interface.
Quoted 94 lines
Quoted 94 lines
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a singlefsync()for all of PACKs, their metadata files, commit-graph etc., and thenrename()things in-place as appropriate afterwards.Documentation/config/core.txt +24
-39@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination with
Quoted 8 lines
Documentation/config/core.txt backed up by any standard (e.g. POSIX), but worked in practice on some · line 673
Quoted 6 lines
Documentation/config/core.txt +2
-1-`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the oldI'm at the point where I don't want to endlessly revisit this discussion.
Sorry, my intention isn't to frustrate you, but I do think it's important to get this right.
Particularly since this is now in "next", and we're getting closer to a release. We can either talk about this now and decide on something, or it'll be in a release, and then publicly documented promises will be harder to back out of.
I think your suggestion of just hiding the relevant documentation would be a good band-aid solution to that.
But I also think that given how I was altering this in my RFC series that the premise of how this could be structured has been called into question in a way that we didn't (or I don't recall) us having discussed before.
I.e. that we can say "sync loose, but not index", or "sync index, but not loose" with this config schema. When with "bulk" we it really isn't any more expensive to do both if one is true (even cheaper, actually).
Re: do we have too much fsync() configuration in 'next'?
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 5 lines
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
I'm prettty sure (but Patrick knows more) that GitLab's plan for this is to keep it at whatever the safest setting is, presumably GitHub's as well (but I don't know at all on that front).
I thought we've already settled it long ago, and it wasn't like you were taking vacation from the list for a few weeks. Why are we talking about this again now? Did we discover any new argument against it?
Puzzled.
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
In reply to Ævar Arnfjörð BjarmasonQuoted 222 lines · Ævar Arnfjörð Bjarmason
Quoted 73 lines · Neeraj Singh
Quoted 62 lines · Ævar Arnfjörð Bjarmason
On Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the
wildmatch()based thing you didn't go for.But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commandsReview
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.Quoted 8 lines
I.e. I think we should just do our default
fsync()of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
Quoted 5 lines
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
I'm prettty sure (but Patrick knows more) that GitLab's plan for this is to keep it at whatever the safest setting is, presumably GitHub's as well (but I don't know at all on that front).
Quoted 15 lines
Quoted 9 lines
If someone really needs this level of detail they can
LD_PRELOADsomething to have fsync intercept fd's and paths, and act appropriately.Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" upfsync()operations across these configuration boundaries, which this level of configuration would seem to preclude.Quoted 5 lines
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call
fsync()on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/Quoted 3 lines
99.9% of users don't care and won't look. The ones who do look deeper and understand the issues have source code and access to this ML discussion to understand why this works this way.
Exactly, so we can hopefully have a simpler interface.
Quoted 94 lines
Quoted 94 lines
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a singlefsync()for all of PACKs, their metadata files, commit-graph etc., and thenrename()things in-place as appropriate afterwards.Documentation/config/core.txt +24
-39@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination withQuoted 8 lines
Documentation/config/core.txt backed up by any standard (e.g. POSIX), but worked in practice on some · line 673
Quoted 6 lines
Documentation/config/core.txt +2
-1-`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the oldI'm at the point where I don't want to endlessly revisit this discussion.
Sorry, my intention isn't to frustrate you, but I do think it's important to get this right.
Particularly since this is now in "next", and we're getting closer to a release. We can either talk about this now and decide on something, or it'll be in a release, and then publicly documented promises will be harder to back out of.
I think your suggestion of just hiding the relevant documentation would be a good band-aid solution to that.
Quoted 9 lines
But I also think that given how I was altering this in my RFC series that the premise of how this could be structured has been called into question in a way that we didn't (or I don't recall) us having discussed before.
I.e. that we can say "sync loose, but not index", or "sync index, but not loose" with this config schema. When with "bulk" we it really isn't any more expensive to do both if one is true (even cheaper, actually).
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 234 lines · Neeraj Singh
Quoted 222 lines · Ævar Arnfjörð Bjarmason
Quoted 73 lines · Neeraj Singh
Quoted 62 lines · Ævar Arnfjörð Bjarmason
On Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the
wildmatch()based thing you didn't go for.But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commandsReview
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.Quoted 8 lines
I.e. I think we should just do our default
fsync()of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
Quoted 5 lines
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
I'm prettty sure (but Patrick knows more) that GitLab's plan for this is to keep it at whatever the safest setting is, presumably GitHub's as well (but I don't know at all on that front).
Quoted 15 lines
Quoted 9 lines
If someone really needs this level of detail they can
LD_PRELOADsomething to have fsync intercept fd's and paths, and act appropriately.Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" upfsync()operations across these configuration boundaries, which this level of configuration would seem to preclude.Quoted 5 lines
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call
fsync()on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/Quoted 3 lines
99.9% of users don't care and won't look. The ones who do look deeper and understand the issues have source code and access to this ML discussion to understand why this works this way.
Exactly, so we can hopefully have a simpler interface.
Quoted 94 lines
Quoted 94 lines
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a singlefsync()for all of PACKs, their metadata files, commit-graph etc., and thenrename()things in-place as appropriate afterwards.Documentation/config/core.txt +24
-39@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination withQuoted 8 lines
Documentation/config/core.txt backed up by any standard (e.g. POSIX), but worked in practice on some · line 673
Quoted 6 lines
Documentation/config/core.txt +2
-1-`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the oldI'm at the point where I don't want to endlessly revisit this discussion.
Sorry, my intention isn't to frustrate you, but I do think it's important to get this right.
Particularly since this is now in "next", and we're getting closer to a release. We can either talk about this now and decide on something, or it'll be in a release, and then publicly documented promises will be harder to back out of.
I think your suggestion of just hiding the relevant documentation would be a good band-aid solution to that.
Quoted 9 lines
But I also think that given how I was altering this in my RFC series that the premise of how this could be structured has been called into question in a way that we didn't (or I don't recall) us having discussed before.
I.e. that we can say "sync loose, but not index", or "sync index, but not loose" with this config schema. When with "bulk" we it really isn't any more expensive to do both if one is true (even cheaper, actually).
Quoted 7 lines
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
So for that use-case you'd like to fsync the loose objects (if any), but not the index? So the FS will "flush" up to the index, and then queue the index for later syncing to platter?
But even in that case don't the settings need to be tied to one another, because in the method=bulk sync=index && sync=!loose case wouldn't we be syncing "loose" in any case?
Quoted 4 lines
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Yeah that would alleviate most of my concerns here, but the docs aren't saying anything like that. Since you added them & they just landed, do you mind doing a small follow-up where we e.g. say that these new settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 260 lines · Ævar Arnfjörð Bjarmason
Quoted 234 lines · Neeraj Singh
Quoted 222 lines · Ævar Arnfjörð Bjarmason
Quoted 73 lines · Neeraj Singh
Quoted 62 lines · Ævar Arnfjörð Bjarmason
On Tue, Mar 15 2022, Neeraj Singh wrote:
I know this is probably 80% my fault by egging you on about initially adding the
wildmatch()based thing you didn't go for.But having looked at this with fresh eyes quite deeply I really think we're severely over-configuring things here:
Quoted 33 lines
(quoted diff) +33
-0+core.fsync:: + A comma-separated list of components of the repository that + should be hardened via the core.fsyncMethod when created or + modified. You can disable hardening of any component by + prefixing it with a '-'. Items that are not hardened may be + lost in the event of an unclean system shutdown. Unless you + have special requirements, it is recommended that you leave + this option empty or pick one of `committed`, `added`, + or `all`. ++ +When this configuration is encountered, the set of components starts with +the platform default value, disabled components are removed, and additional +components are added. `none` resets the state so that the platform default +is ignored. ++ +The empty string resets the fsync configuration to the platform +default. The default on most platforms is equivalent to +`core.fsync=committed,-loose-object`, which has good performance, +but risks losing recent work in the event of an unclean system shutdown. ++ +* `none` clears the set of fsynced components. +* `loose-object` hardens objects added to the repo in loose-object form. +* `pack` hardens objects added to the repo in packfile form. +* `pack-metadata` hardens packfile bitmaps and indexes. +* `commit-graph` hardens the commit graph file. +* `index` hardens the index when it is modified. +* `objects` is an aggregate option that is equivalent to + `loose-object,pack`. +* `derived-metadata` is an aggregate option that is equivalent to + `pack-metadata,commit-graph`. +* `committed` is an aggregate option that is currently equivalent to + `objects`. This mode sacrifices some performance to ensure that work + that is committed to the repository with `git commit` or similar commandsReview
Quoted 10 lines
(quoted diff) +7
-0+ is hardened. +* `added` is an aggregate option that is currently equivalent to + `committed,index`. This mode sacrifices additional performance to + ensure that the results of commands like `git add` and similar operations + are hardened. +* `all` is an aggregate option that syncs all individual components above. + core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives.On top of my
https://lore.kernel.org/git/[email protected]/
which makes the tmp-objdir part of your not-in-next-just-seen follow-up series configurable via "fsyncMethod.batch.quarantine" I really think we should just go for something like the belwo patch (note that misspelled/mistook "bulk" for "batch" in that linked-t patch, fixed below.Quoted 8 lines
I.e. I think we should just do our default
fsync()of everything, and probably SOON make the fsync-ing of loose objects the default. Those who care about performance will have "batch" (or "writeout-only"), which we can have OS-specific detections for.But really, all of the rest of this is unduly boxing us into overconfiguration that I think nobody really needs.
Quoted 5 lines
We've gone over this a few times already, but just wanted to state it again. The really detailed settings are really there for Git hosters like GitLab or GitHub. I'd be happy to remove the per-component core.fsync values from the documentation and leave just the ones we point the user to.
I'm prettty sure (but Patrick knows more) that GitLab's plan for this is to keep it at whatever the safest setting is, presumably GitHub's as well (but I don't know at all on that front).
Quoted 15 lines
Quoted 9 lines
If someone really needs this level of detail they can
LD_PRELOADsomething to have fsync intercept fd's and paths, and act appropriately.Worse, as the RFC series I sent
(https://lore.kernel.org/git/[email protected]/)
shows we can and should "batch" upfsync()operations across these configuration boundaries, which this level of configuration would seem to preclude.Quoted 5 lines
Or, we'd need to explain why "core.fsync=loose-object" won't *actually* call
fsync()on a single loose object's fd under "batch" as I had to do on top of this in
https://lore.kernel.org/git/[email protected]/Quoted 3 lines
99.9% of users don't care and won't look. The ones who do look deeper and understand the issues have source code and access to this ML discussion to understand why this works this way.
Exactly, so we can hopefully have a simpler interface.
Quoted 94 lines
Quoted 94 lines
The same is going to apply for almost all of the rest of these configuration categories.
I.e. a natural follow-up to e.g. batching across objects & index as I'm doing in
https://lore.kernel.org/git/[email protected]/
is to do likewise for all the PACK-related stuff before we rename it in-place. Or even have "git gc" issue only a singlefsync()for all of PACKs, their metadata files, commit-graph etc., and thenrename()things in-place as appropriate afterwards.Documentation/config/core.txt +24
-39@@ -548,49 +548,35 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsync:: - A comma-separated list of components of the repository that - should be hardened via the core.fsyncMethod when created or - modified. You can disable hardening of any component by - prefixing it with a '-'. Items that are not hardened may be - lost in the event of an unclean system shutdown. Unless you - have special requirements, it is recommended that you leave - this option empty or pick one of `committed`, `added`, - or `all`. -+ -When this configuration is encountered, the set of components starts with -the platform default value, disabled components are removed, and additional -components are added. `none` resets the state so that the platform default -is ignored. -+ -The empty string resets the fsync configuration to the platform -default. The default on most platforms is equivalent to -`core.fsync=committed,-loose-object`, which has good performance, -but risks losing recent work in the event of an unclean system shutdown. -+ -* `none` clears the set of fsynced components. -* `loose-object` hardens objects added to the repo in loose-object form. -* `pack` hardens objects added to the repo in packfile form. -* `pack-metadata` hardens packfile bitmaps and indexes. -* `commit-graph` hardens the commit graph file. -* `index` hardens the index when it is modified. -* `objects` is an aggregate option that is equivalent to - `loose-object,pack`. -* `derived-metadata` is an aggregate option that is equivalent to - `pack-metadata,commit-graph`. -* `committed` is an aggregate option that is currently equivalent to - `objects`. This mode sacrifices some performance to ensure that work - that is committed to the repository with `git commit` or similar commands - is hardened. -* `added` is an aggregate option that is currently equivalent to - `committed,index`. This mode sacrifices additional performance to - ensure that the results of commands like `git add` and similar operations - are hardened. -* `all` is an aggregate option that syncs all individual components above. + A boolen defaulting to `true`. To ensure data integrity git + will fsync() its objects, index and refu updates etc. This can + be set to `false` to disable `fsync()`-ing. ++ +Only set this to `false` if you know what you're doing, and are +prepared to deal with data corruption. Valid use-cases include +throwaway uses of repositories on ramdisks, one-off mass-imports +followed by calling `sync(1)` etc. ++ +Note that the syncing of loose objects is currently excluded from +`core.fsync=true`. To turn on all fsync-ing you'll need +`core.fsync=true` and `core.fsyncObjectFiles=true`, but see +`core.fsyncMethod=batch` below for a much faster alternative that's +just as safe on various modern OS's. ++ +The default is in flux and may change in the future, in particular the +equivalent of the already-deprecated `core.fsyncObjectFiles` setting +might soon default to `true`, and `core.fsyncMethod`'s default of +`fsync` might default to a setting deemed to be safe on the local OS, +suc has `batch` or `writeout-only` core.fsyncMethod:: A value indicating the strategy Git will use to harden repository data using fsync and related primitives. + +Defaults to `fsync`, but as discussed for `core.fsync` above might +change to use one of the values below taking advantage of +platform-specific "faster `fsync()`". ++ * `fsync` uses the fsync() system call or platform equivalents. * `writeout-only` issues pagecache writeback requests, but depending on the filesystem and storage hardware, data added to the repository may not be@@ -680,8 +666,8 @@ backed up by any standard (e.g. POSIX), but worked in practice on some Linux setups. + Nowadays you should almost certainly want to use -`core.fsync=loose-object` instead in combination withQuoted 8 lines
Documentation/config/core.txt backed up by any standard (e.g. POSIX), but worked in practice on some · line 673
Quoted 6 lines
Documentation/config/core.txt +2
-1-`core.fsyncMethod=bulk`, and possibly with +`core.fsync=true` instead in combination with +`core.fsyncMethod=batch`, and possibly with `fsyncMethod.batch.quarantine=true`, see above. On modern OS's (Linux, OSX, Windows) that gives you most of the performance benefit of `core.fsyncObjectFiles=false` with all of the safety of the oldI'm at the point where I don't want to endlessly revisit this discussion.
Sorry, my intention isn't to frustrate you, but I do think it's important to get this right.
Particularly since this is now in "next", and we're getting closer to a release. We can either talk about this now and decide on something, or it'll be in a release, and then publicly documented promises will be harder to back out of.
I think your suggestion of just hiding the relevant documentation would be a good band-aid solution to that.
Quoted 9 lines
But I also think that given how I was altering this in my RFC series that the premise of how this could be structured has been called into question in a way that we didn't (or I don't recall) us having discussed before.
I.e. that we can say "sync loose, but not index", or "sync index, but not loose" with this config schema. When with "bulk" we it really isn't any more expensive to do both if one is true (even cheaper, actually).
Quoted 7 lines
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
So for that use-case you'd like to fsync the loose objects (if any), but not the index? So the FS will "flush" up to the index, and then queue the index for later syncing to platter?
But even in that case don't the settings need to be tied to one another, because in the method=bulk sync=index && sync=!loose case wouldn't we be syncing "loose" in any case?
Quoted 4 lines
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Quoted 4 lines
Yeah that would alleviate most of my concerns here, but the docs aren't saying anything like that. Since you added them & they just landed, do you mind doing a small follow-up where we e.g. say that these new settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
The doc is already pretty prescriptive. It has this line at the end of the first paragraph:
"Unless you
have special requirements, it is recommended that you leave this option empty or pick one of committed, added,
or all."
Those values are already designed to change as Git changes.
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 8 lines · Neeraj Singh
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · Neeraj Singh
On Fri, Mar 25, 2022 at 5:33 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
[...]
Quoted 27 lines
Quoted 22 lines
Quoted 7 lines
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
So for that use-case you'd like to fsync the loose objects (if any), but not the index? So the FS will "flush" up to the index, and then queue the index for later syncing to platter?
But even in that case don't the settings need to be tied to one another, because in the method=bulk sync=index && sync=!loose case wouldn't we be syncing "loose" in any case?
Quoted 4 lines
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Quoted 4 lines
Yeah that would alleviate most of my concerns here, but the docs aren't saying anything like that. Since you added them & they just landed, do you mind doing a small follow-up where we e.g. say that these new settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
Quoted 8 lines
The doc is already pretty prescriptive. It has this line at the end of the first paragraph:
"Unless you
have special requirements, it is recommended that you leave this option empty or pick one ofcommitted,added,
orall."Those values are already designed to change as Git changes.
I'm referring to the documentation as it stands not being marked as experimental in the sense that we might decide to re-do this to a large extent, i.e. something like the diff I suggested upthread in
https://lore.kernel.org/git/[email protected]/
So yes, I agree that it e.g. clearly states that you can add a new core.git=foobar or whatever down the line, but it clearly doesn't suggest that e.g. core.fsync might have boolean semantics in some later version, or that the rest might simply be ignored, even if that e.g. means that we wouldn't sync loose objects on core.fsync=loose-object, as we'd just warn with a "we don't provide this anymore".
Or do you disagree with that? IOW I mean that we'd do something like this, either in docs or code:
config.c +6 -0
@@ -1675,6 +1675,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsync" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var); fsync_components =parse_fsync_components (var, value);
@@ -1682,6 +1685,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsyncmethod" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var);if (!strcmp (value,"fsync" ))
repo-settings.c +1 -0
@@ -31,6 +31,7 @@ void prepare_repo_settings(struct repository *r)/* Booleans config or default, cascades to other settings */ repo_cfg_bool (r,"feature.manyfiles" , &manyfiles,0 );repo_cfg_bool (r,"feature.experimental" , &experimental,0 ); + r->settings.feature_experimental = experimental;/* Defaults modified by feature.* */ if (experimental) {
repository.h +1 -0
@@ -28,6 +28,7 @@ enum fetch_negotiation_setting {struct repo_settings {int initialized; +int feature_experimental;int core_commit_graph;int commit_graph_read_changed_paths;int gc_write_commit_graph;
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 79 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Neeraj Singh
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · Neeraj Singh
On Fri, Mar 25, 2022 at 5:33 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
[...]
Quoted 27 lines
Quoted 22 lines
Quoted 7 lines
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
So for that use-case you'd like to fsync the loose objects (if any), but not the index? So the FS will "flush" up to the index, and then queue the index for later syncing to platter?
But even in that case don't the settings need to be tied to one another, because in the method=bulk sync=index && sync=!loose case wouldn't we be syncing "loose" in any case?
Quoted 4 lines
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Quoted 4 lines
Yeah that would alleviate most of my concerns here, but the docs aren't saying anything like that. Since you added them & they just landed, do you mind doing a small follow-up where we e.g. say that these new settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
Quoted 8 lines
The doc is already pretty prescriptive. It has this line at the end of the first paragraph:
"Unless you
have special requirements, it is recommended that you leave this option empty or pick one ofcommitted,added,
orall."Those values are already designed to change as Git changes.
I'm referring to the documentation as it stands not being marked as experimental in the sense that we might decide to re-do this to a large extent, i.e. something like the diff I suggested upthread in
https://lore.kernel.org/git/[email protected]/So yes, I agree that it e.g. clearly states that you can add a new core.git=foobar or whatever down the line, but it clearly doesn't suggest that e.g. core.fsync might have boolean semantics in some later version, or that the rest might simply be ignored, even if that e.g. means that we wouldn't sync loose objects on core.fsync=loose-object, as we'd just warn with a "we don't provide this anymore".
Or do you disagree with that? IOW I mean that we'd do something like this, either in docs or code:
config.c +3
-0@@ -1675,6 +1675,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsync" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var); fsync_components =parse_fsync_components (var, value);
config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1693
Quoted 9 lines
config.c +3
-0@@ -1682,6 +1685,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsyncmethod" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var);if (!strcmp (value,"fsync" ))
Let's please not tie this to feature.experimental=true. Setting that option has unintended sideeffects and will also change defaults which we may not want to have in production. I don't mind adding a warning in the docs though that the specific items which can be configured may be subject to change in the future.
At GitLab, we've got a three-step plan:
1. We need to migrate to core.fsync in the first place. In order to not migrate and change behaviour at the same point in time we already benefit from the fine-grainedness of this config because we can simply say core.fsync=loose-objects and have the same behaviour as before with core.fsyncLooseObjects=true.
2. We'll want to enable syncing of packfiles, which I think wasn't previously covered by core.fsyncLooseobjects.
3. We'll add refs to also sync loose refs to disk.
So while the end result will be the same as committed, having this level of control helps us to assess the impact in a nicer way by being able to do this step by step with feature flags.
On the other hand, many of the other parts we don't really care about. Auxiliary metadata like the commit-graph or pack indices are data that can in the worst case be regenerated by us, so it's not clear to me whether it makes to also enable fsyncing those in production.
So altogether, I agree with Neeraj: having the fine-grainedness greatly helps us to roll out changes like this and be able to pick what we deem to be important. Personally I would be fine with explicitly pointing out that there are two groups of this config in our docs though:
1. The "porcelain" group: "committed", "added", "all", "none". These are abstract groups whose behaviour should adapt as we change implementations, and are those that should typically be set by a user, if intended.
2. The "plumbing" or "expert" group: these are fine-grained options which shouldn't typically be used by Git users. They still have merit though in hosting environments, where requirements are typically a lot more specific.
We may also provide different guarantees for both groups. The first one should definitely be stable, but we might state that the second group is subject to change in the future.
Patrick
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 83 lines · Patrick Steinhardt
[[PGP Signed Part:Undecided]]
Quoted 81 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Neeraj Singh
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · Neeraj Singh
On Fri, Mar 25, 2022 at 5:33 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
[...]
Quoted 27 lines
Quoted 22 lines
Quoted 7 lines
I want to make a comment about the Index here. Syncing the index is strictly required for the "added" level of consistency, so that we don't lose stuff that leaves the work tree but is staged. But my Windows enlistment has an index that's 266MB, which would be painful to sync even with all the optimizations. Maybe with split-index, this wouldn't be so bad, but I just wanted to call out that some advanced users may really care about the configurability.
So for that use-case you'd like to fsync the loose objects (if any), but not the index? So the FS will "flush" up to the index, and then queue the index for later syncing to platter?
But even in that case don't the settings need to be tied to one another, because in the method=bulk sync=index && sync=!loose case wouldn't we be syncing "loose" in any case?
Quoted 4 lines
As Git's various database implementations improve, the fsync stuff will hopefully be more optimal and self-tuning. But as that happens, Git could just start ignoring settings that lose meaning without tying anyones hands.
Quoted 4 lines
Yeah that would alleviate most of my concerns here, but the docs aren't saying anything like that. Since you added them & they just landed, do you mind doing a small follow-up where we e.g. say that these new settings are "EXPERIMENTAL" or whatever, and subject to drastic change?
Quoted 8 lines
The doc is already pretty prescriptive. It has this line at the end of the first paragraph:
"Unless you
have special requirements, it is recommended that you leave this option empty or pick one ofcommitted,added,
orall."Those values are already designed to change as Git changes.
I'm referring to the documentation as it stands not being marked as experimental in the sense that we might decide to re-do this to a large extent, i.e. something like the diff I suggested upthread in
https://lore.kernel.org/git/[email protected]/So yes, I agree that it e.g. clearly states that you can add a new core.git=foobar or whatever down the line, but it clearly doesn't suggest that e.g. core.fsync might have boolean semantics in some later version, or that the rest might simply be ignored, even if that e.g. means that we wouldn't sync loose objects on core.fsync=loose-object, as we'd just warn with a "we don't provide this anymore".
Or do you disagree with that? IOW I mean that we'd do something like this, either in docs or code:
config.c +3
-0@@ -1675,6 +1675,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }if (!strcmp (var,"core.fsync" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var); fsync_components =parse_fsync_components (var, value);@@ -1682,6 +1685,9 @@ static int git_default_core_config(const char *var, const char *value, void *cb) }
Quoted 13 lines
config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1693
Quoted 7 lines
config.c +3
-0if (!strcmp (var,"core.fsyncmethod" )) { +if (!the_repository->settings.feature_experimental) +warning (_ ("the '%s' configuration option is EXPERIMENTAL. opt-in to use it with feature.experimental=true" ), + var);if (!value)return config_error_nonbool (var);if (!strcmp (value,"fsync" ))Let's please not tie this to
feature.experimental=true. Setting that option has unintended sideeffects and will also change defaults which we may not want to have in production. I don't mind adding a warning in the docs though that the specific items which can be configured may be subject to change in the future.
Yes, that was a bad (throwaway) idea. I think probably any sort of warning is over-doing it, but having the same in the docs would be good, as in:
git gre EXPERIMENTAL -- Documentation
Quoted 7 lines
At GitLab, we've got a three-step plan:
1. We need to migrate to
core.fsyncin the first place. In order to not migrate and change behaviour at the same point in time we already benefit from the fine-grainedness of this config because we can simply saycore.fsync=loose-objectsand have the same behaviour as before withcore.fsyncLooseObjects=true.
*nod*.
Quoted 2 lines
2. We'll want to enable syncing of packfiles, which I think wasn't previously covered by
core.fsyncLooseobjects.
We've always fsynced packfiles and other things that use the finalize_hashfile() API. I.e. the pack metadata (idx,midx,bitmap etc.), commit-graph etc.
Which is one thing I find a bit uncomfortable about the proposed config schema, i.e. it's allowing *granular* unsafe behavior we didn't allow before.
I think we *should* allow disabling fsync() entirely via:
core.fsync=false
Per Eric Wong's [added to CC] proposal here:
https://lore.kernel.org/git/[email protected]/; I think that's useful for e.g. running git in CI, one off scripted mass-imports where you run "sync(1)" after (or not...).
But I don't really see the use-case for turning off say "index" or "pack-metadata", but otherwise keeping the default fsync().
Quoted 1 line
3. We'll add
refsto also sync loose refs to disk.
Maybe I'm reading this wrong, but AFAICT if you upgrade from pre-v2.36.0 to v2.36.0 you'll have no way to do fsync()-ing as it was done before with your bc22d845c43 (core.fsync: new option to harden references, 2022-03-11).
I.e. I first thought you meant to start with:
core.fsync=-loose-objects
core.fsync=-reference
And then remove that "core.fsync=-reference" line to get the behavior that'll be new in v2.36.0, but that won't do that. The new "reference" category doesn't just affect loose refs, but all ref updates.
So we don't have any way to move to v2.36.0 and get exactly the fsync() behavior we did before, or have I misread the code?
Now, I don't think we need it to be configurable at all.
I.e. I think your is a bc22d845c43 good change, but it feels weird to make it and leave the default of core.fsyncLooseObjects=false on the table. I.e. what you're summarizing there is true, but it's also true of the loose objects.
To the extent that we've had any reason at all not to sync those by default (which has really mostly been "we didn't re-visit it for a while") it's been performance.
And both loose refs & loose objects will suffer from the same degradation in performance from many fsync()'s.
IOW I think it's perfectly fine not to add a config knob for it other than core.fsync=false, and VERY SOON turn on core.fsyncLooseobjects=true, especially if we can get most of the performance benefits with the "bulk" mode.
But why half-way with bc22d845c43? I mean, *that change* should be narrow, but in terms of where we go next what do you think of the above?
Quoted 3 lines
So while the end result will be the same as
committed, having this level of control helps us to assess the impact in a nicer way by being able to do this step by step with feature flags.
*Nod*, although leaving aside the new syncing of loose refs the plan you
outlined above could be done with the proposal of the simpler:
core.fsync=true
core.fsyncLooseObjects=false
Quoted 4 lines
On the other hand, many of the other parts we don't really care about. Auxiliary metadata like the commit-graph or pack indices are data that can in the worst case be regenerated by us, so it's not clear to me whether it makes to also enable fsyncing those in production.
I'm not familiar with all of those in detail, i.e. how we behave specifically in the face of them being corrupt.
The commit-graph I am, we *should* recover "gracefully" there, but you might have an incident shortly there after due to "for-each-ref --contains" slowdowns by 1000x or whatever.
For e.g. *.idx we'd be hosed until manual recovery.
So I think those area all in the same bucket as core.fsync=false, and that we don't need the granularity.
Quoted 4 lines
So altogether, I agree with Neeraj: having the fine-grainedness greatly helps us to roll out changes like this and be able to pick what we deem to be important. Personally I would be fine with explicitly pointing out that there are two groups of this config in our docs though:
Yes, that's fair. But pending replies to the above I think the main point & proposal of us having too much config stands. I.e. depending on what you want to do with loose object refs we'd just need this:
core.fsync=[<bool>] # true by default core.fsyncLooseObjects=[<bool>] # false by default core.fsyncLooseRefs=[<bool>] # true by default?
And then the "bulk" config, which would be orthagonal to this.
I.e. do we really have a use-case for the rest of the kitchen sink?
Quoted 5 lines
1. The "porcelain" group: "committed", "added", "all", "none". These are abstract groups whose behaviour should adapt as we change implementations, and are those that should typically be set by a user, if intended.
Quoted 8 lines
2. The "plumbing" or "expert" group: these are fine-grained options which shouldn't typically be used by Git users. They still have merit though in hosting environments, where requirements are typically a lot more specific.
We may also provide different guarantees for both groups. The first one should definitely be stable, but we might state that the second group is subject to change in the future.
I hope we can work something out :)
Overall: I think you've left one of the the main things I brought up[1] unaddressed, i.e. that the core.fsync config schema in its current form assumes that we can sync A or B, and configure those separately.
Which AFAIKT is because Neeraj's initial implementation & the discussion was focused on finishing A or B with a per-"group" "cookie" to flush the files.
But as [2] shows it's more performant for us to simply defer the fsync of A until the committing of B.
Which is the main reason I think we should be re-visiting this. Sure, if we were just syncing A, B or C having per-[ABC] config options might be a bit overdoing it, but would be relatively simple.
But once we start using a more optimized version of the "bulk" mode the config schema will be making promises about individual steps in a transaction that I think we'll want to leave opaque, and only promise that when git returns it will have synced all the relevant assets as efficiently as possible.
1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 18 lines · Ævar Arnfjörð Bjarmason
I hope we can work something out :)
Overall: I think you've left one of the the main things I brought up[1] unaddressed, i.e. that the core.fsync config schema in its current form assumes that we can sync A or B, and configure those separately.
Which AFAIKT is because Neeraj's initial implementation & the discussion was focused on finishing A or B with a per-"group" "cookie" to flush the files.
But as [2] shows it's more performant for us to simply defer the fsync of A until the committing of B.
Which is the main reason I think we should be re-visiting this. Sure, if we were just syncing A, B or C having per-[ABC] config options might be a bit overdoing it, but would be relatively simple.
Quoted 8 lines
But once we start using a more optimized version of the "bulk" mode the config schema will be making promises about individual steps in a transaction that I think we'll want to leave opaque, and only promise that when git returns it will have synced all the relevant assets as efficiently as possible.
1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
I think the current documentation is fine (obviously since I wrote it). Let's reproduce the first part again:
---
core.fsync::
A comma-separated list of components of the repository that should be hardened via the core.fsyncMethod when created or modified. You can disable hardening of any component by prefixing it with a '-'. Items that are not hardened may be lost in the event of an unclean system shutdown. Unless you have special requirements, it is recommended that you leave this option empty or pick one of committed, added,
or all.
+
When this configuration is encountered, the set of components starts with the platform default value, disabled components are removed, and additional components are added. none resets the state so that the platform default is ignored.
+
The empty string resets the fsync configuration to the platform default. The default on most platforms is equivalent to core.fsync=committed,-loose-object, which has good performance, but risks losing recent work in the event of an unclean system shutdown. +
---
We're only talking about "hardening" parts of the repository, and we say we'll do it using the "fsyncMethod". If you don't harden something, you could lose it if the system dies. All of these statements are true and don't say so much about the implementation of how the hardening happens. It's perfectly valid to not force any component out of the disk cache, and a straightforward implementation of repo transactions can put the sync point anywhere. We also explicitly point the user at a "porcelain" setting in the first paragraph.
So I think you're alone in thinking that anything needs to change here.
Thanks,
Neeraj
Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)
Quoted 6 lines · Neeraj Singh
On Mon, Mar 28, 2022 at 5:15 AM Ævar Arnfjörð Bjarmason <[email protected]> wrote:
So I think you're alone in thinking that anything needs to change here.
Ævar,
I apologize for the negative tone and content of this comment.
Thanks,
Neeraj
Quoted 6 lines · Neeraj K. Singh via GitGitGadget
After this change, new persistent data files added to the repo will need to be added to the
fsync_componentenum and documented in theDocumentation/config/core.txttext.V6 changes:
Quoted 16 lines
* Move only the windows csprng includes into
wrapper.crather than all of them. This fixes the specific build issue due to broken Windows headers.
[6]
* Split the configuration parsing of core.fsync from the mechanism to focus the review.
* Incorporate Patrick's patch at [7] into the core.fsync mechanism patch.
* Pick the stricter one of core.fsyncObjectFiles and (fsync_components&FSYNC_COMPONENT_LOOSE_OBJECTS), to respect the older setting.
* Issue a deprecation warning but keep parsing and honoring core.fsyncObjectFiles.
* Change configuration parsing of core.fsync to always start with the platform default. none resets to the empty set. The comma separated list implies a set without regards to ordering now. This follows Junio's suggestion in [8].
* Change the documentation of the core.fsync option to reflect the way the new parsing code works.
Hmph, this seems to make one test fail.
t5801-remote-helpers.sh (Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14-16, 31
Non-zero exit status: 1
Files=1, Tests=31, 2 wallclock secs ( 0.04 usr 0.00 sys + 1.40 cusr 1.62 csys = 3.06 CPU)
Result: FAIL
Quoted 26 lines · Junio C Hamano
Quoted 6 lines · Neeraj K. Singh via GitGitGadget
After this change, new persistent data files added to the repo will need to be added to the
fsync_componentenum and documented in theDocumentation/config/core.txttext.V6 changes:
Quoted 16 lines
* Move only the windows csprng includes into
wrapper.crather than all of them. This fixes the specific build issue due to broken Windows headers.
[6]
* Split the configuration parsing of core.fsync from the mechanism to focus the review.
* Incorporate Patrick's patch at [7] into the core.fsync mechanism patch.
* Pick the stricter one of core.fsyncObjectFiles and (fsync_components&FSYNC_COMPONENT_LOOSE_OBJECTS), to respect the older setting.
* Issue a deprecation warning but keep parsing and honoring core.fsyncObjectFiles.
* Change configuration parsing of core.fsync to always start with the platform default. none resets to the empty set. The comma separated list implies a set without regards to ordering now. This follows Junio's suggestion in [8].
* Change the documentation of the core.fsync option to reflect the way the new parsing code works.
Quoted 7 lines
Hmph, this seems to make one test fail.
t5801-remote-helpers.sh(Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14-16, 31
Non-zero exit status: 1
Files=1, Tests=31, 2 wallclock secs ( 0.04 usr 0.00 sys + 1.40 cusr 1.62 csys = 3.06 CPU)
Result: FAIL
Thanks for reporting this. I didn't see a failure in CI, nor when running that specific test in mingw. I also munged my config to include core.fsyncObjectFiles and didn't see a failure.
Could you please share some more verbose output of the test, so I can look a bit deeper? In parallel, I'm trying again after merging my changes onto seen.
Thanks,
Neeraj
Quoted 40 lines · Neeraj Singh
Quoted 26 lines · Junio C Hamano
Quoted 6 lines · Neeraj K. Singh via GitGitGadget
After this change, new persistent data files added to the repo will need to be added to the
fsync_componentenum and documented in theDocumentation/config/core.txttext.V6 changes:
Quoted 16 lines
* Move only the windows csprng includes into
wrapper.crather than all of them. This fixes the specific build issue due to broken Windows headers.
[6]
* Split the configuration parsing of core.fsync from the mechanism to focus the review.
* Incorporate Patrick's patch at [7] into the core.fsync mechanism patch.
* Pick the stricter one of core.fsyncObjectFiles and (fsync_components&FSYNC_COMPONENT_LOOSE_OBJECTS), to respect the older setting.
* Issue a deprecation warning but keep parsing and honoring core.fsyncObjectFiles.
* Change configuration parsing of core.fsync to always start with the platform default. none resets to the empty set. The comma separated list implies a set without regards to ordering now. This follows Junio's suggestion in [8].
* Change the documentation of the core.fsync option to reflect the way the new parsing code works.Quoted 7 lines
Hmph, this seems to make one test fail.
t5801-remote-helpers.sh(Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14-16, 31
Non-zero exit status: 1
Files=1, Tests=31, 2 wallclock secs ( 0.04 usr 0.00 sys + 1.40 cusr 1.62 csys = 3.06 CPU)
Result: FAILThanks for reporting this. I didn't see a failure in CI, nor when running that specific test in mingw. I also munged my config to include core.fsyncObjectFiles and didn't see a failure.
Quoted 6 lines
Could you please share some more verbose output of the test, so I can look a bit deeper? In parallel, I'm trying again after merging my changes onto seen.
Thanks,
Neeraj
Hi Junio,
I've also tested v6-on-seen on Linux and I'm still not seeing the failure. Does the
failure still happen on your end?
Thanks,
Neeraj
Quoted 7 lines · Junio C Hamano
Hmph, this seems to make one test fail.
t5801-remote-helpers.sh(Wstat: 256 Tests: 31 Failed: 4)
Failed tests: 14-16, 31
Non-zero exit status: 1
Files=1, Tests=31, 2 wallclock secs ( 0.04 usr 0.00 sys + 1.40 cusr 1.62 csys = 3.06 CPU)
Result: FAIL
False alarm. This byitself, or merged to 'seen' with other random topics, no longer seem to break these tests.
PATCH v2 1/1 core.fsync: new option to harden references
In reply to Neeraj K. Singh via GitGitGadgetWhen 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)
---
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(-)
Documentation/config/core.txt +1 -0
@@ -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
cache.h +5 -2
@@ -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.
config.c +1 -0
@@ -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 },
refs/files-backend.c +1 -0
@@ -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));
refs/packed-backend.c +2 -1
@@ -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));
Signature · 1 line
2.35.1
Re: [PATCH v2] core.fsync: new option to harden references
Quoted 15 lines · Patrick Steinhardt
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.
Quoted 3 lines
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.
t/t5541-http-push-smart.sh +1 -1
@@ -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.
Quoted 123 lines
[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 (seeauto_da_alloc)Signed-off-by Patrick Steinhardt <[email protected]>---
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(-)Documentation/config/core.txt +1
-0@@ -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 tocache.h +5
-2@@ -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. config.c +1
-0@@ -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 },refs/files-backend.c +1
-0@@ -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));refs/packed-backend.c +2
-1@@ -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));Signature · 1 line
2.35.1