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

Neeraj Singh [off-list ref] writes:

BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

On Tue, Sep 07, 2021 at 11:44:52PM -0700, Junio C Hamano wrote:

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

Basically yes.  XFS on Linux has shutdown ioctls that allow to simulate
that crash by shutting the file system down which really helps debugging
that kind of code.  A bunch of other file systems (ext4, f2fs) have
also picked this up now (grep for {XFS,EXT4,F2FS}_IOC_SHUTDOWN).

On September 8, 2021 2:50 AM, Christoph Hellwig wrote:

To: Junio C Hamano <redacted>
Cc: Neeraj Singh <redacted>; Neeraj K. Singh via GitGitGadget <redacted>; Git List <redacted>;
Johannes Schindelin [off-list ref]; Jeff King [off-list ref]; Jeff Hostetler [off-list ref]; Christoph
Hellwig [off-list ref]; Ævar Arnfjörð Bjarmason [off-list ref]; Neeraj K. Singh [off-list ref]
Subject: Re: [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles

On Tue, Sep 07, 2021 at 11:44:52PM -0700, Junio C Hamano wrote:

quoted

I doubt that fsyncObjectFiles is something we can reliably test in CI,
either with the new batched thing or with the original "when we close
one, make sure the changes hit the disk platter" approach.  So I am
not sure what conclusion we should draw from such an experiment, other
than "ok, it compiles cleanly."  After all, unless we cause system
crashes, what we thought we have written and close(2) would be seen by
another process that we spawn after that, with or without sync, no?

Basically yes.  XFS on Linux has shutdown ioctls that allow to simulate that crash by shutting the file system down which really

helps

debugging that kind of code.  A bunch of other file systems (ext4, f2fs) have also picked this up now (grep for
{XFS,EXT4,F2FS}_IOC_SHUTDOWN).

I strongly doubt this concept will work in an MPP architecture, particularly one where "shutting the file system down" is not
possible. I know of at least 3 operating systems where that is a bad plan, and if you did, you would take the test suite down while
you were at it.
-Randall

On Wed, Sep 08, 2021 at 09:57:34AM -0400, Randall S. Becker wrote:

possible. I know of at least 3 operating systems where that is a bad plan, and if you did, you would take the test suite down while
you were at it.

I've just mentioned a good way to write a test for this feature on a
specific platform.  This is absolutely no judgement if that is a good
plan on other platforms.

On September 8, 2021 10:13 AM, Christoph Hellwig wrote:

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

On Wed, Sep 08, 2021 at 09:57:34AM -0400, Randall S. Becker wrote:

quoted

possible. I know of at least 3 operating systems where that is a bad
plan, and if you did, you would take the test suite down while you were at it.

I've just mentioned a good way to write a test for this feature on a specific platform.  This is absolutely no judgement if that is

a good plan

on other platforms.

Thank you for the clarification. I do appreciate it.
-Randall

On Tue, Sep 7, 2021 at 11:44 PM Junio C Hamano [off-list ref] wrote:

Neeraj Singh [off-list ref] writes:

quoted

BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

The main failure mode I was worried about is that some test or other part
of Git is relying on a loose object being immediately available after it is
added to the ODB. With batch mode, the loose objects aren't actually
available until the bulk checkin is unplugged.

I agree that it is not easy to test whether the data is actually going
to durable
storage at the expected time.  FWIW, I did take a disk IO trace on Windows to
verify that we are issuing disk writes and flushes at the right time.
But that's a
one-time test that would be hard to make automated.

Neeraj Singh [off-list ref] writes:

On Tue, Sep 7, 2021 at 11:44 PM Junio C Hamano [off-list ref] wrote:

quoted

Neeraj Singh [off-list ref] writes:

quoted

BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

The main failure mode I was worried about is that some test or other part
of Git is relying on a loose object being immediately available after it is
added to the ODB. With batch mode, the loose objects aren't actually
available until the bulk checkin is unplugged.

Ah, I see.  If there are two processes that communicate over pipes
to decide whose turn it is (perhaps a producer of data that feeds
fast-import may wait for fast-import to say "I gave this label to
the object you requested" and goes ahead to use that object), and at
the point that the "other" process takes its turn, if the objects
are not "flushed" yet, things can break.  That's a valid concern.

On Wed, Sep 8, 2021 at 12:12 PM Junio C Hamano [off-list ref] wrote:

Neeraj Singh [off-list ref] writes:

quoted

On Tue, Sep 7, 2021 at 11:44 PM Junio C Hamano [off-list ref] wrote:

quoted

Neeraj Singh [off-list ref] writes:

quoted

BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

The main failure mode I was worried about is that some test or other part
of Git is relying on a loose object being immediately available after it is
added to the ODB. With batch mode, the loose objects aren't actually
available until the bulk checkin is unplugged.

Ah, I see.  If there are two processes that communicate over pipes
to decide whose turn it is (perhaps a producer of data that feeds
fast-import may wait for fast-import to say "I gave this label to
the object you requested" and goes ahead to use that object), and at
the point that the "other" process takes its turn, if the objects
are not "flushed" yet, things can break.  That's a valid concern.

That's right. This appears to be a possibility in the existing bulk
checkin code that produces packfiles for large objects as well, but
my change makes the situation much more common.

On Wed, Sep 08 2021, Neeraj Singh wrote:

On Tue, Sep 7, 2021 at 11:44 PM Junio C Hamano [off-list ref] wrote:

quoted

Neeraj Singh [off-list ref] writes:

quoted

BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.

I doubt that fsyncObjectFiles is something we can reliably test in
CI, either with the new batched thing or with the original "when we
close one, make sure the changes hit the disk platter" approach.  So
I am not sure what conclusion we should draw from such an experiment,
other than "ok, it compiles cleanly."  After all, unless we cause
system crashes, what we thought we have written and close(2) would
be seen by another process that we spawn after that, with or without
sync, no?

The main failure mode I was worried about is that some test or other part
of Git is relying on a loose object being immediately available after it is
added to the ODB. With batch mode, the loose objects aren't actually
available until the bulk checkin is unplugged.

I agree that it is not easy to test whether the data is actually going
to durable
storage at the expected time.  FWIW, I did take a disk IO trace on Windows to
verify that we are issuing disk writes and flushes at the right time.
But that's a
one-time test that would be hard to make automated.

I have some semi-related patches I need to dig up and finish sometime
which add a "git gc" test mode to the test suite, i.e. any time we call
"git gc --auto" it will go ahead and actually run, and some adversarial
options to run always, right away, prune with --expire=now. It found
some false positives, but also some genuine races and bugs at the time.

Similarly, I think a good longer term goal for better fsync() and data
integrity in git is to refactor the various codepaths where we write to
disk (grepping for fsync_or_die() is a good start to find those) to all
live in one place, we could then easily instrument that code to run in a
hostile test mode.

E.g. make anything that expects to write out a "foo" file actually write
out "foo.not-synced-yet" as long as fsync() etc. hasn't been called, or
with signals/timers/atexit() handlers fake up known FS edge cases such
as a write of "foo" only renaming "foo.not-synced-yet" to "foo" 1s after
the last close() call not followed by an fsync, etc.

Anyway, I expect given your occupation that you may have better ideas in
that area, presumably needing to instrument and test behavior under I/O
pressure, deferred syncs etc. is something mature FS's need to deal with
as part of their own regression tests...

1. https://lore.kernel.org/git/cover-v2-0.4-0000000000-20210908T003631Z-avarab@gmail.com/