Re: [PATCH 2/2] core.fsyncobjectfiles: batch disk flushes

> On Wed, Aug 25 2021, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > When adding many objects to a repo with core.fsyncObjectFiles set to
> > true, the cost of fsync'ing each object file can become prohibitive.
> >
> > One major source of the cost of fsync is the implied flush of the
> > hardware writeback cache within the disk drive. Fortunately, Windows,
> > MacOS, and Linux each offer mechanisms to write data from the filesystem
> > page cache without initiating a hardware flush.
> >
> > This patch introduces a new 'core.fsyncObjectFiles = 2' option that
> > takes advantage of the bulk-checkin infrastructure to batch up hardware
> > flushes.
> >
> > When the new mode is enabled we do the following for new objects:
> >
> > 1. Create a tmp_obj_XXXX file and write the object data to it.
> > 2. Issue a pagecache writeback request and wait for it to complete.
> > 3. Record the tmp name and the final name in the bulk-checkin state for
> >    later name.
> >
> > At the end of the entire transaction we:
> > 1. Issue a fsync against the lock file to flush the hardware writeback
> >    cache, which should by now have processed the tmp file writes.
> > 2. Rename all of the temp files to their final names.
> > 3. When updating the index and/or refs, we will issue another fsync
> >    internal to that operation.
> >
> > On a filesystem with a singular journal that is updated during name
> > operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
> > would expect the fsync to trigger a journal writeout so that this
> > sequence is enough to ensure that the user's data is durable by the time
> > the git command returns.
> >
> > This change also updates the MacOS code to trigger a real hardware flush
> > via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
> > MacOS there was no guarantee of durability since a simple fsync(2) call
> > does not flush any hardware caches.
>
> Thanks for working on this, good to see fsck issues picked up after some
> on-list pause.
>
> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> > index c04f62a54a1..3b672c2db67 100644
> > --- a/Documentation/config/core.txt
> > +++ b/Documentation/config/core.txt
> > @@ -548,12 +548,17 @@ core.whitespace::
> >    errors. The default tab width is 8. Allowed values are 1 to 63.
> >
> >  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").
> > +     A boolean value or the number '2', indicating the level of durability
> > +     applied to object files.
> > ++
> > +This setting controls how much effort Git makes to ensure that data added to
> > +the object store are durable in the case of an unclean system shutdown. If
> > +'false', Git allows data to remain in file system caches according to operating
> > +system policy, whence they may be lost if the system loses power or crashes. A
> > +value of 'true' instructs Git to force objects to stable storage immediately
> > +when they are added to the object store. The number '2' is an experimental
> > +value that also preserves durability but tries to perform hardware flushes in a
> > +batch.
>
> Some feedback/thoughts:
>
> 0) Let's not expose "2" to users, but give it some friendly config name
> and just translate this to the enum internally.

>
> 2) You say "when adding many [loose] objects to a repo[...]", and the
> test-case is "git stash push", but for e.g. accepting pushes we have
> transfer.unpackLimit.
>
> It would be interesting to see if/how this impacts performance there,
> and also if not that should at least be called out in
> documentation. I.e. users might want to set this differently on servers
> v.s. checkouts.
>
> But also, is this sort of thing something we could mitigate even more in
> commands like "git stash push" by just writing a pack instead of N loose
> objects?
>
> I don't think such questions should preclude changing the fsync
> approach, or offering more options, but they should inform our
> longer-term goals.

>
> 3) Re some of the musings about fsync() recently in
> https://lore.kernel.org/git/877dhs20x3.fsf@evledraar.gmail.com/; is this
> method of doing not-quite-an-fsync guaranteed by some OS's / POSIX etc,
> or is it more like the initial approach before core.fsyncObjectFiles,
> i.e. the happy-go-lucky approach described in the "[...]that orders data
> writes properly[...]" documentation you're removing.

>
> 4) While that documentation written by Linus long ago is rather
> flippant, I think just removing it and not replacing it with some
> discussion about how this is a trade-off v.s. real-world filesystem
> semantics isn't a good change.

>
> 5) On a similar thought as transfer.unpackLimit in #2, I wonder if this
> fsync() setting shouldn't really be something we should be splitting
> up. I.e. maybe handle batch loose object writes one way, ref updates
> another way etc. I think moving core.fsync* to a setting like what we
> have for fsck.* and <cmd>.fsck.* is probably a better thing to do in the
> longer term.
>
> I.e. being able to do things like:
>
>     fsync.objectFiles = none
>     fsync.refFiles = cache # or "hardware"
>     receive.fsync.objectFiles = hardware
>     receive.fsync.refFiles = hardware
>
> Or whatever, i.e. we're using one hammer for all of these now, but I
> suspect most users who care about fsync care about /some/ fsync, not
> everything.
>

> 6) Inline comments below.
>
> > +struct object_rename {
> > +     char *src;
> > +     char *dst;
> > +};
> > +
> > +static struct bulk_rename_state {
> > +     struct object_rename *renames;
> > +     uint32_t alloc_renames;
> > +     uint32_t nr_renames;
> > +} bulk_rename_state;
>
> In a crash of git itself it seems we're going to leave some litter
> behind in the object dir now, and "git gc" won't know how to clean it
> up. I think this is going to want to just use the tmp-objdir.[ch] API,
> which might or might not need to be extended for loose objects / some
> oddities of this use-case.
>

> >  static struct bulk_checkin_state {
> >       unsigned plugged:1;
> >
> > @@ -21,13 +33,15 @@ static struct bulk_checkin_state {
> >       struct pack_idx_entry **written;
> >       uint32_t alloc_written;
> >       uint32_t nr_written;
> > -} state;
>
>
> > +
> > +             free(state->renames);
> > +             memset(state, 0, sizeof(*state));
>
> So with this and other use of the "state" variable is this part of
> bulk-checkin going to become thread-unsafe, was that already the case?

> > +static void add_rename_bulk_checkin(struct bulk_rename_state *state,
> > +                                 const char *src, const char *dst)
> > +{
> > +     struct object_rename *rename;
> > +
> > +     ALLOC_GROW(state->renames, state->nr_renames + 1, state->alloc_renames);
> > +
> > +     rename = &state->renames[state->nr_renames++];
> > +     rename->src = xstrdup(src);
> > +     rename->dst = xstrdup(dst);
> > +}
>
> All boilerplate duplicating things you'd get with a string-list for free...

>
> > +             /*
> > +              * If we have a plugged bulk checkin, we issue a call that
> > +              * cleans the filesystem page cache but avoids a hardware flush
> > +              * command. Later on we will issue a single hardware flush
> > +              * before renaming files as part of do_sync_and_rename.
> > +              */
>
> So this is the sort of thing I meant by extending Linus's docs, I know
> some FS's work this way, but not all do.
>
> Also there's no guarantee in git that your .git is on one FS, so I think
> even for the FS's you have in mind this might not be an absolute
> guarantee...