git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: avarab@gmail.com, gitster@pobox.com
Subject: [PATCH 9/9] pack-objects: rename .idx files into place after .bitmap files
Date: Wed, 8 Sep 2021 23:25:22 -0400	[thread overview]
Message-ID: <d8286cf1075dc85231128145c5abb0db3881032b.1631157880.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1631157880.git.me@ttaylorr.com>

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

In preceding commits the race of renaming .idx files in place before
.rev files and other auxiliary files was fixed in pack-write.c's
finish_tmp_packfile(), builtin/repack.c's "struct exts", and
builtin/index-pack.c's final(). As noted in the change to pack-write.c
we left in place the issue of writing *.bitmap files after the *.idx,
let's fix that issue.

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 this commit and preceding ones only close any race condition
with *.idx files being written before their auxiliary files if we're
optimistic about our lack of fsync()-ing in this are not tripping us
over. See the thread at [1] 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).

In particular, in this case of writing to ".git/objects/pack" we only
write and fsync() the individual files, but if we wanted to guarantee
that the metadata update was seen in that way by concurrent processes
we'd need to fsync() on the "fd" of the containing directory. That
concern is probably more theoretical than not, modern OS's tend to be
more on the forgiving side than the overly pedantic side of
implementing POSIX FS semantics.

1. https://lore.kernel.org/git/8735qgkvv1.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/pack-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 944134b6f2..a01767a384 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1250,7 +1250,6 @@ static void write_pack_file(void)
 			stage_tmp_packfiles(&tmpname, pack_tmp_name,
 					    written_list, nr_written,
 					    &pack_idx_opts, hash, &idx_tmp_name);
-			rename_tmp_packfile_idx(&tmpname, &idx_tmp_name);
 
 			if (write_bitmap_index) {
 				size_t tmpname_len = tmpname.len;
@@ -1267,6 +1266,8 @@ static void write_pack_file(void)
 				strbuf_setlen(&tmpname, tmpname_len);
 			}
 
+			rename_tmp_packfile_idx(&tmpname, &idx_tmp_name);
+
 			free(idx_tmp_name);
 			strbuf_release(&tmpname);
 			free(pack_tmp_name);
-- 
2.33.0.96.g73915697e6

  parent reply	other threads:[~2021-09-09  3:26 UTC|newest]

Thread overview: 81+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01  2:05 [PATCH 0/2] pack-write,repack: prevent opening packs too early Taylor Blau
2021-09-01  2:06 ` [PATCH 1/2] pack-write.c: rename `.idx` file into place last Taylor Blau
2021-09-01  2:06 ` [PATCH 2/2] builtin/repack.c: move `.idx` files " Taylor Blau
2021-09-01  3:53 ` [PATCH 0/2] pack-write,repack: prevent opening packs too early Jeff King
2021-09-01  4:29   ` Taylor Blau
2021-09-01  4:59     ` Jeff King
2021-09-01  5:12       ` Taylor Blau
2021-09-01  6:08         ` Jeff King
2021-09-01 21:40           ` Taylor Blau
2021-09-07 16:07             ` Jeff King
2021-09-07 19:42 ` [PATCH 0/3] rename *.idx file into place last (also after *.bitmap) Ævar Arnfjörð Bjarmason
2021-09-07 19:42   ` [PATCH 1/3] pack-write: use more idiomatic strbuf usage for packname construction Ævar Arnfjörð Bjarmason
2021-09-07 22:21     ` Taylor Blau
2021-09-07 23:22       ` Ævar Arnfjörð Bjarmason
2021-09-07 19:42   ` [PATCH 2/3] pack-write: split up finish_tmp_packfile() function Ævar Arnfjörð Bjarmason
2021-09-07 22:28     ` Taylor Blau
2021-09-07 19:42   ` [PATCH 3/3] pack-write: rename *.idx file into place last (really!) Ævar Arnfjörð Bjarmason
2021-09-07 22:31     ` Taylor Blau
2021-09-07 22:36   ` [PATCH 0/3] rename *.idx file into place last (also after *.bitmap) Taylor Blau
2021-09-07 19:48 ` [PATCH 0/2] pack-write,repack: prevent opening packs too early Ævar Arnfjörð Bjarmason
2021-09-08  0:38 ` [PATCH v2 0/4] rename *.idx file into place last (also after *.bitmap) Ævar Arnfjörð Bjarmason
2021-09-08  0:38   ` [PATCH v2 1/4] pack.h: line-wrap the definition of finish_tmp_packfile() Ævar Arnfjörð Bjarmason
2021-09-08  0:38   ` [PATCH v2 2/4] pack-write: refactor renaming in finish_tmp_packfile() Ævar Arnfjörð Bjarmason
2021-09-08  4:22     ` Taylor Blau
2021-09-08  0:38   ` [PATCH v2 3/4] pack-write: split up finish_tmp_packfile() function Ævar Arnfjörð Bjarmason
2021-09-08  0:38   ` [PATCH v2 4/4] pack-write: rename *.idx file into place last (really!) Ævar Arnfjörð Bjarmason
2021-09-08  1:14     ` Ævar Arnfjörð Bjarmason
2021-09-08  9:18       ` Ævar Arnfjörð Bjarmason
2021-09-08  4:24     ` Taylor Blau
2021-09-08 22:17 ` [PATCH v2 0/3] prevent opening packs too early Taylor Blau
2021-09-08 22:17   ` [PATCH v2 1/3] pack-write.c: rename `.idx` files into place last Taylor Blau
2021-09-08 22:17   ` [PATCH v2 2/3] builtin/repack.c: move " Taylor Blau
2021-09-08 22:17   ` [PATCH v2 3/3] builtin/index-pack.c: " Taylor Blau
2021-09-08 23:52   ` [PATCH v2 0/3] prevent opening packs too early Ævar Arnfjörð Bjarmason
2021-09-09  0:50     ` Ævar Arnfjörð Bjarmason
2021-09-09  1:13       ` Taylor Blau
2021-09-09  1:33         ` Ævar Arnfjörð Bjarmason
2021-09-09  2:36           ` Ævar Arnfjörð Bjarmason
2021-09-09  2:49             ` Taylor Blau
2021-09-09  3:24 ` [PATCH 0/9] packfile: avoid .idx rename races Taylor Blau
2021-09-09  3:24   ` [PATCH 1/9] pack.h: line-wrap the definition of finish_tmp_packfile() Taylor Blau
2021-09-09  3:24   ` [PATCH 2/9] bulk-checkin.c: store checksum directly Taylor Blau
2021-09-09  7:38     ` Ævar Arnfjörð Bjarmason
2021-09-09  3:24   ` [PATCH 3/9] pack-write: refactor renaming in finish_tmp_packfile() Taylor Blau
2021-09-09 19:29     ` Junio C Hamano
2021-09-09 21:07       ` Taylor Blau
2021-09-09 23:30         ` Junio C Hamano
2021-09-09 23:31           ` Taylor Blau
2021-09-10  1:29         ` Junio C Hamano
2021-09-09  3:24   ` [PATCH 4/9] pack-write.c: rename `.idx` files after `*.rev` Taylor Blau
2021-09-09  7:46     ` Ævar Arnfjörð Bjarmason
2021-09-09 14:37       ` Taylor Blau
2021-09-09 19:32     ` Junio C Hamano
2021-09-09  3:25   ` [PATCH 5/9] builtin/repack.c: move `.idx` files into place last Taylor Blau
2021-09-09 19:38     ` Junio C Hamano
2021-09-09 21:08       ` Taylor Blau
2021-09-09  3:25   ` [PATCH 6/9] index-pack: refactor renaming in final() Taylor Blau
2021-09-09 19:45     ` Junio C Hamano
2021-09-09 21:11       ` Taylor Blau
2021-09-09  3:25   ` [PATCH 7/9] builtin/index-pack.c: move `.idx` files into place last Taylor Blau
2021-09-09  7:52     ` Ævar Arnfjörð Bjarmason
2021-09-09 19:45     ` Junio C Hamano
2021-09-09  3:25   ` [PATCH 8/9] pack-write: split up finish_tmp_packfile() function Taylor Blau
2021-09-09  3:25   ` Taylor Blau [this message]
2021-09-09  7:54     ` [PATCH 9/9] pack-objects: rename .idx files into place after .bitmap files Ævar Arnfjörð Bjarmason
2021-09-09 19:52       ` Junio C Hamano
2021-09-09 21:13         ` Taylor Blau
2021-09-09  8:06   ` [PATCH 0/9] packfile: avoid .idx rename races Ævar Arnfjörð Bjarmason
2021-09-09 14:40     ` Taylor Blau
2021-09-09 19:52       ` Junio C Hamano
2021-09-09 23:24   ` [PATCH v2 " Taylor Blau
2021-09-09 23:24     ` [PATCH v2 1/9] pack.h: line-wrap the definition of finish_tmp_packfile() Taylor Blau
2021-09-09 23:24     ` [PATCH v2 2/9] bulk-checkin.c: store checksum directly Taylor Blau
2021-09-09 23:24     ` [PATCH v2 3/9] pack-write: refactor renaming in finish_tmp_packfile() Taylor Blau
2021-09-09 23:24     ` [PATCH v2 4/9] pack-write.c: rename `.idx` files after `*.rev` Taylor Blau
2021-09-09 23:24     ` [PATCH v2 5/9] builtin/repack.c: move `.idx` files into place last Taylor Blau
2021-09-09 23:24     ` [PATCH v2 6/9] index-pack: refactor renaming in final() Taylor Blau
2021-09-09 23:24     ` [PATCH v2 7/9] builtin/index-pack.c: move `.idx` files into place last Taylor Blau
2021-09-09 23:24     ` [PATCH v2 8/9] pack-write: split up finish_tmp_packfile() function Taylor Blau
2021-09-09 23:25     ` [PATCH v2 9/9] pack-objects: rename .idx files into place after .bitmap files Taylor Blau
2021-09-10  1:35     ` [PATCH v2 0/9] packfile: avoid .idx rename races Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d8286cf1075dc85231128145c5abb0db3881032b.1631157880.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).