[PATCH v6 00/12] core.fsyncmethod: add 'batch' mode for faster fsyncing of multiple objects

[nksingh85@gmail.com]

From: Neeraj Singh <neerajsi@microsoft.com>

GGG closed this series erroneously, so I'm trying out git-send-email. Apologies for any mistakes.

This series is also available at https://github.com/neerajsi-msft/git/git.git ns/batched-fsync-v6.

V6 changes:

* Based on master at faa21c1 to pick up ns/fsync-or-die-message-fix. Also resolved a conflict with 8aa0209 in t/perf/p7519-fsmonitor.sh.

* Some independent patches were submitted separately on-list. This series is now dependent on ns/fsync-or-die-message-fix.

* Rename bulk_checkin_state to bulk_checkin_packfile to discourage future authors from adding any non-packfile related stuff to it. Each individual component of bulk_checkin should have its own state variable(s) going forward, and they should only be tied together by odb_transaction_nesting.

* Rename finish_bulk_checkin and do_batch_fsync to flush_bulk_checkin and flush_batch_fsync. The "finish" step is going to be the end_odb_transaction. The "flush" terminology should be consistently used for making changes visible.

* Add flush_odb_transaction and use it in update-index before printing verbose output to mitigate risk of missing objects for a tricky stdin feeder.

* Re-add shell "local with assignment": now these are all on a separate line with quotes around any values, to comply with dash. I'm running on ubuntu 20.04 LTS where I saw some of the dash issues before.

Neeraj Singh (12):
  bulk-checkin: rename 'state' variable and separate 'plugged' boolean
  bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'
  core.fsyncmethod: batched disk flushes for loose-objects
  cache-tree: use ODB transaction around writing a tree
  builtin/add: add ODB transaction around add_files_to_cache
  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
  test-lib-functions: add parsing helpers for ls-files and ls-tree
  core.fsyncmethod: tests for batch mode
  t/perf: add iteration setup mechanism to perf-lib
  core.fsyncmethod: performance tests for batch mode

 Documentation/config/core.txt          |   8 ++
 builtin/add.c                          |  13 ++-
 builtin/unpack-objects.c               |   3 +
 builtin/update-index.c                 |  20 +++++
 bulk-checkin.c                         | 117 +++++++++++++++++++++----
 bulk-checkin.h                         |  27 +++++-
 cache-tree.c                           |   3 +
 cache.h                                |  12 ++-
 compat/mingw.h                         |   3 +
 config.c                               |   4 +-
 git-compat-util.h                      |   2 +
 object-file.c                          |   7 +-
 t/lib-unique-files.sh                  |  34 +++++++
 t/perf/p0008-odb-fsync.sh              |  82 +++++++++++++++++
 t/perf/p4220-log-grep-engines.sh       |   3 +-
 t/perf/p4221-log-grep-engines-fixed.sh |   3 +-
 t/perf/p5302-pack-index.sh             |  15 ++--
 t/perf/p7519-fsmonitor.sh              |  18 +---
 t/perf/p7820-grep-engines.sh           |   6 +-
 t/perf/perf-lib.sh                     |  63 +++++++++++--
 t/t3700-add.sh                         |  28 ++++++
 t/t3903-stash.sh                       |  20 +++++
 t/t5300-pack-object.sh                 |  41 ++++++---
 t/t5317-pack-objects-filter-objects.sh |  91 ++++++++++---------
 t/test-lib-functions.sh                |  10 +++
 25 files changed, 513 insertions(+), 120 deletions(-)
 create mode 100644 t/lib-unique-files.sh
 create mode 100755 t/perf/p0008-odb-fsync.sh

Range-diff against v5:
 1:  c7a2a7efe6d <  -:  ----------- bulk-checkin: rename 'state' variable and separate 'plugged' boolean
 -:  ----------- >  1:  adabdaa0290 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
 2:  d045b13795b !  2:  72a6cd36c9c bulk-checkin: rebrand plug/unplug APIs as 'odb transactions'
    @@ Commit message
         writing code can optimize their operations without caring whether the
         top-level code has a transaction active.
     
    +    Add a flush_odb_transaction API that will be used in update-index to
    +    make objects visible even if a transaction is active. The flush call may
    +    also be useful in future cases if we hold a transaction active around
    +    calling hooks.
    +
         Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
     
      ## builtin/add.c ##
    @@ bulk-checkin.c
     -static int bulk_checkin_plugged;
     +static int odb_transaction_nesting;
      
    - static struct bulk_checkin_state {
    + static struct bulk_checkin_packfile {
      	char *pack_tmp_name;
     @@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
      {
    - 	int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
    + 	int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
      				     path, flags);
     -	if (!bulk_checkin_plugged)
     +	if (!odb_transaction_nesting)
    - 		finish_bulk_checkin(&bulk_checkin_state);
    + 		flush_bulk_checkin_packfile(&bulk_checkin_packfile);
      	return status;
      }
      
    @@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
      }
      
     -void unplug_bulk_checkin(void)
    -+void end_odb_transaction(void)
    ++void flush_odb_transaction(void)
      {
     -	assert(bulk_checkin_plugged);
     -	bulk_checkin_plugged = 0;
    + 	flush_bulk_checkin_packfile(&bulk_checkin_packfile);
    + }
    ++
    ++void end_odb_transaction(void)
    ++{
     +	odb_transaction_nesting -= 1;
     +	if (odb_transaction_nesting < 0)
     +		BUG("Unbalanced ODB transaction nesting");
    @@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
     +	if (odb_transaction_nesting)
     +		return;
     +
    - 	if (bulk_checkin_state.f)
    - 		finish_bulk_checkin(&bulk_checkin_state);
    - }
    ++	flush_odb_transaction();
    ++}
     
      ## bulk-checkin.h ##
     @@ bulk-checkin.h: int index_bulk_checkin(struct object_id *oid,
    @@ bulk-checkin.h: int index_bulk_checkin(struct object_id *oid,
     +/*
     + * Tell the object database to optimize for adding
     + * multiple objects. end_odb_transaction must be called
    -+ * to make new objects visible.
    ++ * to make new objects visible. Transactions can be nested,
    ++ * and objects are only visible after the outermost transaction
    ++ * is complete or the transaction is flushed.
     + */
     +void begin_odb_transaction(void);
     +
     +/*
    ++ * Make any objects that are currently part of a pending object
    ++ * database transaction visible. It is valid to call this function
    ++ * even if no transaction is active.
    ++ */
    ++void flush_odb_transaction(void);
    ++
    ++/*
     + * Tell the object database to make any objects from the
    -+ * current transaction visible.
    ++ * current transaction visible if this is the final nested
    ++ * transaction.
     + */
     +void end_odb_transaction(void);
      
 3:  2d1bc4568ac <  -:  ----------- object-file: pass filename to fsync_or_die
 4:  9e7ae22fa4a !  3:  57539f104ef core.fsyncmethod: batched disk flushes for loose-objects
    @@ Documentation/config/core.txt: core.fsyncMethod::
     +  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.
    ++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.
    @@ bulk-checkin.c
      
     +static struct tmp_objdir *bulk_fsync_objdir;
     +
    - static struct bulk_checkin_state {
    + static struct bulk_checkin_packfile {
      	char *pack_tmp_name;
      	struct hashfile *f;
    -@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state)
    +@@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
      	reprepare_packed_git(the_repository);
      }
      
     +/*
     + * Cleanup after batch-mode fsync_object_files.
     + */
    -+static void do_batch_fsync(void)
    ++static void flush_batch_fsync(void)
     +{
     +	struct strbuf temp_path = STRBUF_INIT;
     +	struct tempfile *temp;
    @@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state
     +	bulk_fsync_objdir = NULL;
     +}
     +
    - static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
    + static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
      {
      	int i;
    -@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
    +@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_packfile *state,
      	return 0;
      }
      
    @@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
     +	 * If we have an active ODB transaction, we issue a call that
     +	 * cleans the filesystem page cache but avoids a hardware flush
     +	 * command. Later on we will issue a single hardware flush
    -+	 * before as part of do_batch_fsync.
    ++	 * before renaming the objects to their final names as part of
    ++	 * flush_batch_fsync.
     +	 */
     +	if (!bulk_fsync_objdir ||
     +	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
    @@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
      int index_bulk_checkin(struct object_id *oid,
      		       int fd, size_t size, enum object_type type,
      		       const char *path, unsigned flags)
    -@@ bulk-checkin.c: void end_odb_transaction(void)
    +@@ bulk-checkin.c: void begin_odb_transaction(void)
      
    - 	if (bulk_checkin_state.f)
    - 		finish_bulk_checkin(&bulk_checkin_state);
    -+
    -+	do_batch_fsync();
    + void flush_odb_transaction(void)
    + {
    ++	flush_batch_fsync();
    + 	flush_bulk_checkin_packfile(&bulk_checkin_packfile);
      }
    + 
     
      ## bulk-checkin.h ##
     @@
 5:  83fa4a5f3a5 =  4:  f47631e6a28 cache-tree: use ODB transaction around writing a tree
 6:  d514842ad49 =  5:  08c9b234942 builtin/add: add ODB transaction around add_files_to_cache
 7:  8cac94598a5 !  6:  bc37cdbd226 update-index: use the bulk-checkin infrastructure
    @@ Commit message
         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, so callers
         using the --stdin option will not see them until update-index is done.
    -    This risk is mitigated by not keeping an ODB transaction open around
    -    --stdin processing if in --verbose mode. Without --verbose mode,
    -    a caller feeding update-index via --stdin wouldn't know when
    -    update-index adds an object, event without an ODB transaction.
    +    This risk is mitigated by flushing the ODB transaction prior to
    +    reporting any verbose output so that objects will be visible to callers
    +    that are synchronizing with update-index by snooping its output.
     
         Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
     
    @@ builtin/update-index.c
      #include "config.h"
      #include "lockfile.h"
      #include "quote.h"
    +@@ builtin/update-index.c: static void report(const char *fmt, ...)
    + 	if (!verbose)
    + 		return;
    + 
    ++	/*
    ++	 * It is possible, though unlikely, that a caller could use the verbose
    ++	 * output to synchronize with addition of objects to the object
    ++	 * database. The current implementation of ODB transactions leaves
    ++	 * objects invisible while a transaction is active, so flush the
    ++	 * transaction here before reporting a change made by update-index.
    ++	 */
    ++	flush_odb_transaction();
    + 	va_start(vp, fmt);
    + 	vprintf(fmt, vp);
    + 	putchar('\n');
     @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
      	 */
      	parse_options_start(&ctx, argc, argv, prefix,
    @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const
      	while (ctx.argc) {
      		if (parseopt_state != PARSE_OPT_DONE)
      			parseopt_state = parse_options_step(&ctx, options,
    -@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
    - 		the_index.version = preferred_index_format;
    - 	}
    - 
    -+	/*
    -+	 * It is possible, though unlikely, that a caller could use the verbose
    -+	 * output to synchronize with addition of objects to the object
    -+	 * database. The current implementation of ODB transactions leaves
    -+	 * objects invisible while a transaction is active, so end the
    -+	 * transaction here if verbose output is enabled.
    -+	 */
    -+
    -+	if (verbose)
    -+		end_odb_transaction();
    -+
    - 	if (read_from_stdin) {
    - 		struct strbuf buf = STRBUF_INIT;
    - 		struct strbuf unquoted = STRBUF_INIT;
     @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
      		strbuf_release(&buf);
      	}
    @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const
     +	/*
     +	 * By now we have added all of the new objects
     +	 */
    -+	if (!verbose)
    -+		end_odb_transaction();
    ++	end_odb_transaction();
     +
      	if (split_index > 0) {
      		if (git_config_get_split_index() == 0)
 8:  523e5fbd63e =  7:  9cf584cdb67 unpack-objects: use the bulk-checkin infrastructure
 9:  faacc19aab2 =  8:  5039b596064 core.fsync: use batch mode and sync loose objects by default on Windows
10:  4de7300a7b0 =  9:  67205b3ac25 test-lib-functions: add parsing helpers for ls-files and ls-tree
11:  1a4aff8c350 ! 10:  148e562ddb8 core.fsyncmethod: tests for batch mode
    @@ t/lib-unique-files.sh (new)
     +	local dirs="$1" &&
     +	local files="$2" &&
     +	local basedir="$3" &&
    -+	local counter=0 &&
    ++	local counter="0" &&
     +	local i &&
     +	local j &&
     +	test_tick &&
    -+	local basedata=$basedir$test_tick &&
    ++	local basedata="$basedir$test_tick" &&
     +	rm -rf "$basedir" &&
     +	for i in $(test_seq $dirs)
     +	do
    -+		local dir=$basedir/dir$i &&
    ++		local dir="$basedir/dir$i" &&
     +		mkdir -p "$dir" &&
     +		for j in $(test_seq $files)
     +		do
12:  47cc63e1dda ! 11:  1a8320828f7 t/perf: add iteration setup mechanism to perf-lib
    @@ t/perf/p7519-fsmonitor.sh: then
     -	fi
     -fi
     -
    - trace_start() {
    + trace_start () {
      	if test -n "$GIT_PERF_7519_TRACE"
      	then
    -@@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor() {
    +@@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor_hook () {
      
      test_perf_w_drop_caches () {
      	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
    @@ t/perf/p7519-fsmonitor.sh: setup_for_fsmonitor() {
     -	test_perf "$@"
      }
      
    - test_fsmonitor_suite() {
    + test_fsmonitor_suite () {
     
      ## t/perf/p7820-grep-engines.sh ##
     @@ t/perf/p7820-grep-engines.sh: do
    @@ t/perf/perf-lib.sh: exit $ret' >&3 2>&4
      }
      
      test_wrapper_ () {
    -+	local test_wrapper_func_ test_title_
    - 	test_wrapper_func_=$1; shift
    -+	test_title_=$1; shift
    +-	test_wrapper_func_=$1; shift
    ++	local test_wrapper_func_="$1"; shift
    ++	local test_title_="$1"; shift
      	test_start_
     -	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
     -	test "$#" = 2 ||
13:  26be6ecb28b ! 12:  3eb5a6720cb core.fsyncmethod: performance tests for batch mode
    @@ t/perf/p0008-odb-fsync.sh (new)
     +}
     +
     +test_perf_fsync_cfgs () {
    -+	local method cfg &&
    ++	local method &&
    ++	local cfg &&
     +	for method in none fsync batch writeout-only
     +	do
     +		case $method in
14:  88c1f84d4c3 <  -:  ----------- core.fsyncmethod: correctly camel-case warning message
-- 
2.34.1.78.g86e39b8f8d