core.fsyncmethod: add 'batch' mode for faster fsyncing of multiple objects

When core.fsync includes loose-object, we issue an fsync after every written object. For a 'git-add' or similar command that adds a lot of files to the repo, the costs of these fsyncs adds up. One major factor in this cost is the time it takes for the physical storage controller to flush its caches to durable media.

This series takes advantage of the writeout-only mode of git_fsync to issue OS cache writebacks for all of the objects being added to the repository followed by a single fsync to a dummy file, which should trigger a filesystem log flush and storage controller cache flush. This mechanism is known to be safe on common Windows filesystems and expected to be safe on macOS. Some linux filesystems, such as XFS, will probably do the right thing as well. See [1] for previous discussion on the predecessor of this patch series.

This series is important on Windows, where loose-objects are included in the fsync set by default in Git-For-Windows. In this series, I'm also setting the default mode for Windows to turn on loose object fsyncing with batch mode, so that we can get CI coverage of the actual git-for-windows configuration upstream. We still don't actually issue fsyncs for the test suite since GIT_TEST_FSYNC is set to 0, but we exercise all of the surrounding batch mode code.

This work is based on 'seen' at 367f447f0f0cf39e9830c865e8373e42a3c45303. It's dependent on ns/core-fsyncmethod.

[1]
https://lore.kernel.org/git/2c1ddef6057157d85da74a7274e03eacf0374e45.1629856293.git.gitgitgadget@gmail.com/

Neeraj Singh (7):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean core.fsyncmethod: batched disk flushes for loose-objects
update-index: use the bulk-checkin infrastructure
unpack-objects: use the bulk-checkin infrastructure core.fsync: use batch mode and sync loose objects by default on
Windows
core.fsyncmethod: tests for batch mode
core.fsyncmethod: performance tests for add and stash

 Documentation/config/core.txt |  5 ++
 builtin/unpack-objects.c      |  3 ++
 builtin/update-index.c        |  6 +++
 bulk-checkin.c                | 89 +++++++++++++++++++++++++++++++----
 bulk-checkin.h                |  2 +
 cache.h                       | 12 ++++-
 compat/mingw.h                |  3 ++
 config.c                      |  4 +-
 git-compat-util.h             |  2 +
 object-file.c                 |  2 +
 t/lib-unique-files.sh         | 36 ++++++++++++++
 t/perf/p3700-add.sh           | 59 +++++++++++++++++++++++
 t/perf/p3900-stash.sh         | 62 ++++++++++++++++++++++++
 t/perf/perf-lib.sh            |  4 +-
 t/t3700-add.sh                | 22 +++++++++
 t/t3903-stash.sh              | 17 +++++++
 t/t5300-pack-object.sh        | 32 ++++++++-----
 17 files changed, 335 insertions(+), 25 deletions(-)
 create mode 100644 t/lib-unique-files.sh
 create mode 100755 t/perf/p3700-add.sh
 create mode 100755 t/perf/p3900-stash.sh

base-commit: 367f447f0f0cf39e9830c865e8373e42a3c45303
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1134%2Fneerajsi-msft%2Fns%2Fbatched-fsync-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1134/neerajsi-msft/ns/batched-fsync-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1134

gitgitgadget

From: Neeraj Singh <[email protected]>

Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.

* Rename 'state' variable to 'bulk_checkin_state', since we will later be adding 'bulk_fsync_objdir'. This also makes the variable easier to find in the debugger, since the name is more unique.

* Move the 'plugged' data member of 'bulk_checkin_state' into a separate static variable. Doing this avoids resetting the variable in finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we seem to unintentionally disable the plugging functionality the first time a new packfile must be created due to packfile size limits. While disabling the plugging state only results in suboptimal behavior for the current code, it would be fatal for the bulk-fsync functionality later in this patch series.

---
 bulk-checkin.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

index e988a388b65..93b1dc5138a 100644
@@ -10,9 +10,9 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static struct bulk_checkin_state {
-	unsigned plugged:1;
+static int bulk_checkin_plugged;
 
+static struct bulk_checkin_state {
 	char *pack_tmp_name;
 	struct hashfile *f;
 	off_t offset;

@@ -21,7 +21,7 @@ static struct bulk_checkin_state {
 	struct pack_idx_entry **written;
 	uint32_t alloc_written;
 	uint32_t nr_written;
-} state;
+} bulk_checkin_state;
 
 static void finish_tmp_packfile(struct strbuf *basename,
 				const char *pack_tmp_name,

@@ -278,21 +278,23 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags)
 {
-	int status = deflate_to_pack(&state, oid, fd, size, type,
+	int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
 				     path, flags);
-	if (!state.plugged)
-		finish_bulk_checkin(&state);
+	if (!bulk_checkin_plugged)
+		finish_bulk_checkin(&bulk_checkin_state);
 	return status;
 }
 
 void plug_bulk_checkin(void)
 {
-	state.plugged = 1;
+	assert(!bulk_checkin_plugged);
+	bulk_checkin_plugged = 1;
 }
 
 void unplug_bulk_checkin(void)
 {
-	state.plugged = 0;
-	if (state.f)
-		finish_bulk_checkin(&state);
+	assert(bulk_checkin_plugged);
+	bulk_checkin_plugged = 0;
+	if (bulk_checkin_state.f)
+		finish_bulk_checkin(&bulk_checkin_state);
 }

gitgitgadget

Sorry, but I am confused. The bulk-checkin infrastructure is there so that we can send many little objects into a single packfile instead of creating many little loose object files. Everything we throw at object-file.c::index_stream() will be concatenated into the single packfile while we are "plugged" until we get "unplugged".

My understanding of what you are doing in this series is to still create many little loose object files, but avoid the overhead of having to fsync them individually. And I am not sure how well the original idea behind the bulk-checkin infrastructure to avoid overhead of having to create many loose objects by creating a single packfile (and presumably having to fsync at the end, but that is just a single .pack file) with your goal of still creating many loose object files but synching them more efficiently.

Is it just the new feature is piggybacking on the existing bulk checkin infrastructure, even though these two have nothing in common?

I noticed that you invented bulk-checkin back in 2011, but I don't think your description matches what the code actually does. index_bulk_checkin is only called from index_stream, which is only called from index_fd. index_fd goes down the index_bulk_checkin path for large files (512MB by default). It looks like the effect of the 'plug/unplug' code is to allow multiple large blobs to
go into a single packfile rather than each getting one getting its own separate packfile.

I think my new usage is congruent with the existing API, which seems to be about combining multiple add operations into a large transaction, where we can do some cleanup operations once we're finished. In the preexisting code, the transaction is about adding a bunch of large objects to a single pack file (while leaving small objects loose), and then completing the packfile when the adds are finished.

---
On a side note, I've also been thinking about how we could use a packfile approach as an alternative means to achieve faster addition of many small objects. It's essentially what you stated above, where we'd send our little objects
into a pack file. But to avoid frequent repacking overhead, we might want to reuse
the 'latest' packfile across multiple Git invocations by appending objects to it, with
an fsync on the file at the end.

We'd need sufficient padding between objects created by different Git invocations to
ensure that previously synced data doesn't get disturbed by later operations. We'd
need to rewrite the pack indexes each time, but that's at least derived metadata, so it
doesn't need to be fsynced. To make the pack indexes more incrementally-updatable,
we might want to have the fanout table be checksummed, with checksummed pointers to
leaf blocks. If we detect corruption during an index lookup, we could recreate the index
from the packfile.

Essentially the above proposal is to move away from storing loose objects in the filesystem
and instead to index the data within Git itself.

Thanks,
Neeraj

OK, so it was part me, and part a suboptimal presentation, I guess ;-)

Let me rephrase the idea to see if I got it right this time.

The bulk-checkin API has two interesting entry points, "plug" that signals that we are about to repeat possibly many operations to add new objects to the object store, and "unplug" that signals that we are done such adding. They are meant to serve as a hint for the object layer to optimize its operation.

So far the only way the hint was used was that the logic that sends an overly large object into a packfile (instead of storing it loose, which leaves it subject to expensive repacking later) can shove more than one such objects in the same packfile.

This series invents another use of the "plug"-"unplug" hint. By knowing that many loose object files are created and when the series of object creation ended, we can avoid having to fsync each and every one of them on certain filesystems and achieve the same robustness. The new "batch" option to core.fsyncmethod triggers this mechanism.

Did I get it right, more-or-less?

Thanks.

Yes, that's my understanding as well.

Thanks,
Neeraj

I guess what I wrote would make a useful material for early part of the log message to help future developers.

Thanks.

Will do. I changed the commit message to explain the current functionality of bulk-checkin and how it's similar to batched-fsync.

From: Neeraj Singh <[email protected]>

When adding many objects to a repo with core.fsync=loose-object, 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. This commit introduces a new core.fsyncMethod=batch option that batches up hardware flushes. It hooks into the bulk-checkin plugging and unplugging functionality, takes advantage of tmp-objdir, and uses the writeout-only support code.

When the new mode is enabled, we do the following for each new object:
1. Create the object in a tmp-objdir.
2. Issue a pagecache writeback request and wait for it to complete.

At the end of the entire transaction when unplugging bulk checkin:
1. Issue an fsync against a dummy file to flush the hardware writeback cache, which should by now have seen the tmp-objdir writes.
2. Rename all of the tmp-objdir files to their final names.
3. When updating the index and/or refs, we assume that Git will issue another fsync internal to that operation. This is not the default today, but the user now has the option of syncing the index and there is a separate patch series to implement syncing of refs.

On a filesystem with a singular journal that is updated during name operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS 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.

Batch mode is only enabled if core.fsyncObjectFiles is false or unset.

_Performance numbers_:

Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD. Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD. Windows - Same host as Linux, a preview version of Windows 11.

Adding 500 files to the repo with 'git add' Times reported in seconds.

object file syncing | Linux | Mac   | Windows
--------------------|-------|-------|--------
           disabled | 0.06  |  0.35 | 0.61
              fsync | 1.88  | 11.18 | 2.47
              batch | 0.15  |  0.41 | 1.53

---
 Documentation/config/core.txt |  5 +++
 bulk-checkin.c                | 67 +++++++++++++++++++++++++++++++++++
 bulk-checkin.h                |  2 ++
 cache.h                       |  8 ++++-
 config.c                      |  2 ++
 object-file.c                 |  2 ++
 6 files changed, 85 insertions(+), 1 deletion(-)

index 062e5259905..c041ed33801 100644
@@ -628,6 +628,11 @@ core.fsyncMethod::
 * `writeout-only` issues pagecache writeback requests, but depending on the
   filesystem and storage hardware, data added to the repository may not be
   durable in the event of a system crash. This is the default mode on macOS.
+* `batch` enables a mode that uses writeout-only flushes to stage multiple
+  updates in the disk writeback cache and then a single full fsync to trigger
+  the disk cache flush at the end of the operation. This mode is expected to
+  be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
+  and on Windows for repos stored on NTFS or ReFS filesystems.
 
 core.fsyncObjectFiles::
 	This boolean will enable 'fsync()' when writing object files.

index b26f3dc3b74..08f292379b6 100644
@@ -6,6 +6,8 @@
 
 #include "cache.h"
 
+void fsync_loose_object_bulk_checkin(int fd);
+
 int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);

index d347d0757f7..4d07691e791 100644
@@ -1040,7 +1040,8 @@ extern int use_fsync;
 
 enum fsync_method {
 	FSYNC_METHOD_FSYNC,
-	FSYNC_METHOD_WRITEOUT_ONLY
+	FSYNC_METHOD_WRITEOUT_ONLY,
+	FSYNC_METHOD_BATCH
 };
 
 extern enum fsync_method fsync_method;

@@ -1766,6 +1767,11 @@ void fsync_or_die(int fd, const char *);
 int fsync_component(enum fsync_component component, int fd);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
+static inline int batch_fsync_enabled(enum fsync_component component)
+{
+	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);

index 261ee7436e0..0b28f90de8b 100644
@@ -1688,6 +1688,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 			fsync_method = FSYNC_METHOD_FSYNC;
 		else if (!strcmp(value, "writeout-only"))
 			fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
+		else if (!strcmp(value, "batch"))
+			fsync_method = FSYNC_METHOD_BATCH;
 		else
 			warning(_("ignoring unknown core.fsyncMethod value '%s'"), value);
 

index 295cb899e22..ef6621ffe56 100644
@@ -1894,6 +1894,8 @@ static void close_loose_object(int fd)
 
 	if (fsync_object_files > 0)
 		fsync_or_die(fd, "loose object file");
+	else if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
+		fsync_loose_object_bulk_checkin(fd);
 	else
 		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,
 				       "loose object file");

gitgitgadget

Can do. I'm hoping that you'll revive your batch-mode refs change too so that we get batching across the ODB and Refs, which are the two data stores that may receive many updates in a single Git command. This documentation comment will read:

```
* `batch` enables a mode that uses writeout-only flushes to stage multiple
  updates in the disk writeback cache and then does a single full fsync of
  a dummy file to trigger the disk cache flush at the end of the operation.
  Currently `batch` mode only applies to loose-object files. Other repository
  data is made durable as if `fsync` was specified. This mode is expected to
  be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
  and on Windows for repos stored on NTFS or ReFS filesystems.
```

Will Fix.

Thanks,
Neeraj

Huh, I completely forgot that my previous implementation already had such a mechanism. I may have a go at it again, but it would take me a while given that I'll be OOO most of April.

Reads good to me, thanks!

Patrick

But what about ext4? Will fsync-ing trigger writing journal?

An old man doll... just what I always wanted! - Clara

That's a good question. So I did an experiment on ext4 which gives me some confidence:

Here's my ext4 configuration: /dev/sdc on / type ext4 (rw,relatime,discard,errors=remount-ro,data=ordered)

I added a new mode called core.fsyncMethod=batch-extra-fsync. This issues an extra open,fsync,close during migration from the tmp-objdir (which I confirmed is really happening using strace). The added cost of this extra operation is relatively small compared to core.fsyncMethod=fsync. That leads me to believe that (barring fs bugs), ext4 thinks that the data is already sufficiently durable that it doesn't need to issue an extra disk cache flush. See
https://github.com/neerajsi-msft/git/commit/131466dd95165efc5c480d971c69ea1e9182657e
for the test code. I don't particularly want to add this as a built-in mode at this point since it will be somewhat hard to document which mode a user should choose.

Thanks,
Neeraj

From: Neeraj Singh <[email protected]>

The update-index functionality is used internally by 'git stash push' to setup the internal stashed commit.

This change enables bulk-checkin for update-index infrastructure to speed up adding new objects to the object database by leveraging the batch fsync functionality.

There is some risk with this change, since under batch fsync, the object files will be in a tmp-objdir until update-index is complete. This usage is unlikely, since any tool invoking update-index and expecting to see objects would have to synchronize with the update-index process after passing it a file path.

---
 builtin/update-index.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 75d646377cc..38e9d7e88cb 100644
@@ -5,6 +5,7 @@
  */
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "bulk-checkin.h"
 #include "config.h"
 #include "lockfile.h"
 #include "quote.h"

@@ -1110,6 +1111,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	the_index.updated_skipworktree = 1;
 
+	/* we might be adding many objects to the object database */
+	plug_bulk_checkin();
+
 	/*
 	 * Custom copy of parse_options() because we want to handle
 	 * filename arguments as they come.

@@ -1190,6 +1194,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
+	/* by now we must have added all of the new objects */
+	unplug_bulk_checkin();
 	if (split_index > 0) {
 		if (git_config_get_split_index() == 0)
 			warning(_("core.splitIndex is set to false; "

gitgitgadget

From: Neeraj Singh <[email protected]>

The unpack-objects functionality is used by fetch, push, and fast-import to turn the transfered data into object database entries when there are fewer objects than the 'unpacklimit' setting.

By enabling bulk-checkin when unpacking objects, we can take advantage of batched fsyncs.

---
 builtin/unpack-objects.c | 3 +++
 1 file changed, 3 insertions(+)

index dbeb0680a58..c55b6616aed 100644
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "bulk-checkin.h"
 #include "config.h"
 #include "object-store.h"
 #include "object.h"

@@ -503,10 +504,12 @@ static void unpack_all(void)
 	if (!quiet)
 		progress = start_progress(_("Unpacking objects"), nr_objects);
 	CALLOC_ARRAY(obj_list, nr_objects);
+	plug_bulk_checkin();
 	for (i = 0; i < nr_objects; i++) {
 		unpack_one(i);
 		display_progress(progress, i + 1);
 	}
+	unplug_bulk_checkin();
 	stop_progress(&progress);
 
 	if (delta_list)

gitgitgadget

From: Neeraj Singh <[email protected]>

Git for Windows has defaulted to core.fsyncObjectFiles=true since September 2017. We turn on syncing of loose object files with batch mode in upstream Git so that we can get broad coverage of the new code upstream.

We don't actually do fsyncs in the test suite, since GIT_TEST_FSYNC is set to 0. However, we do exercise all of the surrounding batch mode code since GIT_TEST_FSYNC merely makes the maybe_fsync wrapper always appear to succeed.

change fsyncmethod to batch as well

---
 cache.h           | 4 ++++
 compat/mingw.h    | 3 +++
 config.c          | 2 +-
 git-compat-util.h | 2 ++
 4 files changed, 10 insertions(+), 1 deletion(-)

index 4d07691e791..04193d87246 100644
@@ -1031,6 +1031,10 @@ enum fsync_component {
 			      FSYNC_COMPONENT_INDEX | \
 			      FSYNC_COMPONENT_REFERENCE)
 
+#ifndef FSYNC_COMPONENTS_PLATFORM_DEFAULT
+#define FSYNC_COMPONENTS_PLATFORM_DEFAULT FSYNC_COMPONENTS_DEFAULT
+#endif
+
 /*
  * A bitmask indicating which components of the repo should be fsynced.
  */

index 6074a3d3ced..afe30868c04 100644
@@ -332,6 +332,9 @@ int mingw_getpagesize(void);
 int win32_fsync_no_flush(int fd);
 #define fsync_no_flush win32_fsync_no_flush
 
+#define FSYNC_COMPONENTS_PLATFORM_DEFAULT (FSYNC_COMPONENTS_DEFAULT | FSYNC_COMPONENT_LOOSE_OBJECT)
+#define FSYNC_METHOD_DEFAULT (FSYNC_METHOD_BATCH)
+
 struct rlimit {
 	unsigned int rlim_cur;
 };

index 0b28f90de8b..c76443dc556 100644
@@ -1342,7 +1342,7 @@ static const struct fsync_component_name {
 
 static enum fsync_component parse_fsync_components(const char *var, const char *string)
 {
-	enum fsync_component current = FSYNC_COMPONENTS_DEFAULT;
+	enum fsync_component current = FSYNC_COMPONENTS_PLATFORM_DEFAULT;
 	enum fsync_component positive = 0, negative = 0;
 
 	while (string) {

index 0892e209a2f..fffe42ce7c1 100644
@@ -1257,11 +1257,13 @@ __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
 
+#ifndef FSYNC_METHOD_DEFAULT
 #ifdef __APPLE__
 #define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY
 #else
 #define FSYNC_METHOD_DEFAULT FSYNC_METHOD_FSYNC
 #endif
+#endif
 
 enum fsync_action {
 	FSYNC_WRITEOUT_ONLY,

gitgitgadget

From: Neeraj Singh <[email protected]>

Add test cases to exercise batch mode for:
* 'git add'
* 'git stash'
* 'git update-index'
* 'git unpack-objects'

These tests ensure that the added data winds up in the object database.

In this change we introduce a new test helper lib-unique-files.sh. The goal of this library is to create a tree of files that have different oids from any other files that may have been created in the current test repo. This helps us avoid missing validation of an object being added due to it already being in the repo.

We aren't actually issuing any fsyncs in these tests, since GIT_TEST_FSYNC is 0, but we still exercise all of the tmp_objdir logic in bulk-checkin.

---
 t/lib-unique-files.sh  | 36 ++++++++++++++++++++++++++++++++++++
 t/t3700-add.sh         | 22 ++++++++++++++++++++++
 t/t3903-stash.sh       | 17 +++++++++++++++++
 t/t5300-pack-object.sh | 32 +++++++++++++++++++++-----------
 4 files changed, 96 insertions(+), 11 deletions(-)
 create mode 100644 t/lib-unique-files.sh

new file mode 100644
index 00000000000..a7de4ca8512
@@ -0,0 +1,36 @@
+# Helper to create files with unique contents
+
+
+# Create multiple files with unique contents. Takes the number of
+# directories, the number of files in each directory, and the base
+# directory.
+#
+# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
+#					 each in my_dir, all with unique
+#					 contents.
+
+test_create_unique_files() {
+	test "$#" -ne 3 && BUG "3 param"
+
+	local dirs=$1
+	local files=$2
+	local basedir=$3
+	local counter=0
+	test_tick
+	local basedata=$test_tick
+
+
+	rm -rf $basedir
+
+	for i in $(test_seq $dirs)
+	do
+		local dir=$basedir/dir$i
+
+		mkdir -p "$dir"
+		for j in $(test_seq $files)
+		do
+			counter=$((counter + 1))
+			echo "$basedata.$counter"  >"$dir/file$j.txt"
+		done
+	done
+}

index b1f90ba3250..1f349f52ad3 100755
@@ -8,6 +8,8 @@ test_description='Test of git add, including the -- option.'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+. $TEST_DIRECTORY/lib-unique-files.sh
+
 # Test the file mode "$1" of the file "$2" in the index.
 test_mode_in_index () {
 	case "$(git ls-files -s "$2")" in

@@ -34,6 +36,26 @@ test_expect_success \
     'Test that "git add -- -q" works' \
     'touch -- -q && git add -- -q'
 
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'git add: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 fsync-files &&
+	git $BATCH_CONFIGURATION add -- ./fsync-files/ &&
+	rm -f fsynced_files &&
+	git ls-files --stage fsync-files/ > fsynced_files &&
+	test_line_count = 8 fsynced_files &&
+	awk -- '{print \$2}' fsynced_files | xargs -n1 git cat-file -e
+"
+
+test_expect_success 'git update-index: core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 fsync-files2 &&
+	find fsync-files2 ! -type d -print | xargs git $BATCH_CONFIGURATION update-index --add -- &&
+	rm -f fsynced_files2 &&
+	git ls-files --stage fsync-files2/ > fsynced_files2 &&
+	test_line_count = 8 fsynced_files2 &&
+	awk -- '{print \$2}' fsynced_files2 | xargs -n1 git cat-file -e
+"
+
 test_expect_success \
 	'git add: Test that executable bit is not used if core.filemode=0' \
 	'git config core.filemode 0 &&

index 4abbc8fccae..877276c1ca3 100755
@@ -9,6 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
+. $TEST_DIRECTORY/lib-unique-files.sh
 
 test_expect_success 'usage on cmd and subcommand invalid option' '
 	test_expect_code 129 git stash --invalid-option 2>usage &&

@@ -1410,6 +1411,22 @@ test_expect_success 'stash handles skip-worktree entries nicely' '
 	git rev-parse --verify refs/stash:A.t
 '
 
+
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'stash with core.fsyncmethod=batch' "
+	test_create_unique_files 2 4 fsync-files &&
+	git $BATCH_CONFIGURATION stash push -u -- ./fsync-files/ &&
+	rm -f fsynced_files &&
+
+	# The files were untracked, so use the third parent,
+	# which contains the untracked files
+	git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+	test_line_count = 8 fsynced_files &&
+	awk -- '{print \$3}' fsynced_files | xargs -n1 git cat-file -e
+"
+
+
 test_expect_success 'git stash succeeds despite directory/file change' '
 	test_create_repo directory_file_switch_v1 &&
 	(

index a11d61206ad..8e2f73cc68f 100755
@@ -162,23 +162,25 @@ test_expect_success 'pack-objects with bogus arguments' '
 
 check_unpack () {
 	test_when_finished "rm -rf git2" &&
-	git init --bare git2 &&
-	git -C git2 unpack-objects -n <"$1".pack &&
-	git -C git2 unpack-objects <"$1".pack &&
-	(cd .git && find objects -type f -print) |
-	while read path
-	do
-		cmp git2/$path .git/$path || {
-			echo $path differs.
-			return 1
-		}
-	done
+	git $2 init --bare git2 &&
+	(
+		git $2 -C git2 unpack-objects -n <"$1".pack &&
+		git $2 -C git2 unpack-objects <"$1".pack &&
+		git $2 -C git2 cat-file --batch-check="%(objectname)"
+	) <obj-list >current &&
+	cmp obj-list current
 }
 
 test_expect_success 'unpack without delta' '
 	check_unpack test-1-${packname_1}
 '
 
+BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+
+test_expect_success 'unpack without delta (core.fsyncmethod=batch)' '
+	check_unpack test-1-${packname_1} "$BATCH_CONFIGURATION"
+'
+
 test_expect_success 'pack with REF_DELTA' '
 	packname_2=$(git pack-objects --progress test-2 <obj-list 2>stderr) &&
 	check_deltas stderr -gt 0

@@ -188,6 +190,10 @@ test_expect_success 'unpack with REF_DELTA' '
 	check_unpack test-2-${packname_2}
 '
 
+test_expect_success 'unpack with REF_DELTA (core.fsyncmethod=batch)' '
+       check_unpack test-2-${packname_2} "$BATCH_CONFIGURATION"
+'
+
 test_expect_success 'pack with OFS_DELTA' '
 	packname_3=$(git pack-objects --progress --delta-base-offset test-3 \
 			<obj-list 2>stderr) &&

@@ -198,6 +204,10 @@ test_expect_success 'unpack with OFS_DELTA' '
 	check_unpack test-3-${packname_3}
 '
 
+test_expect_success 'unpack with OFS_DELTA (core.fsyncmethod=batch)' '
+       check_unpack test-3-${packname_3} "$BATCH_CONFIGURATION"
+'
+
 test_expect_success 'compare delta flavors' '
 	perl -e '\''
 		defined($_ = -s $_) or die for @ARGV;

gitgitgadget

From: Neeraj Singh <[email protected]>

Add basic performance tests for "git add" and "git stash" of a lot of new objects with various fsync settings. This shows the benefit of batch mode relative to an ordinary stash command.

---
 t/perf/p3700-add.sh   | 59 ++++++++++++++++++++++++++++++++++++++++
 t/perf/p3900-stash.sh | 62 +++++++++++++++++++++++++++++++++++++++++++
 t/perf/perf-lib.sh    |  4 +--
 3 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100755 t/perf/p3700-add.sh
 create mode 100755 t/perf/p3900-stash.sh

new file mode 100755
index 00000000000..2ea78c9449d
@@ -0,0 +1,59 @@
+#!/bin/sh
+#
+# This test measures the performance of adding new files to the object database
+# and index. The test was originally added to measure the effect of the
+# core.fsyncMethod=batch mode, which is why we are testing different values
+# of that setting explicitly and creating a lot of unique objects.
+
+test_description="Tests performance of add"
+
+# Fsync is normally turned off for the test suite.
+GIT_TEST_FSYNC=1
+export GIT_TEST_FSYNC
+
+. ./perf-lib.sh
+
+. $TEST_DIRECTORY/lib-unique-files.sh
+
+test_perf_default_repo
+test_checkout_worktree
+
+dir_count=10
+files_per_dir=50
+total_files=$((dir_count * files_per_dir))
+
+# We need to create the files each time we run the perf test, but
+# we do not want to measure the cost of creating the files, so run
+# the test once.
+if test "${GIT_PERF_REPEAT_COUNT-1}" -ne 1
+then
+	echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
+	GIT_PERF_REPEAT_COUNT=1
+fi
+
+for m in false true batch
+do
+	test_expect_success "create the files for object_fsyncing=$m" '
+		git reset --hard &&
+		# create files across directories
+		test_create_unique_files $dir_count $files_per_dir files
+	'
+
+	case $m in
+	false)
+		FSYNC_CONFIG='-c core.fsync=-loose-object -c core.fsyncmethod=fsync'
+		;;
+	true)
+		FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=fsync'
+		;;
+	batch)
+		FSYNC_CONFIG='-c core.fsync=loose-object -c core.fsyncmethod=batch'
+		;;
+	esac
+
+	test_perf "add $total_files files (object_fsyncing=$m)" "
+		git $FSYNC_CONFIG add files
+	"
+done
+
+test_done

index 932105cd12c..d270d1d962a 100644
@@ -98,8 +98,8 @@ test_perf_create_repo_from () {
 	mkdir -p "$repo/.git"
 	(
 		cd "$source" &&
-		{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
-			cp -R "$objects_dir" "$repo/.git/"; } &&
+		{ cp -Rl "$objects_dir" "$repo/.git/" ||
+			cp -R "$objects_dir" "$repo/.git/" 2>/dev/null;} &&
 
 		# common_dir must come first here, since we want source_git to
 		# take precedence and overwrite any overlapping files

gitgitgadget

V2 changes:

* Change doc to indicate that only some repo updates are batched
* Null and zero out control variables in do_batch_fsync under unplug_bulk_checkin
* Make batch mode default on Windows.
* Update the description for the initial patch that cleans up the bulk-checkin infrastructure.
* Rebase onto 'seen' at 0cac37f38f9.

--Original definition-- When core.fsync includes loose-object, we issue an fsync after every written object. For a 'git-add' or similar command that adds a lot of files to the repo, the costs of these fsyncs adds up. One major factor in this cost is the time it takes for the physical storage controller to flush its caches to durable media.

This series takes advantage of the writeout-only mode of git_fsync to issue OS cache writebacks for all of the objects being added to the repository followed by a single fsync to a dummy file, which should trigger a filesystem log flush and storage controller cache flush. This mechanism is known to be safe on common Windows filesystems and expected to be safe on macOS. Some linux filesystems, such as XFS, will probably do the right thing as well. See [1] for previous discussion on the predecessor of this patch series.

This series is important on Windows, where loose-objects are included in the fsync set by default in Git-For-Windows. In this series, I'm also setting the default mode for Windows to turn on loose object fsyncing with batch mode, so that we can get CI coverage of the actual git-for-windows configuration upstream. We still don't actually issue fsyncs for the test suite since GIT_TEST_FSYNC is set to 0, but we exercise all of the surrounding batch mode code.

This work is based on 'seen' at . It's dependent on ns/core-fsyncmethod.

[1]
https://lore.kernel.org/git/2c1ddef6057157d85da74a7274e03eacf0374e45.1629856293.git.gitgitgadget@gmail.com/

Neeraj Singh (7):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean core.fsyncmethod: batched disk flushes for loose-objects
update-index: use the bulk-checkin infrastructure
unpack-objects: use the bulk-checkin infrastructure core.fsync: use batch mode and sync loose objects by default on
Windows
core.fsyncmethod: tests for batch mode
core.fsyncmethod: performance tests for add and stash

 Documentation/config/core.txt |  7 +++
 builtin/unpack-objects.c      |  3 ++
 builtin/update-index.c        |  6 +++
 bulk-checkin.c                | 92 +++++++++++++++++++++++++++++++----
 bulk-checkin.h                |  2 +
 cache.h                       | 12 ++++-
 compat/mingw.h                |  3 ++
 config.c                      |  4 +-
 git-compat-util.h             |  2 +
 object-file.c                 |  2 +
 t/lib-unique-files.sh         | 36 ++++++++++++++
 t/perf/p3700-add.sh           | 59 ++++++++++++++++++++++
 t/perf/p3900-stash.sh         | 62 +++++++++++++++++++++++
 t/perf/perf-lib.sh            |  4 +-
 t/t3700-add.sh                | 22 +++++++++
 t/t3903-stash.sh              | 17 +++++++
 t/t5300-pack-object.sh        | 32 +++++++-----
 17 files changed, 340 insertions(+), 25 deletions(-)
 create mode 100644 t/lib-unique-files.sh
 create mode 100755 t/perf/p3700-add.sh
 create mode 100755 t/perf/p3900-stash.sh

base-commit: 0cac37f38f94bb93550eb164b5d574cd96e23785
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1134%2Fneerajsi-msft%2Fns%2Fbatched-fsync-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1134/neerajsi-msft/ns/batched-fsync-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1134

Range-diff vs v1:

1: a77d02df626 ! 1: 9c2abd12bbb bulk-checkin: rename 'state' variable and separate 'plugged' boolean
@@ Metadata
## Commit message ##
bulk-checkin: rename 'state' variable and separate 'plugged' boolean

- Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.
+ This commit prepares for adding batch-fsync to the bulk-checkin
+ infrastructure.
+
+ The bulk-checkin infrastructure is currently used to batch up addition
+ of large blobs to a packfile. When a blob is larger than
+ big_file_threshold, we unconditionally add it to a pack. If bulk
+ checkins are 'plugged', we allow multiple large blobs to be added to a
+ single pack until we reach the packfile size limit; otherwise, we simply
+ make a new packfile for each large blob. The 'unplug' call tells us when
+ the series of blob additions is done so that we can finish the packfiles
+ and make their objects available to subsequent operations.
+
+ Stated another way, bulk-checkin allows callers to define a transaction
+ that adds multiple objects to the object database, where the object
+ database can optimize its internal operations within the transaction
+ boundary.
+
+ Batched fsync will fit into bulk-checkin by taking advantage of the
+ plug/unplug functionality to determine the appropriate time to fsync
+ and make newly-added objects available in the primary object database.

* Rename 'state' variable to 'bulk_checkin_state', since we will later be adding 'bulk_fsync_objdir'. This also makes the variable easier to
2: d38f20b4430 ! 2: 3ed1dcd9b9b core.fsyncmethod: batched disk flushes for loose-objects
@@ Documentation/config/core.txt: core.fsyncMethod::
filesystem and storage hardware, data added to the repository may not be durable in the event of a system crash. This is the default mode on macOS.
+* batch enables a mode that uses writeout-only flushes to stage multiple
-+ updates in the disk writeback cache and then a single full fsync to trigger -+ the disk cache flush at the end of the operation. This mode is expected to ++ updates in the disk writeback cache and then does a single full fsync of ++ a dummy file to trigger the disk cache flush at the end of the operation. ++ Currently batch mode only applies to loose-object files. Other repository ++ data is made durable as if fsync was specified. This mode is expected to
+ be as safe as fsync on macOS for repos stored on HFS+ or APFS filesystems
+ and on Windows for repos stored on NTFS or ReFS filesystems.

@@ bulk-checkin.c: clear_exit:
+ fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
+ delete_tempfile(&temp);
+ strbuf_release(&temp_path);
++ needs_batch_fsync = 0;
+ }
+
-+ if (bulk_fsync_objdir)
++ 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)

 3:  b0480f0c814 = 3:  54797dbc520 update-index: use the bulk-checkin infrastructure
 4:  99e3a61b919 = 4:  6662e2dae0f unpack-objects: use the bulk-checkin infrastructure
 5:  4e56c58c8cb ! 5:  03bf591742a core.fsync: use batch mode and sync loose objects by default on Windows
     @@ Commit message
      
          Signed-off-by: Neeraj Singh <[email protected]>
      
     -    change fsyncmethod to batch as well
     -
       ## cache.h ##
      @@ cache.h: enum fsync_component {
       			      FSYNC_COMPONENT_INDEX | \
 6:  88e47047d79 = 6:  1937746df47 core.fsyncmethod: tests for batch mode
 7:  876741f1ef9 = 7:  624244078c7 core.fsyncmethod: performance tests for add and stash

gitgitgadget

From: Neeraj Singh <[email protected]>

This commit prepares for adding batch-fsync to the bulk-checkin infrastructure.

The bulk-checkin infrastructure is currently used to batch up addition of large blobs to a packfile. When a blob is larger than big_file_threshold, we unconditionally add it to a pack. If bulk checkins are 'plugged', we allow multiple large blobs to be added to a single pack until we reach the packfile size limit; otherwise, we simply make a new packfile for each large blob. The 'unplug' call tells us when the series of blob additions is done so that we can finish the packfiles and make their objects available to subsequent operations.

Stated another way, bulk-checkin allows callers to define a transaction that adds multiple objects to the object database, where the object database can optimize its internal operations within the transaction boundary.

Batched fsync will fit into bulk-checkin by taking advantage of the plug/unplug functionality to determine the appropriate time to fsync and make newly-added objects available in the primary object database.

* Rename 'state' variable to 'bulk_checkin_state', since we will later be adding 'bulk_fsync_objdir'. This also makes the variable easier to find in the debugger, since the name is more unique.

* Move the 'plugged' data member of 'bulk_checkin_state' into a separate static variable. Doing this avoids resetting the variable in finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we seem to unintentionally disable the plugging functionality the first time a new packfile must be created due to packfile size limits. While disabling the plugging state only results in suboptimal behavior for the current code, it would be fatal for the bulk-fsync functionality later in this patch series.

---
 bulk-checkin.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

index e988a388b65..93b1dc5138a 100644
@@ -10,9 +10,9 @@
 #include "packfile.h"
 #include "object-store.h"
 
-static struct bulk_checkin_state {
-	unsigned plugged:1;
+static int bulk_checkin_plugged;
 
+static struct bulk_checkin_state {
 	char *pack_tmp_name;
 	struct hashfile *f;
 	off_t offset;

@@ -21,7 +21,7 @@ static struct bulk_checkin_state {
 	struct pack_idx_entry **written;
 	uint32_t alloc_written;
 	uint32_t nr_written;
-} state;
+} bulk_checkin_state;
 
 static void finish_tmp_packfile(struct strbuf *basename,
 				const char *pack_tmp_name,

@@ -278,21 +278,23 @@ int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags)
 {
-	int status = deflate_to_pack(&state, oid, fd, size, type,
+	int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
 				     path, flags);
-	if (!state.plugged)
-		finish_bulk_checkin(&state);
+	if (!bulk_checkin_plugged)
+		finish_bulk_checkin(&bulk_checkin_state);
 	return status;
 }
 
 void plug_bulk_checkin(void)
 {
-	state.plugged = 1;
+	assert(!bulk_checkin_plugged);
+	bulk_checkin_plugged = 1;
 }
 
 void unplug_bulk_checkin(void)
 {
-	state.plugged = 0;
-	if (state.f)
-		finish_bulk_checkin(&state);
+	assert(bulk_checkin_plugged);
+	bulk_checkin_plugged = 0;
+	if (bulk_checkin_state.f)
+		finish_bulk_checkin(&bulk_checkin_state);
 }

gitgitgadget

From: Neeraj Singh <[email protected]>

When adding many objects to a repo with core.fsync=loose-object, 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. This commit introduces a new core.fsyncMethod=batch option that batches up hardware flushes. It hooks into the bulk-checkin plugging and unplugging functionality, takes advantage of tmp-objdir, and uses the writeout-only support code.

When the new mode is enabled, we do the following for each new object:
1. Create the object in a tmp-objdir.
2. Issue a pagecache writeback request and wait for it to complete.

At the end of the entire transaction when unplugging bulk checkin:
1. Issue an fsync against a dummy file to flush the hardware writeback cache, which should by now have seen the tmp-objdir writes.
2. Rename all of the tmp-objdir files to their final names.
3. When updating the index and/or refs, we assume that Git will issue another fsync internal to that operation. This is not the default today, but the user now has the option of syncing the index and there is a separate patch series to implement syncing of refs.

On a filesystem with a singular journal that is updated during name operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS 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.

Batch mode is only enabled if core.fsyncObjectFiles is false or unset.

_Performance numbers_:

Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD. Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD. Windows - Same host as Linux, a preview version of Windows 11.

Adding 500 files to the repo with 'git add' Times reported in seconds.

object file syncing | Linux | Mac   | Windows
--------------------|-------|-------|--------
           disabled | 0.06  |  0.35 | 0.61
              fsync | 1.88  | 11.18 | 2.47
              batch | 0.15  |  0.41 | 1.53

---
 Documentation/config/core.txt |  7 ++++
 bulk-checkin.c                | 70 +++++++++++++++++++++++++++++++++++
 bulk-checkin.h                |  2 +
 cache.h                       |  8 +++-
 config.c                      |  2 +
 object-file.c                 |  2 +
 6 files changed, 90 insertions(+), 1 deletion(-)

index 889522956e4..a3798dfc334 100644
@@ -628,6 +628,13 @@ core.fsyncMethod::
 * `writeout-only` issues pagecache writeback requests, but depending on the
   filesystem and storage hardware, data added to the repository may not be
   durable in the event of a system crash. This is the default mode on macOS.
+* `batch` enables a mode that uses writeout-only flushes to stage multiple
+  updates in the disk writeback cache and then does a single full fsync of
+  a dummy file to trigger the disk cache flush at the end of the operation.
+  Currently `batch` mode only applies to loose-object files. Other repository
+  data is made durable as if `fsync` was specified. This mode is expected to
+  be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
+  and on Windows for repos stored on NTFS or ReFS filesystems.
 
 core.fsyncObjectFiles::
 	This boolean will enable 'fsync()' when writing object files.

index b26f3dc3b74..08f292379b6 100644
@@ -6,6 +6,8 @@
 
 #include "cache.h"
 
+void fsync_loose_object_bulk_checkin(int fd);
+
 int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);

index 3160bc1e489..d1ae51388c9 100644
@@ -1040,7 +1040,8 @@ extern int use_fsync;
 
 enum fsync_method {
 	FSYNC_METHOD_FSYNC,
-	FSYNC_METHOD_WRITEOUT_ONLY
+	FSYNC_METHOD_WRITEOUT_ONLY,
+	FSYNC_METHOD_BATCH
 };
 
 extern enum fsync_method fsync_method;

@@ -1767,6 +1768,11 @@ void fsync_or_die(int fd, const char *);
 int fsync_component(enum fsync_component component, int fd);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
+static inline int batch_fsync_enabled(enum fsync_component component)
+{
+	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);

index 261ee7436e0..0b28f90de8b 100644
@@ -1688,6 +1688,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 			fsync_method = FSYNC_METHOD_FSYNC;
 		else if (!strcmp(value, "writeout-only"))
 			fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
+		else if (!strcmp(value, "batch"))
+			fsync_method = FSYNC_METHOD_BATCH;
 		else
 			warning(_("ignoring unknown core.fsyncMethod value '%s'"), value);
 

index 5258d9ed827..bdb0a38328f 100644
@@ -1895,6 +1895,8 @@ static void close_loose_object(int fd)
 
 	if (fsync_object_files > 0)
 		fsync_or_die(fd, "loose object file");
+	else if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
+		fsync_loose_object_bulk_checkin(fd);
 	else
 		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,
 				       "loose object file");

gitgitgadget

Thanks for noticing the camelCasing. The config var name was also wrong. Now it will read:

Do you have any recommendations on how to easily ferry the correct errno out of tmp_objdir_create?
It looks like the remerge-diff usage has the same die behavior w/o errno, and the builtin/receive-pack.c usage
doesn't die, but also loses the errno. I'm concerned about preserving the errno across the or tmp_objdir_destroy
calls. I could introduce a temp errno var to preserve it across those. Is that what you had in mind?

Thanks,
Neeraj

Re my question in
https://lore.kernel.org/git/[email protected]/
(which you *partially* replied to per my reading, i.e. not the fsync_nth() question) I still don't get why the tmp-objdir part of this is needed.

For "git stash" which is one thing sped up by this let's go over what commands/FS ops we do. I changed the test like this:

	diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
	index 3fc16944e9e..479a495c68c 100755
	--- a/t/t3903-stash.sh
	+++ b/t/t3903-stash.sh
	@@ -1383,7 +1383,7 @@ BATCH_CONFIGURATION='-c core.fsync=loose-object -c core.fsyncmethod=batch'
	 
	 test_expect_success 'stash with core.fsyncmethod=batch' "
	 	test_create_unique_files 2 4 fsync-files &&
	-	git $BATCH_CONFIGURATION stash push -u -- ./fsync-files/ &&
	+	strace -f git $BATCH_CONFIGURATION stash push -u -- ./fsync-files/ &&
	 	rm -f fsynced_files &&
	 
	 	# The files were untracked, so use the third parent,

Then we get this output, with my comments, and I snipped some output:

	$ ./t3903-stash.sh --run=1-4,114 -vixd 2>&1|grep --color -e 89772c935031c228ed67890f9 -e .git/stash -e bulk_fsync -e .git/index
	[pid 14703] access(".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/89772c935031c228ed67890f953c0a2b5c8316", F_OK) = -1 ENOENT (No such file or directory)
	[pid 14703] access(".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", F_OK) = -1 ENOENT (No such file or directory)
	[pid 14703] link(".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/tmp_obj_bdUlzu", ".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/89772c935031c228ed67890f953c0a2b5c8316") = 0

Here we're creating the tmp_objdir() files. We then sync_file_range() and close() this.

	[pid 14703] openat(AT_FDCWD, "/home/avar/g/git/t/trash directory.t3903-stash/.git/objects/tmp_objdir-bulk-fsync-rR3AQI/bulk_fsync_HsDRl7", O_RDWR|O_CREAT|O_EXCL, 0600) = 9
	[pid 14703] unlink("/home/avar/g/git/t/trash directory.t3903-stash/.git/objects/tmp_objdir-bulk-fsync-rR3AQI/bulk_fsync_HsDRl7") = 0

This is the flushing of the "cookie" in do_batch_fsync().

	[pid 14703] newfstatat(AT_FDCWD, ".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/89772c935031c228ed67890f953c0a2b5c8316", {st_mode=S_IFREG|0444, st_size=29, ...}, 0) = 0
	[pid 14703] link(".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/89772c935031c228ed67890f953c0a2b5c8316", ".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316") = 0

Here we're going through the object dir migration with unplug_bulk_checkin().

	[pid 14703] unlink(".git/objects/tmp_objdir-bulk-fsync-rR3AQI/fb/89772c935031c228ed67890f953c0a2b5c8316") = 0
	newfstatat(AT_FDCWD, ".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", {st_mode=S_IFREG|0444, st_size=29, ...}, AT_SYMLINK_NOFOLLOW) = 0
	[pid 14705] access(".git/objects/tmp_objdir-bulk-fsync-0F7DGy/fb/89772c935031c228ed67890f953c0a2b5c8316", F_OK) = -1 ENOENT (No such file or directory)
	[pid 14705] access(".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", F_OK) = 0
	[pid 14705] utimensat(AT_FDCWD, ".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", NULL, 0) = 0
	[pid 14707] openat(AT_FDCWD, ".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", O_RDONLY|O_CLOEXEC) = 9

We then update the index itself, first a temporary index.stash :

    openat(AT_FDCWD, "/home/avar/g/git/t/trash directory.t3903-stash/.git/index.stash.19141.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 8
    openat(AT_FDCWD, ".git/index.stash.19141", O_RDONLY) = 9
    newfstatat(AT_FDCWD, ".git/objects/fb/89772c935031c228ed67890f953c0a2b5c8316", {st_mode=S_IFREG|0444, st_size=29, ...}, AT_SYMLINK_NOFOLLOW) = 0
    newfstatat(AT_FDCWD, "/home/avar/g/git/t/trash directory.t3903-stash/.git/index.stash.19141.lock", {st_mode=S_IFREG|0644, st_size=927, ...}, 0) = 0
    rename("/home/avar/g/git/t/trash directory.t3903-stash/.git/index.stash.19141.lock", "/home/avar/g/git/t/trash directory.t3903-stash/.git/index.stash.19141") = 0
    unlink(".git/index.stash.19141")        = 0

Followed by the same and a later rename of the actual index:

    [pid 19146] rename("/home/avar/g/git/t/trash directory.t3903-stash/.git/index.lock", "/home/avar/g/git/t/trash directory.t3903-stash/.git/index") = 0

So, my question is still why the temporary object dir migration part of this is needed.

We are writing N loose object files, and we write those to temporary names already.

AFAIKT we could do all of this by doing the same tmp/rename/sync_file_range dance on the main object store.

Then instead of the "bulk_fsync" cookie file don't close() the last file object file we write until we issue the fsync on it.

But maybe this is all needed, I just can't understand from the commit message why the "bulk checkin" part is being done.

I think since we've been over this a few times without any success it would really help to have some example of the smallest set of syscalls to write a file like this safely. I.e. this is doing (pseudocode):

    /* first the bulk path */
    open("bulk/x.tmp");
    write("bulk/x.tmp");
    sync_file_range("bulk/x.tmp");
    close("bulk/x.tmp");
    rename("bulk/x.tmp", "bulk/x");
    open("bulk/y.tmp");
    write("bulk/y.tmp");
    sync_file_range("bulk/y.tmp");
    close("bulk/y.tmp");
    rename("bulk/y.tmp", "bulk/y");
    /* Rename to "real" */
    rename("bulk/x", x");
    rename("bulk/y", y");
    /* sync a cookie */
    fsync("cookie");

And I'm asking why it's not:

    /* Rename to "real" as we go */
    open("x.tmp");
    write("x.tmp");
    sync_file_range("x.tmp");
    close("x.tmp");
    rename("x.tmp", "x");
    last_fd = open("y.tmp"); /* don't close() the last one yet */
    write("y.tmp");
    sync_file_range("y.tmp");
    rename("y.tmp", "y");
    /* sync a cookie */
    fsync(last_fd);

Which I guess is two questions:

A. do we need the cookie, or can we re-use the fd of the last thing we
write?
B. Is the bulk indirection needed?

Sorry for not fully answering your question. I think part of the issue might be background, where it's not clear to me what's different between your understanding
and mine, so may not have included something that's questionable to you but not to me.

Your syscall description below makes the issues very concrete, so I think we'll get it this round :).

Why not the main object store? We want to maintain the invariant that any name in the main object store refers to a file that durably has the correct contents.
If we do sync_file_range and then rename, and then crash, we now have a file in the main object store with some SHA name, whose contents may or may not match the SHA. However, if we ensure an fsync happens before the rename, a crash at any point will leave us either with no file in the main object store or
with a file that is durable on the disk.

The '/* Rename to "real" */' and '/* sync a cookie */' steps are reversed in your above sequence. It should be
1: (for each file)
a) open
b) write
c) sync_file_range
d) close
e) rename in tmp_objdir -- The rename step is not required for
bulk-fsync. An earlier version of this series didn't do it, but Jeff King pointed out that it was required for concurrency:
https://lore.kernel.org/all/YVOrikAl%2Fu5%[email protected]/

2: fsync something on the same volume to flush the filesystem log and disk cache. This functions as a "barrier".
3: Rename to final names. At this point we know that the "contents" are durable, so if the final name exists, we can read through it to get the data.

We can re-use the FD of the last thing we write, but that results in a tricker API which
is more intrusive on callers. I was originally using a lockfile, but found a usage where
there was no lockfile in unpack-objects.

Hopefully the explanation above makes it clear why we need the indirection. To state it again,
we need a real fsync before creating the final name in the objdir, otherwise on a crash a name
could exist that points at contents which could have been lost, since they weren't durable. I
updated the comment in do_batch_fsync to make this a little clearer.

This line is copied from the preexisting version of the same code in close_loose_object.
If I'm understanding it correctly, the entire chain of messages is untranslated and would
remain as english. fsync_or_die doesn't have a _(). Can we just leave it that way, since
this is not a situation that should actually happen to many users? Alternatively, I think it
would be pretty trivial to just pass through the file name, so I'll just do that.

In the original implementation [1], I did some custom thing for renaming the files
rather than tmp_objdir. But you suggested at the time that I use tmp_objdir, which
was a good decision, since it made access to the objects possible in-process and for descendents in the middle of the transaction.

It sounds to me like I just shouldn't plug the bulk checkin for cases where we're not
going to add to the ODB. Plugging the bulk checkin is always optional. But when
I wrote the code, I didn't love the result, since it makes arbitrary callers harder. So
I changed the code to lazily create the tmp objdir the first time an object shows up, which has the same effect of avoiding the cost when we aren't adding any
objects. This also avoids the need to write an error message, since failing to create
the tmp objdir will just result in a fsync. The main downside here is that it's another thing
that will have to change if we want to make adding to the ODB multithreaded.

Thanks,
Neeraj

[1] https://lore.kernel.org/all/12cad737635663ed596e52f89f0f4f22f58bfe38.1632176111.git.gitgitgadget@gmail.com/

On Mon, Mar 21 2022, Neeraj Singh wrote:

[Don't have time for a full reply, sorry, just something quick]

Ah, I see.

Why does that matter? If the "bulk" mode works as advertised we might have such a corrupt loose or pack file, but we won't have anything referring to it as far as reachability goes.

I'm aware that the various code paths that handle OID writing don't deal too well with it in practice to say the least, which one can try with say:

    $ echo foo | git hash-object -w --stdin
    45b983be36b73c0788dc9cbcb76cbb80fc7bb057
    $ echo | sudo tee .git/objects/45/b983be36b73c0788dc9cbcb76cbb80fc7bb057

I.e. "fsck", "show" etc. will all scream bloddy murder, and re-running that hash-object again even returns successful (we see it's there already, and think it's OK).

But in any case, I think it would me much easier to both review and reason about this code if these concerns were split up.

I.e. things that want no fsync at all (I'd think especially so) might want to have such updates serialized in this manner, and as Junio pointed out making these things inseparable as you've done creates API concerns & fallout that's got nothing to do with what we need for the performance gains of the bulk checkin fsyncing technique, e.g. concurrent "update-index" consumers not being able to assume reported objects exist as soon as they're reported.

Sorry.

Yes we definitely need the rename, I was wondering about why we needed it 2x for each file, but that was answered above.

Ok, so it's something we could do, but passing down 2-3 functions to object-file.c was a hassle.

I tried to hack that up earlier and found that it wasn't *too bad*. I.e. we'd pass some "flags" about our intent, and amend various functions to take "don't close this one" and pass up the fd (or even do that as a global).

In any case, having the commit message clearly document what's needed for what & what's essential & just shortcut taken for the convenience of the current implementation would be really useful.

Then we can always e.g. change this later to just do the the fsync() on the last of N we write.

[Ran out of time, sorry]

I was under the impression that in-practice a corrupt loose-object can create persistent problems in the repo for future commands, since we might not aggressively verify that an existing file with a certain OID really is valid when
adding a new instance of the data with the same OID.

If you don't have an fsync barrier before producing the final content-addressable
name, you can't reason about "this operation happened before that operation," so it wouldn't really be valid to say that "we won't have anything referring to it as far
as reachability goes."

It's entirely possible that you'd have trees pointing to other trees or blobs that aren't
valid, since data writes can be durable in any order. At this point, future attempts add
the same blobs or trees might silently drop the updates. I'm betting that's why core.fsyncObjectFiles was added in the first place, since someone observed severe
persistent consequences for this form of corruption.

I want to explicitly not respond to this concern. I don't believe this 100 line patch
can be usefully split.

I left a comment in the (now very long) commit message that indicates the dummy file is there to make the API simpler.

Thanks,
Neeraj

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/[email protected]/

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.

Because we have a content-addressable store which generally trusts its contents are valid (at least when adding new instances of the same content), the mere existence of a loose-object with a certain name is enough to make it "reachable" to future operations, even if there are no other immediate ways to get to that object.

On Windows, where we want to have a consistent ODB by default, I'm adding a faster way
to achieve that safety. No user is asking for a world where we are doing half the
work to make a consistent ODB but not the other half.

This one patch works holistically to provide the full batch safety feature, and splitting it
into two patches (which in the new version wouldn't be as clean as you've done it above)
doesn't make the correctness of the whole thing more reviewable. In fact it's less reviewable
since the fsync and objdir migration are in two separate patches and a future historian
wouldn't get as clear of a picture of the whole mechanism.

That seems appropriate for our mailing list discussion, but I don't see how it helps the patch series, because we'd be doing work to fsync the final object and then reversing that work when producing the final end state, which uses the dummy file.

Thanks,
Neeraj

This RFC series is a continuation of the thread at
https://lore.kernel.org/git/CANQDOde2OG8fVSM1hQE3FBmzWy5FkgQCWAUYhFztB8UGFyJELg@mail.gmail.com/;
More details in individual commit messages.

I'd suggested (upthread of) there pass new object flags down to the object machinery instead of the {un,}plug_bulk_checkin() API route. This has advantages described in more details in individual patches.

This also shows that the not-using tmpdir approach can be significantly faster than using it, and per my understanding just as safe fsync-wise for those willing to deal with the caveat of possibly having truncated *unreachable* objects.

I thought that showing some working code with what I was suggesting was more productive than continuing the current back & forth :)

Ævar Arnfjörð Bjarmason (7):
write-or-die.c: remove unused fsync_component() function
unpack-objects: add skeleton HASH_N_OBJECTS{,_{FIRST,LAST}} flags
object-file: pass down unpack-objects.c flags for "bulk" checkin
update-index: use a utility function for stdin consumption
update-index: pass down an "oflags" argument
update-index: rename "buf" to "line"
update-index: make use of HASH_N_OBJECTS{,_{FIRST,LAST}} flags

 builtin/add.c            |  3 --
 builtin/unpack-objects.c | 62 ++++++++++++++------------
 builtin/update-index.c   | 96 ++++++++++++++++++++++++++--------------
 bulk-checkin.c           | 86 -----------------------------------
 bulk-checkin.h           |  6 ---
 cache.h                  |  9 ++--
 object-file.c            | 39 +++++++++++-----
 t/t1050-large.sh         |  3 ++
 write-or-die.c           |  7 ---
 9 files changed, 131 insertions(+), 180 deletions(-)

2.35.1.1428.g1c1a0152d61

This function added in 020406eaa52 (core.fsync: introduce granular fsync control infrastructure, 2022-03-10) hasn't been used, and appears not to be used by the follow-up series either?

---
 cache.h        | 1 -
 write-or-die.c | 7 -------
 2 files changed, 8 deletions(-)

index 84fafe2ed71..5d863f8c5e8 100644
@@ -1766,7 +1766,6 @@ int copy_file_with_time(const char *dst, const char *src, int mode);
 
 void write_or_die(int fd, const void *buf, size_t count);
 void fsync_or_die(int fd, const char *);
-int fsync_component(enum fsync_component component, int fd);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
 static inline int batch_fsync_enabled(enum fsync_component component)

index c4fd91b5b43..103698450c3 100644
@@ -76,13 +76,6 @@ void fsync_or_die(int fd, const char *msg)
 		die_errno("fsync error on '%s'", msg);
 }
 
-int fsync_component(enum fsync_component component, int fd)
-{
-	if (fsync_components & component)
-		return maybe_fsync(fd);
-	return 0;
-}
-
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
 {
 	if (fsync_components & component)

2.35.1.1428.g1c1a0152d61

This helper was put in for Patrick's patch at
https://lore.kernel.org/git/f1e8a7bb3bf0f4c0414819cb1d5579dc08fd2a4f.1646905589.git.ps@pks.im/.

Thanks,
Neeraj

In preparation for making the bulk-checkin.c logic operate from object-file.c itself in some common cases let's add HASH_N_OBJECTS{,_{FIRST,LAST}} flags.

This will allow us to adjust for-loops that add N objects to just pass down whether they have >1 objects (HASH_N_OBJECTS), as well as passing down flags for whether we have the first or last object.

We'll thus be able to drive any sort of batch-object mechanism from write_object_file_flags() directly, which until now didn't know if it was doing one object, or some arbitrary N.

---
 builtin/unpack-objects.c | 60 +++++++++++++++++++++++-----------------
 cache.h                  |  3 ++
 2 files changed, 37 insertions(+), 26 deletions(-)

index c55b6616aed..ec40c6fd966 100644
@@ -233,7 +233,8 @@ static void write_rest(void)
 }
 
 static void added_object(unsigned nr, enum object_type type,
-			 void *data, unsigned long size);
+			 void *data, unsigned long size,
+			 unsigned oflags);
 
 /*
  * Write out nr-th object from the list, now we know the contents

@@ -241,21 +242,21 @@ static void added_object(unsigned nr, enum object_type type,
  * to be checked at the end.
  */
 static void write_object(unsigned nr, enum object_type type,
-			 void *buf, unsigned long size)
+			 void *buf, unsigned long size, unsigned oflags)
 {
 	if (!strict) {
-		if (write_object_file(buf, size, type,
-				      &obj_list[nr].oid) < 0)
+		if (write_object_file_flags(buf, size, type,
+				      &obj_list[nr].oid, oflags) < 0)
 			die("failed to write object");
-		added_object(nr, type, buf, size);
+		added_object(nr, type, buf, size, oflags);
 		free(buf);
 		obj_list[nr].obj = NULL;
 	} else if (type == OBJ_BLOB) {
 		struct blob *blob;
-		if (write_object_file(buf, size, type,
-				      &obj_list[nr].oid) < 0)
+		if (write_object_file_flags(buf, size, type,
+					    &obj_list[nr].oid, oflags) < 0)
 			die("failed to write object");
-		added_object(nr, type, buf, size);
+		added_object(nr, type, buf, size, oflags);
 		free(buf);
 
 		blob = lookup_blob(the_repository, &obj_list[nr].oid);

@@ -269,7 +270,7 @@ static void write_object(unsigned nr, enum object_type type,
 		int eaten;
 		hash_object_file(the_hash_algo, buf, size, type,
 				 &obj_list[nr].oid);
-		added_object(nr, type, buf, size);
+		added_object(nr, type, buf, size, oflags);
 		obj = parse_object_buffer(the_repository, &obj_list[nr].oid,
 					  type, size, buf,
 					  &eaten);

@@ -283,7 +284,7 @@ static void write_object(unsigned nr, enum object_type type,
 
 static void resolve_delta(unsigned nr, enum object_type type,
 			  void *base, unsigned long base_size,
-			  void *delta, unsigned long delta_size)
+			  void *delta, unsigned long delta_size, unsigned oflags)
 {
 	void *result;
 	unsigned long result_size;

@@ -294,7 +295,7 @@ static void resolve_delta(unsigned nr, enum object_type type,
 	if (!result)
 		die("failed to apply delta");
 	free(delta);
-	write_object(nr, type, result, result_size);
+	write_object(nr, type, result, result_size, oflags);
 }
 
 /*

@@ -302,7 +303,7 @@ static void resolve_delta(unsigned nr, enum object_type type,
  * resolve all the deltified objects that are based on it.
  */
 static void added_object(unsigned nr, enum object_type type,
-			 void *data, unsigned long size)
+			 void *data, unsigned long size, unsigned oflags)
 {
 	struct delta_info **p = &delta_list;
 	struct delta_info *info;

@@ -313,7 +314,7 @@ static void added_object(unsigned nr, enum object_type type,
 			*p = info->next;
 			p = &delta_list;
 			resolve_delta(info->nr, type, data, size,
-				      info->delta, info->size);
+				      info->delta, info->size, oflags);
 			free(info);
 			continue;
 		}

@@ -322,18 +323,19 @@ static void added_object(unsigned nr, enum object_type type,
 }
 
 static void unpack_non_delta_entry(enum object_type type, unsigned long size,
-				   unsigned nr)
+				   unsigned nr, unsigned oflags)
 {
 	void *buf = get_data(size);
 
 	if (!dry_run && buf)
-		write_object(nr, type, buf, size);
+		write_object(nr, type, buf, size, oflags);
 	else
 		free(buf);
 }
 
 static int resolve_against_held(unsigned nr, const struct object_id *base,
-				void *delta_data, unsigned long delta_size)
+				void *delta_data, unsigned long delta_size,
+				unsigned oflags)
 {
 	struct object *obj;
 	struct obj_buffer *obj_buffer;

@@ -344,12 +346,12 @@ static int resolve_against_held(unsigned nr, const struct object_id *base,
 	if (!obj_buffer)
 		return 0;
 	resolve_delta(nr, obj->type, obj_buffer->buffer,
-		      obj_buffer->size, delta_data, delta_size);
+		      obj_buffer->size, delta_data, delta_size, oflags);
 	return 1;
 }
 
 static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
-			       unsigned nr)
+			       unsigned nr, unsigned oflags)
 {
 	void *delta_data, *base;
 	unsigned long base_size;

@@ -366,7 +368,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 		if (has_object_file(&base_oid))
 			; /* Ok we have this one */
 		else if (resolve_against_held(nr, &base_oid,
-					      delta_data, delta_size))
+					      delta_data, delta_size, oflags))
 			return; /* we are done */
 		else {
 			/* cannot resolve yet --- queue it */

@@ -428,7 +430,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 		}
 	}
 
-	if (resolve_against_held(nr, &base_oid, delta_data, delta_size))
+	if (resolve_against_held(nr, &base_oid, delta_data, delta_size, oflags))
 		return;
 
 	base = read_object_file(&base_oid, &type, &base_size);

@@ -440,11 +442,11 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 		has_errors = 1;
 		return;
 	}
-	resolve_delta(nr, type, base, base_size, delta_data, delta_size);
+	resolve_delta(nr, type, base, base_size, delta_data, delta_size, oflags);
 	free(base);
 }
 
-static void unpack_one(unsigned nr)
+static void unpack_one(unsigned nr, unsigned oflags)
 {
 	unsigned shift;
 	unsigned char *pack;

@@ -472,11 +474,11 @@ static void unpack_one(unsigned nr)
 	case OBJ_TREE:
 	case OBJ_BLOB:
 	case OBJ_TAG:
-		unpack_non_delta_entry(type, size, nr);
+		unpack_non_delta_entry(type, size, nr, oflags);
 		return;
 	case OBJ_REF_DELTA:
 	case OBJ_OFS_DELTA:
-		unpack_delta_entry(type, size, nr);
+		unpack_delta_entry(type, size, nr, oflags);
 		return;
 	default:
 		error("bad object type %d", type);

@@ -491,6 +493,7 @@ static void unpack_all(void)
 {
 	int i;
 	struct pack_header *hdr = fill(sizeof(struct pack_header));
+	unsigned oflags;
 
 	nr_objects = ntohl(hdr->hdr_entries);
 

@@ -505,9 +508,14 @@ static void unpack_all(void)
 		progress = start_progress(_("Unpacking objects"), nr_objects);
 	CALLOC_ARRAY(obj_list, nr_objects);
 	plug_bulk_checkin();
+	oflags = nr_objects > 1 ? HASH_N_OBJECTS : 0;
 	for (i = 0; i < nr_objects; i++) {
-		unpack_one(i);
-		display_progress(progress, i + 1);
+		int nth = i + 1;
+		unsigned f = i == 0 ? HASH_N_OBJECTS_FIRST :
+			nr_objects == nth ? HASH_N_OBJECTS_LAST : 0;
+
+		unpack_one(i, oflags | f);
+		display_progress(progress, nth);
 	}
 	unplug_bulk_checkin();
 	stop_progress(&progress);

index 5d863f8c5e8..320248a54e0 100644
@@ -896,6 +896,9 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *,
 #define HASH_FORMAT_CHECK 2
 #define HASH_RENORMALIZE  4
 #define HASH_SILENT 8
+#define HASH_N_OBJECTS 1<<4
+#define HASH_N_OBJECTS_FIRST 1<<5
+#define HASH_N_OBJECTS_LAST 1<<6
 int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags);
 int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags);
 

2.35.1.1428.g1c1a0152d61

Remove much of this as a POC for exploring some of what I mentioned in
https://lore.kernel.org/git/[email protected]/

This commit is obviously not what we *should* do as end-state, but demonstrates what's needed (I think) for a bare-minimum implementation of just the "bulk" syncing method for loose objects without the part where we do the tmp-objdir.c dance.

Performance with this is already quite promising. Benchmarking with:

	git hyperfine -L rev ns/batched-fsync,HEAD -s 'make CFLAGS=-O3' \
	    	-p 'rm -rf r.git && git init --bare r.git' \
		'./git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack'

I.e. unpacking a small packfile (my dotfiles) yields, on a Linux ramdisk:

	Benchmark 1: ./git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'ns/batched-fsync
	  Time (mean ± σ):     815.9 ms ±   8.2 ms    [User: 522.9 ms, System: 287.9 ms]
	  Range (min … max):   805.6 ms … 835.9 ms    10 runs

	Benchmark 2: ./git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'HEAD
	  Time (mean ± σ):     779.4 ms ±  15.4 ms    [User: 505.7 ms, System: 270.2 ms]
	  Range (min … max):   763.1 ms … 813.9 ms    10 runs

	Summary
	  './git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'HEAD' ran
	    1.05 ± 0.02 times faster than './git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'ns/batched-fsync'

Doing the same with "strace --summary-only", which probably helps to emulate cases with slower syscalls is ~15% faster than using the tmp-objdir indirection:

	Summary
	  'strace --summary-only ./git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'HEAD' ran
	    1.16 ± 0.01 times faster than 'strace --summary-only ./git -C r.git -c core.fsync=loose-object -c core.fsyncMethod=batch unpack-objects </tmp/pack-dotfiles.pack' in 'ns/batched-fsync'

Which makes sense in terms of syscalls. In my case HEAD has ~101k calls, and the parent topic is making ~129k calls, with around 2x the number of unlink(), link() as expected.

Of course some users will want to use the tmp-objdir.c method. So a version of this commit could be rewritten to come earlier in the series, with the "bulk" on top being optional.

It seems to me that it's a much better strategy to do this whole thing in close_loose_object() after passing down the new HASH_N_OBJECTS / HASH_N_OBJECTS_FIRST / HASH_N_OBJECTS_LAST flags.

Doing that for the "builtin/add.c" and "builtin/unpack-objects.c" code having its {un,}plug_bulk_checkin() removed here is then just a matter of passing down a similar set of flags indicating whether we're dealing with N objects, and if so if we're dealing with the last one or not.

As we'll see in subsequent commits doing it this way also effortlessly integrates with other HASH_* flags. E.g. for "update-index" the code being rm'd here doesn't handle the interaction with "HASH_WRITE_OBJECT" properly, but once we've moved all this sync bootstrapping logic to close_loose_object() we'll never get to it if we're not actually writing something.

This code currently doesn't use the HASH_N_OBJECTS_FIRST flag, but that's what we'd use later to optionally call tmp_objdir_create().

Aside: This also changes logic that was a bit confusing and repetitive in close_loose_object(). Previously we'd first call batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT) which is just as shorthand for:

	fsync_components & FSYNC_COMPONENT_LOOSE_OBJECT &&
	fsync_method == FSYNC_METHOD_BATCH

We'd then proceed to call fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT) later in the same function, which is just a way of calling fsync_or_die() if:

fsync_components & FSYNC_COMPONENT_LOOSE_OBJECT

Now we instead just define a local "fsync_loose" variable by checking "fsync_components & FSYNC_COMPONENT_LOOSE_OBJECT", which shows us that the previous case of fsync_component_or_die(...)" could just be added to the existing "fsync_object_files > 0" branch.

---
 builtin/add.c            |  3 --
 builtin/unpack-objects.c |  2 -
 builtin/update-index.c   |  4 --
 bulk-checkin.c           | 86 ----------------------------------------
 bulk-checkin.h           |  6 ---
 cache.h                  |  5 ---
 object-file.c            | 37 ++++++++++++-----
 t/t1050-large.sh         |  3 ++
 8 files changed, 29 insertions(+), 117 deletions(-)

index 3ffb86a4338..a6d7f4dc1e1 100644
@@ -670,8 +670,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		string_list_clear(&only_match_skip_worktree, 0);
 	}
 
-	plug_bulk_checkin();
-
 	if (add_renormalize)
 		exit_status |= renormalize_tracked_files(&pathspec, flags);
 	else

@@ -682,7 +680,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (chmod_arg && pathspec.nr)
 		exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
-	unplug_bulk_checkin();
 
 finish:
 	if (write_locked_index(&the_index, &lock_file,

index ec40c6fd966..93da436581b 100644
@@ -507,7 +507,6 @@ static void unpack_all(void)
 	if (!quiet)
 		progress = start_progress(_("Unpacking objects"), nr_objects);
 	CALLOC_ARRAY(obj_list, nr_objects);
-	plug_bulk_checkin();
 	oflags = nr_objects > 1 ? HASH_N_OBJECTS : 0;
 	for (i = 0; i < nr_objects; i++) {
 		int nth = i + 1;

@@ -517,7 +516,6 @@ static void unpack_all(void)
 		unpack_one(i, oflags | f);
 		display_progress(progress, nth);
 	}
-	unplug_bulk_checkin();
 	stop_progress(&progress);
 
 	if (delta_list)

index cbd2b0d633b..95ed3c47b2e 100644
@@ -1118,8 +1118,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	parse_options_start(&ctx, argc, argv, prefix,
 			    options, PARSE_OPT_STOP_AT_NON_OPTION);
 
-	/* optimize adding many objects to the object database */
-	plug_bulk_checkin();
 	while (ctx.argc) {
 		if (parseopt_state != PARSE_OPT_DONE)
 			parseopt_state = parse_options_step(&ctx, options,

@@ -1194,8 +1192,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
-	/* by now we must have added all of the new objects */
-	unplug_bulk_checkin();
 	if (split_index > 0) {
 		if (git_config_get_split_index() == 0)
 			warning(_("core.splitIndex is set to false; "

index 181d3447ff9..76fc33e0c8f 100644
@@ -6,14 +6,8 @@
 
 #include "cache.h"
 
-void prepare_loose_object_bulk_checkin(void);
-void fsync_loose_object_bulk_checkin(int fd, const char *filename);
-
 int index_bulk_checkin(struct object_id *oid,
 		       int fd, size_t size, enum object_type type,
 		       const char *path, unsigned flags);
 
-void plug_bulk_checkin(void);
-void unplug_bulk_checkin(void);
-
 #endif

index 320248a54e0..997bf2f57fd 100644
@@ -1771,11 +1771,6 @@ void write_or_die(int fd, const void *buf, size_t count);
 void fsync_or_die(int fd, const char *);
 void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);
 
-static inline int batch_fsync_enabled(enum fsync_component component)
-{
-	return (fsync_components & component) && (fsync_method == FSYNC_METHOD_BATCH);
-}
-
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);

index cd0ddb49e4b..dbeb3df502d 100644
@@ -1886,19 +1886,37 @@ void hash_object_file(const struct git_hash_algo *algo, const void *buf,
 	hash_object_file_literally(algo, buf, len, type_name(type), oid);
 }
 
+static void sync_loose_object_batch(int fd, const char *filename,
+				    const unsigned oflags)
+{
+	const int last = oflags & HASH_N_OBJECTS_LAST;
+
+	/*
+	 * We're doing a sync_file_range() (or equivalent) for 1..N-1
+	 * objects, and then a "real" fsync() for N. On some OS's
+	 * enabling core.fsync=loose-object && core.fsyncMethod=batch
+	 * improves the performance by a lot.
+	 */
+	if (last || (!last && git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0))
+		fsync_or_die(fd, filename);
+}
+
 /* Finalize a file on disk, and close it. */
-static void close_loose_object(int fd, const char *filename)
+static void close_loose_object(int fd, const char *filename,
+			       const unsigned oflags)
 {
+	int fsync_loose;
+
 	if (the_repository->objects->odb->will_destroy)
 		goto out;
 
-	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
-		fsync_loose_object_bulk_checkin(fd, filename);
-	else if (fsync_object_files > 0)
+	fsync_loose = fsync_components & FSYNC_COMPONENT_LOOSE_OBJECT;
+
+	if (oflags & HASH_N_OBJECTS && fsync_loose &&
+	    fsync_method == FSYNC_METHOD_BATCH)
+		sync_loose_object_batch(fd, filename, oflags);
+	else if (fsync_object_files > 0 || fsync_loose)
 		fsync_or_die(fd, filename);
-	else
-		fsync_component_or_die(FSYNC_COMPONENT_LOOSE_OBJECT, fd,
-				       filename);
 
 out:
 	if (close(fd) != 0)

@@ -1962,9 +1980,6 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
 	static struct strbuf tmp_file = STRBUF_INIT;
 	static struct strbuf filename = STRBUF_INIT;
 
-	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
-		prepare_loose_object_bulk_checkin();
-
 	loose_object_path(the_repository, &filename, oid);
 
 	fd = create_tmpfile(&tmp_file, filename.buf);

@@ -2015,7 +2030,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
 		die(_("confused by unstable object source data for %s"),
 		    oid_to_hex(oid));
 
-	close_loose_object(fd, tmp_file.buf);
+	close_loose_object(fd, tmp_file.buf, flags);
 
 	if (mtime) {
 		struct utimbuf utb;

index 4f3aa17c994..1baaa8024c8 100755
@@ -5,6 +5,9 @@ test_description='adding and checking out large blobs'
 
 . ./test-lib.sh
 
+skip_all='TODO: migrate the builtin/add.c code'
+test_done
+
 test_expect_success setup '
 	# clone does not allow us to pass core.bigfilethreshold to
 	# new repos, so set core.bigfilethreshold globally

2.35.1.1428.g1c1a0152d61

---
 builtin/update-index.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

index 95ed3c47b2e..80b96ec5721 100644
@@ -971,6 +971,25 @@ static enum parse_opt_result reupdate_callback(
 	return 0;
 }
 
+static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
+			    const char *prefix, int prefix_length,
+			    const int nul_term_line, const int set_executable_bit)
+{
+	char *p;
+
+	if (!nul_term_line && buf->buf[0] == '"') {
+		strbuf_reset(unquoted);
+		if (unquote_c_style(unquoted, buf->buf, NULL))
+			die("line is badly quoted");
+		strbuf_swap(buf, unquoted);
+	}
+	p = prefix_path(prefix, prefix_length, buf->buf);
+	update_one(p);
+	if (set_executable_bit)
+		chmod_path(set_executable_bit, p);
+	free(p);
+}
+
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
 	int newfd, entries, has_errors = 0, nul_term_line = 0;

@@ -1174,20 +1193,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		struct strbuf unquoted = STRBUF_INIT;
 
 		setup_work_tree();
-		while (getline_fn(&buf, stdin) != EOF) {
-			char *p;
-			if (!nul_term_line && buf.buf[0] == '"') {
-				strbuf_reset(&unquoted);
-				if (unquote_c_style(&unquoted, buf.buf, NULL))
-					die("line is badly quoted");
-				strbuf_swap(&buf, &unquoted);
-			}
-			p = prefix_path(prefix, prefix_length, buf.buf);
-			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
-			free(p);
-		}
+		while (getline_fn(&buf, stdin) != EOF)
+			line_from_stdin(&buf, &unquoted, prefix, prefix_length,
+					nul_term_line, set_executable_bit);
 		strbuf_release(&unquoted);
 		strbuf_release(&buf);
 	}

2.35.1.1428.g1c1a0152d61

We do nothing with this yet, but will soon.

---
 builtin/update-index.c | 37 +++++++++++++++++++++----------------
 object-file.c          |  2 +-
 2 files changed, 22 insertions(+), 17 deletions(-)

index 80b96ec5721..1884124224c 100644
@@ -267,10 +267,12 @@ static int process_lstat_error(const char *path, int err)
 	return error("lstat(\"%s\"): %s", path, strerror(err));
 }
 
-static int add_one_path(const struct cache_entry *old, const char *path, int len, struct stat *st)
+static int add_one_path(const struct cache_entry *old, const char *path,
+			int len, struct stat *st, const unsigned oflags)
 {
 	int option;
 	struct cache_entry *ce;
+	unsigned f;
 
 	/* Was the old index entry already up-to-date? */
 	if (old && !ce_stage(old) && !ce_match_stat(old, st, 0))

@@ -283,8 +285,8 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len
 	fill_stat_cache_info(&the_index, ce, st);
 	ce->ce_mode = ce_mode_from_stat(old, st->st_mode);
 
-	if (index_path(&the_index, &ce->oid, path, st,
-		       info_only ? 0 : HASH_WRITE_OBJECT)) {
+	f = oflags | (info_only ? 0 : HASH_WRITE_OBJECT);
+	if (index_path(&the_index, &ce->oid, path, st, f)) {
 		discard_cache_entry(ce);
 		return -1;
 	}

@@ -320,7 +322,8 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len
  *  - it doesn't exist at all in the index, but it is a valid
  *    git directory, and it should be *added* as a gitlink.
  */
-static int process_directory(const char *path, int len, struct stat *st)
+static int process_directory(const char *path, int len, struct stat *st,
+			     const unsigned oflags)
 {
 	struct object_id oid;
 	int pos = cache_name_pos(path, len);

@@ -334,7 +337,7 @@ static int process_directory(const char *path, int len, struct stat *st)
 			if (resolve_gitlink_ref(path, "HEAD", &oid) < 0)
 				return 0;
 
-			return add_one_path(ce, path, len, st);
+			return add_one_path(ce, path, len, st, oflags);
 		}
 		/* Should this be an unconditional error? */
 		return remove_one_path(path);

@@ -358,13 +361,14 @@ static int process_directory(const char *path, int len, struct stat *st)
 
 	/* No match - should we add it as a gitlink? */
 	if (!resolve_gitlink_ref(path, "HEAD", &oid))
-		return add_one_path(NULL, path, len, st);
+		return add_one_path(NULL, path, len, st, oflags);
 
 	/* Error out. */
 	return error("%s: is a directory - add files inside instead", path);
 }
 
-static int process_path(const char *path, struct stat *st, int stat_errno)
+static int process_path(const char *path, struct stat *st, int stat_errno,
+			const unsigned oflags)
 {
 	int pos, len;
 	const struct cache_entry *ce;

@@ -395,9 +399,9 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		return process_lstat_error(path, stat_errno);
 
 	if (S_ISDIR(st->st_mode))
-		return process_directory(path, len, st);
+		return process_directory(path, len, st, oflags);
 
-	return add_one_path(ce, path, len, st);
+	return add_one_path(ce, path, len, st, oflags);
 }
 
 static int add_cacheinfo(unsigned int mode, const struct object_id *oid,

@@ -446,7 +450,7 @@ static void chmod_path(char flip, const char *path)
 	die("git update-index: cannot chmod %cx '%s'", flip, path);
 }
 
-static void update_one(const char *path)
+static void update_one(const char *path, const unsigned oflags)
 {
 	int stat_errno = 0;
 	struct stat st;

@@ -485,7 +489,7 @@ static void update_one(const char *path)
 		report("remove '%s'", path);
 		return;
 	}
-	if (process_path(path, &st, stat_errno))
+	if (process_path(path, &st, stat_errno, oflags))
 		die("Unable to process path %s", path);
 	report("add '%s'", path);
 }

@@ -776,7 +780,7 @@ static int do_reupdate(int ac, const char **av,
 		 */
 		save_nr = active_nr;
 		path = xstrdup(ce->name);
-		update_one(path);
+		update_one(path, 0);
 		free(path);
 		discard_cache_entry(old);
 		if (save_nr != active_nr)

@@ -973,7 +977,8 @@ static enum parse_opt_result reupdate_callback(
 
 static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
 			    const char *prefix, int prefix_length,
-			    const int nul_term_line, const int set_executable_bit)
+			    const int nul_term_line, const int set_executable_bit,
+			    const unsigned oflags)
 {
 	char *p;
 

@@ -984,7 +989,7 @@ static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
 		strbuf_swap(buf, unquoted);
 	}
 	p = prefix_path(prefix, prefix_length, buf->buf);
-	update_one(p);
+	update_one(p, oflags);
 	if (set_executable_bit)
 		chmod_path(set_executable_bit, p);
 	free(p);

@@ -1157,7 +1162,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
-			update_one(p);
+			update_one(p, 0);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
 			free(p);

@@ -1195,7 +1200,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		setup_work_tree();
 		while (getline_fn(&buf, stdin) != EOF)
 			line_from_stdin(&buf, &unquoted, prefix, prefix_length,
-					nul_term_line, set_executable_bit);
+					nul_term_line, set_executable_bit, 0);
 		strbuf_release(&unquoted);
 		strbuf_release(&buf);
 	}

index dbeb3df502d..8999fce2b15 100644
@@ -2211,7 +2211,7 @@ static int index_mem(struct index_state *istate,
 	}
 
 	if (write_object)
-		ret = write_object_file(buf, size, type, oid);
+		ret = write_object_file_flags(buf, size, type, oid, flags);
 	else
 		hash_object_file(the_hash_algo, buf, size, type, oid);
 	if (re_allocated)

2.35.1.1428.g1c1a0152d61

This variable renaming makes a subsequent more meaningful change smaller.

---
 builtin/update-index.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 1884124224c..af02ff39756 100644
@@ -1194,15 +1194,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_from_stdin) {
-		struct strbuf buf = STRBUF_INIT;
+		struct strbuf line = STRBUF_INIT;
 		struct strbuf unquoted = STRBUF_INIT;
 
 		setup_work_tree();
-		while (getline_fn(&buf, stdin) != EOF)
-			line_from_stdin(&buf, &unquoted, prefix, prefix_length,
+		while (getline_fn(&line, stdin) != EOF)
+			line_from_stdin(&line, &unquoted, prefix, prefix_length,
 					nul_term_line, set_executable_bit, 0);
 		strbuf_release(&unquoted);
-		strbuf_release(&buf);
+		strbuf_release(&line);
 	}
 
 	if (split_index > 0) {

2.35.1.1428.g1c1a0152d61

As with unpack-objects in a preceding commit have update-index.c make use of the HASH_N_OBJECTS{,_{FIRST,LAST}} flags. We now have a "batch" mode again for "update-index".

Adding the t/* directory from git.git on a Linux ramdisk is a bit faster than with the tmp-objdir indirection:

	git hyperfine -L rev ns/batched-fsync,HEAD -s 'make CFLAGS=-O3' -p 'rm -rf repo && git init repo && cp -R t repo/' 'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' --warmup 1 -r 10
	Benchmark 1: git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync
	  Time (mean ± σ):     289.8 ms ±   4.0 ms    [User: 186.3 ms, System: 103.2 ms]
	  Range (min … max):   285.6 ms … 297.0 ms    10 runs

	Benchmark 2: git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD
	  Time (mean ± σ):     273.9 ms ±   7.3 ms    [User: 189.3 ms, System: 84.1 ms]
	  Range (min … max):   267.8 ms … 291.3 ms    10 runs

	Summary
	  'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD' ran
	    1.06 ± 0.03 times faster than 'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync'

And as before running that with "strace --summary-only" slows things down a bit (probably mimicking slower I/O a bit). I then get:

	Summary
	  'git ls-files -- t | strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD' ran
	    1.21 ± 0.02 times faster than 'git ls-files -- t | strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync'

We also go from ~51k syscalls to ~39k, with ~2x the number of link() and unlink() in ns/batched-fsync.

In the process of doing this conversion we lost the "bulk" mode for files added on the command-line. I don't think it's useful to optimize that, but we could if anyone cared.

We've also converted this to a string_list, we could walk with getline_fn() and get one line "ahead" to see what we have left, but I found that state machine a bit painful, and at least in my testing buffering this doesn't harm things. But we could also change this to stream again, at the cost of some getline_fn() twiddling.

---
 builtin/update-index.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

index af02ff39756..c7cbfe1123b 100644
@@ -1194,15 +1194,38 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_from_stdin) {
+		struct string_list list = STRING_LIST_INIT_NODUP;
 		struct strbuf line = STRBUF_INIT;
 		struct strbuf unquoted = STRBUF_INIT;
+		size_t i, nr;
+		unsigned oflags;
 
 		setup_work_tree();
-		while (getline_fn(&line, stdin) != EOF)
-			line_from_stdin(&line, &unquoted, prefix, prefix_length,
-					nul_term_line, set_executable_bit, 0);
+		while (getline_fn(&line, stdin) != EOF) {
+			size_t len = line.len;
+			char *str = strbuf_detach(&line, NULL);
+
+			string_list_append_nodup(&list, str)->util = (void *)len;
+		}
+
+		nr = list.nr;
+		oflags = nr > 1 ? HASH_N_OBJECTS : 0;
+		for (i = 0; i < nr; i++) {
+			size_t nth = i + 1;
+			unsigned f = i == 0 ? HASH_N_OBJECTS_FIRST :
+				  nr == nth ? HASH_N_OBJECTS_LAST : 0;
+			struct strbuf buf = STRBUF_INIT;
+			struct string_list_item *item = list.items + i;
+			const size_t len = (size_t)item->util;
+
+			strbuf_attach(&buf, item->string, len, len);
+			line_from_stdin(&buf, &unquoted, prefix, prefix_length,
+					nul_term_line, set_executable_bit,
+					oflags | f);
+			strbuf_release(&buf);
+		}
 		strbuf_release(&unquoted);
-		strbuf_release(&line);
+		string_list_clear(&list, 0);
 	}
 
 	if (split_index > 0) {

2.35.1.1428.g1c1a0152d61

This buffering introduces the same potential risk of the "stdin-feeder" process not being able to see objects right away as my version had. I'm planning to mitigate the issue by unplugging the bulk checkin when issuing a verbose report so that anyone who's using that output to synchronize can still see what they're expecting.

I think the code you've presented here is a lot of diff to accomplish the same thing that my series does, where this specific update-index caller has been roto-tilled to provide the needed begin/end-transaction points. And I think there will be a lot of complexity in supporting the same hints for command-line additions (which is roughly equivalent to the git-add workflow). Every caller that wants batch treatment will have to either implement a state machine or implement a buffering mechanism in order to figure out the begin-end points. Having a separate plug/unplug call eliminates this complexity on each caller.

Btw, I'm planning in a future series to reduce the system calls involved in renaming a file by taking advantage of the renameat2 system call and equivalents on other platforms. There's a pretty strong motivation to do that on Windows.

Thanks for the concrete code,
-Neeraj

I was rather terse in the commit message, I meant (but forgot some words) "doesn't harm thing for performance [in the above test]", but converting this to a string_list is clearly & regression that shouldn't be kept.

I just wanted to demonstrate method of doing this by passing down the HASH_* flags, and found that writing the state-machine to "buffer ahead" by one line so that we can eventually know in the loop if we're in the "last" line or not was tedious, so I came up with this POC. But we clearly shouldn't lose the "streaming" aspect.

But anyway, now that I look at this again the smart thing here (surely?) is to keep the simple getline() loop and not ever issue a HASH_N_OBJECTS_LAST for the Nth item, instead we should in this case do the "checkpoint fsync" at the point that we write the actual index.

Because an existing redundancy in your series is that you'll do the fsync() the same way for "git unpack-objects" as for "git {update-index,add}".

I.e. in the former case adding the N objects is all we're doing, so the "last object" is the point at which we need to flush the previous N to disk.

But for "update-index/add" you'll do at least 2 fsync()'s in the bulk mode, when it should be one. I.e. the equivalent of (leaving aside the tmp-objdir migration part of it), if writing objects A && B:

    ## METHOD ONE
    # A
    write(objects/A.tmp)
    bulk_fsync(objects/A.tmp)
    rename(objects/A.tmp, objects/A)
    # B
    write(objects/B.tmp)
    bulk_fsync(objects/B.tmp)
    rename(objects/B.tmp, objects/B)
    # "cookie"
    write(bulk_fsync_XXXXXX)
    fsync(bulk_fsync_XXXXXX)
    # ref
    write(INDEX.tmp, $(git rev-parse B))
    fsync(INDEX.tmp)
    rename(INDEX.tmp, INDEX)

This series on top changes that so we know that we're doing N, so we don't need the seperate "cookie", we can just use the B object as the cookie, as we know it comes last;

## METHOD TWO
# A -- SAME as above write(objects/A.tmp) bulk_fsync(objects/A.tmp) rename(objects/A.tmp, objects/A)
# B -- SAME as above, with s/bulk_fsync/fsync/ write(objects/B.tmp) fsync(objects/B.tmp) rename(objects/B.tmp, objects/B)
# "cookie" -- GONE!
# ref -- SAME
write(INDEX.tmp, $(git rev-parse B)) fsync(INDEX.tmp)
rename(INDEX.tmp, INDEX)

But really, we should instead realize that we're not doing "unpack-objects", but have a "ref update" at the end (whether that's a ref, or an index etc.) and do:

## METHOD THREE
# A -- SAME as above
write(objects/A.tmp)
bulk_fsync(objects/A.tmp)
rename(objects/A.tmp, objects/A)
# B -- SAME as the first
write(objects/B.tmp)
bulk_fsync(objects/B.tmp)
rename(objects/B.tmp, objects/B)
# ref -- SAME
write(INDEX.tmp, $(git rev-parse B))
fsync(INDEX.tmp)
rename(INDEX.tmp, INDEX)

Which cuts our number of fsync() operations down from 2 to 1, ina addition to removing the need for the "cookie", which is only there because we didn't keep track of where we were in the sequence as in my 2/7 and 5/7.

And it would be the same for tmp-objdir, the rename dance is a bit different, but we'd do the "full" fsync() while on the INDEX.tmp, then migrate() the tmp-objdir, and once that's done do the final:

rename(INDEX.tmp, INDEX)

I.e. we'd fsync() the content once, and only have the renme() or link() operations left. For POSIX we'd need a few more fsync() for the metadata, but this (i.e. your) series already makes the hard assumption that we don't need to do that for rename().

Any caller of these APIs will need the "unsigned oflags" sooner than later anyway, as they need to pass down e.g. HASH_WRITE_OBJECT. We just do it slightly earlier.

And because of that in the general case it's really not the same, I think it's a better approach. You've already got the bug in yours of needlessly setting up the bulk checkin for !HASH_WRITE_OBJECT in update-index, which this neatly solves by deferring the "bulk" mechanism until the codepath that's past that and into the "real" object writing.

We can also die() or error out in the object writing before ever getting to writing the object, in which case we'd do some setup that we'd need to tear down again, by deferring it until the last moment...

I left that out due to Junio's comment in
https://lore.kernel.org/git/[email protected]/; i.e. I don't see why we'd find it worthwhile to optimize that case, but we easily could (especially per the "just sync the INDEX.tmp" above).

But even if we don't do "THREE" above I think it's still easy, for "TWO" we already have as parse_options() state machine to parse argv as it comes in. Doing the fsync() on the last object is just a matter of "looking ahead" there).

This is subjective, but I really think that's rather easy to do, and much easier to reason about than the global state on the side via singletons that your method of avoiding modifying these callers and instead having them all consult global state via bulk-checkin.c and cache.h demands.

That API also currently assumes single-threaded writers, if we start writing some of this in parallel in e.g. "unpack-objects" we'd need mutexes in bulk-object.[ch]. Isn't that a lot easier when the caller would instead know something about the special nature of the transaction they're interacting with, and that the 1st and last item are important (for a "BEGIN" and "FLUSH").

What do you have in mind for renameat2() specifically? I.e. which of the 3x flags it implements will benefit us? RENAME_NOREPLACE to "move" the tmp_OBJ to an eventual OBJ?

Generally: There's some low-hanging fruit there. E.g. for tmp-objdir we slavishly go through the motion of writing an tmp_OBJ, writing (and possibly syncing it), then renaming that tmp_OBJ to OBJ.

We could clearly just avoid that in some/all cases that use tmp-objdir. I.e. we're writing to a temporary store anyway, so why the tmp_OBJ files? We could just write to the final destinations instead, they're not reachable (by ref or OID lookup) from anyone else yet.

But even then I don't see how you'd get away with reducing some classes of syscalls past the 2x increase for some (leading an overall increase, but not a ~2x overall increase as noted in:
https://lore.kernel.org/git/[email protected]/)
as long as you use the tmp-objdir API. It's always going to have to write tmpdir/OBJ and link()/rename() that to OBJ.

Now, I do think there's an easy way by extending the API use I've introduced in this RFC to do it. I.e. we'd just do:

## METHOD FOUR
# A -- SAME as THREE, except no rename()
write(objects/A.tmp)
bulk_fsync(objects/A.tmp)
# B -- SAME as THREE, except no rename()
write(objects/B.tmp)
bulk_fsync(objects/B.tmp)
# ref -- SAME
write(INDEX.tmp, $(git rev-parse B))
fsync(INDEX.tmp)
# NEW: do all the renames at the end:
rename(objects/A.tmp, objects/A)
rename(objects/B.tmp, objects/B)
rename(INDEX.tmp, INDEX)

That seems like an obvious win to me in any case. I.e. the tmp-objdir API isn't really a close fit for what we *really* want to do in this case.

I.e. the reason it does everything this way is because it was explicitly designed for 722ff7f876c (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), where it's the right trade-off, because we'd like to cheaply "rm -rf" the whole thing if e.g. the "pre-receive" hook rejects the push.

*AND* because it's made for the case of other things concurrently needing access to those objects. So pedantically you would need it for some modes of "git update-index", but not e.g. "git unpack-objects" where we really are expecting to keep all of them.

..but no thanks? I.e. it would be useful to explicitly know if you're interested or open to running with some of the approach in this RFC.

I'm going to respond in more detail to your individual patches, (expect the last mail to contain a comment at the end "LAST MAIL").

From my experience working on several state machines in the Windows OS, they are notoriously difficult to understand and extend. I wouldn't want every top-level command that does something interesting to have to deal with that.

I agree that this is a great direction to go in as an extension to this work (i.e. a subsequent patch). I saw in one of your mails on v2 of your rfc series that you mentioned a "lightweight transaction-y thing". I've been thinking along the same lines myself, but wanted to treat that as a separable concern. In my ideal world, we'd just use a real database for loose objects, the index, and refs and let that handle the transaction management. But in lieu of that, having a transaction that looks across the ODB, index, and refs would let us batch syncs optimally.

I'll be submitting a new version to the list which sets up the tmp objdir lazily on first actual write, so the concern about writing to the ODB needlessly should go away.

The nice thing about having the ODB handle the batch stuff internally is that it can present a nice minimal interface to all of the callers. Yes, it has a complex implementation internally, but that complexity backs a rather simple API surface:
1. Begin/end transaction (plug/unplug checkin).
2. Find-object by SHA
3. Add object if it doesn't exist
4. Get the SHA without adding anything.

The ODB work is implemented once and callers can easily adopt the transaction API without having to implement their own stuff on the side. Future series can make the transaction span nicely across the ODB, index, and refs.

The API as sketched above doesn't deeply assume single-threadedness for the "find object by SHA" or "add object if it doesn't exist". There is a single-threaded assumption for begin/end-transaction. The implementation can use pthread_once to handle anything that needs to be done lazily when adding objects.

Yes RENAME_NOREPLACE. I'd want to introduce a helper called git_rename_noreplace and use it instead of the link dance.

We were thinking before that there could be some concurrency in the tmp_objdir, though I personally don't believe it's possible for the typical bulk checkin case. Using the final name in the tmp objdir would be a nice optimization, but I think that it's a separable concern that shouldn't block the bigger win from eliminating the cache flushes.

I think this is the right place to get to eventually. I believe the best way to get there is to keep the plug/unplug bulk checkin functionality (rebranding it as an 'ODB transaction') and then make that a sub-transaction of a larger 'git repo transaction.'

I'm still at the point of arguing with you about your RFC, but I'm _not_ currently leaning toward adopting your approach. I think from a separation-of-concerns perspective, we shouldn't change top-level git commands to try hard to track first/last object. The ODB should conceptually handle it internally as part of a higher-level transaction. Consider cmd_add, which does its interesting add_file_to_index from the update_callback coming from the diff code:
I believe it would be hopelessly complex/impossible to do the tracking required to pass the LAST_OF_N flag to a multiplexed write API.

We have a pretty clear example from the database world that begin/end-transaction is the right way to design the API for the task we want to accomplish. It's also how many filesystems work internally. I don't want to reinvent the bicycle here.

Thanks,
Neeraj

Quite a bit less WIP-y but still RFC version of patcehs to integrate/squash into some form in Neeraj's fsync() series at
https://lore.kernel.org/git/[email protected]/

As noted in the v1 this starts (in 2/7) by removing the tmp-objdir part of the "bulk checkin" as a POC. Clearly Neeraj wants to keep it, so we should have it eventually. But this series argues that in both patch organization and configurability (see the new 7/7!) that the "do quarantine" should be split up and optional to "do bulk fsync".

The new documentation in 7/7 currently documents a vaporware setting, but it's what we'd get if this were rebased early into Neeraj's series, and we made the tmp-objdir part contingent on a configuration setting.

Unlike v1 (I overzealously ripped out some unrelated bulk-checkin.c code then) this doesn't fail any tests.

But most importantly the whole fsync() schema here is *much better* in terms of semantics. We still do away with the "cookie" placeholder to force an fsync, but now as can be seen in the 4/7 and 5/7 we'll "fsync()" by using the updated index at the end as our cookie.

I.e. there's no need to introduce a "bulk_fsync" cookie file to force an fsync() if we can instead alter the relevant calling code to be aware of the new "fsync() transaction". It can then do the "flush" by doing the fsync() on the file it wanted to update anyway (now an index, in the future a ref). So this implements the "METHOD THREE" noted in [1].

1. https://lore.kernel.org/git/[email protected]/

Ævar Arnfjörð Bjarmason (7):
unpack-objects: add skeleton HASH_N_OBJECTS{,_{FIRST,LAST}} flags
object-file: pass down unpack-objects.c flags for "bulk" checkin
update-index: pass down skeleton "oflags" argument
update-index: have the index fsync() flush the loose objects
add: use WLI_NEED_LOOSE_FSYNC for new "only the index" bulk fsync() fsync docs: update for new syncing semantics
fsync docs: add new fsyncMethod.batch.quarantine, elaborate on old

 Documentation/config/core.txt | 101 +++++++++++++++++++++++++++++-----
 builtin/add.c                 |   6 +-
 builtin/unpack-objects.c      |  62 +++++++++++----------
 builtin/update-index.c        |  39 ++++++-------
 bulk-checkin.c                |  74 -------------------------
 bulk-checkin.h                |   3 -
 cache.h                       |  10 ++--
 object-file.c                 |  39 +++++++++----
 read-cache.c                  |  37 ++++++++++++-
 9 files changed, 214 insertions(+), 157 deletions(-)

Range-diff against v1:

1:  e03c119c784 < -:  ----------- write-or-die.c: remove unused fsync_component() function
2:  00dbffc2331 = 1:  98921aa2052 unpack-objects: add skeleton HASH_N_OBJECTS{,_{FIRST,LAST}} flags
3:  beda9f99529 ! 2:  c6f776fc2bc object-file: pass down unpack-objects.c flags for "bulk" checkin

@@ Commit message
the previous case of fsync_component_or_die(...)" could just be added to the existing "fsync_object_files > 0" branch.

- Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
+ Note: This commit reverts much of "core.fsyncmethod: batched disk
+ flushes for loose-objects". We'll set up new structures to bring what
+ it was doing back in a different way. I.e. to do the tmp-objdir
+ plug-in in object-file.c

- ## builtin/add.c ##
-@@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix)
- string_list_clear(&only_match_skip_worktree, 0);
- }
-
-- plug_bulk_checkin();
--
- if (add_renormalize)
- exit_status |= renormalize_tracked_files(&pathspec, flags);
- else
-@@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix) -
- if (chmod_arg && pathspec.nr)
- exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
-- unplug_bulk_checkin();
-
- finish:
- if (write_locked_index(&the_index, &lock_file,
+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>

## builtin/unpack-objects.c ##
@@ builtin/unpack-objects.c: static void unpack_all(void)
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const

## bulk-checkin.c ##
@@
+ */
+ #include "cache.h"
+ #include "bulk-checkin.h"
+-#include "lockfile.h"
+ #include "repository.h"
+ #include "csum-file.h"
#include "pack.h"
#include "strbuf.h"
- #include "string-list.h"
+-#include "string-list.h"
-#include "tmp-objdir.h"
#include "packfile.h"
#include "object-store.h"
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
int fd, size_t size, enum object_type type, const char *path, unsigned flags)
@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
- finish_bulk_checkin(&bulk_checkin_state);
- return status;
- }
--
--void plug_bulk_checkin(void)
--{
-- assert(!bulk_checkin_plugged);
+ void plug_bulk_checkin(void)
+ {
+ assert(!bulk_checkin_plugged);
-
- /*
- * A temporary object directory is used to hold the files
@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
- tmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);
- }
-
-- bulk_checkin_plugged = 1;
--}
--
--void unplug_bulk_checkin(void)
--{
-- assert(bulk_checkin_plugged);
-- bulk_checkin_plugged = 0;
-- if (bulk_checkin_state.f)
-- finish_bulk_checkin(&bulk_checkin_state);
+ bulk_checkin_plugged = 1;
+ }
+
+@@ bulk-checkin.c: void unplug_bulk_checkin(void)
+ bulk_checkin_plugged = 0;
+ if (bulk_checkin_state.f)
+ finish_bulk_checkin(&bulk_checkin_state);
-
- do_batch_fsync();
--}
+ }

## bulk-checkin.h ##
@@
@@ bulk-checkin.h
int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type, const char *path, unsigned flags);
-
--void plug_bulk_checkin(void);
--void unplug_bulk_checkin(void);
--
- #endif

## cache.h ##
-@@ cache.h: void write_or_die(int fd, const void *buf, size_t count);
- void fsync_or_die(int fd, const char *);
+@@ cache.h: void fsync_or_die(int fd, const char *);
+ int fsync_component(enum fsync_component component, int fd);
void fsync_component_or_die(enum fsync_component component, int fd, const char *msg);

-static inline int batch_fsync_enabled(enum fsync_component component)
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *

if (mtime) {
struct utimbuf utb;
-
- ## t/t1050-large.sh ##
-@@ t/t1050-large.sh: test_description='adding and checking out large blobs' -
- . ./test-lib.sh
-
-+skip_all='TODO: migrate the builtin/add.c code'
-+test_done
-+
- test_expect_success setup '
- # clone does not allow us to pass core.bigfilethreshold to
- # new repos, so set core.bigfilethreshold globally
5: a1474968991 ! 3: 4df8012100a update-index: pass down an "oflags" argument
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- update-index: pass down an "oflags" argument
+ update-index: pass down skeleton "oflags" argument

- We do nothing with this yet, but will soon.
+ As with a preceding change to "unpack-objects" add an "oflags" going
+ from cmd_update_index() all the way down to the code in
+ object-file.c. Note also how index_mem() will now call
+ write_object_file_flags().

@@ builtin/update-index.c: static int do_reupdate(int ac, const char **av,
free(path);
discard_cache_entry(old);
if (save_nr != active_nr)
-@@ builtin/update-index.c: static enum parse_opt_result reupdate_callback(
-
- static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
- const char *prefix, int prefix_length,
-- const int nul_term_line, const int set_executable_bit)
-+ const int nul_term_line, const int set_executable_bit,
-+ const unsigned oflags)
- {
- char *p;
-
-@@ builtin/update-index.c: static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
- strbuf_swap(buf, unquoted);
- }
- p = prefix_path(prefix, prefix_length, buf->buf);
-- update_one(p);
-+ update_one(p, oflags);
- if (set_executable_bit)
- chmod_path(set_executable_bit, p);
- free(p);
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)

setup_work_tree();
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const
chmod_path(set_executable_bit, p);
free(p);
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- setup_work_tree();
- while (getline_fn(&buf, stdin) != EOF)
- line_from_stdin(&buf, &unquoted, prefix, prefix_length,
-- nul_term_line, set_executable_bit);
-+ nul_term_line, set_executable_bit, 0);
- strbuf_release(&unquoted);
- strbuf_release(&buf);
- }
+ strbuf_swap(&buf, &unquoted);
+ }
+ p = prefix_path(prefix, prefix_length, buf.buf);
+- update_one(p);
++ update_one(p, 0);
+ if (set_executable_bit)
+ chmod_path(set_executable_bit, p);
+ free(p);

## object-file.c ##
@@ object-file.c: static int index_mem(struct index_state *istate,
7: 481f1d771cb ! 4: 61f4f3d7ef4 update-index: make use of HASH_N_OBJECTS{,_{FIRST,LAST}} flags
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- update-index: make use of HASH_N_OBJECTS{,_{FIRST,LAST}} flags
+ update-index: have the index fsync() flush the loose objects

As with unpack-objects in a preceding commit have update-index.c make use of the HASH_N_OBJECTS{,_{FIRST,LAST}} flags. We now have a "batch"
@@ Commit message
Adding the t/* directory from git.git on a Linux ramdisk is a bit faster than with the tmp-objdir indirection:

- git hyperfine -L rev ns/batched-fsync,HEAD -s 'make CFLAGS=-O3' -p 'rm -rf repo && git init repo && cp -R t repo/' 'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' --warmup 1 -r 10
- Benchmark 1: git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync
- Time (mean ± σ): 289.8 ms ± 4.0 ms [User: 186.3 ms, System: 103.2 ms]
- Range (min … max): 285.6 ms … 297.0 ms 10 runs
+ $ git hyperfine -L rev ns/batched-fsync,HEAD -s 'make CFLAGS=-O3 && rm -rf repo && git init repo && cp -R t repo/ && git ls-files -- t >repo/.git/to-add.txt' -p 'rm -rf repo/.git/objects/* repo/.git/index' './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' --warmup 1 -r 10Benchmark 1: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'ns/batched-fsync
+ Time (mean ± σ): 281.1 ms ± 2.6 ms [User: 186.2 ms, System: 92.3 ms]
+ Range (min … max): 278.3 ms … 287.0 ms 10 runs

- Benchmark 2: git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD
- Time (mean ± σ): 273.9 ms ± 7.3 ms [User: 189.3 ms, System: 84.1 ms]
- Range (min … max): 267.8 ms … 291.3 ms 10 runs
+ Benchmark 2: ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'HEAD
+ Time (mean ± σ): 265.9 ms ± 2.6 ms [User: 181.7 ms, System: 82.1 ms]
+ Range (min … max): 262.0 ms … 270.3 ms 10 runs

Summary
- 'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD' ran
- 1.06 ± 0.03 times faster than 'git ls-files -- t | ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync'
+ './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'HEAD' ran
+ 1.06 ± 0.01 times faster than './git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'ns/batched-fsync'

And as before running that with "strace --summary-only" slows things down a bit (probably mimicking slower I/O a bit). I then get:

Summary
- 'git ls-files -- t | strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'HEAD' ran
- 1.21 ± 0.02 times faster than 'git ls-files -- t | strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin' in 'ns/batched-fsync'
+ 'strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'HEAD' ran
+ 1.19 ± 0.03 times faster than 'strace --summary-only ./git -c core.fsync=loose-object -c core.fsyncMethod=batch -C repo update-index --add --stdin <repo/.git/to-add.txt' in 'ns/batched-fsync'
+
+ This one has a twist though, instead of fsync()-ing on the last object
+ we write we'll not do that, and instead defer the fsync() until we
+ write the index itself. This is outlined in [1] (as "METHOD THREE").
+
+ Because of this under FSYNC_METHOD_BATCH we'll do the N
+ objects (possibly only one, because we're lazy) as HASH_N_OBJECTS, and
+ we'll even now support doing this via N arguments on the command-line.
+
+ Then we won't fsync() any of it, but we will rename it
+ in-place (which, if we were still using the tmp-objdir, would leave it
+ "staged" in the tmp-objdir).
+
+ We'll then have the fsync() for the index update "flush" that out, and
+ thus avoid two fsync() calls when one will do.
+
+ Running this with the "git hyperfine" command mentioned in a preceding
+ commit with "strace --summary-only" shows that we do 1 fsync() now
+ instead of 2, and have one more sync_file_range(), as expected.

We also go from ~51k syscalls to ~39k, with ~2x the number of link()
- and unlink() in ns/batched-fsync.
+ and unlink() in ns/batched-fsync, and of course one fsync() instead of
+ two()>

- In the process of doing this conversion we lost the "bulk" mode for
- files added on the command-line. I don't think it's useful to optimize
- that, but we could if anyone cared.
+ The flow of this code isn't quite set up for re-plugging the
+ tmp-objdir back in. In particular we no longer pass
+ HASH_N_OBJECTS_FIRST (but doing so would be trivial)< and there's no
+ HASH_N_OBJECTS_LAST.

- We've also converted this to a string_list, we could walk with
- getline_fn() and get one line "ahead" to see what we have left, but I
- found that state machine a bit painful, and at least in my testing
- buffering this doesn't harm things. But we could also change this to
- stream again, at the cost of some getline_fn() twiddling.
+ So this and other callers would need some light transaction-y API, or
+ to otherwise pass down a "yes, I'd like to flush it" down to
+ finalize_hashfile(), but doing so will be trivial.
+
+ And since we've started structuring it this way it'll become easy to
+ do any arbitrary number of things down the line that would "bulk
+ fsync" before the final fsync(). Now we write some objects and fsync()
+ on the index, but between those two could do any number of other
+ things where we'd defer the fsync().
+
+ This sort of thing might be especially interesting for "git repack"
+ when it writes e.g. a *.bitmap, *.rev, *.pack and *.idx. In that case
+ we could skip the fsync() on all of those, and only do it on the *.idx
+ before we renamed it in-place. I *think* nothing cares about a *.pack
+ without an *.idx, but even then we could fsync *.idx, rename *.pack,
+ rename *.idx and still safely do only one fsync(). See "git show
+ --first-parent" on 62874602032 (Merge branch
+ 'tb/pack-finalize-ordering' into maint, 2021-10-12) for a good
+ overview of the code involved in that.
+
+ 1. https://lore.kernel.org/git/[email protected]/

## builtin/update-index.c ##
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
+
+ setup_work_tree();
+ p = prefix_path(prefix, prefix_length, path);
+- update_one(p, 0);
++ update_one(p, HASH_N_OBJECTS);
+ if (set_executable_bit)
+ chmod_path(set_executable_bit, p);
+ free(p);
+@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
+ strbuf_swap(&buf, &unquoted);
+ }
+ p = prefix_path(prefix, prefix_length, buf.buf);
+- update_one(p, 0);
++ update_one(p, HASH_N_OBJECTS);
+ if (set_executable_bit)
+ chmod_path(set_executable_bit, p);
+ free(p);
+@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
+ exit(128);
+ unable_to_lock_die(get_index_file(), lock_error);
+ }
+- if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
++ if (write_locked_index(&the_index, &lock_file,
++ COMMIT_LOCK | WLI_NEED_LOOSE_FSYNC))
+ die("Unable to write new index file");
}

- if (read_from_stdin) {
-+ struct string_list list = STRING_LIST_INIT_NODUP;
- struct strbuf line = STRBUF_INIT;
- struct strbuf unquoted = STRBUF_INIT;
-+ size_t i, nr;
-+ unsigned oflags;
+
+ ## cache.h ##
+@@ cache.h: void ensure_full_index(struct index_state *istate);
+ /* For use with write_locked_index(). */
+ #define COMMIT_LOCK (1 << 0)
+ #define SKIP_IF_UNCHANGED (1 << 1)
++#define WLI_NEED_LOOSE_FSYNC (1 << 2)

- setup_work_tree();
-- while (getline_fn(&line, stdin) != EOF)
-- line_from_stdin(&line, &unquoted, prefix, prefix_length,
-- nul_term_line, set_executable_bit, 0);
-+ while (getline_fn(&line, stdin) != EOF) {
-+ size_t len = line.len;
-+ char *str = strbuf_detach(&line, NULL);
-+
-+ string_list_append_nodup(&list, str)->util = (void *)len;
-+ }
+ /*
+ * Write the index while holding an already-taken lock. Close the lock,
+
+ ## read-cache.c ##
+@@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
+ int ieot_entries = 1;
+ struct index_entry_offset_table *ieot = NULL;
+ int nr, nr_threads;
++ unsigned int wflags = FSYNC_COMPONENT_INDEX;
+
-+ nr = list.nr;
-+ oflags = nr > 1 ? HASH_N_OBJECTS : 0;
-+ for (i = 0; i < nr; i++) {
-+ size_t nth = i + 1;
-+ unsigned f = i == 0 ? HASH_N_OBJECTS_FIRST :
-+ nr == nth ? HASH_N_OBJECTS_LAST : 0;
-+ struct strbuf buf = STRBUF_INIT;
-+ struct string_list_item *item = list.items + i;
-+ const size_t len = (size_t)item->util;
+
-+ strbuf_attach(&buf, item->string, len, len);
-+ line_from_stdin(&buf, &unquoted, prefix, prefix_length,
-+ nul_term_line, set_executable_bit,
-+ oflags | f);
-+ strbuf_release(&buf);
-+ }
- strbuf_release(&unquoted);
-- strbuf_release(&line);
-+ string_list_clear(&list, 0);
- }
++ /*
++ * TODO: This is abuse of the API recently modified
++ * finalize_hashfile() which reveals a shortcoming of its
++ * "fsync" design.
++ *
++ * I.e. It expects a "enum fsync_component component" label,
++ * but here we're passing it an OR of the two, knowing that
++ * it'll call fsync_component_or_die() which (in
++ * write-or-die.c) will do "(fsync_components & wflags)" (to
++ * our "wflags" here).
++ *
++ * But the API really should be changed to explicitly take
++ * such flags, because in this case we'd like to fsync() the
++ * index if we're in the bulk mode, *even if* our
++ * "core.fsync=index" isn't configured.
++ *
++ * That's because at this point we've been queuing up object
++ * writes that we didn't fsync(), and are going to use this
++ * fsync() to "flush" the whole thing. Doing it this way
++ * avoids redundantly calling fsync() twice when once will do.
++ */
++ if (fsync_method == FSYNC_METHOD_BATCH &&
++ flags & WLI_NEED_LOOSE_FSYNC)
++ wflags |= FSYNC_COMPONENT_LOOSE_OBJECT;
+
+ f = hashfd(tempfile->fd, tempfile->filename.buf);
+
+@@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
+ if (!alternate_index_output && (flags & COMMIT_LOCK))
+ csum_fsync_flag = CSUM_FSYNC;
+
+- finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,
++ finalize_hashfile(f, istate->oid.hash, wflags,
+ CSUM_HASH_IN_STREAM | csum_fsync_flag);

- if (split_index > 0) {
+ if (close_tempfile_gently(tempfile)) {

-:  ----------- > 5:  2bf14fd4946 add: use WLI_NEED_LOOSE_FSYNC for new "only the index" bulk fsync()
6:  4fad333e9a1 ! 6:  c20301d7967 update-index: rename "buf" to "line"

@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- update-index: rename "buf" to "line"
-
- This variable renaming makes a subsequent more meaningful change
- smaller.
+ fsync docs: update for new syncing semantics

- ## builtin/update-index.c ##
-@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- }
-
- if (read_from_stdin) {
-- struct strbuf buf = STRBUF_INIT;
-+ struct strbuf line = STRBUF_INIT;
- struct strbuf unquoted = STRBUF_INIT;
-
- setup_work_tree();
-- while (getline_fn(&buf, stdin) != EOF)
-- line_from_stdin(&buf, &unquoted, prefix, prefix_length,
-+ while (getline_fn(&line, stdin) != EOF)
-+ line_from_stdin(&line, &unquoted, prefix, prefix_length,
- nul_term_line, set_executable_bit, 0);
- strbuf_release(&unquoted);
-- strbuf_release(&buf);
-+ strbuf_release(&line);
- }
+ ## Documentation/config/core.txt ##
+@@ Documentation/config/core.txt: core.fsyncMethod::
+ filesystem and storage hardware, data added to the repository may not be
+ durable in the event of a system crash. This is the default mode on macOS.
+ * batch enables a mode that uses writeout-only flushes to stage multiple
+- updates in the disk writeback cache and then does a single full fsync of
+- a dummy file to trigger the disk cache flush at the end of the operation.
+- Currently batch mode only applies to loose-object files. Other repository +- data is made durable as if fsync was specified. This mode is expected to
+- be as safe as fsync on macOS for repos stored on HFS+ or APFS filesystems +- and on Windows for repos stored on NTFS or ReFS filesystems.
++ updates in the disk writeback cache and, before doing a full fsync() of
++ on the "last" file that to trigger the disk cache flush at the end of the
++ operation.
+++
++Other repository data is made durable as if fsync was
++specified. This mode is expected to be as safe as fsync on macOS for
++repos stored on HFS+ or APFS filesystems and on Windows for repos
++stored on NTFS or ReFS filesystems.
+++
++The batch is currently only applies to loose-object files and will
++kick in when using the linkgit:git-unpack-objects[1] and ++linkgit:update-index[1] commands. Note that the "last" file to be
++synced may be the last object, as in the case of ++linkgit:git-unpack-objects[1], or relevant "index" (or in the future,
++"ref") update, as in the case of linkgit:git-update-index[1]. I.e. the
++batch syncing of the loose objects may be deferred until a subsequent
++fsync() to a file that makes them "active".

- if (split_index > 0) {
+ core.fsyncObjectFiles::
+ This boolean will enable 'fsync()' when writing object files.
4: 2c5395a3716 ! 7: a5951366c6e update-index: use a utility function for stdin consumption
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- update-index: use a utility function for stdin consumption
+ fsync docs: add new fsyncMethod.batch.quarantine, elaborate on old
+
+ Add a new fsyncMethod.batch.quarantine setting which defaults to
+ "false". Preceding (RFC, and not meant to flip-flop like that
+ eventually) commits ripped out the "tmp-objdir" part of the
+ core.fsyncMethod=batch.
+
+ This documentation proposes to keep that as the default for the
+ reasons discussed in it, while allowing users to set
+ "fsyncMethod.batch.quarantine=true".
+
+ Furthermore update the discussion of "core.fsyncObjectFiles" with
+ information about what it *really* does, why you probably shouldn't
+ use it, and how to safely emulate most of what it gave users in the
+ past in terms of performance benefit.

- ## builtin/update-index.c ##
-@@ builtin/update-index.c: static enum parse_opt_result reupdate_callback(
- return 0;
- }
+ ## Documentation/config/core.txt ##
+@@ Documentation/config/core.txt: stored on NTFS or ReFS filesystems.
+ +
+ The batch is currently only applies to loose-object files and will
+ kick in when using the linkgit:git-unpack-objects[1] and +-linkgit:update-index[1] commands. Note that the "last" file to be ++linkgit:git-update-index[1] commands. Note that the "last" file to be
+ synced may be the last object, as in the case of
+ linkgit:git-unpack-objects[1], or relevant "index" (or in the future,
+ "ref") update, as in the case of linkgit:git-update-index[1]. I.e. the
+ batch syncing of the loose objects may be deferred until a subsequent
+ fsync() to a file that makes them "active".

-+static void line_from_stdin(struct strbuf *buf, struct strbuf *unquoted,
-+ const char *prefix, int prefix_length,
-+ const int nul_term_line, const int set_executable_bit)
-+{
-+ char *p;
-+
-+ if (!nul_term_line && buf->buf[0] == '"') {
-+ strbuf_reset(unquoted);
-+ if (unquote_c_style(unquoted, buf->buf, NULL))
-+ die("line is badly quoted");
-+ strbuf_swap(buf, unquoted);
-+ }
-+ p = prefix_path(prefix, prefix_length, buf->buf);
-+ update_one(p);
-+ if (set_executable_bit)
-+ chmod_path(set_executable_bit, p);
-+ free(p);
-+}
++fsyncMethod.batch.quarantine::
++ A boolean which if set to true will cause "batched" writes
++ to objects to be "quarantined" if
++ core.fsyncMethod=batch. This is false by default.
+++
++The primary object of these fsync() settings is to protect against
++repository corruption of things which are reachable, i.e. "reachable",
++via references, the index etc. Not merely objects that were present in
++the object store.
+++
++Historically setting core.fsyncObjectFiles=false assumed that on a ++filesystem with where an fsync() would flush all preceding outstanding
++I/O that we might end up with a corrupt loose object, but that was OK
++as long as no reference referred to it. We'd eventually the corrupt
++object with linkgit:git-gc[1], and linkgit:git-fsck[1] would only
++report it as a minor annoyance
+++
++Setting fsyncMethod.batch.quarantine=true takes the view that
++something like a corrupt *unreferenced* loose object in the object
++store is something we'd like to avoid, at the cost of reduced
++performance when using core.fsyncMethod=batch.
+++
++Currently this uses the same mechanism described in the "QUARANTINE ++ENVIRONMENT" in the linkgit:git-receive-pack[1] documentation, but
++that's subject to change. The performance loss is because we need to
++"stage" the objects in that quarantine environment, fsync() it, and
++once that's done rename() or link() it in-place into the main object
++store, possibly with an fsync() of the index or ref at the end
+++
++With fsyncMethod.batch.quarantine=false we'll "stage" things in the
++main object store, and then do one fsync() at the very end, either on
++the last object we write, or file (index or ref) that'll make it
++"reachable".
+++
++The bad thing about setting this to true is lost performance, as
++well as not being able to access the objects as they're written (which
++e.g. consumers of linkgit:git-update-index[1]'s --verbose mode might
++want to do).
+++
++The good thing is that you should be guaranteed not to get e.g. short
++or otherwise corrupt loose objects if you pull your power cord, in
++practice various git commands deal quite badly with discovering such a
++stray corrupt object (including perhaps assuming it's valid based on
++its existence, or hard dying on an error rather than replacing
++it). Repairing such "unreachable corruption" can require manual
++intervention.
+
- int cmd_update_index(int argc, const char **argv, const char *prefix)
- {
- int newfd, entries, has_errors = 0, nul_term_line = 0;
-@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- struct strbuf unquoted = STRBUF_INIT;
+ core.fsyncObjectFiles::
+- This boolean will enable 'fsync()' when writing object files.
+- This setting is deprecated. Use core.fsync instead.
+-+
+-This setting affects data added to the Git repository in loose-object
+-form. When set to true, Git will issue an fsync or similar system call
+-to flush caches so that loose-objects remain consistent in the face
+-of a unclean system shutdown.
++ This boolean will enable 'fsync()' when writing loose object
++ files.
+++
++This setting is the historical fsync configuration setting. It's now ++*deprecated*, you should use core.fsync instead, perhaps in
++combination with core.fsyncMethod=batch.
+++
++The core.fsyncObjectFiles was initially added based on integrity ++assumptions that early (pre-ext-4) versions of Linux's "ext"
++filesystems provided.
+++
++I.e. that a write of file A without an fsync() followed by a write
++of file B with fsync() would implicitly guarantee that `A' would
++be fsync()'d by calling fsync() on B. This asssumption is *not*
++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 ++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 ++core.fsyncObjectFiles=true.

- setup_work_tree();
-- while (getline_fn(&buf, stdin) != EOF) {
-- char *p;
-- if (!nul_term_line && buf.buf[0] == '"') {
-- strbuf_reset(&unquoted);
-- if (unquote_c_style(&unquoted, buf.buf, NULL))
-- die("line is badly quoted");
-- strbuf_swap(&buf, &unquoted);
-- }
-- p = prefix_path(prefix, prefix_length, buf.buf);
-- update_one(p);
-- if (set_executable_bit)
-- chmod_path(set_executable_bit, p);
-- free(p);
-- }
-+ while (getline_fn(&buf, stdin) != EOF)
-+ line_from_stdin(&buf, &unquoted, prefix, prefix_length,
-+ nul_term_line, set_executable_bit);
- strbuf_release(&unquoted);
- strbuf_release(&buf);
- }
+ core.preloadIndex::
+ Enable parallel index preload for operations like 'git diff'