Re: [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Neeraj Singh <nksingh85@gmail.com>
Cc: "Neeraj K. Singh via GitGitGadget" <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jeff King <peff@peff.net>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Christoph Hellwig <hch@lst.de>,
	"Neeraj K. Singh" <neerajsi@microsoft.com>
Subject: Re: [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles
Date: Tue, 07 Sep 2021 21:50:48 +0200	[thread overview]
Message-ID: <8735qgkvv1.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CANQDOdeEic1ktyGU=dLEPi=FkU84Oqv9hDUEkfAXcS0WTwRJtQ@mail.gmail.com>

On Tue, Sep 07 2021, Neeraj Singh wrote:

> On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget
> <gitgitgadget@gmail.com> 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

Thanks, I've been meaning to take a look at this, and also as a
note-to-self: check how this interacts with the fsync()-impacted race I
noted in my just-sent:
https://lore.kernel.org/git/cover-0.3-00000000000-20210907T193600Z-avarab@gmail.com/