CC: Johannes Schindelin On Fri, Jul 15, 2022 at 10:18 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
