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

On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:

quoted

From: Neeraj Singh <redacted>

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 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.

Looks good!

quoted

* We support negation, so users can choose one of the default
  aggregate options and then remove components that they don't
  want. The user would then harden any new components added in
  a Git version update.

I think this config schema makes sense, but just a (I think important)
comment on the "how" not "what" of it. It's really much better to define
config as:

    [some]
    key = value
    key = value2

Than:

    [some]
    key = value,value2

The reason is that "git config" has good support for working with
multi-valued stuff, so you can do e.g.:

    git config --get-all -z some.key

And you can easily (re)set such config e.g. with --replace-all etc., but
for comma-delimited you (and users) need to do all that work themselves.

Similarly instead of:

    some.key = want-this
    some.key = -not-this
    some.key = but-want-this

I think it's better to just have two lists, one inclusive another
exclusive. E.g. see "log.decorate" and "log.excludeDecoration",
"transfer.hideRefs"

Which would mean:

    core.fsync = want-this
    core.fsyncExcludes = -not-this

For some value of "fsyncExcludes", maybe "noFsync"? Anyway, just a
suggestion on making this easier for users & the implementation.

Maybe there's some way to handle this I'm unaware of, but a
disadvantage of your multi-valued config proposal is that it's harder,
for example, for a per-repo config store to reasonably override a
per-user config store.  With the configuration scheme as-is, I can
have a per-user setting like `core.fsync=all` which covers my typical
repos, but then have a maintainer repo with a private setting of
`core.fsync=none` to speed up cases where I'm mostly working with
other people's changes that are backed up in email or server-side
repos.  The latter setting conveniently overrides the former setting
in all aspects.

Also, with the core.fsync and core.fsyncExcludes, how would you spell
"don't sync anything"? Would you still have the aggregate options.?

quoted

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.

Let's just use the git_parse_maybe_bool() or git_parse_maybe_bool_text()
so we'll accept "false", "off", "no" like most other such config?

Junio's previous feedback when discussing batch mode [1] was to offer
less flexibility when parsing new values of these configuration
options. I agree with his statement that "making machine-readable
tokens be spelled in different ways is a 'disease'."  I'd like to
leave this as-is so that the documentation can clearly state the exact
set of allowable values.

[1] https://lore.kernel.org/git/xmqqr1dqzyl7.fsf@gitster.g/ (local)

quoted

Signed-off-by: Neeraj Singh <redacted>
---
 Documentation/config/core.txt | 27 +++++++++----
 builtin/fast-import.c         |  2 +-
 builtin/index-pack.c          |  4 +-
 builtin/pack-objects.c        |  8 ++--
 bulk-checkin.c                |  5 ++-
 cache.h                       | 39 +++++++++++++++++-
 commit-graph.c                |  3 +-
 config.c                      | 76 ++++++++++++++++++++++++++++++++++-
 csum-file.c                   |  5 ++-
 csum-file.h                   |  3 +-
 environment.c                 |  1 +
 midx.c                        |  3 +-
 object-file.c                 |  3 +-
 pack-bitmap-write.c           |  3 +-
 pack-write.c                  | 13 +++---
 read-cache.c                  |  2 +-
 16 files changed, 164 insertions(+), 33 deletions(-)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index dbb134f7136..4f1747ec871 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt

@@ -547,6 +547,25 @@ 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 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 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.
++
+* `none` disables fsync completely. This must be specified alone.
+* `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.
+* `objects` is an aggregate option that includes `loose-objects`, `pack`,
+  `pack-metadata`, and `commit-graph`.
+* `default` is an aggregate option that is equivalent to `objects,-loose-object`
+* `all` is an aggregate option that syncs all individual components above.
+

It's probably a *bit* more work to set up, but I wonder if this wouldn't
be simpler if we just said (and this is partially going against what I
noted above):

== BEGIN DOC

core.fsync is a multi-value config variable where each item is a
pathspec that'll get matched the same way as 'git-ls-files' et al.

When we sync pretend that a path like .git/objects/de/adbeef... is
relative to the top-level of the git
directory. E.g. "objects/de/adbeaf.." or "objects/pack/...".

You can then supply a list of wildcards and exclusions to configure
syncing.  or "false", "off" etc. to turn it off. These are synonymous
with:

    ; same as "false"
    core.fsync = ":!*"

Or:

    ; same as "true"
    core.fsync = "*"

Or, to selectively sync some things and not others:

    ;; Sync objects, but not "info"
    core.fsync = ":!objects/info/**"
    core.fsync = "objects/**"

See gitrepository-layout(5) for details about what sort of paths you
might be expected to match. Not all paths listed there will go through
this mechanism (e.g. currently objects do, but nothing to do with config
does).

We can and will match this against "fake paths", e.g. when writing out
packs we may match against just the string "objects/pack", we're not
going to re-check if every packfile we're writing matches your globs,
ditto for loose objects. Be reasonable!

This metharism is intended as a shorthand that provides some flexibility
when fsyncing, while not forcing git to come up with labels for all
paths the git dir, or to support crazyness like "objects/de/adbeef*"

More paths may be added or removed in the future, and we make no
promises that we won't move things around, so if in doubt use
e.g. "true" or a wide pattern match like "objects/**". When in doubt
stick to the golden path of examples provided in this documentation.

== END DOC


It's a tad more complex to set up, but I wonder if that isn't worth
it. It nicely gets around any current and future issues of deciding what
labels such as "loose-object" etc. to pick, as well as slotting into an
existing method of doing exclude/include lists.

I think this proposal is a lot of complexity to avoid coming up with a
new name for syncable things as they are added to Git.  A path based
mechanism makes it hard to document for the (advanced) user what the
full set of things is and how it might change from release to release.
I think the current core.fsync scheme is a bit easier to understand,
query, and extend.

quoted

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 857be7826f3..916c55d6ce9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -1204,11 +1204,13 @@ static void write_pack_file(void)
               * If so, rewrite it like in fast-import
               */
              if (pack_to_stdout) {
-                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
+                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
+                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);

Not really related to this per-se, but since you're touching the API
everything goes through I wonder if callers should just always try to
fsync, and we can just catch EROFS and EINVAL in the wrapper if someone
tries to flush stdout, or catch the fd at that lower level.

Or maybe there's a good reason for this...

It's platform dependent, but I'd expect fsync would do something for
pipes or stdout redirected to a file.  In these cases we really don't
want to fsync since we have no idea what we're talking to and we're
potentially worsening performance for probably no benefit.

quoted

[...]
+/*
+ * These values are used to help identify parts of a repository to fsync.
+ * FSYNC_COMPONENT_NONE identifies data that will not be a persistent part of the
+ * repository and so shouldn't be fsynced.
+ */
+enum fsync_component {
+     FSYNC_COMPONENT_NONE                    = 0,

I haven't read ahead much but in most other such cases we don't define
the "= 0", just start at 1<<0, then check the flags elsewhere...

quoted

+static const struct fsync_component_entry {
+     const char *name;
+     enum fsync_component component_bits;
+} fsync_component_table[] = {
+     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
+     { "pack", FSYNC_COMPONENT_PACK },
+     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
+     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
+     { "objects", FSYNC_COMPONENTS_OBJECTS },
+     { "default", FSYNC_COMPONENTS_DEFAULT },
+     { "all", FSYNC_COMPONENTS_ALL },
+};
+
+static enum fsync_component parse_fsync_components(const char *var, const char *string)
+{
+     enum fsync_component output = 0;
+
+     if (!strcmp(string, "none"))
+             return output;
+
+     while (string) {
+             int i;
+             size_t len;
+             const char *ep;
+             int negated = 0;
+             int found = 0;
+
+             string = string + strspn(string, ", \t\n\r");

Aside from the "use a list" isn't this hardcoding some windows-specific
assumptions with \n\r? Maybe not...

I shamelessly stole this code from parse_whitespace_rule. I thought
about making a helper to be called by both functions, but the amount
of state going into and out of the wrapper via arguments was
substantial and seemed to negate the benefit of deduplication.

Thanks for the review,
Neeraj