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