git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Derrick Stolee <stolee@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH 0/2] midx: prevent bitmap corruption when permuting pack order
Date: Fri, 10 Dec 2021 17:31:27 -0500	[thread overview]
Message-ID: <YbPVP0BvYcVsfOrf@nand.local> (raw)
In-Reply-To: <YbOeEjNic5ETGcdy@nand.local>

On Fri, Dec 10, 2021 at 01:36:02PM -0500, Taylor Blau wrote:
> On Wed, Dec 08, 2021 at 02:55:13PM -0500, Jeff King wrote:
> > On Wed, Dec 08, 2021 at 02:30:17PM -0500, Derrick Stolee wrote:
> >
> > > > Taylor Blau (2):
> > > >   t5326: demonstrate bitmap corruption after permutation
> > > >   midx.c: make changing the preferred pack safe
> > >
> > > Just chiming in to say that I reviewed an earlier version of this series
> > > and the version in this submission looks good to me.
> >
> > Ditto. ;)
>
> All three of us missed that this PORD chunk actually contains the
> psuedo-pack position for every object in the MIDX. That is OK, but it's
> definitely adding more than 4 bytes per pack to the MIDX (in practice,
> it's adding 4 bytes per object).
>
> I'm semi-OK with this direction, since it's tantamount to storing the
> .rev file's contents in the MIDX itself. And even though we're not
> reading from it, it is doing the thing we need it to which is causing
> the MIDX to change its checksum when the object order changes.
>
> But I'm curious what both of your thoughts are before moving forward.

To just add a little bit more detail before I mostly close my computer
for the weekend:

The key part of this bug is that the MIDX checksum could remain
unchanged even when the object order used to write the MIDX bitmap does,
and that's what the first patch here is demonstrating.

Having PORD contain the same data as the .rev file still accomplishes
our original goal of preventing this bug, because it forces the checksum
to change when the object order does. But it's definitely more invasive
than I had imagined.

I had originally imagined that storing the preferred pack's identity
alone would be enough to solve this bug. But that isn't quite so,
because we break ties among duplicate objects first by prefered-ness,
then by their pack's mtime. So that could change too, and it would cause
us to break in the same way.

At the bare minimum you need an ordering of all of the packs in the
MIDX (like I had originally imagined here). At most, we could do
something like what is unintentionally written here, which would allow
us to get rid of MIDX .rev files entirely. I think doing the former is
simpler, and I am not sure if there are practical advantages to the
latter.

But I'm definitely curious as to what others think would be a good
direction to pursue.

Thanks,
Taylor

  reply	other threads:[~2021-12-10 22:31 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-08 19:26 [PATCH 0/2] midx: prevent bitmap corruption when permuting pack order Taylor Blau
2021-12-08 19:26 ` [PATCH 1/2] t5326: demonstrate bitmap corruption after permutation Taylor Blau
2021-12-08 19:26 ` [PATCH 2/2] midx.c: make changing the preferred pack safe Taylor Blau
2021-12-08 19:30 ` [PATCH 0/2] midx: prevent bitmap corruption when permuting pack order Derrick Stolee
2021-12-08 19:55   ` Jeff King
2021-12-10 18:36     ` Taylor Blau
2021-12-10 22:31       ` Taylor Blau [this message]
2021-12-11  1:39         ` Taylor Blau
2021-12-13 14:00           ` Derrick Stolee
2021-12-13 14:31             ` Taylor Blau
2021-12-14  1:55 ` [PATCH v2 0/8] " Taylor Blau
2021-12-14  1:55   ` [PATCH v2 1/8] t5326: demonstrate bitmap corruption after permutation Taylor Blau
2021-12-14  1:55   ` [PATCH v2 2/8] midx.c: make changing the preferred pack safe Taylor Blau
2021-12-14  1:55   ` [PATCH v2 3/8] pack-revindex.c: instrument loading on-disk reverse index Taylor Blau
2021-12-14  1:55   ` [PATCH v2 4/8] t5326: drop unnecessary setup Taylor Blau
2021-12-14  1:55   ` [PATCH v2 5/8] t5326: extract `test_rev_exists` Taylor Blau
2021-12-20 18:33     ` Derrick Stolee
2022-01-04 15:33       ` Taylor Blau
2021-12-14  1:55   ` [PATCH v2 6/8] t5326: move tests to t/lib-bitmap.sh Taylor Blau
2021-12-14  1:55   ` [PATCH v2 7/8] t/lib-bitmap.sh: parameterize tests over reverse index source Taylor Blau
2021-12-14  1:55   ` [PATCH v2 8/8] midx: read `RIDX` chunk when present Taylor Blau
2021-12-20 18:42     ` Derrick Stolee
2022-01-04 15:21       ` Taylor Blau
2021-12-15 19:46   ` [PATCH v2 0/8] midx: prevent bitmap corruption when permuting pack order Junio C Hamano
2021-12-15 21:37     ` Taylor Blau
2021-12-15 22:17       ` Junio C Hamano
2021-12-15 22:55         ` Junio C Hamano
2021-12-20 18:51     ` Derrick Stolee
2021-12-20 19:52       ` Taylor Blau
2021-12-20 20:09         ` Derrick Stolee
2021-12-15 22:58   ` Junio C Hamano
2021-12-15 23:01     ` Taylor Blau
2022-01-04 18:15 ` [PATCH v3 0/9] " Taylor Blau
2022-01-04 18:15   ` [PATCH v3 1/9] t5326: demonstrate bitmap corruption after permutation Taylor Blau
2022-01-20 17:55     ` Jonathan Tan
2022-01-20 22:11       ` Taylor Blau
2022-01-20 22:41         ` Junio C Hamano
2022-01-20 22:46           ` Taylor Blau
2022-01-24 17:40         ` Jonathan Tan
2022-01-04 18:15   ` [PATCH v3 2/9] midx.c: make changing the preferred pack safe Taylor Blau
2022-01-14 21:35     ` Junio C Hamano
2022-01-14 21:43       ` Junio C Hamano
2022-01-15  0:59         ` Taylor Blau
2022-01-15  6:27           ` Junio C Hamano
2022-01-20 18:08     ` Jonathan Tan
2022-01-20 22:13       ` Taylor Blau
2022-01-04 18:15   ` [PATCH v3 3/9] pack-revindex.c: instrument loading on-disk reverse index Taylor Blau
2022-01-20 18:15     ` Jonathan Tan
2022-01-20 22:18       ` Taylor Blau
2022-01-24 17:53         ` Jonathan Tan
2022-01-04 18:15   ` [PATCH v3 4/9] t5326: drop unnecessary setup Taylor Blau
2022-01-04 18:15   ` [PATCH v3 5/9] t5326: extract `test_rev_exists` Taylor Blau
2022-01-04 18:15   ` [PATCH v3 6/9] t5326: move tests to t/lib-bitmap.sh Taylor Blau
2022-01-04 18:15   ` [PATCH v3 7/9] t/lib-bitmap.sh: parameterize tests over reverse index source Taylor Blau
2022-01-24 19:15     ` Jonathan Tan
2022-01-25 21:40       ` Taylor Blau
2022-01-26 21:00         ` Jonathan Tan
2022-01-04 18:16   ` [PATCH v3 8/9] midx: read `RIDX` chunk when present Taylor Blau
2022-01-24 19:27     ` Jonathan Tan
2022-01-25 21:45       ` Taylor Blau
2022-01-26 21:28         ` Jonathan Tan
2022-01-04 18:16   ` [PATCH v3 9/9] pack-bitmap.c: gracefully fallback after opening pack/MIDX Taylor Blau
2022-01-24 19:29     ` Jonathan Tan
2022-01-25 21:46       ` Taylor Blau
2022-01-25 22:40 ` [PATCH v4 0/9] midx: prevent bitmap corruption when permuting pack order Taylor Blau
2022-01-25 22:41   ` [PATCH v4 1/9] t5326: demonstrate bitmap corruption after permutation Taylor Blau
2022-01-26 15:01     ` Ævar Arnfjörð Bjarmason
2022-01-26 20:18       ` Taylor Blau
2022-01-25 22:41   ` [PATCH v4 2/9] midx.c: make changing the preferred pack safe Taylor Blau
2022-01-25 22:41   ` [PATCH v4 3/9] pack-revindex.c: instrument loading on-disk reverse index Taylor Blau
2022-01-26 15:03     ` Ævar Arnfjörð Bjarmason
2022-01-25 22:41   ` [PATCH v4 4/9] t5326: drop unnecessary setup Taylor Blau
2022-01-25 22:41   ` [PATCH v4 5/9] t5326: extract `test_rev_exists` Taylor Blau
2022-01-26 15:04     ` Ævar Arnfjörð Bjarmason
2022-01-26 20:19       ` Taylor Blau
2022-01-25 22:41   ` [PATCH v4 6/9] t5326: move tests to t/lib-bitmap.sh Taylor Blau
2022-01-25 22:41   ` [PATCH v4 7/9] t/lib-bitmap.sh: parameterize tests over reverse index source Taylor Blau
2022-01-25 22:41   ` [PATCH v4 8/9] midx: read `RIDX` chunk when present Taylor Blau
2022-01-26 15:10     ` Ævar Arnfjörð Bjarmason
2022-01-26 20:23       ` Taylor Blau
2022-01-25 22:41   ` [PATCH v4 9/9] pack-bitmap.c: gracefully fallback after opening pack/MIDX Taylor Blau
2022-01-26 15:08     ` Ævar Arnfjörð Bjarmason
2022-01-26 17:50   ` [PATCH v4 0/9] midx: prevent bitmap corruption when permuting pack order Ævar Arnfjörð Bjarmason
2022-01-26 20:24     ` Taylor Blau
2022-01-27 17:15       ` Jonathan Tan
2022-02-24 22:50         ` Taylor Blau
2022-01-27 14:13   ` Derrick Stolee

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=YbPVP0BvYcVsfOrf@nand.local \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=stolee@gmail.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).