Re: ab/squelch-empty-fsync-traces & hx/unpack-streaming bug (was: What's cooking in git.git (Jul 2022, #04; Wed, 13))

[Han Xin <chiyutianyi@gmail.com>]

CC: Johannes Schindelin

On Fri, Jul 15, 2022 at 10:18 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> I hadn't had time to look at this until now. There's some interesting
> behavior here.
>
> The code to check the hardware flush was added in aaf81223f48
> (unpack-objects: use stream_loose_object() to unpack large objects,
> 2022-06-11) (that series is now on master).
>
> But as my ab/squelch-empty-fsync-traces notes we always add this to the
> event, so the:
>
>         grep fsync/hardware-flush trace2.txt &&
>
> Is equivalent to:
>
>         true &&
>
> I.e. it's not testing worthwhile at all. The reason you're seeing a
> failure is deu to 412e4caee38 (tests: disable fsync everywhere,
> 2021-10-29), i.e. our tests disable fsync(). What you have queued will
> pass as:
>
>         GIT_TEST_FSYNC=true ./t5351-unpack-large-objects.sh
>
> But I think that would be meaningless, since we'll write out that on
> FSYNC_HARDWARE_FLUSH whether we actually support "bulk" or not. AFAICT
> the way to detect if we support "bulk" at all is to check for
> fsync/writeout-only.
>
> *Except* that we we unconditionally increment the "writeout only"
> counter, even if we don't actually support that "bulk" mode. We're just
> doing a regular fsync().
>

Agree with you.

In fact, since stream_loose_object() only works with objects of type
*blob*, the rest objects of *commit* and *tree* will still use
write_loose_object(), so "grep fsync/hardware-flush trace2.txt" did
not check for the changes in stream_loose_object() at all.

I haven't found any reference cases in the existing tests.

Perhaps, we need more efficient "fsync" test cases?

Thanks.
-Han Xin