Re: [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles
From: Neeraj Singh <hidden>
Date: 2021-09-07 19:44:29
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
Thanks to everyone for review so far! I've responded to the previous feedback and changed the patch series a bit. Changes since v1: * Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary to dscho's suggestion, I'm still implementing the Windows version in the same patch and I'm not doing autoconf detection since this is a POSIX function. * Introduce a separate preparatory patch to the bulk-checkin infrastructure to separate the 'plugged' variable and rename the 'state' variable, as suggested by dscho. * Add performance numbers to the commit message of the main bulk fsync patch, as suggested by dscho. * Add a comment about the non-thread-safety of the bulk-checkin infrastructure, as suggested by avarab. * Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested by dscho and avarab and others. * Add more details to Documentation/config/core.txt about the various settings and their intended effects, as suggested by avarab. * Switch to the string-list API to hold the rename state, as suggested by avarab. * Create a separate update-index patch to use bulk-checkin as suggested by dscho. * Add Windows support in the upstream git. This is done in a way that should not conflict with git-for-windows. * Add new performance tests that shows the delta based on fsync mode. NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct on Linux, since sync_file_range does not provide data integrity guarantees. There is currently no kernel interface suitable to achieve disk flush batching as is, but he suggested that he might implement a 'syncfs' variant on top of this patchset. This code is still useful on macOS and Windows, and the config documentation makes that clear. Neeraj Singh (6): object-file: use futimens rather than utime bulk-checkin: rename 'state' variable and separate 'plugged' boolean core.fsyncobjectfiles: batched disk flushes core.fsyncobjectfiles: add windows support for batch mode update-index: use the bulk-checkin infrastructure core.fsyncobjectfiles: performance tests for add and stash Documentation/config/core.txt | 26 ++++++-- Makefile | 6 ++ builtin/add.c | 3 +- builtin/update-index.c | 3 + bulk-checkin.c | 92 +++++++++++++++++++++++++---- bulk-checkin.h | 4 +- cache.h | 8 ++- compat/mingw.c | 53 +++++++++++------ compat/mingw.h | 5 ++ compat/win32/flush.c | 29 +++++++++ config.c | 8 ++- config.mak.uname | 4 ++ configure.ac | 8 +++ contrib/buildsystems/CMakeLists.txt | 3 +- environment.c | 2 +- git-compat-util.h | 7 +++ object-file.c | 23 ++------ t/perf/lib-unique-files.sh | 32 ++++++++++ t/perf/p3700-add.sh | 43 ++++++++++++++ t/perf/p3900-stash.sh | 46 +++++++++++++++ wrapper.c | 40 +++++++++++++ write-or-die.c | 2 +- 22 files changed, 389 insertions(+), 58 deletions(-) create mode 100644 compat/win32/flush.c create mode 100644 t/perf/lib-unique-files.sh create mode 100755 t/perf/p3700-add.sh create mode 100755 t/perf/p3900-stash.sh base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v2 Pull-Request: https://github.com/git/git/pull/1076
Hello everyone, I'd like to bump this review up in people's inboxes since Patch V2 hasn't gotten any traction in over a week. Thanks in advance for taking a look, - Neeraj Singh Windows Core Filesystems Team