{"thread":{"id":"58083","subject":"Race condition between repack and loose-objects maintenance task","startedAt":"2022-06-29T16:56:10Z","lastAt":"2022-07-26T18:18:32Z","messageCount":16,"participants":["Gregory Szorc","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"458105","messageId":"CAKQoGamCrRMqtzziuzi8mL6E7uA3SC1WXiMGT_4rpbk1jcu_OQ@mail.gmail.com","threadId":"58083","inReplyTo":null,"subject":"Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-06-29T16:55:54Z","receivedAt":"2022-06-29T16:56:10Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"`git repack -A` (such as invoked by `git gc`) loosens unreachable\nobjects from packfiles.\n\n`git maintenance`'s `loose-objects` task consolidates loose objects\ninto a packfile.\n\nAs neither process takes a lock or is otherwise aware of the existence\nof the other, there is a race condition:\n\n1. `git repack -A` creates loose objects\n2. `git maintenance`'s `loose-objects` task deletes those loose objects\n3. `git repack -A` fails to find the loose objects it just created and\naborts with `fatal: unable to add recent objects`\n\nI see this failure with regularity in a custom environment where high\nmutation volume repositories invoke `git maintenance run` with\nregularity (effectively in reaction to repo mutations). We have a Git\nhousekeeping strategy that frequently invokes `git maintenance` with\nall but the `gc` task. On a ~daily frequency, we invoke `git gc` to\npurge accumulated garbage. This `git gc` frequently fails due to the\naforementioned race condition since the long wall time of `git gc`\npractically guarantees a `git maintenance` would have been triggered\nsince the repo is mutated with such high frequency.\n\nIn my scenario, I believe we have a workaround by omitting\n`--task=loose-objects` from the frequent `git maintenance` invocation\nif there exists a `gc.pid` file. However, this is only a partial\nsolution, as `git repack` and the `loose-objects` task aren't aware of\neach other. (I suppose running the `gc` maintenance task and relying\non the maintenance lock file could also work. However, we're invoking\n`git gc` explicitly since `git maintenance` doesn't allow passing\ncustom arguments through to `git gc`.)\n\nGregory\n"},{"id":"458109","messageId":"YryF+vkosJOXf+mQ@nand.local","threadId":"58083","inReplyTo":"CAKQoGamCrRMqtzziuzi8mL6E7uA3SC1WXiMGT_4rpbk1jcu_OQ@mail.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T17:03:54Z","receivedAt":"2022-06-29T17:04:00Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Greg,\n\nOn Wed, Jun 29, 2022 at 09:55:54AM -0700, Gregory Szorc wrote:\n> 1. `git repack -A` creates loose objects\n> 2. `git maintenance`'s `loose-objects` task deletes those loose objects\n> 3. `git repack -A` fails to find the loose objects it just created and\n> aborts with `fatal: unable to add recent objects`\n\nThis is a somewhat well-known race that occurs when one Git process\ndecides unreachable objects are safe to be deleted, but an incoming push\nor reference update makes those to-be-deleted objects reachable before\nthey are actually removed, leaving the repository in a corrupt state.\n\nI'm surprised that the loose-objects maintenance task deletes those\nobjects, though, since it just runs `prune-packed` or (the equivalent\nof) `repack -d`, neither of which will actually delete objects from the\nrepository.\n\nI see that Stolee is already on the CC list, so perhaps he could chime\nin on the above.\n\nIn either case, my recommendation would be to keep those unreachable\nobjects which haven't yet aged out of the repository around for longer,\nwhich will decrease the likelihood of seeing the race. If your\nrepository has a lot of unreachable objects (such that storing each\none of them individually as loose is impractical), I would recommend\nusing cruft packs (e.g., by running either `git repack --cruft -d` or\n`git gc --cruft`) to collect those unreachable objects together into a\nsingle pack.\n\nSee Documentation/technical/cruft-packs.txt for more information about\ncruft packs.\n\nThanks,\nTaylor\n"},{"id":"458110","messageId":"YryHoXhmkvkl6X2j@nand.local","threadId":"58083","inReplyTo":"YryF+vkosJOXf+mQ@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T17:10:57Z","receivedAt":"2022-06-29T17:11:04Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 29, 2022 at 01:03:54PM -0400, Taylor Blau wrote:\n> I see that Stolee is already on the CC list, so perhaps he could chime\n> in on the above.\n\nI haven't looked at the maintenance code too closely as of yet, but I\nhave a plausible explanation for why maintenance is removing loose\n*unreachable* objects.\n\nOrdinarily the loose-objects task alone will not remove any loose\nobjects which don't already appear in a pack. That's because we first\ntry to run prune-packed, and then the equivalent of `git repack -d` to\nconsolidate loose objects together into a single pack.\n\nBut, we only do that for the first 50,000 loose objects. Anything else\ngets left around and then is likely removed by the `git gc` task which\nis run separately, and after the loose-objects task.\n\nUsing cruft packs won't change the race fundamentally, but storing the\nunreachable objects in a cruft pack will mean that you can keep more\nobjects around stored together in a single pack. In other words, making\nit safe to increase the grace period beyond what you would ordinarily be\nable to do with the classic pruning mechanism.\n\nThanks,\nTaylor\n"},{"id":"458111","messageId":"CAKQoGakSFaNm10ZeTKc8XtTcD0JW19CZP1OwA4j7W__iBQaJfg@mail.gmail.com","threadId":"58083","inReplyTo":"YryF+vkosJOXf+mQ@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-06-29T17:16:38Z","receivedAt":"2022-06-29T17:16:53Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"On Wed, Jun 29, 2022 at 10:03 AM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> Hi Greg,\n>\n> On Wed, Jun 29, 2022 at 09:55:54AM -0700, Gregory Szorc wrote:\n> > 1. `git repack -A` creates loose objects\n> > 2. `git maintenance`'s `loose-objects` task deletes those loose objects\n> > 3. `git repack -A` fails to find the loose objects it just created and\n> > aborts with `fatal: unable to add recent objects`\n>\n> This is a somewhat well-known race that occurs when one Git process\n> decides unreachable objects are safe to be deleted, but an incoming push\n> or reference update makes those to-be-deleted objects reachable before\n> they are actually removed, leaving the repository in a corrupt state.\n>\n> I'm surprised that the loose-objects maintenance task deletes those\n> objects, though, since it just runs `prune-packed` or (the equivalent\n> of) `repack -d`, neither of which will actually delete objects from the\n> repository.\n>\n> I see that Stolee is already on the CC list, so perhaps he could chime\n> in on the above.\n>\n> In either case, my recommendation would be to keep those unreachable\n> objects which haven't yet aged out of the repository around for longer,\n> which will decrease the likelihood of seeing the race.\n\nWe had to lower gc.pruneExpire from its default of 1 hour because\notherwise this would result in the creation of thousands of loose\nobjects. This is normally acceptable. However, on NFS, the churn from\nlots of file creation and deletion resulted in acceptable performance\ndegradation. We had to lower gc.pruneExpire to minimize the damage.\n\n> If your\n> repository has a lot of unreachable objects (such that storing each\n> one of them individually as loose is impractical), I would recommend\n> using cruft packs (e.g., by running either `git repack --cruft -d` or\n> `git gc --cruft`) to collect those unreachable objects together into a\n> single pack.\n>\n> See Documentation/technical/cruft-packs.txt for more information about\n> cruft packs.\n\nYes, I saw this new feature in the Git release this week and am hoping\nto roll it out to better mitigate this general problem! With cruft\npacks, I anticipate being able to restore a longer gc.pruneExpire\nvalue as they should mitigate the NFS performance badness we're\nworking around. Thank you for confirming it should help with the\nproblem.\n"},{"id":"458112","messageId":"YryKCl5J1Em89d3e@nand.local","threadId":"58083","inReplyTo":"CAKQoGakSFaNm10ZeTKc8XtTcD0JW19CZP1OwA4j7W__iBQaJfg@mail.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T17:21:14Z","receivedAt":"2022-06-29T17:21:22Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 29, 2022 at 10:16:38AM -0700, Gregory Szorc wrote:\n> > In either case, my recommendation would be to keep those unreachable\n> > objects which haven't yet aged out of the repository around for longer,\n> > which will decrease the likelihood of seeing the race.\n>\n> We had to lower gc.pruneExpire from its default of 1 hour because\n> otherwise this would result in the creation of thousands of loose\n> objects. This is normally acceptable. However, on NFS, the churn from\n> lots of file creation and deletion resulted in acceptable performance\n> degradation. We had to lower gc.pruneExpire to minimize the damage.\n\nMakes sense. Having a shorter gc.pruneExpire makes the race much more\nlikely to occur in practice, so I'm glad that we have a plausible\nconfirmation that that's what's going on in your repository.\n\n> > See Documentation/technical/cruft-packs.txt for more information about\n> > cruft packs.\n>\n> Yes, I saw this new feature in the Git release this week and am hoping\n> to roll it out to better mitigate this general problem! With cruft\n> packs, I anticipate being able to restore a longer gc.pruneExpire\n> value as they should mitigate the NFS performance badness we're\n> working around. Thank you for confirming it should help with the\n> problem.\n\nThat should definitely help. Let me know if you have any questions, and\nthanks for trying it out!\n\nThanks,\nTaylor\n"},{"id":"458142","messageId":"CAKQoGanPBec6wRO6uWrETaoJXdszpjRWytXaJwx6jw0mrrj-gQ@mail.gmail.com","threadId":"58083","inReplyTo":"YryKCl5J1Em89d3e@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-06-30T00:44:25Z","receivedAt":"2022-06-30T00:44:41Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"On Wed, Jun 29, 2022 at 10:21 AM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Jun 29, 2022 at 10:16:38AM -0700, Gregory Szorc wrote:\n> > > In either case, my recommendation would be to keep those unreachable\n> > > objects which haven't yet aged out of the repository around for longer,\n> > > which will decrease the likelihood of seeing the race.\n> >\n> > We had to lower gc.pruneExpire from its default of 1 hour because\n> > otherwise this would result in the creation of thousands of loose\n> > objects. This is normally acceptable. However, on NFS, the churn from\n> > lots of file creation and deletion resulted in acceptable performance\n> > degradation. We had to lower gc.pruneExpire to minimize the damage.\n>\n> Makes sense. Having a shorter gc.pruneExpire makes the race much more\n> likely to occur in practice, so I'm glad that we have a plausible\n> confirmation that that's what's going on in your repository.\n>\n> > > See Documentation/technical/cruft-packs.txt for more information about\n> > > cruft packs.\n> >\n> > Yes, I saw this new feature in the Git release this week and am hoping\n> > to roll it out to better mitigate this general problem! With cruft\n> > packs, I anticipate being able to restore a longer gc.pruneExpire\n> > value as they should mitigate the NFS performance badness we're\n> > working around. Thank you for confirming it should help with the\n> > problem.\n>\n> That should definitely help. Let me know if you have any questions, and\n> thanks for trying it out!\n\nRevising my initial post, not running `loose-objects` is insufficient:\nwe also need to prevent `incremental-repack` from running while a `git\ngc` / `git repack` is in progress. If an `incremental-repack` runs\nconcurrently with `git repack`, `repack` can error after bitmap\ngeneration but before the temp packfile is renamed with \"could not\nfind 'pack-*.pack'.\" I suspect this has to do with\n`incremental-repack` deleting packs that were initially in place when\n`git repack` started running. But I haven't looked into where exactly\nrepack is failing.\n\nI deployed Git 2.37 and enabled cruft packs via `gc.cruftPacks=true`\nin the global gitconfig. I simultaneously increased `gc.pruneExpire`\nto 1 day (as we're no longer concerned about each unreferenced object\nturning into a single file). And I changed the frequently-invoked `git\nmaintenance` code to not run `loose-objects` or `incremental-repack`\nif a gc.pid file is present (this is still racy but should hopefully\nwork enough of the time). This appeared to work: `git gc` created a\ncruft pack and didn't complain about anything disappearing out from\nunder it, despite `git maintenance` running `commit-graph` and\n`pack-refs` tasks during the long-running `git gc` / `git repack`.\n\nHowever, I think there is yet another bug at play: running\n`incremental-repack` appears to be able to repack the cruft packfile.\nIn doing so, it deletes its .mtimes file and the metadata inside. This\nbehavior seems wrong to me: I was expecting the cruft packfile to\nlinger until the next `git gc` and/or for its mtimes metadata to be\npreserved across future repacks. Otherwise, what is the point in\nretaining granular mtime metadata? Is this the behavior expected? Or\ndo cruft packfiles just not work as intended alongside use of the\n`incremental-task` maintenance task / multi-pack index in Git 2.37?\n"},{"id":"458155","messageId":"Yr0WLebMfBXZ1K7D@nand.local","threadId":"58083","inReplyTo":"CAKQoGanPBec6wRO6uWrETaoJXdszpjRWytXaJwx6jw0mrrj-gQ@mail.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-30T03:19:09Z","receivedAt":"2022-06-30T03:19:14Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 29, 2022 at 05:44:25PM -0700, Gregory Szorc wrote:\n> Revising my initial post, not running `loose-objects` is insufficient:\n> we also need to prevent `incremental-repack` from running while a `git\n> gc` / `git repack` is in progress. If an `incremental-repack` runs\n> concurrently with `git repack`, `repack` can error after bitmap\n> generation but before the temp packfile is renamed with \"could not\n> find 'pack-*.pack'.\" I suspect this has to do with\n> `incremental-repack` deleting packs that were initially in place when\n> `git repack` started running. But I haven't looked into where exactly\n> repack is failing.\n\nYeah, you would need to prevent other writers from removing packs while\nwriting a cruft pack.\n\nI think the canonical way to do this would be to let `git\nmaintenance` use its own locking to ensure that it wasn't trying to\nremove packs while simultaneously generating a cruft pack.\n\nBut assuming that the cruft pack generation is running independently and\nconcurrently with the maintenance incremental-repack task (which will\ndelete packs in the background), then there is definitely a TOCTOU race\nthere.\n\nThe race is that `repack` will signal to `pack-objects` which packs will\nstay and which packs will be deleted during the repack. To generate a\ncruft pack, the packs that stay are formed by doing a reachability\ntraversal and writing a pack that contains all reachable objects. That\nway anything that is in `all-packs \\ reachable` (which form the contents\nof the cruft pack) are going to be just the unreachable objects.\n\nBut if a pack that was going to be deleted by repack *after* generating\nthe cruft pack disappears while `repack` is running (particularly after\nit starts, but before it generates the cruft pack), then the cruft pack\ngeneration can't proceed, since it has no idea what objects it needs to\nadd.\n\nNamely, if it is told that pack P is going to be deleted, any objects in\nP which don't appear in a pack that *isn't* going to be deleted need to\nbe saved. If we don't have a copy of P anymore, then there is no way to\ncome up with what that set of objects is, meaning that it's impossible\nto generate the cruft pack.\n\nI should mention, BTW, that this isn't a problem unique to cruft packs.\nGeometric repacking, which uses `pack-objects --stdin-packs` has a\nsimilar issue where if a pack being combined is removed from underneath\nwhile `repack` is running, `pack-objects` cannot successfully generate\nthe pack.\n\n> However, I think there is yet another bug at play: running\n> `incremental-repack` appears to be able to repack the cruft packfile.\n> In doing so, it deletes its .mtimes file and the metadata inside.\n\nThat sounds like a bug to me. I'll take a look into it and see what I\ncan find.\n\nThanks,\nTaylor\n"},{"id":"458156","messageId":"Yr0XMWWyD5C9uhlb@nand.local","threadId":"58083","inReplyTo":"Yr0WLebMfBXZ1K7D@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-30T03:23:29Z","receivedAt":"2022-06-30T03:23:37Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n> > However, I think there is yet another bug at play: running\n> > `incremental-repack` appears to be able to repack the cruft packfile.\n> > In doing so, it deletes its .mtimes file and the metadata inside.\n>\n> That sounds like a bug to me. I'll take a look into it and see what I\n> can find.\n\nI actually think that there are two bugs here.\n\nOne is that we run a MIDX repack and expire, which could lead to us\nrepacking the entire contents of the cruft pack and then expiring the\nmetadata file. This is a bug, and we should exclude cruft packs from\nthis computation.\n\nAnother bug can happen when the cruft pack gets written into the MIDX\nand is MIDX-expireable (meaning that no objects are selected from the\ncruft pack). In that case, the `git multi-pack-index expire` step would\nremove the cruft pack entirely, which is also incorrect.\n\nI'll take a look at fixing both of these, and thanks for pointing them\nout!\n\nThanks,\nTaylor\n"},{"id":"458244","messageId":"xmqqletdewrf.fsf@gitster.g","threadId":"58083","inReplyTo":"Yr0XMWWyD5C9uhlb@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-30T20:12:20Z","receivedAt":"2022-06-30T20:12:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n>> > However, I think there is yet another bug at play: running\n>> > `incremental-repack` appears to be able to repack the cruft packfile.\n>> > In doing so, it deletes its .mtimes file and the metadata inside.\n>>\n>> That sounds like a bug to me. I'll take a look into it and see what I\n>> can find.\n>\n> I actually think that there are two bugs here.\n>\n> One is that we run a MIDX repack and expire, which could lead to us\n> repacking the entire contents of the cruft pack and then expiring the\n> metadata file. This is a bug, and we should exclude cruft packs from\n> this computation.\n>\n> Another bug can happen when the cruft pack gets written into the MIDX\n> and is MIDX-expireable (meaning that no objects are selected from the\n> cruft pack). In that case, the `git multi-pack-index expire` step would\n> remove the cruft pack entirely, which is also incorrect.\n>\n> I'll take a look at fixing both of these, and thanks for pointing them\n> out!\n\nThanks, both.\n\nThe fact that the semantics of the .mtimes file being not equivalent\nto the mtime on individual loose objects does not help thinking\nabout the possible ways the \"cruft\" pack can break, and both of the\npossible issues you discuss above are indeed tricky ones.\n\n\n"},{"id":"458492","messageId":"CAKQoGakLzF+bvSnU7JD7yBibmYe0-8j3eo+CgN7EchUiNc2NkA@mail.gmail.com","threadId":"58083","inReplyTo":"xmqqletdewrf.fsf@gitster.g","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-07-05T18:43:34Z","receivedAt":"2022-07-05T18:43:50Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"Thinking about this some more, it is clear that running `git gc` /\n`git repack` *and* `git maintenance` simultaneously is prone to\nbadness since their locking mechanisms aren't aware of the other.\n\nI think it makes sense to consolidate onto `git maintenace` going\nforward as this seems to be where the inertia is. However, a barrier\nto that for me is the objects/maintenance.lock file has no acquisition\ntimeout and will wait indefinitely. So in scenarios like mine where\nyou have multiple processes looping over repos invoking `git\nmaintenance run`, you can have extended execution stalls during\nlong-running operations like `gc`.\n\nThere should probably be a configurable timeout for the maintenance\nlock like there is for other locks. This shouldn't be too much work\nand I may throw up a patch if others feel this is a good idea.\n\nOn Thu, Jun 30, 2022 at 1:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n> >> > However, I think there is yet another bug at play: running\n> >> > `incremental-repack` appears to be able to repack the cruft packfile.\n> >> > In doing so, it deletes its .mtimes file and the metadata inside.\n> >>\n> >> That sounds like a bug to me. I'll take a look into it and see what I\n> >> can find.\n> >\n> > I actually think that there are two bugs here.\n> >\n> > One is that we run a MIDX repack and expire, which could lead to us\n> > repacking the entire contents of the cruft pack and then expiring the\n> > metadata file. This is a bug, and we should exclude cruft packs from\n> > this computation.\n> >\n> > Another bug can happen when the cruft pack gets written into the MIDX\n> > and is MIDX-expireable (meaning that no objects are selected from the\n> > cruft pack). In that case, the `git multi-pack-index expire` step would\n> > remove the cruft pack entirely, which is also incorrect.\n> >\n> > I'll take a look at fixing both of these, and thanks for pointing them\n> > out!\n>\n> Thanks, both.\n>\n> The fact that the semantics of the .mtimes file being not equivalent\n> to the mtime on individual loose objects does not help thinking\n> about the possible ways the \"cruft\" pack can break, and both of the\n> possible issues you discuss above are indeed tricky ones.\n>\n>\n"},{"id":"458513","messageId":"220706.86bku2a9px.gmgdl@evledraar.gmail.com","threadId":"58083","inReplyTo":"CAKQoGakLzF+bvSnU7JD7yBibmYe0-8j3eo+CgN7EchUiNc2NkA@mail.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-06T08:50:56Z","receivedAt":"2022-07-06T09:13:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 05 2022, Gregory Szorc wrote:\n\n> Thinking about this some more, it is clear that running `git gc` /\n> `git repack` *and* `git maintenance` simultaneously is prone to\n> badness since their locking mechanisms aren't aware of the other.\n\nYes, there are outstanding issues with the \"maintenance\" and \"gc\" locks,\nthey should be unified, and the gc.lock has bugs & race conditions that\nneed to be fixed. See these past threads:\n\n    https://lore.kernel.org/git/87d02fi75p.fsf@evledraar.gmail.com/\n    https://lore.kernel.org/git/87ef3o7ws1.fsf@evledraar.gmail.com/\n\nI also have some old WIP patches to add a \"gc testing\" mode to our test\nsuite, similar to the \"commit graph\" instrumentation if you're\ninterested in working on this.\n\nI.e. almost every command will fork off a \"gi gc --auto\", and we'll see\nbased on the errors whether our locking is still buggy/racy.\n\n> I think it makes sense to consolidate onto `git maintenace` going\n> forward as this seems to be where the inertia is. However, a barrier\n> to that for me is the objects/maintenance.lock file has no acquisition\n> timeout and will wait indefinitely. So in scenarios like mine where\n> you have multiple processes looping over repos invoking `git\n> maintenance run`, you can have extended execution stalls during\n> long-running operations like `gc`.\n> [...]\n> There should probably be a configurable timeout for the maintenance\n> lock like there is for other locks. This shouldn't be too much work\n> and I may throw up a patch if others feel this is a good idea.\n\n...but while I think this is all worthwhile I think you're on the wrong\npath if you think this will help much or at all with the issue being\nreported here.\n\nFixing the gc.lock (and maintenance lock...) would be nice because we'd\nhave \"gc\" be less dumb, and it wouldn't get itself into lock races etc.\n\nBut you cannot hope to fix the underlying \"object expiry as things\nbecome reachable\" race condition that way, because that's those tasks\nracing with *other* object/ref creation.\n\nSuch a fix would either need some repository-global lock (which would\nentail all sorts of nastyness), or other workarounds around the inherent\nrace between different object store & ref operations, see Taylor's\nrecent \"cruft pack write-out\" series & my links to some past discussions\nof the race here:\n\n    https://lore.kernel.org/git/220630.86y1xeeeik.gmgdl@evledraar.gmail.com/\n"},{"id":"459491","messageId":"da675dd2-c9cf-c3b4-0231-58b3cf3ce9d7@gmail.com","threadId":"58083","inReplyTo":"Yr0XMWWyD5C9uhlb@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-07-20T01:40:19Z","receivedAt":"2022-07-20T01:42:47Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"On 6/29/2022 8:23 PM, Taylor Blau wrote:\n> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n>>> However, I think there is yet another bug at play: running\n>>> `incremental-repack` appears to be able to repack the cruft packfile.\n>>> In doing so, it deletes its .mtimes file and the metadata inside.\n>>\n>> That sounds like a bug to me. I'll take a look into it and see what I\n>> can find.\n> \n> I actually think that there are two bugs here.\n> \n> One is that we run a MIDX repack and expire, which could lead to us\n> repacking the entire contents of the cruft pack and then expiring the\n> metadata file. This is a bug, and we should exclude cruft packs from\n> this computation.\n> \n> Another bug can happen when the cruft pack gets written into the MIDX\n> and is MIDX-expireable (meaning that no objects are selected from the\n> cruft pack). In that case, the `git multi-pack-index expire` step would\n> remove the cruft pack entirely, which is also incorrect.\n> \n> I'll take a look at fixing both of these, and thanks for pointing them\n> out!\n\nFor posterity, when I disabled cruft packfiles after having it enabled \nfor a few weeks, the next `git gc` invocation on a high traffic repo \nresulted in >100k loose objects/files being created before they were \nsummarily deleted by the GC's prune. This is significantly greater than \nthe unreferenced object creation rate of the underlying repo. So it \nappears as if the MIDX stripping of the cruft packfile mtimes\neffectively disabled pruning, leading to a build-up of unreferenced objects.\n\nFortunately I hadn't deployed cruft packfiles to production. If I had, \nthe excessive filesystem churn on NFS would have caused an incident due \nto degraded performance.\n\nSince the interaction between cruft packfiles and MIDX appears to be \nbuggy, I think I'm going to leave cruft packfiles disabled until these \nfeatures work better together.\n\n\n"},{"id":"459492","messageId":"CAKQoGakg9+cvsHSepXyWFT9Jzg0_anq-YDBNZch95TnUAZ-Ohg@mail.gmail.com","threadId":"58083","inReplyTo":"Yr0XMWWyD5C9uhlb@nand.local","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-07-20T01:41:13Z","receivedAt":"2022-07-20T01:43:03Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"On Wed, Jun 29, 2022 at 8:23 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n> > > However, I think there is yet another bug at play: running\n> > > `incremental-repack` appears to be able to repack the cruft packfile.\n> > > In doing so, it deletes its .mtimes file and the metadata inside.\n> >\n> > That sounds like a bug to me. I'll take a look into it and see what I\n> > can find.\n>\n> I actually think that there are two bugs here.\n>\n> One is that we run a MIDX repack and expire, which could lead to us\n> repacking the entire contents of the cruft pack and then expiring the\n> metadata file. This is a bug, and we should exclude cruft packs from\n> this computation.\n>\n> Another bug can happen when the cruft pack gets written into the MIDX\n> and is MIDX-expireable (meaning that no objects are selected from the\n> cruft pack). In that case, the `git multi-pack-index expire` step would\n> remove the cruft pack entirely, which is also incorrect.\n>\n> I'll take a look at fixing both of these, and thanks for pointing them\n> out!\n\nFor posterity, when I disabled cruft packfiles after having it enabled\nfor a few weeks, the next `git gc` invocation on a high traffic repo\nresulted in >100k loose objects/files being created before they were\nsummarily deleted by the GC's prune. This is significantly greater\nthan the unreferenced object creation rate of the underlying repo. So\nit appears as if the MIDX stripping of the cruft packfile mtimes\neffectively disabled pruning, leading to a build-up of unreferenced objects.\n\nFortunately I hadn't deployed cruft packfiles to production. If I had,\nthe excessive filesystem churn on NFS would have caused an incident\ndue to degraded performance.\n\nSince the interaction between cruft packfiles and MIDX appears to be\nbuggy, I think I'm going to leave cruft packfiles disabled until these\nfeatures work better together.\n"},{"id":"459500","messageId":"220720.86cze0um88.gmgdl@evledraar.gmail.com","threadId":"58083","inReplyTo":"da675dd2-c9cf-c3b4-0231-58b3cf3ce9d7@gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-20T09:52:11Z","receivedAt":"2022-07-20T10:12:18Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 19 2022, Gregory Szorc wrote:\n\n> On 6/29/2022 8:23 PM, Taylor Blau wrote:\n>> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n>>>> However, I think there is yet another bug at play: running\n>>>> `incremental-repack` appears to be able to repack the cruft packfile.\n>>>> In doing so, it deletes its .mtimes file and the metadata inside.\n>>>\n>>> That sounds like a bug to me. I'll take a look into it and see what I\n>>> can find.\n>> I actually think that there are two bugs here.\n>> One is that we run a MIDX repack and expire, which could lead to us\n>> repacking the entire contents of the cruft pack and then expiring the\n>> metadata file. This is a bug, and we should exclude cruft packs from\n>> this computation.\n>> Another bug can happen when the cruft pack gets written into the\n>> MIDX\n>> and is MIDX-expireable (meaning that no objects are selected from the\n>> cruft pack). In that case, the `git multi-pack-index expire` step would\n>> remove the cruft pack entirely, which is also incorrect.\n>> I'll take a look at fixing both of these, and thanks for pointing\n>> them\n>> out!\n>\n> For posterity, when I disabled cruft packfiles after having it enabled\n> for a few weeks, the next `git gc` invocation on a high traffic repo \n> resulted in >100k loose objects/files being created before they were\n> summarily deleted by the GC's prune. This is significantly greater\n> than the unreferenced object creation rate of the underlying repo. So\n> it appears as if the MIDX stripping of the cruft packfile mtimes\n> effectively disabled pruning, leading to a build-up of unreferenced objects.\n>\n> Fortunately I hadn't deployed cruft packfiles to production. If I had,\n> the excessive filesystem churn on NFS would have caused an incident\n> due to degraded performance.\n>\n> Since the interaction between cruft packfiles and MIDX appears to be\n> buggy, I think I'm going to leave cruft packfiles disabled until these \n> features work better together.\n\nI haven't looked deeply into whether there's any cruft packfile & MIDX\ninteraction at play here, but I suspect based on past experience that\nthis has nothing whatsoever to do with the MIDX.\n\nIt's because git suddenly found a bunch of objects that should be\nevicted from the packs, as you disabled the cruft pack feature.\n\nHere's a past test-case & report of mine where I ran into that:\n\n    https://lore.kernel.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n\nThat was way before cruft packfiles were integrated, but from my\nunderstanding of how they work the mechanism is exactly the same.\n\nI.e. in that case I deleted a bunch of refs (tag objects), which caused\nthose to become unreferenced, so on the next \"gc/repack\" they were all\nevicted from their respective packs, at which point the \"gc.pruneExpire\"\nclock started ticking for them.\n\nWhereas in your case you disabled \"cruft packs\", which are basically a\nmechanism where git keeps such \"unused\" objects in a pack. Once you\ndisabled it git stopped considering the new *.mtimes file, and started\nextracting these loose objects en-masse.\n\nWhich b.t.w. is something I think we might want to reconsider. I.e. we\njust pass \"--cruft\" to pack-objects (see b757353676d\n(builtin/pack-objects.c: --cruft without expiration, 2022-05-20)), but\ndon't have a \"write cruft\" v.s. \"read cruft\" setting (but see below).\n\nThe one thing that gives me pause here is your mention of \"summarily\ndeleted by the GC's prune\". Did you run \"git prune --expire=now\"\nmanually, or do you have a really aggressive \"gc.pruneExpire\" setting?\nIt's also posible that I'm entirely misunderstanding this whole thing...\n\nAnyway.\n\nSince that 2018-era post of mine I've thought a bit about how to best\nhandle this particular edge case.\n\nI think the simplest implementation that would work well enough would be\nto teach \"git repack\" to limit how many objects it's willing to extract\nfrom a pack. I.e. we'd have the amount of \"unreachable\" objects we'd be\nwilling to eject from the pack limited, with check similar to how\n\"gc.auto\" works (i.e. once we reach the limit we'd implicitly turn on\n\"--keep-unreachable\").\n\nIt's far from perfect, and e.g. if the N objects you're extracting\nhappen to delta-compress particularly well you could still have cases\nwhere e.g. a 1GB repo becomes a 10GB one with the new loose objects.\n\nBut it would allow us to limit the common case where we'd e.g. create 1k\nnew loose objects, not 100k or whatever.\n\nIt does have the downside that unless you'd disable that limit the \"gc\"\n-> \"gc.pruneExpire\" window would become even more fuzzy. I.e. now if you\nrun \"gc\" the clock starts ticking right away, after this the clock would\nstart ticking whenever we happened to extract those objects.\n\nSo there's certainly less dumb ways to do this, and we'd need to make\nsure that whatever limit we set is aggressive enough that we'd always\n\"stay ahead\". I.e. if a repo just has a very high creation rate of\nobjects that become unreachable we'd need to ensure that we wouldn't\nhave an ever growing .git as our eviction rate wouldn't keep up with our\nadditions.\n\nFor cruft packs (this is a continuation of the \"see below\" above) we'd\ndo the same thing, but could benefit from knowing that such objects are\n\"already expired\", so we could read the *.mtimes, and possibly expire\nsome right away.\n\nAside from some very obscure use-cases I think these these sort of\n\"loose explosion\" are one-off events, so if we can smooth those out. The\nvarious gc.* settings can be hard to reason about, but we really do try\nto be gently by default, but notably fail very badly at that in some\nknown edge cases, such as this one.\n\n\n\n"},{"id":"459982","messageId":"CAKQoGakMtqFW3FR3WqaKrX4P_xbjq2-KceD4ScnVc0-D=-3X=A@mail.gmail.com","threadId":"58083","inReplyTo":"220720.86cze0um88.gmgdl@evledraar.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2022-07-26T16:22:19Z","receivedAt":"2022-07-26T16:22:41Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"On Wed, Jul 20, 2022 at 3:12 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Tue, Jul 19 2022, Gregory Szorc wrote:\n>\n> > On 6/29/2022 8:23 PM, Taylor Blau wrote:\n> >> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n> >>>> However, I think there is yet another bug at play: running\n> >>>> `incremental-repack` appears to be able to repack the cruft packfile.\n> >>>> In doing so, it deletes its .mtimes file and the metadata inside.\n> >>>\n> >>> That sounds like a bug to me. I'll take a look into it and see what I\n> >>> can find.\n> >> I actually think that there are two bugs here.\n> >> One is that we run a MIDX repack and expire, which could lead to us\n> >> repacking the entire contents of the cruft pack and then expiring the\n> >> metadata file. This is a bug, and we should exclude cruft packs from\n> >> this computation.\n> >> Another bug can happen when the cruft pack gets written into the\n> >> MIDX\n> >> and is MIDX-expireable (meaning that no objects are selected from the\n> >> cruft pack). In that case, the `git multi-pack-index expire` step would\n> >> remove the cruft pack entirely, which is also incorrect.\n> >> I'll take a look at fixing both of these, and thanks for pointing\n> >> them\n> >> out!\n> >\n> > For posterity, when I disabled cruft packfiles after having it enabled\n> > for a few weeks, the next `git gc` invocation on a high traffic repo\n> > resulted in >100k loose objects/files being created before they were\n> > summarily deleted by the GC's prune. This is significantly greater\n> > than the unreferenced object creation rate of the underlying repo. So\n> > it appears as if the MIDX stripping of the cruft packfile mtimes\n> > effectively disabled pruning, leading to a build-up of unreferenced objects.\n> >\n> > Fortunately I hadn't deployed cruft packfiles to production. If I had,\n> > the excessive filesystem churn on NFS would have caused an incident\n> > due to degraded performance.\n> >\n> > Since the interaction between cruft packfiles and MIDX appears to be\n> > buggy, I think I'm going to leave cruft packfiles disabled until these\n> > features work better together.\n>\n> I haven't looked deeply into whether there's any cruft packfile & MIDX\n> interaction at play here, but I suspect based on past experience that\n> this has nothing whatsoever to do with the MIDX.\n\nTaylor seemed to acknowledge a bug here a few posts back. Essentially:\n\n1. Cruft packfile created\n2. MIDX repack and expire treats the cruft packfile as a regular pack\nand folds its content into a new, regular packfile, deleting the\nmtimes file/metadata in the process.\n\nI don't fully grok the mechanism, but I believe the loss of the mtimes\nmetadata effectively results in indefinite retention of unreferenced\nobjects.\n\n>\n> It's because git suddenly found a bunch of objects that should be\n> evicted from the packs, as you disabled the cruft pack feature.\n>\n> Here's a past test-case & report of mine where I ran into that:\n>\n>     https://lore.kernel.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n>\n> That was way before cruft packfiles were integrated, but from my\n> understanding of how they work the mechanism is exactly the same.\n>\n> I.e. in that case I deleted a bunch of refs (tag objects), which caused\n> those to become unreferenced, so on the next \"gc/repack\" they were all\n> evicted from their respective packs, at which point the \"gc.pruneExpire\"\n> clock started ticking for them.\n>\n> Whereas in your case you disabled \"cruft packs\", which are basically a\n> mechanism where git keeps such \"unused\" objects in a pack. Once you\n> disabled it git stopped considering the new *.mtimes file, and started\n> extracting these loose objects en-masse.\n>\n> Which b.t.w. is something I think we might want to reconsider. I.e. we\n> just pass \"--cruft\" to pack-objects (see b757353676d\n> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20)), but\n> don't have a \"write cruft\" v.s. \"read cruft\" setting (but see below).\n>\n> The one thing that gives me pause here is your mention of \"summarily\n> deleted by the GC's prune\". Did you run \"git prune --expire=now\"\n> manually, or do you have a really aggressive \"gc.pruneExpire\" setting?\n> It's also posible that I'm entirely misunderstanding this whole thing...\n>\n> Anyway.\n>\n> Since that 2018-era post of mine I've thought a bit about how to best\n> handle this particular edge case.\n>\n> I think the simplest implementation that would work well enough would be\n> to teach \"git repack\" to limit how many objects it's willing to extract\n> from a pack. I.e. we'd have the amount of \"unreachable\" objects we'd be\n> willing to eject from the pack limited, with check similar to how\n> \"gc.auto\" works (i.e. once we reach the limit we'd implicitly turn on\n> \"--keep-unreachable\").\n>\n> It's far from perfect, and e.g. if the N objects you're extracting\n> happen to delta-compress particularly well you could still have cases\n> where e.g. a 1GB repo becomes a 10GB one with the new loose objects.\n>\n> But it would allow us to limit the common case where we'd e.g. create 1k\n> new loose objects, not 100k or whatever.\n>\n> It does have the downside that unless you'd disable that limit the \"gc\"\n> -> \"gc.pruneExpire\" window would become even more fuzzy. I.e. now if you\n> run \"gc\" the clock starts ticking right away, after this the clock would\n> start ticking whenever we happened to extract those objects.\n>\n> So there's certainly less dumb ways to do this, and we'd need to make\n> sure that whatever limit we set is aggressive enough that we'd always\n> \"stay ahead\". I.e. if a repo just has a very high creation rate of\n> objects that become unreachable we'd need to ensure that we wouldn't\n> have an ever growing .git as our eviction rate wouldn't keep up with our\n> additions.\n\nI agree this is a hard problem! In my case, the underlying repo sees a\nfew thousand pushes daily. This results in ~30k new unreferenced\nobjects on a typical workday. With NFS/EFS, we've observed that the\nentire filesystem can get slow once object loosening has written as\nfew as a few thousand files. Given our high volume, I worry that\nimposing a loose object creation ceiling will perpetually throttle and\nwe'll accumulate unreferenced objects indefinitely.\n\n> For cruft packs (this is a continuation of the \"see below\" above) we'd\n> do the same thing, but could benefit from knowing that such objects are\n> \"already expired\", so we could read the *.mtimes, and possibly expire\n> some right away.\n\nI don't fully grok how pruning works in a cruft packfile world. My\nnaive hope is that the process creating the cruft packfile is able to\nelide expired objects so we don't even bother with them during cruft\npackfile generation. (It seems inefficient/avoidable that we write an\nexpired object - loose or cruft packed - from the context of a gc only\nto delete it moments later.)\n\n> Aside from some very obscure use-cases I think these these sort of\n> \"loose explosion\" are one-off events, so if we can smooth those out. The\n> various gc.* settings can be hard to reason about, but we really do try\n> to be gently by default, but notably fail very badly at that in some\n> known edge cases, such as this one.\n\nI agree that my scenario of creating an explosion of loose objects is\na one-off. IMO not worth spending too much time thinking about. That\nbeing said, ~daily pruning on a large enough repo starts to resemble\nthese one-off events. And I think you need something like cruft\npackfiles to mitigate it. if cruft packfiles are ever enabled by\ndefault, it may have a nice side-effect of preventing these one-offs!\n(But again, I don't grok how pruning works in a cruft packfile world.)\n"},{"id":"459993","messageId":"220726.86czdrn3f2.gmgdl@evledraar.gmail.com","threadId":"58083","inReplyTo":"CAKQoGakMtqFW3FR3WqaKrX4P_xbjq2-KceD4ScnVc0-D=-3X=A@mail.gmail.com","subject":"Re: Race condition between repack and loose-objects maintenance task","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-26T18:11:34Z","receivedAt":"2022-07-26T18:18:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 26 2022, Gregory Szorc wrote:\n\n> On Wed, Jul 20, 2022 at 3:12 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>>\n>> On Tue, Jul 19 2022, Gregory Szorc wrote:\n>>\n>> > On 6/29/2022 8:23 PM, Taylor Blau wrote:\n>> >> On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:\n>>> [...]\n>> > Since the interaction between cruft packfiles and MIDX appears to be\n>> > buggy, I think I'm going to leave cruft packfiles disabled until these\n>> > features work better together.\n>>\n>> I haven't looked deeply into whether there's any cruft packfile & MIDX\n>> interaction at play here, but I suspect based on past experience that\n>> this has nothing whatsoever to do with the MIDX.\n>\n> Taylor seemed to acknowledge a bug here a few posts back. Essentially:\n>\n> 1. Cruft packfile created\n> 2. MIDX repack and expire treats the cruft packfile as a regular pack\n> and folds its content into a new, regular packfile, deleting the\n> mtimes file/metadata in the process.\n>\n> I don't fully grok the mechanism, but I believe the loss of the mtimes\n> metadata effectively results in indefinite retention of unreferenced\n> objects.\n\nMaybe, but I don't see how indefinite retention would happen unless\nyou're flip-flopping back & forth.\n\nIf we're deleting the mtimes file but keeping the pack they're in we'd\nnotice that those packed objects are unreferenced on the next gc, so\nwe'd explode them into loose objects, and then the timer starts ticking\non their expiry.\n\nIt's a lot of churn, but once that timer expires we'll then expire those\nobjects.\n\n>>\n>> It's because git suddenly found a bunch of objects that should be\n>> evicted from the packs, as you disabled the cruft pack feature.\n>>\n>> Here's a past test-case & report of mine where I ran into that:\n>>\n>>     https://lore.kernel.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n>>\n>> That was way before cruft packfiles were integrated, but from my\n>> understanding of how they work the mechanism is exactly the same.\n>>\n>> I.e. in that case I deleted a bunch of refs (tag objects), which caused\n>> those to become unreferenced, so on the next \"gc/repack\" they were all\n>> evicted from their respective packs, at which point the \"gc.pruneExpire\"\n>> clock started ticking for them.\n>>\n>> Whereas in your case you disabled \"cruft packs\", which are basically a\n>> mechanism where git keeps such \"unused\" objects in a pack. Once you\n>> disabled it git stopped considering the new *.mtimes file, and started\n>> extracting these loose objects en-masse.\n>>\n>> Which b.t.w. is something I think we might want to reconsider. I.e. we\n>> just pass \"--cruft\" to pack-objects (see b757353676d\n>> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20)), but\n>> don't have a \"write cruft\" v.s. \"read cruft\" setting (but see below).\n>>\n>> The one thing that gives me pause here is your mention of \"summarily\n>> deleted by the GC's prune\". Did you run \"git prune --expire=now\"\n>> manually, or do you have a really aggressive \"gc.pruneExpire\" setting?\n>> It's also posible that I'm entirely misunderstanding this whole thing...\n>>\n>> Anyway.\n>>\n>> Since that 2018-era post of mine I've thought a bit about how to best\n>> handle this particular edge case.\n>>\n>> I think the simplest implementation that would work well enough would be\n>> to teach \"git repack\" to limit how many objects it's willing to extract\n>> from a pack. I.e. we'd have the amount of \"unreachable\" objects we'd be\n>> willing to eject from the pack limited, with check similar to how\n>> \"gc.auto\" works (i.e. once we reach the limit we'd implicitly turn on\n>> \"--keep-unreachable\").\n>>\n>> It's far from perfect, and e.g. if the N objects you're extracting\n>> happen to delta-compress particularly well you could still have cases\n>> where e.g. a 1GB repo becomes a 10GB one with the new loose objects.\n>>\n>> But it would allow us to limit the common case where we'd e.g. create 1k\n>> new loose objects, not 100k or whatever.\n>>\n>> It does have the downside that unless you'd disable that limit the \"gc\"\n>> -> \"gc.pruneExpire\" window would become even more fuzzy. I.e. now if you\n>> run \"gc\" the clock starts ticking right away, after this the clock would\n>> start ticking whenever we happened to extract those objects.\n>>\n>> So there's certainly less dumb ways to do this, and we'd need to make\n>> sure that whatever limit we set is aggressive enough that we'd always\n>> \"stay ahead\". I.e. if a repo just has a very high creation rate of\n>> objects that become unreachable we'd need to ensure that we wouldn't\n>> have an ever growing .git as our eviction rate wouldn't keep up with our\n>> additions.\n>\n> I agree this is a hard problem! In my case, the underlying repo sees a\n> few thousand pushes daily. This results in ~30k new unreferenced\n> objects on a typical workday. With NFS/EFS, we've observed that the\n> entire filesystem can get slow once object loosening has written as\n> few as a few thousand files. Given our high volume, I worry that\n> imposing a loose object creation ceiling will perpetually throttle and\n> we'll accumulate unreferenced objects indefinitely.\n\nIf you're feeling adventurous you may want to try to rebase or otherwise\nexperiment with this old series of mine, that never made it in:\nhttps://lore.kernel.org/git/20181028225023.26427-1-avarab@gmail.com/\n\nI don't run git on NFS anymore, but when I did I found that the main\ncontributor to loose object-related slowness on pushes was the collision\nchecking.\n\nWhereas if you're willing to simply write duplicate objects ....\n"}]}