From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/2] pack-write,repack: prevent opening packs too early
Date: Tue, 31 Aug 2021 23:53:35 -0400 [thread overview]
Message-ID: <YS75P7r33NIBmFh2@coredump.intra.peff.net> (raw)
In-Reply-To: <cover.1630461918.git.me@ttaylorr.com>
On Tue, Aug 31, 2021 at 10:05:46PM -0400, Taylor Blau wrote:
> This pair of patches fixes a race where the .idx is moved into place
> before the .rev file, allowing the pack to be in a state where it
> appears a .rev file wasn't generated.
>
> This can cause Git to inadvertently take the slow "generate the
> reverse index on-the-fly", which does not impact correctness, but is
> unnecessarily slow when compared to reading the .rev file.
>
> The race is fixed by moving the .idx into place only after all other
> pack-related files have already been written. The first patch fixes
> the direct `pack-objects` case, and the second patch fixes `repack`
> (which also renames pack files around).
These both look good to me, but I think we're missing one more spot.
The first patch covers git-pack-objects directly, like:
git pack-objects .git/objects/pack/pack
In practice, though, we never do that. On-disk repacks happen via
git-repack, which always writes to temporary files. So I thought the
case in git-pack-objects wouldn't matter, but it does look like
prepare_packed_git() will actually read .tmp-$$-pack-1234abcd files, and
so might load our temporary pack. Unexpected, but we are covered by your
first patch. And then the second patch covers us as we move those
temporary files into place.
So far so good. But the other obvious way to get a pack idx is via
index-pack (especially "--stdin").
It looks like we'd want the same re-ordering to happen in
builtin/index-pack.c:final(), which is where we rename the temporary
files into place.
We _might_ also want to re-order the calls to write_idx_file() and
write_rev_file() in its caller, given that simultaneous readers are
happy to read our tmp_pack_* files. I guess the same might apply to the
actual file write order pack-objects, too? I'm not sure if that's even
possible, though; do we rely on side effects of generating the .idx when
generating the other meta files?
I think it might be more sensible if the reading side was taught to
ignore ".tmp-*" and "tmp_*" (and possibly even ".*", though it's
possible somebody is relying on that).
-Peff
next prev parent reply other threads:[~2021-09-01 3:53 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 ` Jeff King [this message]
2021-09-01 4:29 ` [PATCH 0/2] pack-write,repack: prevent opening packs too early 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 ` [PATCH 9/9] pack-objects: rename .idx files into place after .bitmap files Taylor Blau
2021-09-09 7:54 ` Æ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=YS75P7r33NIBmFh2@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.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).