{"thread":{"id":"53002","subject":"Avoid race condition between fetch and repack/gc?","startedAt":"2020-03-16T08:35:59Z","lastAt":"2020-03-17T18:41:26Z","messageCount":6,"participants":["Andreas Krey","Derrick Stolee","Nasser Grainawi","Jeff King","Bryan Turner"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"393297","messageId":"20200316082348.GA26581@inner.h.apk.li","threadId":"53002","inReplyTo":null,"subject":"Avoid race condition between fetch and repack/gc?","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2020-03-16T08:23:48Z","receivedAt":"2020-03-16T08:35:59Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi all,\n\nwe occasionally seeing things like this:\n\n| DEBUG: 11:25:20: git -c advice.fetchShowForcedUpdates=false fetch --no-show-forced-updates -q --prune\n| Warning: Permanently added '[socgit.$company.com]:7999' (RSA) to the list of known hosts.\n| remote: fatal: packfile ./objects/pack/pack-20256f2be3bd51b57e519a9f2a4d3df09f231952.pack cannot be accessed        \n| error: git upload-pack: git-pack-objects died with error.\n| fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n| remote: aborting due to possible repository corruption on the remote side.\n| fatal: protocol error: bad pack header\n\nand when you look in the server repository there is a new packfile dated just around\nthat time. It looks like the fetch tries to access a packfile that it assumes to exist,\nbut the GC on the server throws it away just in that moment, and thus upload-pack fails.\n\nIs there a way to avoid this?\n\nShould there be, like git repack waiting a bit before deleting old packfiles?\n\n- Andreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"393298","messageId":"759f4b3b-28a7-c002-ae51-5991bf9ad211@gmail.com","threadId":"53002","inReplyTo":"20200316082348.GA26581@inner.h.apk.li","subject":"Re: Avoid race condition between fetch and repack/gc?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-16T12:10:56Z","receivedAt":"2020-03-16T12:11:04Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/16/2020 4:23 AM, Andreas Krey wrote:\n> Hi all,\n> \n> we occasionally seeing things like this:\n> \n> | DEBUG: 11:25:20: git -c advice.fetchShowForcedUpdates=false fetch --no-show-forced-updates -q --prune\n\nI'm happy to see these options. I hope they are helping you!\n\n> | Warning: Permanently added '[socgit.$company.com]:7999' (RSA) to the list of known hosts.\n> | remote: fatal: packfile ./objects/pack/pack-20256f2be3bd51b57e519a9f2a4d3df09f231952.pack cannot be accessed        \nThis _could_ mean a lot of things, but....\n\n> | error: git upload-pack: git-pack-objects died with error.\n> | fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n> | remote: aborting due to possible repository corruption on the remote side.\n> | fatal: protocol error: bad pack header\n> \n> and when you look in the server repository there is a new packfile dated just around\n> that time. It looks like the fetch tries to access a packfile that it assumes to exist,\n> but the GC on the server throws it away just in that moment, and thus upload-pack fails.\n\n...your intuition about repacking seems accurate. The important part of the\nrace condition is likely that the server process read and holds a read handle\non the .idx file, but when looking for the object contents it tries to open\nthe .pack file which was deleted.\n\nThis error is emitted by use_pack() in packfile.c. I'm surprised there is no\nfallback here, and we simply die().\n\nThe race condition seems to be related to the loop in do_oid_object_info_extended()\nin sha1-file.c looping through packs until finding the object in question: it does\nnot verify that the .pack file is open with a valid handle before terminating the\nloop and calling packed_object_info().\n\nPerhaps the fix is to update do_oid_object_info_extended() to \"accept\" a pack as\nthe home of the object only after verifying the pack is either open or can be\nopened. That seems like the least-invasive fix to me.\n\nThe more-invasive fix is to modify the stack from packed_object_info() to\nuse_pack() to use error messages and return codes instead of die(). This would\nstill need to affect the loop in do_oid_object_info_extended(), but may be a\nbetter way to handle this situation in general.\n\nOf course, this is a very critical code path, and maybe other community members\nhave more context as to why we are not already doing this?\n\n> Is there a way to avoid this?\n> \n> Should there be, like git repack waiting a bit before deleting old packfiles?\n\nThis all depends on how you are managing your server. It is likely that you\ncould create your own maintenance that handles this for you.\n\nThe \"git multi-pack-index (expire|repack)\" cycle is built to prevent this sort\nof issue, but is not yet integrated well with reachability bitmaps. You likely\nrequire the bitmaps to keep your server performance, so that may not be a way\nforward for you.\n\nThanks,\n-Stolee\n"},{"id":"393307","messageId":"06992130-5109-4180-AB26-315AAF536788@codeaurora.org","threadId":"53002","inReplyTo":"759f4b3b-28a7-c002-ae51-5991bf9ad211@gmail.com","subject":"Re: Avoid race condition between fetch and repack/gc?","fromName":"Nasser Grainawi","fromEmail":"nasser@codeaurora.org","sentAt":"2020-03-16T17:17:12Z","receivedAt":"2020-03-16T17:17:35Z","isPatch":false,"sender":{"key":"nasser@codeaurora.org","avatar":"https://avatars.githubusercontent.com/u/757421?v=4"},"body":"\n> On Mar 16, 2020, at 6:10 AM, Derrick Stolee <stolee@gmail.com> wrote:\n> \n> On 3/16/2020 4:23 AM, Andreas Krey wrote:\n>> Hi all,\n>> \n>> we occasionally seeing things like this:\n>> \n>> | DEBUG: 11:25:20: git -c advice.fetchShowForcedUpdates=false fetch --no-show-forced-updates -q --prune\n> \n> I'm happy to see these options. I hope they are helping you!\n> \n>> | Warning: Permanently added '[socgit.$company.com]:7999' (RSA) to the list of known hosts.\n>> | remote: fatal: packfile ./objects/pack/pack-20256f2be3bd51b57e519a9f2a4d3df09f231952.pack cannot be accessed        \n> This _could_ mean a lot of things, but....\n> \n>> | error: git upload-pack: git-pack-objects died with error.\n>> | fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n>> | remote: aborting due to possible repository corruption on the remote side.\n>> | fatal: protocol error: bad pack header\n>> \n>> and when you look in the server repository there is a new packfile dated just around\n>> that time. It looks like the fetch tries to access a packfile that it assumes to exist,\n>> but the GC on the server throws it away just in that moment, and thus upload-pack fails.\n> \n> ...your intuition about repacking seems accurate. The important part of the\n> race condition is likely that the server process read and holds a read handle\n> on the .idx file, but when looking for the object contents it tries to open\n> the .pack file which was deleted.\n> \n\n[snip]\n\n> \n>> Is there a way to avoid this?\n>> \n>> Should there be, like git repack waiting a bit before deleting old packfiles?\n> \n> This all depends on how you are managing your server. It is likely that you\n> could create your own maintenance that handles this for you.\n> \n> The \"git multi-pack-index (expire|repack)\" cycle is built to prevent this sort\n> of issue, but is not yet integrated well with reachability bitmaps. You likely\n> require the bitmaps to keep your server performance, so that may not be a way\n> forward for you.\n\nWe manage this on our servers with a repack wrapper that first creates hard links for all packfiles into a objects/pack/preserved dir and then we have patches on top of JGit [1] that actually know how to recover objects from that dir when the original pack is removed by repacking. It’s worked quite well for us for a couple years now and should be compatible with/without bitmaps (haven’t specifically tested) and any pack/repacking strategy.\n\n[1] https://git.eclipse.org/r/122288"},{"id":"393310","messageId":"20200316172729.GA1072073@coredump.intra.peff.net","threadId":"53002","inReplyTo":"759f4b3b-28a7-c002-ae51-5991bf9ad211@gmail.com","subject":"Re: Avoid race condition between fetch and repack/gc?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-16T17:27:29Z","receivedAt":"2020-03-16T17:27:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 16, 2020 at 08:10:56AM -0400, Derrick Stolee wrote:\n\n> > | error: git upload-pack: git-pack-objects died with error.\n> > | fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n> > | remote: aborting due to possible repository corruption on the remote side.\n> > | fatal: protocol error: bad pack header\n> > \n> > and when you look in the server repository there is a new packfile dated just around\n> > that time. It looks like the fetch tries to access a packfile that it assumes to exist,\n> > but the GC on the server throws it away just in that moment, and thus upload-pack fails.\n> \n> ...your intuition about repacking seems accurate. The important part of the\n> race condition is likely that the server process read and holds a read handle\n> on the .idx file, but when looking for the object contents it tries to open\n> the .pack file which was deleted.\n>\n> This error is emitted by use_pack() in packfile.c. I'm surprised there is no\n> fallback here, and we simply die().\n\nYes, you're correct this is what's going on, but there's some more\nsubtlety (see below).\n\nThe issue is that by the time we get to use_pack(), it's generally too\nlate; we've committed to use this particular packed representation of\nthe object, and it would be very hard to back out of it.\n\nThere are really two cases worth considering independently: normal\naccess of objects via read_object_file(), oid_object_info(), etc versus\npack-objects.\n\nIn the normal code paths, contrary to what you wrote here:\n\n> The race condition seems to be related to the loop in do_oid_object_info_extended()\n> in sha1-file.c looping through packs until finding the object in question: it does\n> not verify that the .pack file is open with a valid handle before terminating the\n> loop and calling packed_object_info().\n>\n> Perhaps the fix is to update do_oid_object_info_extended() to \"accept\" a pack as\n> the home of the object only after verifying the pack is either open or can be\n> opened. That seems like the least-invasive fix to me.\n\nwe do make sure we have an open handle to the pack. This happens in\nfill_pack_entry(), which does:\n\n          /*\n           * We are about to tell the caller where they can locate the\n           * requested object.  We better make sure the packfile is\n           * still here and can be accessed before supplying that\n           * answer, as it may have been deleted since the index was\n           * loaded!\n           */\n          if (!is_pack_valid(p))\n                  return 0;\n\nAnd is_pack_valid() not only checks the pack, but leaves open either the\ndescriptor or our mmap'd handle.\n\nSo in do_oid_object_info_extended(), we call fill_pack_entry() and we\nwon't consider the object available unless we have a handle open to the\npack. And since nobody else is manipulating the handles in between\n(because any multi-threaded locking is much coarser than this), it\nshould be race-proof.\n\nBut pack-objects is not so lucky. Because it wants to look at intimate\ndetails of the packed objects (e.g., reusing on-disk deltas or\nzlib-deflated contents), it stores away the packfile/offset pair for\nobjects during the \"Counting\" phase and then uses it much later in the\n\"Writing\" phase.\n\nSo it's possible for other lookups in the meantime to cause us to drop\nour descriptor or mmap handle, and then later we find we can't re-open\nit. But in practice, this shouldn't happen as long as:\n\n  - you have a reasonable number of packs compared to your file\n    descriptor limit\n\n  - you don't use config like core.packedGitLimit that encourages Git to\n    drop mmap'd windows. On 64-bit systems, this doesn't help at all (we\n    have plenty of address space, and since the maps are read-only, the\n    OS is free to drop the clean pages if there's memory pressure). On\n    32-bit systems, it's a necessity (and I'd expect a 32-bit server to\n    run into this issue a lot for large repositories).\n\nWe used to see this quite a lot at GitHub, but hardly at all since\n4c08018204 (pack-objects: protect against disappearing packs,\n2011-10-14), which made sure that pack-objects does the same\nis_pack_valid() check. The only time we see it now are degenerate pack\ncases (e.g., we accidentally hit a state with 20,000 packs or\nsomething). It's possible there's been a regression recently, though.\nOur version right now is based on v2.24.1.\n\n> The \"git multi-pack-index (expire|repack)\" cycle is built to prevent this sort\n> of issue, but is not yet integrated well with reachability bitmaps. You likely\n> require the bitmaps to keep your server performance, so that may not be a way\n> forward for you.\n\nI wondered if the midx code might suffer from a race by missing out on\nthe is_pack_valid() check that fill_pack_entry() does. But it's right\nthere (including the copied comment!) in nth_midxed_pack_entry(). So\nthey should behave the same.\n\n-Peff\n"},{"id":"393338","messageId":"CAGyf7-HQH7hjuYSmOYeOEvZBWkUzjU4M8Wr50FNPYgG4NZ=5UA@mail.gmail.com","threadId":"53002","inReplyTo":"20200316172729.GA1072073@coredump.intra.peff.net","subject":"Re: Avoid race condition between fetch and repack/gc?","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2020-03-16T23:40:13Z","receivedAt":"2020-03-16T23:40:27Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Mon, Mar 16, 2020 at 10:27 AM Jeff King <peff@peff.net> wrote:\n>\n>   - you don't use config like core.packedGitLimit that encourages Git to\n>     drop mmap'd windows. On 64-bit systems, this doesn't help at all (we\n>     have plenty of address space, and since the maps are read-only, the\n>     OS is free to drop the clean pages if there's memory pressure). On\n>     32-bit systems, it's a necessity (and I'd expect a 32-bit server to\n>     run into this issue a lot for large repositories).\n\nI could be wrong, but I'm going to _guess_ from the :7999 in the\nexample output that the repository is hosted with Bitbucket Server.\nAssuming my guess is correct, Bitbucket Server _does_ set\n\"core.packedgitlimit=256m\" (and \"core.packedgitwindowsize=32m\", for\nwhat it's worth). Those settings are applied specifically because\nwe've found they _do_ impact Git's overall memory usage when serving\nclones in particular, which is important for cases where the system is\nserving dozens (or hundreds) of concurrent clones.\n\nOf course, we don't always do a great job of re-testing\nonce-beneficial settings later, which means sometimes they end up\nbeing based on outdated observations. Perhaps we should prioritize\nsome additional testing here, especially on 64-bit systems. (We've\nbeen setting \"core.packedgitlimit\" since back when Bitbucket Server\nwas called Stash and supported Git 1.7.6 on the server.)\n\nThat said, though, you note \"core.packedgitlimit\" is necessary on\n32-bit servers and, unfortunately, we do still support Bitbucket\nServer on 32-bit OSes. Maybe we should investigate applying (or not)\nthe flags depending on the platform. Sadly, that's not necessarily\nsimple to do since just because the _OS_ is 64-bit doesn't mean _Git_\nis; it's pretty trivial to run 32-bit Git on 64-bit Windows, for\nexample.\n\nBryan\n"},{"id":"393367","messageId":"20200317184124.GA17006@coredump.intra.peff.net","threadId":"53002","inReplyTo":"CAGyf7-HQH7hjuYSmOYeOEvZBWkUzjU4M8Wr50FNPYgG4NZ=5UA@mail.gmail.com","subject":"Re: Avoid race condition between fetch and repack/gc?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-17T18:41:24Z","receivedAt":"2020-03-17T18:41:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 16, 2020 at 04:40:13PM -0700, Bryan Turner wrote:\n\n> On Mon, Mar 16, 2020 at 10:27 AM Jeff King <peff@peff.net> wrote:\n> >\n> >   - you don't use config like core.packedGitLimit that encourages Git to\n> >     drop mmap'd windows. On 64-bit systems, this doesn't help at all (we\n> >     have plenty of address space, and since the maps are read-only, the\n> >     OS is free to drop the clean pages if there's memory pressure). On\n> >     32-bit systems, it's a necessity (and I'd expect a 32-bit server to\n> >     run into this issue a lot for large repositories).\n> \n> I could be wrong, but I'm going to _guess_ from the :7999 in the\n> example output that the repository is hosted with Bitbucket Server.\n> Assuming my guess is correct, Bitbucket Server _does_ set\n> \"core.packedgitlimit=256m\" (and \"core.packedgitwindowsize=32m\", for\n> what it's worth).\n\nThanks for letting me know. That would definitely explain the behavior\nAndreas is seeing.\n\n> Those settings are applied specifically because\n> we've found they _do_ impact Git's overall memory usage when serving\n> clones in particular, which is important for cases where the system is\n> serving dozens (or hundreds) of concurrent clones.\n\nI do think there's an open question here, but it's also really easy to\nbe misled by common metrics. If we imagine a repo with a 512MB packfile\nand two scenarios:\n\n  - Git mmap's the whole packfile at once, and accesses pages within\n    that mmap\n\n  - Git mmap's no more than 256MB at once, shifting its window around as\n    necessary\n\nand then we get a bunch of clones. Then I'd expect to see:\n\n  - virtual memory size for those Git processes will be higher. But much\n    of that will be shared pages with each other. Measuring something\n    like Proportional Set Size (PSS) yields a more useful number.\n\n  - the Resident Set Size (RSS) of those processes will also be higher,\n    because the OS may be leaving pages in the mmap resident. However,\n    in my experience this is often a sign that there _isn't_ memory\n    pressure on the system. Because if there was, then infrequently used\n    pages would be dropped by the OS (and they should be among the first\n    to go, as they're by definition clean pages. Though they do\n    presumably compete with other read-only disk cache).\n\nOr another way to think about it: the mmap patterns don't change the\nworking set patterns of Git. They just change what the OS knows, and\npossibly how it reacts. And that's the \"open question\" for me: does the\noperating system react significantly differently under memory pressure\nfor a big mmap with infrequently accessed pages than it would for a\nseries of smaller maps. I don't know the answer. Our servers are all\nLinux, and we've tended to just trust that the operating system's\npage-level decisions are sensible.\n\nBut I don't have any real numbers to support that. We stopped using\ncore.packedGitLimit in 2012 (because of this issue), and everything has\nbeen good enough since to not bother looking into it more. Memory\npressure for us is usually from actual heap usage (e.g., pack-objects\nhas a high per-object cost plus delta window size times max object\nsize).\n\n> Of course, we don't always do a great job of re-testing\n> once-beneficial settings later, which means sometimes they end up\n> being based on outdated observations. Perhaps we should prioritize\n> some additional testing here, especially on 64-bit systems. (We've\n> been setting \"core.packedgitlimit\" since back when Bitbucket Server\n> was called Stash and supported Git 1.7.6 on the server.)\n\nIt sounds like you don't have any recent numbers, either. :)\n\n> That said, though, you note \"core.packedgitlimit\" is necessary on\n> 32-bit servers and, unfortunately, we do still support Bitbucket\n> Server on 32-bit OSes. Maybe we should investigate applying (or not)\n> the flags depending on the platform. Sadly, that's not necessarily\n> simple to do since just because the _OS_ is 64-bit doesn't mean _Git_\n> is; it's pretty trivial to run 32-bit Git on 64-bit Windows, for\n> example.\n\nThe default for core.packedGitLimit is already 256MB on 32-bit builds of\nGit (based on sizeof(void *), so truly on the build and not the OS). So\nif you just left it unset, it would do what you want.\n\n-Peff\n"}]}