Re: [PATCH] refs: sync loose refs to disk before committing them

From: Jeff King <peff@peff.net>
Date: Nov 5, 2021, 09:34 UTC
Message-ID: <YYT6tDyfBbwot2br@coredump.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.2111051010070.56@tvgsbejvaqbjf.bet>

On Fri, Nov 05, 2021 at 10:12:25AM +0100, Johannes Schindelin wrote:

Show 7 quoted lines

> > So this will definitely hurt at edge / pathological cases.
> 
> Ouch.
> 
> I wonder whether this could be handled similarly to the
> `core.fsyncObjectFiles=batch` mode that has been proposed in
> https://lore.kernel.org/git/pull.1076.v8.git.git.1633366667.gitgitgadget@gmail.com/

Yeah, that was along the lines I was thinking.

I hadn't really looked at the details of the batch-fsync there. The big trick seems to be doing a pagecache writeback for each file, and then stimulating an actual disk write (by fsyncing a tempfile) after the batch is done.

That would be pretty easy to do for the refs (it's git_fsync() while closing each file where Patrick is calling fsync(), followed by a single do_batch_fsync() after everything is closed but before we rename).

Show 5 quoted lines

> Essentially, we would have to find a better layer to do this, where we
> can synchronize after a potentially quite large number of ref updates has
> happened. That would definitely be a different layer than the file-based
> refs backend, of course, and would probably apply in a different way to
> other refs backends.

We do have the concept of a ref_transaction, so that would be the natural place for it. Not every caller uses it, though, because it implies atomicity of the transaction (so some may do a sequence of N independent transactions, because they don't want failure of one to impact others). I think that could be changed, if the ref_transaction learned about non-atomicity, but it may take some surgery.

I expect that reftables would similarly benefit; it is probably much more efficient to write a table slice with N entries than it is to write N slices, even before accounting for fsync(). And once doing that, then the fsync() part becomes trivial.

-Peff