Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs

> 
> On Wed, Nov 10 2021, Patrick Steinhardt wrote:
> 
> > [...]
> > Fix this by introducing a new configuration "core.fsyncRefFiles". This
> > config matches behaviour of "core.fsyncObjectFiles" in that it provides
> > three different modes:
> >
> >     - "off" disables calling fsync on ref files. This is the default
> >       behaviour previous to this change and remains the default after
> >       this change.
> >
> >     - "on" enables calling fsync on ref files, where each reference is
> >       flushed to disk before it is being committed. This is the safest
> >       setting, but may incur significant performance overhead.
> >
> >     - "batch" will flush the page cache of each file as it is written to
> >       ensure its data is persisted. After all refs have been written,
> >       the directories which host refs are flushed.
> >
> > With this change in place and when "core.fsyncRefFiles" is set to either
> > "on" or "batch", this kind of corruption shouldn't happen anymore.
> >
> > [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt
> 
> With the understanding that my grokking of this approach is still
> somewhere between "uh, that works?" and "wow, voodoo FS magic!". ....
> 
> I haven't looked at these changes in much daiter, or Neeraj's recent
> related changes but...
> 
> > +core.fsyncRefFiles::
> > +	A value indicating the level of effort Git will expend in trying to make
> > +	refs added to the repo durable in the event of an unclean system
> > +	shutdown. This setting currently only controls loose refs in the object
> > +	store, so updates to packed refs may not be equally durable. Takes the
> > +	same parameters as `core.fsyncObjectFiles`.
> > +
> 
> ...my understanding of it is basically a way of going back to what Linus
> pointed out way back in aafe9fbaf4f (Add config option to enable
> 'fsync()' of object files, 2008-06-18).
> 
> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all
> and the directory entry, but on some FS's it would be sufficient to
> fsync() just y if they're created in that order. It'll imply an fsync of
> both x and y, is that accurate?
> 
> If not you can probably discard the rest, but forging on:
> 
> Why do we then need to fsync() a "z" file in get_object_directory()
> (i.e. .git/objects) then? That's a new "z", wasn't flushing "y" enough?
> 
> Or if you've written .git/objects/x and .git/refs/y I can imagine
> wanting to create and sync a .git/z if the FS's semantics are to then
> flush all remaining updates from that tree up, but here it's
> .git/objects, not .git. That also seems to contract this above:
> 
> >       ensure its data is persisted. After all refs have been written,
> >       the directories which host refs are flushed.
> 
> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for
> now), not .git/objects or .git?

> And again, forging on but more generally [continued below]...
> 
> > +	if (!strcmp(var, "core.fsyncreffiles")) {
> 
> UX side: now we've got a core.fsyncRefFiles and
> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those
> work together like say "fsck.*" and "fetch.fsck.*" do, i.e. you'd be
> able to configure this once for objects and refs, or in two variables,
> one for objects, one for refs...

> > @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> >  		files_downcast(ref_store, REF_STORE_WRITE,
> >  			       "ref_transaction_prepare");
> >  	size_t i;
> > -	int ret = 0;
> > +	int ret = 0, sync_flags = 0;
> >  	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> >  	char *head_ref = NULL;
> >  	int head_type;
> > @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> >  					&update->new_oid, NULL,
> >  					NULL);
> >  		}
> > +
> > +		sync_flags |= sync_loose_refs_flags(update->refname);
> >  	}
> >  
> > +	ret = sync_loose_refs(refs, sync_flags, err);
> > +	if (ret)
> > +		goto cleanup;
> > +
> >  	if (packed_transaction) {
> >  		if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> >  			ret = TRANSACTION_GENERIC_ERROR;
> 
> ...[continued from above]: Again, per my potentially wrong understanding
> of syncing a "x" and "y" via an fsync of a subsequent "z" that's
> adjacent on the FS to those two.
> 
> Isn't this setting us up for a really bad interaction between this
> series and Neeraj's work? Well "bad" as in "bad for performance".
> 
> I.e. you'll turn on "use the batch thing for objects and refs" and we'll
> do two fsyncs, one for the object update, and one for refs. The common
> case is that we'll have both in play.
> 
> So shouldn't this go to a higher level for both so we only create a "z"
> .git/sync-it-now-please.txt or whatever once we do all pending updates
> on the .git/ directory?

> I can also imagine that we'd want that at an even higher level, e.g. for
> "git pull" surely we'd want it not for refs or objects, but in
> builtin/pull.c somewhere because we'll be updating the .git/index after
> we do both refs and objects, and you'd want to fsync at the very end,
> no?
> 
> None of the above should mean we can't pursue a more narrow approach for
> now. I'm just:
> 
>  1) Seeing if I understand what we're trying to do here, maybe not.