Re: [PATCH v2 2/7] core.fsyncmethod: batched disk flushes for loose-objects

On Mon, Mar 21 2022, Neeraj Singh wrote:

Yes, it can. As the hash-object case shows we don't even check at all.

For "incoming push" we *will* notice, but will just uselessly error out.

I actually had some patches a while ago to turn off our own home-grown SHA-1 collision checking.

It had the nice side effect of making it easier to recover from loose object corruption, since you could (re-)push the corrupted OID as a PACK, we wouldn't check (and die) on the bad loose object, and since we take a PACK over LOOSE we'd recover: https://lore.kernel.org/git/20181028225023.26427-5-avarab@gmail.com/

That's correct, but we're discussing a feature that *does have* that fsync barrier. So if we get an error while writing the loose objects before the "cookie" fsync we'll presumably error out. That'll then be followed by an fsync() of whatever makes the objects reachable.

Well, you can see Linus's original rant-as-documentation for why we added it :) I.e. the original git implementation made some heavy linux-FS assumption about the order of writes and an fsync() flushing any previous writes, which wasn't portable.

Leaving "usefully" aside for a second (since that's subjective), it clearly "can". I just tried this on top of "seen":

	diff --git a/bulk-checkin.c b/bulk-checkin.c
	index a702e0ff203..9e994c4d6ae 100644
	--- a/bulk-checkin.c
	+++ b/bulk-checkin.c
	@@ -9,15 +9,12 @@
	 #include "pack.h"
	 #include "strbuf.h"
	 #include "string-list.h"
	-#include "tmp-objdir.h"
	 #include "packfile.h"
	 #include "object-store.h"
	 
	 static int bulk_checkin_plugged;
	 static int needs_batch_fsync;
	 
	-static struct tmp_objdir *bulk_fsync_objdir;
	-
	 static struct bulk_checkin_state {
	 	char *pack_tmp_name;
	 	struct hashfile *f;
	@@ -110,11 +107,6 @@ static void do_batch_fsync(void)
	 		strbuf_release(&temp_path);
	 		needs_batch_fsync = 0;
	 	}
	-
	-	if (bulk_fsync_objdir) {
	-		tmp_objdir_migrate(bulk_fsync_objdir);
	-		bulk_fsync_objdir = NULL;
	-	}
	 }
	 
	 static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
	@@ -321,7 +313,6 @@ void fsync_loose_object_bulk_checkin(int fd)
	 	 */
	 	if (bulk_checkin_plugged &&
	 	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
	-		assert(bulk_fsync_objdir);
	 		if (!needs_batch_fsync)
	 			needs_batch_fsync = 1;
	 	} else {
	@@ -343,19 +334,6 @@ int index_bulk_checkin(struct object_id *oid,
	 void plug_bulk_checkin(void)
	 {
	 	assert(!bulk_checkin_plugged);
	-
	-	/*
	-	 * A temporary object directory is used to hold the files
	-	 * while they are not fsynced.
	-	 */
	-	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT)) {
	-		bulk_fsync_objdir = tmp_objdir_create("bulk-fsync");
	-		if (!bulk_fsync_objdir)
	-			die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch"));
	-
	-		tmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);
	-	}
	-
	 	bulk_checkin_plugged = 1;
	 }

And then tried running:

    $ GIT_PERF_MAKE_OPTS='CFLAGS=-O3' ./run HEAD~ HEAD -- p3900-stash.sh	

And got:
    
    Test                                              HEAD~              HEAD
    --------------------------------------------------------------------------------------------
    3900.2: stash 500 files (object_fsyncing=false)   0.56(0.08+0.09)    0.60(0.08+0.08) +7.1%
    3900.4: stash 500 files (object_fsyncing=true)    14.50(0.07+0.15)   17.13(0.10+0.12) +18.1%
    3900.6: stash 500 files (object_fsyncing=batch)   1.14(0.08+0.11)    1.03(0.08+0.10) -9.6%

Now, I really don't trust that perf run to say anything except these being in the same ballpark, but it's clearly going to be a *bit* faster since we'll be doing fewer IOps.

As to "usefully" I really do get what you're saying that you only find these useful when you combine the two because you'd like to have 100% safety, and that's fair enough.

But since we are going to have a knob to turn off fsyncing entirely, and we have this "bulk" mode which requires you to carefully reason about your FS semantics to ascertain safety the performance/safety trade-off is clearly something that's useful to have tweaks for.

And with "bulk" the concern about leaving behind stray corrupt objects is entirely orthagonal to corcerns about losing a ref update, which is the main thing we're worried about.

I also don't see how even if you're arguing that nobody would want one without the other because everyone who cares about "bulk" cares about this stray-corrupt-loose-but-no-ref-update case, how it has any business being tied up in the "bulk" mode as far as the implementation goes.

That's because the same edge case is exposed by core.fsyncObjectFiles=false for those who are assuming the initial "ordered" semantics.

I.e. if we're saying that before we write the ref we'd like to not expose the WIP objects in the primary object store because they're not fsync'd yet, how is that mode different than "bulk" if we crash while doing that operation (before the eventual fsync()).

So I really think it's much better to split these concerns up.

I think even if you insist on the same end-state it makes the patch progression much *easier* to reason about. We'd then solve one problem at a time, and start with a commit where just the semantics that are unique to "bulk" are implemented, with nothing else conflated with those.

In terms of more understandable progression I also think this series would be much easier to understand if it converted one caller without needing the "cookie" where doing so is easy, e.g. the unpack-objects.c caller where we're processing nr_objects, so we can just pass down a flag to do the fsync() for i == nr_objects.

That'll then clearly show that the whole business of having the global state on the side is just a replacement for passing down such a flag.