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.
