Re: ab/squelch-empty-fsync-traces & hx/unpack-streaming bug (was: What's cooking in git.git (Jul 2022, #04; Wed, 13))
- From
- Han Xin <chiyutianyi@gmail.com>
- Date
- Jul 16, 2022, 12:23 UTC
- Message-ID
- <CAO0brD3fdjQNfQaUBJRAHxDc24K00zpBUa62zST0=cZ5uz3vGA@mail.gmail.com>
- In-Reply-To
- <220715.86bktqzdb8.gmgdl@evledraar.gmail.com>
CC: Johannes Schindelin
On Fri, Jul 15, 2022 at 10:18 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 33 quoted lines
> > 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