Re: [PATCH v2 4/4] pack-write: rename *.idx file into place last (really!)

On Wed, Sep 08 2021, Ævar Arnfjörð Bjarmason wrote:

Follow-up a preceding commit (pack-write.c: rename `.idx` file into
place last, 2021-08-16)[1] and rename the *.idx file in-place after we
write the *.bitmap. The preceding commit fixed the issue of *.idx
being written before *.rev files, but did not do so for *.idx files.

See 7cc8f971085 (pack-objects: implement bitmap writing, 2013-12-21)
for commentary at the time when *.bitmap was implemented about how
those files are written out, nothing in that commit contradicts what's
being done here.

Note that the referenced earlier commit[1] is overly optimistic about
"clos[ing the] race", i.e. yes we'll now write the files in the right
order, but we might still race due to our sloppy use of fsync(). See
the thread at [2] for a rabbit hole of various discussions about
filesystem races in the face of doing and not doing fsync() (and if
doing fsync(), not doing it properly).

Actually I think it's a bit worse than that, we will unconditionally
fsync() the *.pack we write out, but in stage_tmp_packfiles() (the
behavior pre-dates both this series and its parent, I just think my
stage_tmp_packfiles() is easier to follow) we'll not write the *.idx
file with fsync() since we won't pass WRITE_IDX_VERIFY.

The same goes for *.rev (which oddly makes its fsync() conditional on
WRITE_IDX_VERIFY), but not *.bitmap, which fsyncs unconditionally just
like *.pack does.

And then of course we'll do all these in-place renames but nothing
fsyncs the fd of the directory, so the metadata and new names being
committed to disk & visible to other processes is anyone's guess.

But not only is that metadata commit not made, but due to the
inconsistent fsync() we might end up with an *.idx that's partial and
renamed in-place.

In any case, any such issues pre-date this series and the series by
Taylor it depends on, just adding some #leftoverbits for future fsync()
fixes since I spent time looking into it.