Re: [PATCH v5 0/7] Implement a batched fsync option for core.fsyncObjectFiles
- From
- Neeraj Singh <nksingh85@gmail.com>
- Date
- Sep 24, 2021, 23:31 UTC
- Message-ID
- <CANQDOddqbiiVEt7JEDmF9QsJvPv8W6nZrenpsDPeem0brd-7_A@mail.gmail.com>
- In-Reply-To
- <pull.1076.v5.git.git.1632514331.gitgitgadget@gmail.com>
On Fri, Sep 24, 2021 at 1:12 PM Neeraj K. Singh via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 882 quoted lines
>
> Thanks to everyone for review so far! Changes since v4, all in response to
> review feedback from Ævar Arnfjörð Bjarmason:
>
> * Update core.fsyncobjectfiles documentation to specify 'loose' objects and
> to add a statement about not fsyncing parent directories.
>
> * I still don't want to make any promises on behalf of the Linux FS developers
> in the documentation. However, according to [v4.1] and my understanding
> of how XFS journals are documented to work, it looks like recent versions
> of Linux running on XFS should be as safe as Windows or macOS in 'batch'
> mode. I don't know about ext4, since it's not clear to me when metadata
> updates are made visible to the journal.
>
>
> * Rewrite the core batched fsync change to use the tmp-objdir lib. As Ævar
> pointed out, this lets us access the added loose objects immediately,
> rather than only after unplugging the bulk checkin. This is a hard
> requirement in unpack-objects for resolving OBJ_REF_DELTA packed objects.
>
> * As a preparatory patch, the object-file code now doesn't do a rename if it's in a
> tmp objdir (as determined by the quarantine environment variable).
>
> * I added support to the tmp-objdir lib to replace the 'main' writable odb.
>
> * Instead of using a lockfile for the final full fsync, we now use a new dummy
> temp file. Doing that makes the below unpack-objects change easier.
>
>
> * Add bulk-checkin support to unpack-objects, which is used in fetch and
> push. In addition to making those operations faster, it allows us to
> directly compare performance of packfiles against loose objects. Please
> see [v4.2] for a measurement of 'git push' to a local upstream with
> different numbers of unique new files.
>
> * Rename FSYNC_OBJECT_FILES_MODE to fsync_object_files_mode.
>
> * Remove comment with link to NtFlushBuffersFileEx documentation.
>
> * Make t/lib-unique-files.sh a bit cleaner. We are still creating unique
> contents, but now this uses test_tick, so it should be deterministic from
> run to run.
>
> * Ensure there are tests for all of the modified commands. Make the
> unpack-objects tests validate that the unpacked objects are really
> available in the ODB.
>
> References for v4: [v4.1]
> https://lore.kernel.org/linux-fsdevel/20190419072938.31320-1-amir73il@gmail.com/#t
>
> [v4.2]
> https://docs.google.com/spreadsheets/d/1uxMBkEXFFnQ1Y3lXKqcKpw6Mq44BzhpCAcPex14T-QQ/edit#gid=1898936117
>
> Changes since v3:
>
> * Fix core.fsyncobjectfiles option parsing as suggested by Junio: We now
> accept no value to mean "true" and we require 'batch' to be lowercase.
>
> * Leave the default fsync mode as 'false'. Git for windows can change its
> default when this series makes it over to that fork.
>
> * Use a switch statement in git_fsync, as suggested by Junio.
>
> * Add regression test cases for core.fsyncobjectfiles=batch. This should
> keep the batch functionality basically working in upstream git even if
> few users adopt batch mode initially. I expect git-for-windows will
> provide a good baking area for the new mode.
>
> Neeraj Singh (7):
> object-file.c: do not rename in a temp odb
> bulk-checkin: rename 'state' variable and separate 'plugged' boolean
> core.fsyncobjectfiles: batched disk flushes
> update-index: use the bulk-checkin infrastructure
> unpack-objects: use the bulk-checkin infrastructure
> core.fsyncobjectfiles: tests for batch mode
> core.fsyncobjectfiles: performance tests for add and stash
>
> Documentation/config/core.txt | 29 +++++++--
> Makefile | 6 ++
> builtin/add.c | 1 +
> builtin/unpack-objects.c | 3 +
> builtin/update-index.c | 6 ++
> bulk-checkin.c | 92 +++++++++++++++++++++++---
> bulk-checkin.h | 2 +
> cache.h | 8 ++-
> config.c | 7 +-
> config.mak.uname | 1 +
> configure.ac | 8 +++
> environment.c | 6 +-
> git-compat-util.h | 7 ++
> object-file.c | 118 +++++++++++++++++++++++++++++-----
> object-store.h | 22 +++++++
> object.c | 2 +-
> repository.c | 2 +
> repository.h | 1 +
> t/lib-unique-files.sh | 36 +++++++++++
> t/perf/p3700-add.sh | 43 +++++++++++++
> t/perf/p3900-stash.sh | 46 +++++++++++++
> t/t3700-add.sh | 20 ++++++
> t/t3903-stash.sh | 14 ++++
> t/t5300-pack-object.sh | 30 +++++----
> tmp-objdir.c | 20 +++++-
> tmp-objdir.h | 6 ++
> wrapper.c | 44 +++++++++++++
> write-or-die.c | 2 +-
> 28 files changed, 532 insertions(+), 50 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: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v5
> Pull-Request: https://github.com/git/git/pull/1076
>
> Range-diff vs v4:
>
> -: ----------- > 1: 95315f35a28 object-file.c: do not rename in a temp odb
> 1: d5893e28df1 = 2: df6fab94d67 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
> 2: 12cad737635 ! 3: fe19cdfc930 core.fsyncobjectfiles: batched disk flushes
> @@ Commit message
>
> One major source of the cost of fsync is the implied flush of the
> hardware writeback cache within the disk drive. Fortunately, Windows,
> - macOS, and Linux each offer mechanisms to write data from the filesystem
> - page cache without initiating a hardware flush.
> + and macOS offer mechanisms to write data from the filesystem page cache
> + without initiating a hardware flush. Linux has the sync_file_range API,
> + which issues a pagecache writeback request reliably after version 5.2.
>
> This patch introduces a new 'core.fsyncObjectFiles = batch' option that
> - takes advantage of the bulk-checkin infrastructure to batch up hardware
> - flushes.
> + batches up hardware flushes. It hooks into the bulk-checkin plugging and
> + unplugging functionality and takes advantage of tmp-objdir.
>
> - When the new mode is enabled we do the following for new objects:
> -
> - 1. Create a tmp_obj_XXXX file and write the object data to it.
> + 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.
> - 3. Record the tmp name and the final name in the bulk-checkin state for
> - later rename.
>
> - At the end of the entire transaction we:
> - 1. Issue a fsync against the lock file to flush the hardware writeback
> - cache, which should by now have processed the tmp file writes.
> - 2. Rename all of the temp files to their final names.
> + At the end of the entire transaction when unplugging bulk checkin we:
> + 1. Issue an fsync against a dummy file to flush the hardware writeback
> + cache, which should by now have processed 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.
> + another fsync internal to that operation. This is not the case today,
> + but may be a good extension to those components.
>
> On a filesystem with a singular journal that is updated during name
> - operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
> + 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.
> @@ Documentation/config/core.txt: core.whitespace::
> + A value indicating the level of effort Git will expend in
> + trying to make objects added to the repo durable in the event
> + of an unclean system shutdown. This setting currently only
> -+ controls the object store, so updates to any refs or the
> -+ index may not be equally durable.
> ++ controls loose objects in the object store, so updates to any
> ++ refs or the index may not be equally durable.
> ++
> +* `false` allows data to remain in file system caches according to
> + operating system policy, whence it may be lost if the system loses power
> + or crashes.
> -+* `true` triggers a data integrity flush for each object added to the
> ++* `true` triggers a data integrity flush for each loose object added to the
> + object store. This is the safest setting that is likely to ensure durability
> + across all operating systems and file systems that honor the 'fsync' system
> + call. However, this setting comes with a significant performance cost on
> -+ common hardware.
> ++ common hardware. Git does not currently fsync parent directories for
> ++ newly-added files, so some filesystems may still allow data to be lost on
> ++ system crash.
> +* `batch` enables an experimental mode that uses interfaces available in some
> -+ operating systems to write object data with a minimal set of FLUSH CACHE
> -+ (or equivalent) commands sent to the storage controller. If the operating
> -+ system interfaces are not available, this mode behaves the same as `true`.
> -+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
> -+ filesystems and on Windows for repos stored on NTFS or ReFS.
> ++ operating systems to write loose object data with a minimal set of FLUSH
> ++ CACHE (or equivalent) commands sent to the storage controller. If the
> ++ operating system interfaces are not available, this mode behaves the same as
> ++ `true`. This mode is expected to be as safe as `true` on macOS for repos
> ++ stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or
> ++ ReFS.
>
> core.preloadIndex::
> Enable parallel index preload for operations like 'git diff'
> @@ 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();
> +
> -+ unplug_bulk_checkin(&lock_file);
> + unplug_bulk_checkin();
>
> finish:
> - if (write_locked_index(&the_index, &lock_file,
>
> ## bulk-checkin.c ##
> @@
> @@ bulk-checkin.c
> #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 struct string_list bulk_fsync_state = STRING_LIST_INIT_DUP;
> ++static int needs_batch_fsync;
> +
> ++static struct tmp_objdir *bulk_fsync_objdir;
> +
> static struct bulk_checkin_state {
> char *pack_tmp_name;
> - struct hashfile *f;
> @@ bulk-checkin.c: clear_exit:
> reprepare_packed_git(the_repository);
> }
>
> -+static void do_sync_and_rename(struct string_list *fsync_state, struct lock_file *lock_file)
> ++/*
> ++ * Cleanup after batch-mode fsync_object_files.
> ++ */
> ++static void do_batch_fsync(void)
> +{
> -+ if (fsync_state->nr) {
> -+ struct string_list_item *rename;
> -+
> -+ /*
> -+ * Issue a full hardware flush against the lock file to ensure
> -+ * that all objects are durable before any renames occur.
> -+ * The code in fsync_and_close_loose_object_bulk_checkin has
> -+ * already ensured that writeout has occurred, but it has not
> -+ * flushed any writeback cache in the storage hardware.
> -+ */
> -+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
> -+
> -+ for_each_string_list_item(rename, fsync_state) {
> -+ const char *src = rename->string;
> -+ const char *dst = rename->util;
> -+
> -+ if (finalize_object_file(src, dst))
> -+ die_errno(_("could not rename '%s' to '%s'"), src, dst);
> -+ }
> -+
> -+ string_list_clear(fsync_state, 1);
> ++ /*
> ++ * Issue a full hardware flush against a temporary file to ensure
> ++ * that all objects are durable before any renames occur. The code in
> ++ * fsync_loose_object_bulk_checkin has already issued a writeout
> ++ * request, but it has not flushed any writeback cache in the storage
> ++ * hardware.
> ++ */
> ++
> ++ if (needs_batch_fsync) {
> ++ struct strbuf temp_path = STRBUF_INIT;
> ++ struct tempfile *temp;
> ++
> ++ strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory());
> ++ temp = xmks_tempfile(temp_path.buf);
> ++ fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
> ++ delete_tempfile(&temp);
> ++ strbuf_release(&temp_path);
> + }
> ++
> ++ if (bulk_fsync_objdir)
> ++ tmp_objdir_migrate(bulk_fsync_objdir);
> +}
> +
> static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
> @@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
> return 0;
> }
>
> -+static void add_rename_bulk_checkin(struct string_list *fsync_state,
> -+ const char *src, const char *dst)
> ++void fsync_loose_object_bulk_checkin(int fd)
> +{
> -+ string_list_insert(fsync_state, src)->util = xstrdup(dst);
> -+}
> -+
> -+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
> -+ const char *filename, time_t mtime)
> -+{
> -+ int do_finalize = 1;
> -+ int ret = 0;
> -+
> -+ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
> -+ /*
> -+ * If we have a plugged bulk checkin, 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 renaming files as part of do_sync_and_rename.
> -+ */
> -+ if (bulk_checkin_plugged &&
> -+ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
> -+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
> -+ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
> -+ do_finalize = 0;
> -+
> -+ } else {
> -+ fsync_or_die(fd, "loose object file");
> -+ }
> -+ }
> -+
> -+ if (close(fd))
> -+ die_errno(_("error when closing loose object file"));
> -+
> -+ if (mtime) {
> -+ struct utimbuf utb;
> -+ utb.actime = mtime;
> -+ utb.modtime = mtime;
> -+ if (utime(tmpfile, &utb) < 0)
> -+ warning_errno(_("failed utime() on %s"), tmpfile);
> ++ assert(fsync_object_files == FSYNC_OBJECT_FILES_BATCH);
> ++
> ++ /*
> ++ * If we have a plugged bulk checkin, 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.
> ++ */
> ++ if (bulk_checkin_plugged &&
> ++ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
> ++ assert(the_repository->objects->odb->is_temp);
> ++ if (!needs_batch_fsync)
> ++ needs_batch_fsync = 1;
> ++ } else {
> ++ fsync_or_die(fd, "loose object file");
> + }
> -+
> -+ if (do_finalize)
> -+ ret = finalize_object_file(tmpfile, filename);
> -+
> -+ return ret;
> +}
> +
> 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 plug_bulk_checkin(void)
> +@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
> + void plug_bulk_checkin(void)
> + {
> + assert(!bulk_checkin_plugged);
> ++
> ++ /*
> ++ * Create a temporary object directory if the current
> ++ * object directory is not already temporary.
> ++ */
> ++ if (fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
> ++ !the_repository->objects->odb->is_temp) {
> ++ bulk_fsync_objdir = tmp_objdir_create();
> ++ if (!bulk_fsync_objdir)
> ++ die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch"));
> ++
> ++ tmp_objdir_replace_main_odb(bulk_fsync_objdir);
> ++ }
> ++
> bulk_checkin_plugged = 1;
> }
>
> --void unplug_bulk_checkin(void)
> -+void unplug_bulk_checkin(struct lock_file *lock_file)
> - {
> - assert(bulk_checkin_plugged);
> +@@ bulk-checkin.c: void unplug_bulk_checkin(void)
> bulk_checkin_plugged = 0;
> if (bulk_checkin_state.f)
> finish_bulk_checkin(&bulk_checkin_state);
> +
> -+ do_sync_and_rename(&bulk_fsync_state, lock_file);
> ++ do_batch_fsync();
> }
>
> ## bulk-checkin.h ##
> @@ bulk-checkin.h
>
> #include "cache.h"
>
> -+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
> -+ const char *filename, time_t mtime);
> ++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);
> -
> - void plug_bulk_checkin(void);
> --void unplug_bulk_checkin(void);
> -+void unplug_bulk_checkin(struct lock_file *);
> -
> - #endif
>
> ## cache.h ##
> @@ cache.h: void reset_shared_repository(void);
> @@ cache.h: void reset_shared_repository(void);
> extern char *git_replace_ref_base;
>
> -extern int fsync_object_files;
> -+enum FSYNC_OBJECT_FILES_MODE {
> ++enum fsync_object_files_mode {
> + FSYNC_OBJECT_FILES_OFF,
> + FSYNC_OBJECT_FILES_ON,
> + FSYNC_OBJECT_FILES_BATCH
> +};
> +
> -+extern enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
> ++extern enum fsync_object_files_mode fsync_object_files;
> extern int core_preload_index;
> extern int precomposed_unicode;
> extern int protect_hfs;
> @@ environment.c: const char *git_hooks_path;
> int core_compression_level;
> int pack_compression_level = Z_DEFAULT_COMPRESSION;
> -int fsync_object_files;
> -+enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
> ++enum fsync_object_files_mode fsync_object_files;
> size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
> size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
> size_t delta_base_cache_limit = 96 * 1024 * 1024;
> @@ git-compat-util.h: __attribute__((format (printf, 1, 2))) NORETURN
> * Returns 0 on success, which includes trying to unlink an object that does
>
> ## object-file.c ##
> -@@ object-file.c: int hash_object_file(const struct git_hash_algo *algo, const void *buf,
> - return 0;
> +@@ object-file.c: void add_to_alternates_memory(const char *reference)
> + '\n', NULL, 0);
> }
>
> --/* Finalize a file on disk, and close it. */
> --static void close_loose_object(int fd)
> --{
> ++struct object_directory *set_temporary_main_odb(const char *dir)
> ++{
> ++ struct object_directory *main_odb, *new_odb, *old_next;
> ++
> ++ /*
> ++ * Make sure alternates are initialized, or else our entry may be
> ++ * overwritten when they are.
> ++ */
> ++ prepare_alt_odb(the_repository);
> ++
> ++ /* Copy the existing object directory and make it an alternate. */
> ++ main_odb = the_repository->objects->odb;
> ++ new_odb = xmalloc(sizeof(*new_odb));
> ++ *new_odb = *main_odb;
> ++ *the_repository->objects->odb_tail = new_odb;
> ++ the_repository->objects->odb_tail = &(new_odb->next);
> ++ new_odb->next = NULL;
> ++
> ++ /*
> ++ * Reinitialize the main odb with the specified path, being careful
> ++ * to keep the next pointer value.
> ++ */
> ++ old_next = main_odb->next;
> ++ memset(main_odb, 0, sizeof(*main_odb));
> ++ main_odb->next = old_next;
> ++ main_odb->is_temp = 1;
> ++ main_odb->path = xstrdup(dir);
> ++ return new_odb;
> ++}
> ++
> ++void restore_main_odb(struct object_directory *odb)
> ++{
> ++ struct object_directory **prev, *main_odb;
> ++
> ++ /* Unlink the saved previous main ODB from the list. */
> ++ prev = &the_repository->objects->odb->next;
> ++ assert(*prev);
> ++ while (*prev != odb) {
> ++ prev = &(*prev)->next;
> ++ }
> ++ *prev = odb->next;
> ++ if (*prev == NULL)
> ++ the_repository->objects->odb_tail = prev;
> ++
> ++ /*
> ++ * Restore the data from the old main odb, being careful to
> ++ * keep the next pointer value
> ++ */
> ++ main_odb = the_repository->objects->odb;
> ++ SWAP(*main_odb, *odb);
> ++ main_odb->next = odb->next;
> ++ free_object_directory(odb);
> ++}
> ++
> + /*
> + * Compute the exact path an alternate is at and returns it. In case of
> + * error NULL is returned and the human readable error is added to `err`
> +@@ object-file.c: int hash_object_file(const struct git_hash_algo *algo, const void *buf,
> + /* Finalize a file on disk, and close it. */
> + static void close_loose_object(int fd)
> + {
> - if (fsync_object_files)
> -- fsync_or_die(fd, "loose object file");
> -- if (close(fd) != 0)
> -- die_errno(_("error when closing loose object file"));
> --}
> --
> - /* Size of directory component, including the ending '/' */
> - static inline int directory_size(const char *filename)
> ++ switch (fsync_object_files) {
> ++ case FSYNC_OBJECT_FILES_OFF:
> ++ break;
> ++ case FSYNC_OBJECT_FILES_ON:
> + fsync_or_die(fd, "loose object file");
> ++ break;
> ++ case FSYNC_OBJECT_FILES_BATCH:
> ++ fsync_loose_object_bulk_checkin(fd);
> ++ break;
> ++ default:
> ++ BUG("Invalid fsync_object_files mode.");
> ++ }
> ++
> + if (close(fd) != 0)
> + die_errno(_("error when closing loose object file"));
> + }
> +
> + ## object-store.h ##
> +@@ object-store.h: void add_to_alternates_file(const char *dir);
> + */
> + void add_to_alternates_memory(const char *dir);
> +
> ++/*
> ++ * Replace the current main object directory with the specified temporary
> ++ * object directory. We make a copy of the former main object directory,
> ++ * add it as an in-memory alternate, and return the copy so that it can
> ++ * be restored via restore_main_odb.
> ++ */
> ++struct object_directory *set_temporary_main_odb(const char *dir);
> ++
> ++/*
> ++ * Restore a previous ODB replaced by set_temporary_main_odb.
> ++ */
> ++void restore_main_odb(struct object_directory *odb);
> ++
> + /*
> + * Populate and return the loose object cache array corresponding to the
> + * given object ID.
> +@@ object-store.h: struct oidtree *odb_loose_cache(struct object_directory *odb,
> + /* Empty the loose object cache for the specified object directory. */
> + void odb_clear_loose_cache(struct object_directory *odb);
> +
> ++/* Clear and free the specified object directory */
> ++void free_object_directory(struct object_directory *odb);
> ++
> + struct packed_git {
> + struct hashmap_entry packmap_ent;
> + struct packed_git *next;
> +
> + ## object.c ##
> +@@ object.c: struct raw_object_store *raw_object_store_new(void)
> + return o;
> + }
> +
> +-static void free_object_directory(struct object_directory *odb)
> ++void free_object_directory(struct object_directory *odb)
> {
> -@@ object-file.c: 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));
> + free(odb->path);
> + odb_clear_loose_cache(odb);
> +
> + ## tmp-objdir.c ##
> +@@
> + struct tmp_objdir {
> + struct strbuf path;
> + struct strvec env;
> ++ struct object_directory *prev_main_odb;
> + };
>
> -- close_loose_object(fd);
> --
> -- if (mtime) {
> -- struct utimbuf utb;
> -- utb.actime = mtime;
> -- utb.modtime = mtime;
> -- if (utime(tmp_file.buf, &utb) < 0)
> -- warning_errno(_("failed utime() on %s"), tmp_file.buf);
> -- }
> --
> -- return finalize_object_file(tmp_file.buf, filename.buf);
> -+ return fsync_and_close_loose_object_bulk_checkin(fd, tmp_file.buf,
> -+ filename.buf, mtime);
> + /*
> +@@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
> + * freeing memory; it may cause a deadlock if the signal
> + * arrived while libc's allocator lock is held.
> + */
> +- if (!on_signal)
> ++ if (!on_signal) {
> ++ if (t->prev_main_odb)
> ++ restore_main_odb(t->prev_main_odb);
> + tmp_objdir_free(t);
> ++ }
> ++
> + return err;
> }
>
> - static int freshen_loose_object(const struct object_id *oid)
> +@@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(void)
> + t = xmalloc(sizeof(*t));
> + strbuf_init(&t->path, 0);
> + strvec_init(&t->env);
> ++ t->prev_main_odb = NULL;
> +
> + strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
> +
> +@@ tmp-objdir.c: int tmp_objdir_migrate(struct tmp_objdir *t)
> + if (!t)
> + return 0;
> +
> ++ if (t->prev_main_odb) {
> ++ restore_main_odb(t->prev_main_odb);
> ++ t->prev_main_odb = NULL;
> ++ }
> ++
> + strbuf_addbuf(&src, &t->path);
> + strbuf_addstr(&dst, get_object_directory());
> +
> +@@ tmp-objdir.c: void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)
> + {
> + add_to_alternates_memory(t->path.buf);
> + }
> ++
> ++void tmp_objdir_replace_main_odb(struct tmp_objdir *t)
> ++{
> ++ if (t->prev_main_odb)
> ++ BUG("the main object database is already replaced");
> ++ t->prev_main_odb = set_temporary_main_odb(t->path.buf);
> ++}
> +
> + ## tmp-objdir.h ##
> +@@ tmp-objdir.h: int tmp_objdir_destroy(struct tmp_objdir *);
> + */
> + void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
> +
> ++/*
> ++ * Replaces the main object store in the current process with the temporary
> ++ * object directory and makes the former main object store an alternate.
> ++ */
> ++void tmp_objdir_replace_main_odb(struct tmp_objdir *);
> ++
> + #endif /* TMP_OBJDIR_H */
>
> ## wrapper.c ##
> @@ wrapper.c: int xmkstemp_mode(char *filename_template, int mode)
> 3: a5b3e21b762 < -: ----------- core.fsyncobjectfiles: add windows support for batch mode
> 4: f7f756f3932 ! 4: 485b4a767df update-index: use the bulk-checkin infrastructure
> @@ Commit message
> There is some risk with this change, since under batch fsync, the object
> files will not be available until the update-index is entirely complete.
> This usage is unlikely, since any tool invoking update-index and
> - expecting to see objects would have to snoop the output of --verbose to
> - find out when update-index has actually processed a given path.
> - Additionally the index is locked for the duration of the update.
> + expecting to see objects would have to synchronize with the update-index
> + process after passing it a file path.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
>
> @@ builtin/update-index.c
> #include "lockfile.h"
> #include "quote.h"
> @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
> - struct strbuf unquoted = STRBUF_INIT;
>
> - setup_work_tree();
> -+ plug_bulk_checkin();
> - while (getline_fn(&buf, stdin) != EOF) {
> - char *p;
> - if (!nul_term_line && buf.buf[0] == '"') {
> + 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.
> @@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
> - chmod_path(set_executable_bit, p);
> - free(p);
> - }
> -+ unplug_bulk_checkin(&lock_file);
> - strbuf_release(&unquoted);
> 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; "
> -: ----------- > 5: 889e7668760 unpack-objects: use the bulk-checkin infrastructure
> 5: afb0028e796 ! 6: 0f2e3b25759 core.fsyncobjectfiles: tests for batch mode
> @@ Metadata
> ## Commit message ##
> core.fsyncobjectfiles: tests for batch mode
>
> - Add test cases to exercise batch mode for 'git add'
> - and 'git stash'. These tests ensure that the added
> - data winds up in the object database.
> + Add test cases to exercise batch mode for:
> + * 'git add'
> + * 'git stash'
> + * 'git update-index'
> + * 'git unpack-objects'
>
> - I verified the tests by introducing an incorrect rename
> - in do_sync_and_rename.
> + 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.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
>
> @@ t/lib-unique-files.sh (new)
> @@
> +# Helper to create files with unique contents
> +
> -+test_create_unique_files_base__=$(date -u)
> -+test_create_unique_files_counter__=0
> +
> +# 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 . -- Creates 2 directories with 3 files
> -+# each in the specified directory, all
> -+# with unique contents.
> ++# 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"
> @@ t/lib-unique-files.sh (new)
> + local dirs=$1
> + local files=$2
> + local basedir=$3
> ++ local counter=0
> ++ test_tick
> ++ local basedata=$test_tick
> ++
> +
> -+ rm -rf $basedir >/dev/null
> ++ rm -rf $basedir
> +
> + for i in $(test_seq $dirs)
> + do
> + local dir=$basedir/dir$i
> +
> -+ mkdir -p "$dir" > /dev/null
> ++ mkdir -p "$dir"
> + for j in $(test_seq $files)
> + do
> -+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
> -+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
> ++ counter=$((counter + 1))
> ++ echo "$basedata.$counter" >"$dir/file$j.txt"
> + done
> + done
> +}
> @@ t/t3700-add.sh: test_expect_success \
> + rm -f fsynced_files &&
> + git ls-files --stage fsync-files/ > fsynced_files &&
> + test_line_count = 8 fsynced_files &&
> -+ cat fsynced_files | awk '{print \$2}' | xargs -n1 git cat-file -e
> ++ awk -- '{print \$2}' fsynced_files | xargs -n1 git cat-file -e
> ++"
> ++
> ++test_expect_success 'git update-index: core.fsyncobjectfiles=batch' "
> ++ test_create_unique_files 2 4 fsync-files2 &&
> ++ find fsync-files2 ! -type d -print | xargs git -c core.fsyncobjectfiles=batch 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 \
> @@ t/t3903-stash.sh: test_expect_success 'stash handles skip-worktree entries nicel
> + # which contains the untracked files
> + git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
> + test_line_count = 8 fsynced_files &&
> -+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
> ++ awk -- '{print \$3}' fsynced_files | xargs -n1 git cat-file -e
> +"
> +
> +
> test_expect_success 'stash -c stash.useBuiltin=false warning ' '
> expected="stash.useBuiltin support has been removed" &&
>
> +
> + ## t/t5300-pack-object.sh ##
> +@@ t/t5300-pack-object.sh: 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}
> + '
> +
> ++test_expect_success 'unpack without delta (core.fsyncobjectfiles=batch)' '
> ++ check_unpack test-1-${packname_1} "-c core.fsyncobjectfiles=batch"
> ++'
> ++
> + test_expect_success 'pack with REF_DELTA' '
> + packname_2=$(git pack-objects --progress test-2 <obj-list 2>stderr) &&
> + check_deltas stderr -gt 0
> +@@ t/t5300-pack-object.sh: test_expect_success 'unpack with REF_DELTA' '
> + check_unpack test-2-${packname_2}
> + '
> +
> ++test_expect_success 'unpack with REF_DELTA (core.fsyncobjectfiles=batch)' '
> ++ check_unpack test-2-${packname_2} "-c core.fsyncobjectfiles=batch"
> ++'
> ++
> + test_expect_success 'pack with OFS_DELTA' '
> + packname_3=$(git pack-objects --progress --delta-base-offset test-3 \
> + <obj-list 2>stderr) &&
> +@@ t/t5300-pack-object.sh: test_expect_success 'unpack with OFS_DELTA' '
> + check_unpack test-3-${packname_3}
> + '
> +
> ++test_expect_success 'unpack with OFS_DELTA (core.fsyncobjectfiles=batch)' '
> ++ check_unpack test-3-${packname_3} "-c core.fsyncobjectfiles=batch"
> ++'
> ++
> + test_expect_success 'compare delta flavors' '
> + perl -e '\''
> + defined($_ = -s $_) or die for @ARGV;
> 6: 3e6b80b5fa2 = 7: 6543564376a core.fsyncobjectfiles: performance tests for add and stash
>
> --
> gitgitgadgetApologies for the spam, I'll be submitting a v6 shortly since there were several things wrong with this version.