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

> On Wed, Dec 8, 2021 at 2:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Dec 07 2021, Neeraj Singh wrote:
>>
>> > On Tue, Dec 7, 2021 at 5:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> >>
>> >>
>> >> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>> >>
>> >> > From: Neeraj Singh <neerajsi@microsoft.com>
>> >> >
>> >> > 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!
>> >>
>> >> > * 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.
>>
>> Even if you turn just your comma-delimited proposal into a list proposal
>> can't we just say that the last one wins? Then it won't matter what cmae
>> before, you'd specify "core.fsync=none" in your local .git/config.
>>
>> But this is also a general issue with a bunch of things in git's config
>> space. I'd rather see us use the list-based values and just come up with
>> some general way to reset them that works for all keys, rather than
>> regretting having comma-delimited values that'll be harder to work with
>> & parse, which will be a legacy wart if/when we come up with a way to
>> say "reset all previous settings".
>>
>> > Also, with the core.fsync and core.fsyncExcludes, how would you spell
>> > "don't sync anything"? Would you still have the aggregate options.?
>>
>>     core.fsyncExcludes = *
>>
>> I.e. the same as the core.fsync=none above, anyway I still like the
>> wildcard thing below a bit more...
>
> I'm not going to take this feedback unless there are additional votes
> from the Git community in this direction.  I make the claim that
> single-valued comma-separated config lists are easier to work with in
> the existing Git infrastructure. 

>> >> > 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/
>>
>> I think he's talking about batch, Batch, BATCH, bAtCh etc. there. But
>> the "maybe bool" is a stanard pattern we use.
>>
>> I don't think we'd call one of these 0, off, no or false etc. to avoid
>> confusion, so then you can use git_parse_maybe_...()
>
> I don't see the advantage of having multiple ways of specifying
> "none".  The user can read the doc and know exactly what to write.  If
> they write something unallowable, they get a clear warning and they
> can read the doc again to figure out what to write.  This isn't a
> boolean options at all, so why should we entertain bool-like ways of
> spelling it?

>> >> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
>> >> > ---
>> >> >  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.
>>
>> We document it in gitrepository-layout(5). Yeah it has some
>> disadvantages, but one advantage is that you could make the
>> composability easy. I.e. if last exclude wins then a setting of:
>>
>>     core.fsync = ":!*"
>>     core.fsync = "objects/**"
>>
>> Would reset all previous matches & only match objects/**.
>>
>
> The value of changing this is predicated on taking your previous
> multi-valued config proposal, which I'm still not at all convinced
> about.

> The schema in the current (v1-v2) version of the patch already
> includes an example of extending the list of syncable things, and
> Patrick Steinhardt made it clear that he feels comfortable adding
> 'refs' to the same schema in a future change.
>
> I'll also emphasize that we're talking about a non-functional,
> relatively corner-case behavioral configuration.  These values don't
> change how git's interface behaves except when the system crashes
> during a git command or shortly after one completes.

> While you may not personally love the proposed configuration
> interface, I'd want your view on some questions:
> 1. Is it easy for the (advanced) user to set a configuration?
> 2. Is it easy for the (advanced) user to see what was configured?
> 3. Is it easy for the Git community to build on this as we want to add
> things to the list of things to sync?
>     a) Is there a good best practice configuration so that people can
> avoid losing integrity for new stuff that they are intending to sync.
>     b) If someone has a custom configuration, can that custom
> configuration do something reasonable as they upgrade versions of Git?
>              ** In response to this question, I might see some value
> in adding a 'derived-metadata' aggregate that can be disabled so that
> a custom configuration can exclude those as they change version to
> version.
>     c) Is it too much maintenance overhead to consider how to present
> this configuration knob for any new hashfile or other datafile in the
> git repo?
> 4. Is there a good path forward to change the default syncable set,
> both in git-for-windows and in Git for other platforms?

>> >> > 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.
>>
>> Yeah maybe we should just leave it be.
>>
>> I'd think the C library returning EINVAL would be a trivial performance
>> cost though.
>>
>> It just seemed odd to hardcode assumptions about what can and can't be
>> synced when the POSIX defined function will also tell us that.
>>
>
> Redirecting stdout to a file seems like a common usage for this
> command. That would definitely be fsyncable, but Git has no idea what
> its proper category is since there's no way to know the purpose or
> lifetime of the packfile.  I'm going to leave this be, because I'd
> posit that "can it be fsynced?" is not the same as "should it be
> fsynced?".  The latter question can't be answered for stdout.

>>
>> >> > [...]
>> >> > +/*
>> >> > + * 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...
>> >>
>> >> > +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.
>>
>> FWIW string_list_split() is easier to work with in those cases, or at
>> least I think so...
>
> This code runs at startup for a variable that may be present on some
> installations.  The nice property of the current patch's code is that
> it's already a well-tested pattern that doesn't do any allocations as
> it's working, unlike string_list_split().