| Message ID | pull.1076.v5.git.git.1632514331.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement a batched fsync option for core.fsyncObjectFiles | expand |
On Fri, Sep 24, 2021 at 1:12 PM Neeraj K. Singh via GitGitGadget <gitgitgadget@gmail.com> wrote: > > 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 > > -- > gitgitgadget Apologies for the spam, I'll be submitting a v6 shortly since there were several things wrong with this version.
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