Re: do we have too much fsync() configuration in 'next'? (was: [PATCH v7] core.fsync: documentation and user-friendly aggregate options)

[Neeraj Singh <nksingh85@gmail.com>]

On Sat, Mar 26, 2022 at 8:34 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Fri, Mar 25 2022, Neeraj Singh wrote:
>
> > On Fri, Mar 25, 2022 at 5:33 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >>
> >> On Fri, Mar 25 2022, Neeraj Singh wrote:
> >>
> >> > On Wed, Mar 23, 2022 at 7:46 AM Ævar Arnfjörð Bjarmason
> >> > <avarab@gmail.com> wrote:
> >> >>
> >> >>
> >> >> 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:
> >> >>
> >> >> > +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.
> >> >>
> >> >> On top of my
> >> >> https://lore.kernel.org/git/RFC-patch-v2-7.7-a5951366c6e-20220323T140753Z-avarab@gmail.com/
> >> >> 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.
> >> >>
> >> >
> >> > 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).
> >>
> >> >> 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/RFC-cover-v2-0.7-00000000000-20220323T140753Z-avarab@gmail.com/)
> >> >> 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/RFC-patch-v2-6.7-c20301d7967-20220323T140753Z-avarab@gmail.com/
> >> >>
> >> >
> >> > 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.
> >>
> >> >> 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/RFC-patch-v2-4.7-61f4f3d7ef4-20220323T140753Z-avarab@gmail.com/
> >> >> 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.
> >> >>
> >> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> >> >> index 365a12dc7ae..536238e209b 100644
> >> >> --- a/Documentation/config/core.txt
> >> >> +++ b/Documentation/config/core.txt
> >> >> @@ -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
> >> >
> >> > I'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).
> >>
> >
> > 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?
>
> > 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?

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.