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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 8, 2021, 01:14 UTC
Message-ID: <87lf47kgkh.fsf@evledraar.gmail.com>
In-Reply-To: <patch-v2-4.4-70f4a9767d-20210908T003631Z-avarab@gmail.com>

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

Show 16 quoted lines

> 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.