{"thread":{"id":"58688","subject":"[PATCH] midx.c: clear auxiliary MIDX files first","startedAt":"2022-10-25T18:25:27Z","lastAt":"2022-10-27T20:28:26Z","messageCount":5,"participants":["Taylor Blau","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465724","messageId":"bf36093cd6d7ac83b16241b0199b3a8c904e6774.1666722316.git.me@ttaylorr.com","threadId":"58688","inReplyTo":null,"subject":"[PATCH] midx.c: clear auxiliary MIDX files first","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-25T18:25:20Z","receivedAt":"2022-10-25T18:25:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since they were added in c528e17966 (pack-bitmap: write multi-pack\nbitmaps, 2021-08-31), the routine to remove MIDXs removed the\nmulti-pack-index file itself before removing its associated .bitmap and\n.rev file(s), if any.\n\nThis creates a window where a MIDX's .bitmap file exists without its\ncorresponding MIDX. If a reader tries to load a MIDX bitmap during that\ntime, they will get a warning, and the MIDX bitmap code will gracefully\ndegrade.\n\nRemove this window entirely by removing the MIDX last, and removing its\nauxiliary files first.\n\nThe order here is important, too. We remove the MIDX's .bitmap file\nahead of its .rev, since callers try and read the .bitmap first. The\n.rev file is no longer generated by modern versions of Git, but cleaning\nup old ones generated by previous versions of Git is still important to\ndo.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3a8dcfe98e..994129aecd 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1619,12 +1619,12 @@ void clear_midx_file(struct repository *r)\n \t\tr->objects->multi_pack_index = NULL;\n \t}\n \n-\tif (remove_path(midx.buf))\n-\t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx.buf);\n-\n \tclear_midx_files_ext(r->objects->odb->path, \".bitmap\", NULL);\n \tclear_midx_files_ext(r->objects->odb->path, \".rev\", NULL);\n \n+\tif (remove_path(midx.buf))\n+\t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx.buf);\n+\n \tstrbuf_release(&midx);\n }\n \n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465753","messageId":"Y1jIo1dPl0M2TSHo@coredump.intra.peff.net","threadId":"58688","inReplyTo":"bf36093cd6d7ac83b16241b0199b3a8c904e6774.1666722316.git.me@ttaylorr.com","subject":"Re: [PATCH] midx.c: clear auxiliary MIDX files first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-26T05:41:55Z","receivedAt":"2022-10-26T05:42:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2022 at 02:25:20PM -0400, Taylor Blau wrote:\n\n> Since they were added in c528e17966 (pack-bitmap: write multi-pack\n> bitmaps, 2021-08-31), the routine to remove MIDXs removed the\n> multi-pack-index file itself before removing its associated .bitmap and\n> .rev file(s), if any.\n> \n> This creates a window where a MIDX's .bitmap file exists without its\n> corresponding MIDX. If a reader tries to load a MIDX bitmap during that\n> time, they will get a warning, and the MIDX bitmap code will gracefully\n> degrade.\n> \n> Remove this window entirely by removing the MIDX last, and removing its\n> auxiliary files first.\n\nWe remove that window, but don't we create a new one where a reader may\nsee the midx but not the bitmap? That won't generate a warning (it just\nlooks like a midx that never had a bitmap generated), but it will cause\nthe reader to follow the slow, non-bitmap path.\n\nIdeally this would just be atomic, but short of stuffing the metadata\ninto the same file, we can't do that. But the replacement of the midx\nfile itself is atomic, and I'd think everything would (or should at\nleast) follow from there.\n\nI.e., why are we reading the midx bitmap without having opened the midx\nfile? Ideally we'd be holding a descriptor for it.\n\n-Peff\n"},{"id":"465763","messageId":"143a588a-c98b-733b-2b23-34a87ca89431@github.com","threadId":"58688","inReplyTo":"Y1jIo1dPl0M2TSHo@coredump.intra.peff.net","subject":"Re: [PATCH] midx.c: clear auxiliary MIDX files first","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-10-26T13:31:28Z","receivedAt":"2022-10-26T13:31:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/26/22 1:41 AM, Jeff King wrote:\n> On Tue, Oct 25, 2022 at 02:25:20PM -0400, Taylor Blau wrote:\n> \n>> Since they were added in c528e17966 (pack-bitmap: write multi-pack\n>> bitmaps, 2021-08-31), the routine to remove MIDXs removed the\n>> multi-pack-index file itself before removing its associated .bitmap and\n>> .rev file(s), if any.\n>>\n>> This creates a window where a MIDX's .bitmap file exists without its\n>> corresponding MIDX. If a reader tries to load a MIDX bitmap during that\n>> time, they will get a warning, and the MIDX bitmap code will gracefully\n>> degrade.\n>>\n>> Remove this window entirely by removing the MIDX last, and removing its\n>> auxiliary files first.\n> \n> We remove that window, but don't we create a new one where a reader may\n> see the midx but not the bitmap? That won't generate a warning (it just\n> looks like a midx that never had a bitmap generated), but it will cause\n> the reader to follow the slow, non-bitmap path.\n\nYes, this is the worrisome direction. The midx is read first, then that\npoints to the .bitmap file (based on its trailing hash). If the midx isn't\nthere, then the .bitmap will not be read.\n\n> Ideally this would just be atomic, but short of stuffing the metadata\n> into the same file, we can't do that. But the replacement of the midx\n> file itself is atomic, and I'd think everything would (or should at\n> least) follow from there.\n\nThe interesting case here is that this is in clear_midx_file(), which\nis called when repacking to a single pack and no longer needing a midx\nfile. So it's not using the atomic rewrite from the midx writing code,\nbut instead the \"atomic\" deletion.\n\nIn this case, a reader will check for the midx first, before looking\nfor individual packs. Further, the new pack is written, but the old\npacks have not been deleted (or the midx would be invalid). So the\nnew code introduces the window where a midx exists without a bitmap,\nso some readers will act as if no bitmap exists on-disk.\n\nThis was always possible before, too: the midx could be read by a\nreader process before the repack process deletes that file. However,\nif the reader does not also gain a handle on the corresponding\n.bitmap before the repack process deletes that file, too, then the\nreader is also left thinking that no .bitmap exists.\n\nI think the old code is more correct, here. The window is slightly\nsmaller, and the new code creates a window where the filesystem\ndoesn't need to change for readers to get an imperfect view of\nthings.\n\nAside: in these cases where a .bitmap file is not found for a midx,\ndo we fall back to trying to find a .bitmap file for a pack-file?\nThat would assuage most of the concerns here about what happens in\nthis window where the repack has a new .pack/.bitmap pair but the\nold midx still exists (without a .bitmap, depending on timing).\n\nThanks,\n-Stolee\n\n"},{"id":"465823","messageId":"Y1mRr04M9+5wUs3q@nand.local","threadId":"58688","inReplyTo":"143a588a-c98b-733b-2b23-34a87ca89431@github.com","subject":"Re: [PATCH] midx.c: clear auxiliary MIDX files first","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T19:59:43Z","receivedAt":"2022-10-26T19:59:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 26, 2022 at 09:31:28AM -0400, Derrick Stolee wrote:\n> On 10/26/22 1:41 AM, Jeff King wrote:\n> > On Tue, Oct 25, 2022 at 02:25:20PM -0400, Taylor Blau wrote:\n> >\n> >> Since they were added in c528e17966 (pack-bitmap: write multi-pack\n> >> bitmaps, 2021-08-31), the routine to remove MIDXs removed the\n> >> multi-pack-index file itself before removing its associated .bitmap and\n> >> .rev file(s), if any.\n> >>\n> >> This creates a window where a MIDX's .bitmap file exists without its\n> >> corresponding MIDX. If a reader tries to load a MIDX bitmap during that\n> >> time, they will get a warning, and the MIDX bitmap code will gracefully\n> >> degrade.\n> >>\n> >> Remove this window entirely by removing the MIDX last, and removing its\n> >> auxiliary files first.\n> >\n> > We remove that window, but don't we create a new one where a reader may\n> > see the midx but not the bitmap? That won't generate a warning (it just\n> > looks like a midx that never had a bitmap generated), but it will cause\n> > the reader to follow the slow, non-bitmap path.\n>\n> Yes, this is the worrisome direction. The midx is read first, then that\n> points to the .bitmap file (based on its trailing hash). If the midx isn't\n> there, then the .bitmap will not be read.\n\nYes, thinking on it more I agree with this and (the elided) analysis\nbelow.\n\nLet's drop this one. Thanks, both!\n\n\nThanks,\nTaylor\n"},{"id":"465906","messageId":"Y1rp4d4CXrtnxP9R@coredump.intra.peff.net","threadId":"58688","inReplyTo":"143a588a-c98b-733b-2b23-34a87ca89431@github.com","subject":"Re: [PATCH] midx.c: clear auxiliary MIDX files first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-27T20:28:17Z","receivedAt":"2022-10-27T20:28:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2022 at 09:31:28AM -0400, Derrick Stolee wrote:\n\n> This was always possible before, too: the midx could be read by a\n> reader process before the repack process deletes that file. However,\n> if the reader does not also gain a handle on the corresponding\n> .bitmap before the repack process deletes that file, too, then the\n> reader is also left thinking that no .bitmap exists.\n\nGood point. Neither the writer _nor_ the reader is atomic. :)\n\n> I think the old code is more correct, here. The window is slightly\n> smaller, and the new code creates a window where the filesystem\n> doesn't need to change for readers to get an imperfect view of\n> things.\n\nYeah, I agree that the old code is nicer in that the window is a little\nsmaller.\n\nDo we want to do something about the warning, though? Falling back to a\nslow path sucks, of course, but racily generating user-visible warnings\nfor something that is not actually a problem is also not great.\n\n> Aside: in these cases where a .bitmap file is not found for a midx,\n> do we fall back to trying to find a .bitmap file for a pack-file?\n> That would assuage most of the concerns here about what happens in\n> this window where the repack has a new .pack/.bitmap pair but the\n> old midx still exists (without a .bitmap, depending on timing).\n\nYes, we should. Code paths generally go through open_bitmap(), which\ntries the midx first, then looks for pack bitmaps.\n\nAnd in that sense, the race after this patch is fairly harmless. We fail\nto see the midx bitmap, but we'll see the pack one (which must have been\ncreated before we deleted the midx one, assuming this is a \"git repack\n-adb\" or equivalent).\n\nIs that also true of the race before this patch? I'm not sure which\nwarning is being generated, but if it's in open_midx_bitmap_1(), then\nthe same logic would apply.\n\n-Peff\n"}]}