On Thu, Sep 09 2021, Ævar Arnfjörð Bjarmason wrote: > On Wed, Sep 08 2021, Taylor Blau wrote: > >> Here is a very small reroll of a couple of patches to make sure that packs are >> not read before all of their auxiliary files exist and are moved into place, by >> renaming the .idx file into place last. >> >> New since the original version is a patch to apply similar treatment to >> index-pack, as noticed by Peff in [1]. This should be queued before Ævar's >> series on top. > > I read Junio's earlier <xmqq8s063m7m.fsf@gitster.g>[1] as a suggestion > that we combine the two into a singe series. I do think that yields a > better end-result, in particular your 1/3 is much more readable if the > refactoring in my 2/4. > > I'm mot of the way done with such a rewrite, which also incorporates > your suggestion for how to manage memory in rename_tmp_packfile(), but > I'll hold of on what you & Junio have to say about next steps before > adding to the re-roll competition Junio mentioned... > > 1. https://lore.kernel.org/git/xmqq8s063m7m.fsf@gitster.g I've got that at https://github.com/git/git/compare/master...avar:avar-tb/idx-rename-race-3 Range-diff to my own v2 + your v1 follows (i.e. not my v2 + your v2 here). I can submit this as-is to the list, or something else. Junio nominated you "team leader", so I'll go with your plan :) I think the functional changes on top of the function refactoring make it much easier to see what's going on, e.g.: https://github.com/git/git/commit/832d9fa083d20abab35695037152dfeb1f9fe50a I took the liberty of similarly creating a helper function in index-pack.c, which turns your commit there into this: https://github.com/git/git/commit/8c2dcbf6011c466a806491672c0d7308e8105a1d The relevant commit messages were also adjusted to note that in the earlier *.idx v.s. *.rev we're leaving the similar *.bitmap problem for a later change. 3: 29f57876515 = 1: 4869f97408b pack.h: line-wrap the definition of finish_tmp_packfile() 4: 7b39f4599b1 ! 2: c0b1ec90d43 pack-write: refactor renaming in finish_tmp_packfile() @@ Metadata ## Commit message ## pack-write: refactor renaming in finish_tmp_packfile() - Refactor the renaming in finish_tmp_packfile() so that it takes a - "const struct strbuf *" instead of a non-const, and refactor the - repetitive renaming pattern in finish_tmp_packfile() to use a new - static rename_tmp_packfile() helper function. + Refactor the renaming in finish_tmp_packfile() into a helper + function. The callers are now expected to pass a "name_buffer" ending + in "pack-OID." instead of the previous "pack-", we then append "pack", + "idx" or "rev" to it. + + By doing the strbuf_setlen() in rename_tmp_packfile() we re-use the + buffer and avoid the repeated allocations we'd get if that function + had its own temporary "struct strbuf". + + This approach of re-using the buffer does make the last user in + pack-object.c's write_pack_file() slightly awkward, we needlessly do a + strbuf_setlen() there just before the strbuf_release() for + consistency. In subsequent changes we'll move that bitmap writing code + around, so let's not skip the strbuf_setlen() now. The previous strbuf_reset() idiom originated with 5889271114a (finish_tmp_packfile():use strbuf for pathname @@ Commit message pre-strbuf code added in 0e990530ae (finish_tmp_packfile(): a helper function, 2011-10-28). - Since the memory allocation is not a bottleneck here we can afford a - bit more readability at the cost of re-allocating this new "struct - strbuf sb". - Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/pack-objects.c ## +@@ builtin/pack-objects.c: static void write_pack_file(void) + warning_errno(_("failed utime() on %s"), pack_tmp_name); + } + +- strbuf_addf(&tmpname, "%s-", base_name); ++ strbuf_addf(&tmpname, "%s-%s.", base_name, ++ hash_to_hex(hash)); + + if (write_bitmap_index) { + bitmap_writer_set_checksum(hash); @@ builtin/pack-objects.c: static void write_pack_file(void) &pack_idx_opts, hash); if (write_bitmap_index) { - strbuf_addf(&tmpname, "%s.bitmap", hash_to_hex(hash)); -+ struct strbuf sb = STRBUF_INIT; -+ -+ strbuf_addf(&sb, "%s%s.bitmap", tmpname.buf, -+ hash_to_hex(hash)); ++ size_t tmpname_len = tmpname.len; ++ strbuf_addstr(&tmpname, "bitmap"); stop_progress(&progress_state); + bitmap_writer_show_progress(progress); @@ builtin/pack-objects.c: static void write_pack_file(void) - bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1); - bitmap_writer_build(&to_pack); bitmap_writer_finish(written_list, nr_written, -- tmpname.buf, write_bitmap_options); -+ sb.buf, write_bitmap_options); + tmpname.buf, write_bitmap_options); write_bitmap_index = 0; -+ strbuf_release(&sb); ++ strbuf_setlen(&tmpname, tmpname_len); } strbuf_release(&tmpname); @@ pack-write.c: struct hashfile *create_tmp_packfile(char **pack_tmp_name) return hashfd(fd, *pack_tmp_name); } --void finish_tmp_packfile(struct strbuf *name_buffer, -+static void rename_tmp_packfile(const char *source, -+ const struct strbuf *basename, -+ const unsigned char hash[], -+ const char *ext) ++static void rename_tmp_packfile(struct strbuf *nb, const char *source, ++ const char *ext) +{ -+ struct strbuf sb = STRBUF_INIT; ++ size_t nb_len = nb->len; + -+ strbuf_addf(&sb, "%s%s.%s", basename->buf, hash_to_hex(hash), ext); -+ if (rename(source, sb.buf)) ++ strbuf_addstr(nb, ext); ++ if (rename(source, nb->buf)) + die_errno("unable to rename temporary '*.%s' file to '%s'", -+ ext, sb.buf); -+ strbuf_release(&sb); ++ ext, nb->buf); ++ strbuf_setlen(nb, nb_len); +} + -+void finish_tmp_packfile(const struct strbuf *basename, + void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, - uint32_t nr_written, @@ pack-write.c: void finish_tmp_packfile(struct strbuf *name_buffer, unsigned char hash[]) { @@ pack-write.c: void finish_tmp_packfile(struct strbuf *name_buffer, - - strbuf_setlen(name_buffer, basename_len); - +- strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash)); +- if (rename(idx_tmp_name, name_buffer->buf)) +- die_errno("unable to rename temporary index file"); +- +- strbuf_setlen(name_buffer, basename_len); +- - if (rev_tmp_name) { - strbuf_addf(name_buffer, "%s.rev", hash_to_hex(hash)); - if (rename(rev_tmp_name, name_buffer->buf)) - die_errno("unable to rename temporary reverse-index file"); -- -- strbuf_setlen(name_buffer, basename_len); - } - -- strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash)); -- if (rename(idx_tmp_name, name_buffer->buf)) -- die_errno("unable to rename temporary index file"); -- - strbuf_setlen(name_buffer, basename_len); -+ rename_tmp_packfile(pack_tmp_name, basename, hash, "pack"); ++ rename_tmp_packfile(name_buffer, pack_tmp_name, "pack"); ++ rename_tmp_packfile(name_buffer, idx_tmp_name, "idx"); + if (rev_tmp_name) -+ rename_tmp_packfile(rev_tmp_name, basename, hash, "rev"); -+ rename_tmp_packfile(idx_tmp_name, basename, hash, "idx"); ++ rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); free((void *)idx_tmp_name); } - - ## pack.h ## -@@ pack.h: int encode_in_pack_object_header(unsigned char *hdr, int hdr_len, - int read_pack_header(int fd, struct pack_header *); - - struct hashfile *create_tmp_packfile(char **pack_tmp_name); --void finish_tmp_packfile(struct strbuf *name_buffer, -+void finish_tmp_packfile(const struct strbuf *basename, - const char *pack_tmp_name, - struct pack_idx_entry **written_list, - uint32_t nr_written, -: ----------- > 3: 832d9fa083d pack-write.c: rename `.idx` files after `*.rev' 2: a6a4d2154e8 ! 4: 9f7e005ba03 builtin/repack.c: move `.idx` files into place last @@ Commit message itself). Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/repack.c ## @@ builtin/repack.c: static struct { 1: ea3b1a0d8ed ! 5: 457d4b9787c pack-write.c: rename `.idx` file into place last @@ ## Metadata ## -Author: Taylor Blau <me@xxxxxxxxxxxx> +Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - pack-write.c: rename `.idx` file into place last + index-pack: refactor renaming in final() - We treat the presence of an `.idx` file as the indicator of whether or - not it's safe to use a packfile. But `finish_tmp_packfile()` (which is - responsible for writing the pack and moving the temporary versions of - all of its auxiliary files into place) is inconsistent about the write - order. + Refactor the renaming in final() into a helper function, this is + similar in spirit to a preceding refactoring of finish_tmp_packfile() + in pack-write.c. - But the `.rev` file is moved into place after the `.idx`, so it's - possible for a process to open a pack in between the two (i.e., while - the `.idx` file is in place but the `.rev` file is not) and mistakenly - fall back to generating the pack's reverse index in memory. Though racy, - this amounts to an unnecessary slow-down at worst, and doesn't affect - the correctness of the resulting reverse index. + Before e37d0b8730b (builtin/index-pack.c: write reverse indexes, + 2021-01-25) it probably wasn't worth it to have this sort of helper, + due to the differing "else if" case for "pack" files v.s. "idx" files. - Close this race by moving the .rev file into place before moving the - .idx file into place. + But since we've got "rev" as well now, let's do the renaming via a + helper, this is both a net decrease in lines, and improves the + readability, since we can easily see at a glance that the logic for + writing these three types of files is exactly the same, aside from the + obviously differing cases of "*final_xyz_name" being NULL, and + "else_chmod_if" being different. - While we're here, only call strbuf_setlen() if we actually modified the - buffer by bringing it inside of the same if-statement that calls - rename(). + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> - Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> - - ## pack-write.c ## -@@ pack-write.c: void finish_tmp_packfile(struct strbuf *name_buffer, + ## builtin/index-pack.c ## +@@ builtin/index-pack.c: static void write_special_file(const char *suffix, const char *msg, + strbuf_release(&name_buf); + } - strbuf_setlen(name_buffer, basename_len); ++static void rename_tmp_packfile(const char **final_xyz_name, ++ const char *curr_xyz_name, ++ struct strbuf *xyz_name, unsigned char *hash, ++ const char *ext, int else_chmod_if) ++{ ++ if (*final_xyz_name != curr_xyz_name) { ++ if (!*final_xyz_name) ++ *final_xyz_name = odb_pack_name(xyz_name, hash, ext); ++ if (finalize_object_file(curr_xyz_name, *final_xyz_name)) ++ die(_("unable to rename temporary '*.%s' file to '%s"), ++ ext, *final_xyz_name); ++ } else if (else_chmod_if) { ++ chmod(*final_xyz_name, 0444); ++ } ++} ++ + static void final(const char *final_pack_name, const char *curr_pack_name, + const char *final_index_name, const char *curr_index_name, + const char *final_rev_index_name, const char *curr_rev_index_name, +@@ builtin/index-pack.c: static void final(const char *final_pack_name, const char *curr_pack_name, + write_special_file("promisor", promisor_msg, final_pack_name, + hash, NULL); -- strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash)); -- if (rename(idx_tmp_name, name_buffer->buf)) -- die_errno("unable to rename temporary index file"); +- if (final_pack_name != curr_pack_name) { +- if (!final_pack_name) +- final_pack_name = odb_pack_name(&pack_name, hash, "pack"); +- if (finalize_object_file(curr_pack_name, final_pack_name)) +- die(_("cannot store pack file")); +- } else if (from_stdin) +- chmod(final_pack_name, 0444); - -- strbuf_setlen(name_buffer, basename_len); +- if (final_index_name != curr_index_name) { +- if (!final_index_name) +- final_index_name = odb_pack_name(&index_name, hash, "idx"); +- if (finalize_object_file(curr_index_name, final_index_name)) +- die(_("cannot store index file")); +- } else +- chmod(final_index_name, 0444); - - if (rev_tmp_name) { - strbuf_addf(name_buffer, "%s.rev", hash_to_hex(hash)); - if (rename(rev_tmp_name, name_buffer->buf)) - die_errno("unable to rename temporary reverse-index file"); -+ -+ strbuf_setlen(name_buffer, basename_len); - } - -+ strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash)); -+ if (rename(idx_tmp_name, name_buffer->buf)) -+ die_errno("unable to rename temporary index file"); -+ - strbuf_setlen(name_buffer, basename_len); +- if (curr_rev_index_name) { +- if (final_rev_index_name != curr_rev_index_name) { +- if (!final_rev_index_name) +- final_rev_index_name = odb_pack_name(&rev_index_name, hash, "rev"); +- if (finalize_object_file(curr_rev_index_name, final_rev_index_name)) +- die(_("cannot store reverse index file")); +- } else +- chmod(final_rev_index_name, 0444); +- } ++ rename_tmp_packfile(&final_pack_name, curr_pack_name, &pack_name, ++ hash, "pack", from_stdin); ++ rename_tmp_packfile(&final_index_name, curr_index_name, &index_name, ++ hash, "idx", 1); ++ if (curr_rev_index_name) ++ rename_tmp_packfile(&final_rev_index_name, curr_rev_index_name, ++ &rev_index_name, hash, "rev", 1); - free((void *)idx_tmp_name); + if (do_fsck_object) { + struct packed_git *p; -: ----------- > 6: 8c2dcbf6011 builtin/index-pack.c: move `.idx` files into place last 5: 1205f9d0c25 ! 7: 3a0ee7e4a99 pack-write: split up finish_tmp_packfile() function @@ builtin/pack-objects.c: static void write_pack_file(void) written_list, nr_written, - &pack_idx_opts, hash); + &pack_idx_opts, hash, &idx_tmp_name); -+ rename_tmp_packfile_idx(&tmpname, hash, &idx_tmp_name); ++ rename_tmp_packfile_idx(&tmpname, &idx_tmp_name); if (write_bitmap_index) { - struct strbuf sb = STRBUF_INIT; + size_t tmpname_len = tmpname.len; @@ builtin/pack-objects.c: static void write_pack_file(void) - strbuf_release(&sb); + strbuf_setlen(&tmpname, tmpname_len); } + free(idx_tmp_name); @@ bulk-checkin.c: static struct bulk_checkin_state { uint32_t nr_written; } state; -+static void finish_tmp_packfile(const struct strbuf *basename, ++static void finish_tmp_packfile(struct strbuf *basename, + const char *pack_tmp_name, + struct pack_idx_entry **written_list, + uint32_t nr_written, @@ bulk-checkin.c: static struct bulk_checkin_state { + + stage_tmp_packfiles(basename, pack_tmp_name, written_list, nr_written, + pack_idx_opts, hash, &idx_tmp_name); -+ rename_tmp_packfile_idx(basename, hash, &idx_tmp_name); ++ rename_tmp_packfile_idx(basename, &idx_tmp_name); + + free(idx_tmp_name); +} @@ bulk-checkin.c: static struct bulk_checkin_state { static void finish_bulk_checkin(struct bulk_checkin_state *state) { struct object_id oid; +@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state) + close(fd); + } + +- strbuf_addf(&packname, "%s/pack/pack-", get_object_directory()); ++ strbuf_addf(&packname, "%s/pack/pack-%s.", get_object_directory(), ++ oid_to_hex(&oid)); + finish_tmp_packfile(&packname, state->pack_tmp_name, + state->written, state->nr_written, + &state->pack_idx_opts, oid.hash); ## pack-write.c ## -@@ pack-write.c: static void rename_tmp_packfile(const char *source, - strbuf_release(&sb); +@@ pack-write.c: static void rename_tmp_packfile(struct strbuf *nb, const char *source, + strbuf_setlen(nb, nb_len); } --void finish_tmp_packfile(const struct strbuf *basename, -+void rename_tmp_packfile_idx(const struct strbuf *basename, -+ unsigned char hash[], char **idx_tmp_name) +-void finish_tmp_packfile(struct strbuf *name_buffer, ++void rename_tmp_packfile_idx(struct strbuf *name_buffer, ++ char **idx_tmp_name) +{ -+ rename_tmp_packfile(*idx_tmp_name, basename, hash, "idx"); ++ rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx"); +} + -+void stage_tmp_packfiles(const struct strbuf *basename, ++void stage_tmp_packfiles(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, @@ pack-write.c: static void rename_tmp_packfile(const char *source, die_errno("unable to make temporary index file readable"); rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash, -@@ pack-write.c: void finish_tmp_packfile(const struct strbuf *basename, - rename_tmp_packfile(pack_tmp_name, basename, hash, "pack"); +@@ pack-write.c: void finish_tmp_packfile(struct strbuf *name_buffer, + rename_tmp_packfile(name_buffer, pack_tmp_name, "pack"); if (rev_tmp_name) - rename_tmp_packfile(rev_tmp_name, basename, hash, "rev"); -- rename_tmp_packfile(idx_tmp_name, basename, hash, "idx"); + rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); +- rename_tmp_packfile(name_buffer, idx_tmp_name, "idx"); - - free((void *)idx_tmp_name); } @@ pack.h: int encode_in_pack_object_header(unsigned char *hdr, int hdr_len, int read_pack_header(int fd, struct pack_header *); struct hashfile *create_tmp_packfile(char **pack_tmp_name); --void finish_tmp_packfile(const struct strbuf *basename, -+void stage_tmp_packfiles(const struct strbuf *basename, +-void finish_tmp_packfile(struct strbuf *name_buffer, ++void stage_tmp_packfiles(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, @@ pack.h: int encode_in_pack_object_header(unsigned char *hdr, int hdr_len, - unsigned char sha1[]); + unsigned char hash[], + char **idx_tmp_name); -+void rename_tmp_packfile_idx(const struct strbuf *tmp_basename, -+ unsigned char hash[], char **idx_tmp_name); ++void rename_tmp_packfile_idx(struct strbuf *basename, ++ char **idx_tmp_name); #endif 6: 70f4a9767d3 ! 8: b1b232b80de pack-write: rename *.idx file into place last (really!) @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - pack-write: rename *.idx file into place last (really!) + pack-objects: rename .idx files into place after .bitmap files - 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. + 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 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). + 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). - 1. https://lore.kernel.org/git/a6a4d2154e83d41c10986c5f455279ab249a910c.1630461918.git.me@xxxxxxxxxxxx/ - 2. https://lore.kernel.org/git/8735qgkvv1.fsf@xxxxxxxxxxxxxxxxxxx/ + 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@xxxxxxxxxxxxxxxxxxx/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ builtin/pack-objects.c: 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, hash, &idx_tmp_name); +- rename_tmp_packfile_idx(&tmpname, &idx_tmp_name); if (write_bitmap_index) { - struct strbuf sb = STRBUF_INIT; + size_t tmpname_len = tmpname.len; @@ builtin/pack-objects.c: static void write_pack_file(void) - strbuf_release(&sb); + strbuf_setlen(&tmpname, tmpname_len); } -+ rename_tmp_packfile_idx(&tmpname, hash, &idx_tmp_name); ++ rename_tmp_packfile_idx(&tmpname, &idx_tmp_name); + free(idx_tmp_name); strbuf_release(&tmpname);
