[0/3] rename *.idx file into place last (also after *.bitmap)

Message

Ævar Arnfjörð Bjarmason Sept. 7, 2021, 7:42 p.m. UTC

I came up with this on top of Taylor's series which fixes the order in
which we write files associated with pack files[1]. His series fixes a
race where we write *.idx before *.rev, but left the issue of writing
*.bitmap after *.idx, this series fixes that. Now we'll really write
the *.idx last.

There's still a race in these combined serieses, but now it's to do
with our general lack of fsync(), i.e. there's no guarantee that just
because we write files A, B and C in that order that without doing the
proper fsync() dancing on the files and the fd for the directory that
other processes will see it that way. In practice though there is, as
few/no OS's are so pedantic as to expose inconsistent ordering of
written files in a directory, even though POSIX et al leave that
possibily open.

1. https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (3):
  pack-write: use more idiomatic strbuf usage for packname construction
  pack-write: split up finish_tmp_packfile() function
  pack-write: rename *.idx file into place last (really!)

 builtin/pack-objects.c | 33 +++++++++++++++++------
 builtin/repack.c       |  8 ++++++
 pack-write.c           | 60 ++++++++++++++++++++++++++++--------------
 pack.h                 | 16 ++++++++++-
 4 files changed, 88 insertions(+), 29 deletions(-)

Comments

Taylor Blau Sept. 7, 2021, 10:36 p.m. UTC | #1

On Tue, Sep 07, 2021 at 09:42:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
> I came up with this on top of Taylor's series which fixes the order in
> which we write files associated with pack files[1]. His series fixes a
> race where we write *.idx before *.rev, but left the issue of writing
> *.bitmap after *.idx, this series fixes that. Now we'll really write
> the *.idx last.

Nice catch. And fixing this race (by moving the .bitmap into place
before the .idx) doesn't create another race, because
open_pack_bitmap_1() returns early when the call to open_pack_index()
fails.

(And furthermore, open_pack_bitmap_1() is only called on the list of
packs generated by calling prepare_pack() over each file in
objects/pack, which only adds a pack to the list if its .idx file
exists, too).

I reviewed this series and left a few notes which I
would like to see addressed/responded to before queuing, but this seems
to be generally of the right approach to me.

Thanks,
Taylor