{"thread":{"id":"16649","subject":"[PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","startedAt":"2008-12-09T08:36:27Z","lastAt":"2009-01-09T01:43:46Z","messageCount":49,"participants":["Jan Krüger","R. Tyler Ballance","Shawn O. Pearce","Nicolas Pitre","Linus Torvalds","Junio C Hamano","James Pickens","Boyd Stephen Smith Jr."],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"97408","messageId":"20081209093627.77039a1f@perceptron","threadId":"16649","inReplyTo":null,"subject":"[PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Jan Krüger","fromEmail":"jk@jk.gs","sentAt":"2008-12-09T08:36:27Z","receivedAt":"2008-12-09T08:36:27Z","isPatch":true,"sender":{"key":"jk@jk.gs","avatar":"https://avatars.githubusercontent.com/u/1774?v=4"},"body":"For fixing a corrupted repository by using backup copies of individual\nfiles, allow write_sha1_file() to write loose files even if the object\nalready exists in a pack file, but only if the existing entry is marked\nas corrupted.\n\nSigned-off-by: Jan Krüger \n---\n\nOn IRC I talked to rtyler who had a corrupted pack file and plenty of\nobject backups by way of cloned repositories. We decided to try\nextracting the corrupted objects from the other object database and\ninjecting them into the broken repo as loose objects, but this failed\nbecause sha1_write_file() refuses to write loose objects that are\nalready present in a pack file.\n\nThis patch expands the check to see if the pack entry has been marked\nas corrupted and, if so, allows writing a loose object with the same\nID. Unfortunately, when Tyler tried a merge while using this patch,\nsomething we didn't manage to track down happened and now git doesn't\nconsider the object corrupted anymore. I'm not sure enough that it\nwasn't caused by the patch to submit this patch without hesitation.\n\nApart from that, I think the change is not all too great since it makes\nwrite_sha1_file() walk the list of pack entries twice. That's a bit of\na waste.\n\nSo those are the reasons why I wanted a few opinions first. Another\nreason is that there might be a way smarter method to fix this kind of\nproblem, in which case I'd love hearing about it for future reference.\n\n sha1_file.c | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6c0e251..17085cc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2373,14 +2373,17 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \tchar hdr[32];\n \tint hdrlen;\n \n-\t/* Normally if we have it in the pack then we do not bother writing\n-\t * it out into .git/objects/??/?{38} file.\n-\t */\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n-\tif (has_sha1_file(sha1))\n-\t\treturn 0;\n+\t/* Normally if we have it in the pack then we do not bother writing\n+\t * it out into .git/objects/??/?{38} file. We do, though, if there\n+\t * is no chance that we have an uncorrupted version of the object.\n+\t */\n+\tif (has_sha1_file(sha1)) {\n+\t\tif (has_loose_object(sha1) || !has_packed_and_bad(sha1))\n+\t\t\treturn 0;\n+\t}\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n \n-- \n1.6.0.4.766.g6fc4a\n"},{"id":"97415","messageId":"1228813339.18611.35.camel@starfruit.local","threadId":"16649","inReplyTo":"20081209093627.77039a1f@perceptron","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2008-12-09T09:02:19Z","receivedAt":"2008-12-09T09:02:19Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2008-12-09 at 09:36 +0100, Jan Krüger wrote:\n> For fixing a corrupted repository by using backup copies of individual\n> files, allow write_sha1_file() to write loose files even if the object\n> already exists in a pack file, but only if the existing entry is marked\n> as corrupted.\n> \n> Signed-off-by: Jan Krüger \n> ---\n> \n> On IRC I talked to rtyler who had a corrupted pack file and plenty of\n> object backups by way of cloned repositories. We decided to try\n> extracting the corrupted objects from the other object database and\n> injecting them into the broken repo as loose objects, but this failed\n> because sha1_write_file() refuses to write loose objects that are\n> already present in a pack file.\n\nFigured I'd chime in here with some anecdotal evidence with the error\ncondition that I hit shortly after Jan sent the email.\n\n xdev3 (master-release)% git pull --no-ff . master\n From .\n * branch master -> FETCH_HEAD\n error: failed to read object\n befd9bc4d184b4383569909e4d245f3337c1f8ed at offset 1415784644\n from .git/objects/pack/pack-f7eb06e39f01b528c1d1a2c413ac51b31b8515aa.pack\n fatal: object befd9bc4d184b4383569909e4d245f3337c1f8ed is\n corrupted\n Merge with strategy recursive failed.\n xdev3 (master-release)%\n\nI ran that command a couple of times to make sure it wasn't a fluke, I\nrepeated the error numerous times (without switching branches or pulling\nfrom a remote). This pull was done with a slightly modified internal\nversion of v1.6.0.4\n xdev3 (master-release)% git --version\n git version 1.6.0.4-kb1\n xdev3 (master-release)\n \n\nAfter consulting with Jan, I tried running the same command with a\nmodified version of v1.6.0.5 with Jan's patch\n xdev3 (master-release)% ~/basket/bin/git pull --no-ff . master\n From .\n * branch master -> FETCH_HEAD\n Merge made by recursive.\n ** TOP SECRET MERGES! ;) **\n \n 13 files changed, 51 insertions(+), 21 deletions(-)\n xdev3 (master-release)%\n \n \nPurely anecdotal as I'm not entirely clear what the hell is actually going on here :)\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"97439","messageId":"20081209162402.GP31551@spearce.org","threadId":"16649","inReplyTo":"20081209093627.77039a1f@perceptron","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-09T16:24:02Z","receivedAt":"2008-12-09T16:24:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jan Krüger wrote:\n> For fixing a corrupted repository by using backup copies of individual\n> files, allow write_sha1_file() to write loose files even if the object\n> already exists in a pack file, but only if the existing entry is marked\n> as corrupted.\n\nHuh. So I'm digging around sha1_file.c and I'm not yet sure why\nyour patch makes a difference.\n\nhas_sha1_file() calls find_pack_entry() to determine which pack has\nthe object, and at what offset (if found). It doesn't care about\nthe offset, but it does care about the successful match.\n\nfind_pack_entry() already considers the bad_object_sha1 for each\npack, before it even tries the binary search within the index.\nSo if the entry was known to be bad has_sha1_file() will return 0,\nunless the object is loose.\n\nWhere this breaks down is if the object is being created,\nits very likely we didn't attempt to read it in this process.\nThe bad_object_sha1 table is transient and populated only when\nunpacking an object entry fails. So for example during a merge\nif a tree was stored in a pack and is corrupt and the merge\nresult produces that same tree object we won't write it out with\nwrite_sha1_file() because it exists in a pack, but since we never\nread it we also don't know the pack entry is corrupt.\n\nIts horribly inefficient to read every object before we write it\nback out. The best thing to do when faced with corruption is to\nstop and repack, overlaying the object database from a known good\ncopy of the repository so pack-objects can use the good copy when\na corrupt object is identified.\n\nSo I agree with you that changing this in write_sha1_file() is a\nbad idea for the normal good cases, but I also don't see how this\npatch changes anything at all... the code path you introduced is\nalready implemented.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 6c0e251..17085cc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2373,14 +2373,17 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n> \tchar hdr[32];\n> \tint hdrlen;\n> \n> -\t/* Normally if we have it in the pack then we do not bother writing\n> -\t * it out into .git/objects/??/?{38} file.\n> -\t */\n> \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n> \tif (returnsha1)\n> \t\thashcpy(returnsha1, sha1);\n> -\tif (has_sha1_file(sha1))\n> -\t\treturn 0;\n> +\t/* Normally if we have it in the pack then we do not bother writing\n> +\t * it out into .git/objects/??/?{38} file. We do, though, if there\n> +\t * is no chance that we have an uncorrupted version of the object.\n> +\t */\n> +\tif (has_sha1_file(sha1)) {\n> +\t\tif (has_loose_object(sha1) || !has_packed_and_bad(sha1))\n> +\t\t\treturn 0;\n> +\t}\n> \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n> }\n\n-- \nShawn.\n"},{"id":"99519","messageId":"1231282320.8870.52.camel@starfruit","threadId":"16649","inReplyTo":"20081209093627.77039a1f@perceptron","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-06T22:52:00Z","receivedAt":"2009-01-06T22:52:00Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2008-12-09 at 09:36 +0100, Jan Krüger wrote:\n> For fixing a corrupted repository by using backup copies of individual\n> files, allow write_sha1_file() to write loose files even if the object\n> already exists in a pack file, but only if the existing entry is marked\n> as corrupted.\n\nI figured I'd reply to this again, since the issue cropped up again.\n\nWe started experiencing *large* numbers of corruptions like the ones\nthat started the thread (one developer was receiving them once or twice\na day) with v1.6.0.4\n\nWe went ahead and upgraded to a custom build of v1.6.1 with Jan's patch\n(below) and the issues /seem/ to have resolved themselves. I'm not\ncertain whether Jan's patch was really responsible, or if there was\nanother issue that caused this to correct itself in v1.6.1. \n\nAs it stands, I think it's safe to assume that given the frequency of\nthe occurances that they were not tied to a memory or disk error (or\nother levels of the machine's stack would be suffering as well). The\nonly thing I can think of is that /some/ developers who've experienced\nthe issue are using Samba mount points and changing files in Mac OS X,\nbut using Git on the mounted share (i.e. TextMate changes a file hosted\non Samba, changes are committed in an SSH session on that machine), but\nthat doesn't account for everything.\n\nIf there was something else included in the v1.6.1 release please let me\nknow so I can back Jan's patch out.\n\n\nCheers\n\n\n> \n> Signed-off-by: Jan Krüger \n> ---\n> \n> On IRC I talked to rtyler who had a corrupted pack file and plenty of\n> object backups by way of cloned repositories. We decided to try\n> extracting the corrupted objects from the other object database and\n> injecting them into the broken repo as loose objects, but this failed\n> because sha1_write_file() refuses to write loose objects that are\n> already present in a pack file.\n> \n> This patch expands the check to see if the pack entry has been marked\n> as corrupted and, if so, allows writing a loose object with the same\n> ID. Unfortunately, when Tyler tried a merge while using this patch,\n> something we didn't manage to track down happened and now git doesn't\n> consider the object corrupted anymore. I'm not sure enough that it\n> wasn't caused by the patch to submit this patch without hesitation.\n> \n> Apart from that, I think the change is not all too great since it makes\n> write_sha1_file() walk the list of pack entries twice. That's a bit of\n> a waste.\n> \n> So those are the reasons why I wanted a few opinions first. Another\n> reason is that there might be a way smarter method to fix this kind of\n> problem, in which case I'd love hearing about it for future reference.\n> \n> sha1_file.c | 9 +++++----\n> 1 files changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 6c0e251..17085cc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2373,14 +2373,17 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n> \tchar hdr[32];\n> \tint hdrlen;\n> \n> -\t/* Normally if we have it in the pack then we do not bother writing\n> -\t * it out into .git/objects/??/?{38} file.\n> -\t */\n> \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n> \tif (returnsha1)\n> \t\thashcpy(returnsha1, sha1);\n> -\tif (has_sha1_file(sha1))\n> -\t\treturn 0;\n> +\t/* Normally if we have it in the pack then we do not bother writing\n> +\t * it out into .git/objects/??/?{38} file. We do, though, if there\n> +\t * is no chance that we have an uncorrupted version of the object.\n> +\t */\n> +\tif (has_sha1_file(sha1)) {\n> +\t\tif (has_loose_object(sha1) || !has_packed_and_bad(sha1))\n> +\t\t\treturn 0;\n> +\t}\n> \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n> }\n> \n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99527","messageId":"alpine.LFD.2.00.0901062005290.26118@xanadu.home","threadId":"16649","inReplyTo":"1231282320.8870.52.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-01-07T01:25:38Z","receivedAt":"2009-01-07T01:25:38Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n\n> On Tue, 2008-12-09 at 09:36 +0100, Jan Krüger wrote:\n> > For fixing a corrupted repository by using backup copies of individual\n> > files, allow write_sha1_file() to write loose files even if the object\n> > already exists in a pack file, but only if the existing entry is marked\n> > as corrupted.\n> \n> I figured I'd reply to this again, since the issue cropped up again.\n> \n> We started experiencing *large* numbers of corruptions like the ones\n> that started the thread (one developer was receiving them once or twice\n> a day) with v1.6.0.4\n> \n> We went ahead and upgraded to a custom build of v1.6.1 with Jan's patch\n> (below) and the issues /seem/ to have resolved themselves. I'm not\n> certain whether Jan's patch was really responsible, or if there was\n> another issue that caused this to correct itself in v1.6.1. \n> \n> As it stands, I think it's safe to assume that given the frequency of\n> the occurances that they were not tied to a memory or disk error (or\n> other levels of the machine's stack would be suffering as well). The\n> only thing I can think of is that /some/ developers who've experienced\n> the issue are using Samba mount points and changing files in Mac OS X,\n> but using Git on the mounted share (i.e. TextMate changes a file hosted\n> on Samba, changes are committed in an SSH session on that machine), but\n> that doesn't account for everything.\n> \n> If there was something else included in the v1.6.1 release please let me\n> know so I can back Jan's patch out.\n\nPlease back it out. As it stands, that patch is a no op because of the \nway git is used, and even if the patch was to work as intended, its \npurpose is not to magically fix corruptions without special action from \nyour part. If you have corruption problems coming back only because of \nthe removal of this patch then something is really really fishy and I \nwould really like to know about it.\n\nThere were indeed many changes between v1.6.0.4 and v1.6.1: the exact \nnumber is 1029. A couple of them are especially addressing increased \nrobustness against some kind of pack corruptions. But in any case you \nstill should see error messages appearing about them.\n\nAnd don't underestimate the power of disk corruptions. I started to \nwork on git corruption resilience simply because I ended up with a \ncorrupted pack at some point. Then a while later I got another \ncorrupted pack. Then another while later I lost my filesystem entirely \nand had to reinstall my system (after buying a new disk). Turns out \nthat my old disk is silently corrupting data without signaling any \nerrors to the host.\n\n\nNicolas\n"},{"id":"99528","messageId":"1231292360.8870.61.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901062005290.26118@xanadu.home","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T01:39:20Z","receivedAt":"2009-01-07T01:39:20Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2009-01-06 at 20:25 -0500, Nicolas Pitre wrote:\n> On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> \n> > On Tue, 2008-12-09 at 09:36 +0100, Jan Krüger wrote:\n> > > For fixing a corrupted repository by using backup copies of individual\n> > > files, allow write_sha1_file() to write loose files even if the object\n> > > already exists in a pack file, but only if the existing entry is marked\n> > > as corrupted.\n> > \n> > I figured I'd reply to this again, since the issue cropped up again.\n> > \n> > We started experiencing *large* numbers of corruptions like the ones\n> > that started the thread (one developer was receiving them once or twice\n> > a day) with v1.6.0.4\n> > \n> > We went ahead and upgraded to a custom build of v1.6.1 with Jan's patch\n> > (below) and the issues /seem/ to have resolved themselves. I'm not\n> > certain whether Jan's patch was really responsible, or if there was\n> > another issue that caused this to correct itself in v1.6.1. \n\nI'll back the patch out and redeploy, it's worth mentioning that a\ncoworker of mine just got the issue as well (on 1.6.1). He was able to\n`git pull` and the error went away, but I doubt that it \"magically fixed\nitself\"\n\n\n> Please back it out. As it stands, that patch is a no op because of the \n> way git is used, and even if the patch was to work as intended, its \n> purpose is not to magically fix corruptions without special action from \n> your part. If you have corruption problems coming back only because of \n> the removal of this patch then something is really really fishy and I \n> would really like to know about it.\n> \n> There were indeed many changes between v1.6.0.4 and v1.6.1: the exact \n> number is 1029. A couple of them are especially addressing increased \n> robustness against some kind of pack corruptions. But in any case you \n> still should see error messages appearing about them.\n> \n> And don't underestimate the power of disk corruptions. I started to \n> work on git corruption resilience simply because I ended up with a \n> corrupted pack at some point. Then a while later I got another \n> corrupted pack. Then another while later I lost my filesystem entirely \n> and had to reinstall my system (after buying a new disk). Turns out \n> that my old disk is silently corrupting data without signaling any \n> errors to the host.\n\nI highly doubt this, I've got the issue appearing on at least 7\ndifferent development boxes (not workstations, 2U quad-core ECC RAM, etc\nmachines), while that doesn't mean that they all don't have issues, the\nprobability of them *all* having disk issues, and it somehow only\nmanifesting itself with Git usage, is low ;)\n\nI've tarred one of the repositories that had it in a reproducible state\nso I can create a build and extract the tar and run against that to\nverify any patches anybody might have, but unfortunately at 7GB of\ncompany code and assets, I can't exactly share ;)\n\n\nCheers\n\n\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99530","messageId":"alpine.LFD.2.00.0901062059230.26118@xanadu.home","threadId":"16649","inReplyTo":"1231292360.8870.61.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-01-07T02:09:58Z","receivedAt":"2009-01-07T02:09:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n\n> On Tue, 2009-01-06 at 20:25 -0500, Nicolas Pitre wrote:\n> > On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > > On Tue, 2008-12-09 at 09:36 +0100, Jan Krüger wrote:\n> > > > For fixing a corrupted repository by using backup copies of individual\n> > > > files, allow write_sha1_file() to write loose files even if the object\n> > > > already exists in a pack file, but only if the existing entry is marked\n> > > > as corrupted.\n> > > \n> > > I figured I'd reply to this again, since the issue cropped up again.\n> > > \n> > > We started experiencing *large* numbers of corruptions like the ones\n> > > that started the thread (one developer was receiving them once or twice\n> > > a day) with v1.6.0.4\n> > > \n> > > We went ahead and upgraded to a custom build of v1.6.1 with Jan's patch\n> > > (below) and the issues /seem/ to have resolved themselves. I'm not\n> > > certain whether Jan's patch was really responsible, or if there was\n> > > another issue that caused this to correct itself in v1.6.1. \n> \n> I'll back the patch out and redeploy, it's worth mentioning that a\n> coworker of mine just got the issue as well (on 1.6.1). He was able to\n> `git pull` and the error went away, but I doubt that it \"magically fixed\n> itself\"\n\nPlease describe the \"issue\", ideally with transcripts of error messages, \netc. Normally a simple pull operation should not provide any \"fix\" for \ncorruptions.\n\n> I highly doubt this, I've got the issue appearing on at least 7\n> different development boxes (not workstations, 2U quad-core ECC RAM, etc\n> machines), while that doesn't mean that they all don't have issues, the\n> probability of them *all* having disk issues, and it somehow only\n> manifesting itself with Git usage, is low ;)\n\nAgreed.\n\n> I've tarred one of the repositories that had it in a reproducible state\n\nThat is wonderful.\n\n> so I can create a build and extract the tar and run against that to\n> verify any patches anybody might have, but unfortunately at 7GB of\n> company code and assets, I can't exactly share ;)\n\nFirst step is to understand what is going on. Only then could reliable \npatches be made.\n\n\nNicolas\n"},{"id":"99531","messageId":"1231296475.8870.89.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901062059230.26118@xanadu.home","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T02:47:55Z","receivedAt":"2009-01-07T02:47:55Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2009-01-06 at 21:09 -0500, Nicolas Pitre wrote:\n> > I've tarred one of the repositories that had it in a reproducible\n> state\n> \n> That is wonderful.\n> \n> > so I can create a build and extract the tar and run against that to\n> > verify any patches anybody might have, but unfortunately at 7GB of\n> > company code and assets, I can't exactly share ;)\n> \n> First step is to understand what is going on. Only then could reliable \n> patches be made.\n\nIf you want to point me in the right direction, I have a few hours to\nkill this evening and fscking around with gdb(1) and printf() just might\nbe some of my favorite things ;)\n\nLooking forward to killing this issue\n\n\nCheers\n\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99534","messageId":"alpine.LFD.2.00.0901062212060.26118@xanadu.home","threadId":"16649","inReplyTo":"1231296475.8870.89.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-01-07T03:21:51Z","receivedAt":"2009-01-07T03:21:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n\n> On Tue, 2009-01-06 at 21:09 -0500, Nicolas Pitre wrote:\n> > > I've tarred one of the repositories that had it in a reproducible\n> > state\n> > \n> > That is wonderful.\n> > \n> > > so I can create a build and extract the tar and run against that to\n> > > verify any patches anybody might have, but unfortunately at 7GB of\n> > > company code and assets, I can't exactly share ;)\n> > \n> > First step is to understand what is going on. Only then could reliable \n> > patches be made.\n> \n> If you want to point me in the right direction, I have a few hours to\n> kill this evening and fscking around with gdb(1) and printf() just might\n> be some of my favorite things ;)\n\nHeh. ;-)\n\nTo start with, a simple log of what you need to do to reproduce the \nissue would be nice. Just do\n\n\tscript /tmp/foo\n\nthen reproduce the issue and exit, after which I'd be interrested in the \ncontent of /tmp/foo.\n\n\nNicolas\n"},{"id":"99539","messageId":"alpine.LFD.2.00.0901062026500.3057@localhost.localdomain","threadId":"16649","inReplyTo":"1231292360.8870.61.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-07T04:54:06Z","receivedAt":"2009-01-07T04:54:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> \n> I'll back the patch out and redeploy, it's worth mentioning that a\n> coworker of mine just got the issue as well (on 1.6.1). He was able to\n> `git pull` and the error went away, but I doubt that it \"magically fixed\n> itself\"\n\nQuite frankly, that behaviour sounds like a disk _cache_ corruption issue. \nThe fact that some corruption \"comes and goes\" and sometimes magically \nheals itself sounds very much like some disk cache problem, and then that \nparticular part of the cache gets replaced and then when re-populated it \nis magically correct.\n\nWe had that in one case with a Linux NFS client, where a rename across \ndirectories caused problems.\n\nThis was a networked filesystem on OS X, right? File caching is much more \n\"interesting\" in networked filesystems than it is in normal private \non-disk ones.\n\n> I've tarred one of the repositories that had it in a reproducible state\n> so I can create a build and extract the tar and run against that to\n> verify any patches anybody might have, but unfortunately at 7GB of\n> company code and assets, I can't exactly share ;)\n\nThe thing to do is\n\n - untar it on some trusted machine with a local disk and a known-good \n filesystem.\n\n IOW, not that networked samba share.\n\n - verify that it really does happen on that machine, with that untarred \n image. Because maybe it doesn't. \n\n The hope is that you caught the corruption in the cache, and it \n actually got written out to the tar-file. But if it _is_ a disk cache \n (well, network cache) issue, maybe the IO required to tar everything up \n was enough to flush it, and the tar-file actually _works_ because it \n got repopulated correctly.\n\n So that's why you should double-check that it really ends up being \n corrupt after being untarred again.\n\n - go back and test the original git repo on the network share, preferably \n on another client. See if the error has gone away.\n\n - If so, try to compare that known-corrupt filesystem with the original \n one: and preferably do this on another machine over the network mount. \n\n See if they differ. They obviously should *not* differ, since it's an \n tar/untar of the same files, but ...\n\nThe fact that you seem to get a _lot_ of these errors really does make it \nsound like something in your environment. It's actually really hard to get \ngit to corrupt anything. Especially objects that got packed. They've been \nquiescent for a long time, they got repacked in a very simple way, they \nare totally read-only.\n\nBut it is _not_ hard to corrupt network filesystems. It's downright \ntrivial with some of them, especially with some hardware (eg there's no \nend-to-end checksumming except for the _extremely_ weak 16-bit IP csum, \nand even that has been known to be disabled, or screwed up by ethernet \ncards that do IP packet offloading and thus computing the csum not on the \ndata that tee user actually wrote, but the data that the card received, \nwhich is not necessarily at all the same thing).\n\nAnd while ethernet uses a stronger CRC, that one is not end-to-end, so \ncorruption on the card or in a switch in between easily defeats that too. \n\nJust google for something like\n\n\t\"OS X\" SMB \"file corruption\"\n\nand you'll find quite a bit of hits. Not all that unusual.\n\n\t\t\t\tLinus\n"},{"id":"99553","messageId":"1231314099.8870.415.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901062026500.3057@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T07:41:39Z","receivedAt":"2009-01-07T07:41:39Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2009-01-06 at 20:54 -0800, Linus Torvalds wrote:\n> \n> On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > I'll back the patch out and redeploy, it's worth mentioning that a\n> > coworker of mine just got the issue as well (on 1.6.1). He was able to\n> > `git pull` and the error went away, but I doubt that it \"magically fixed\n> > itself\"\n> \n> Quite frankly, that behaviour sounds like a disk _cache_ corruption issue. \n> The fact that some corruption \"comes and goes\" and sometimes magically \n> heals itself sounds very much like some disk cache problem, and then that \n> particular part of the cache gets replaced and then when re-populated it \n> is magically correct.\n> \n> We had that in one case with a Linux NFS client, where a rename across \n> directories caused problems.\n> \n> This was a networked filesystem on OS X, right? File caching is much more \n> \"interesting\" in networked filesystems than it is in normal private \n> on-disk ones.\n\nNot quite, what I meant was that some users (not all) who've experienced\nthis issue are using Samba to copy files over directly into the Git\nrepository. I was mentioning this in case somewhere between Finder,\nSamba, ext3 and Git, some file system change events were pissing Git off\nand causing it. I don't think this is the case as the coworker that I\nmentioned earlier doesn't use Samba and neither do I (we both experience\nthe issue today, mine disappeared by upgrading to 1.6.1, his by `git\npull`).\n\n\n> \n> > I've tarred one of the repositories that had it in a reproducible state\n> > so I can create a build and extract the tar and run against that to\n> > verify any patches anybody might have, but unfortunately at 7GB of\n> > company code and assets, I can't exactly share ;)\n> \n> The thing to do is\n> \n> - untar it on some trusted machine with a local disk and a known-good \n> filesystem.\n> \n> IOW, not that networked samba share.\n> \n> - verify that it really does happen on that machine, with that untarred \n> image. Because maybe it doesn't. \n\nUnfortunately it doesn't, what I did notice was this when I did a `git\nstatus` in the directory right after untarring:\n tyler@grapefruit:~/jburgess_main> git status\n #\n # ---impressive amount of file names fly by---\n # ----snip---\n #\n # Untracked files:\n # (use \"git add ...\" to include in what will be\n committed)\n #\n # artwork/\n # bt/\n # flash/\n tyler@grapefruit:~/jburgess_main>\n\nBasically, somehow Git thinks that *every* file in the repository is\ndeleted at this point. I went ahead and performed a `git reset --hard`\nto see if the issue would manifest itself thereafter, but it did not.\n\nI did try to do a git-fsck(1), and this is what I got:\n tyler@grapefruit:~/jburgess_main> /usr/local/bin/git fsck --full\n [1] 19381 segmentation fault /usr/local/bin/git fsck --full\n tyler@grapefruit:~/jburgess_main> \n> \n> The hope is that you caught the corruption in the cache, and it \n> actually got written out to the tar-file. But if it _is_ a disk cache \n> (well, network cache) issue, maybe the IO required to tar everything up \n> was enough to flush it, and the tar-file actually _works_ because it \n> got repopulated correctly.\n\nWhen I was working through this with Jan, one of the things that we did\nwas move the actual object file in .git/objects, they existed so maybe I\ncould look into those to check?\n\n> \n> So that's why you should double-check that it really ends up being \n> corrupt after being untarred again.\n> \n> - go back and test the original git repo on the network share, preferably \n> on another client. See if the error has gone away.\n\nUnfortunately the repository is being used by the original developer I\ntarred from with our 1.6.1 build, he hasn't reported any issues, but I\ncan't exactly steal it back (that's why I made the tar)\n\n> The fact that you seem to get a _lot_ of these errors really does make\n> it \n> sound like something in your environment. It's actually really hard to get \n> git to corrupt anything. Especially objects that got packed. They've been \n> quiescent for a long time, they got repacked in a very simple way, they \n> are totally read-only.\n\nI checked with our operations team, and contrary to my suspicion (your\nNFS comment piqued my curiosity), these disks that are actually on the\nmachines are not NFS mounts but rather local disk arrays.\n \n --> is it NFSd? or all local storage\n <== all local\n <== df -h\n <== mount\n <== /dev/sda5 705G 247G 423G 37% /nail\n --> hm, there goes that theory\n <== git corruption?\n --> yeah, looking into it\n <== sucks\n --> Linus had a theory about NFS/etc corruption of the disk\n cache\n <== when the company folds we can all blame you... and your\n silly git games\n <== (think positive, joel)\n --> thanks \n \n;)\n\n\nAny thing else I can do to help debug this? :-/\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99557","messageId":"7vaba3bken.fsf@gitster.siamese.dyndns.org","threadId":"16649","inReplyTo":"1231314099.8870.415.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T08:16:48Z","receivedAt":"2009-01-07T08:16:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"R. Tyler Ballance\" writes:\n\n> Unfortunately it doesn't, what I did notice was this when I did a `git\n> status` in the directory right after untarring:\n> tyler@grapefruit:~/jburgess_main> git status\n> #\n> # ---impressive amount of file names fly by---\n> # ----snip---\n> ...\n> Basically, somehow Git thinks that *every* file in the repository is\n> deleted at this point.\n\nThat makes me suspect that your .git/index file is corrupt.\n"},{"id":"99559","messageId":"1231317131.8870.471.camel@starfruit","threadId":"16649","inReplyTo":"7vaba3bken.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T08:32:11Z","receivedAt":"2009-01-07T08:32:11Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 00:16 -0800, Junio C Hamano wrote:\n> \"R. Tyler Ballance\" writes:\n> \n> > Unfortunately it doesn't, what I did notice was this when I did a `git\n> > status` in the directory right after untarring:\n> > tyler@grapefruit:~/jburgess_main> git status\n> > #\n> > # ---impressive amount of file names fly by---\n> > # ----snip---\n> > ...\n> > Basically, somehow Git thinks that *every* file in the repository is\n> > deleted at this point.\n> \n> That makes me suspect that your .git/index file is corrupt.\n\nWould this be tied to the corrupted pack file issue, or separate.\n\nEither way, how could I verify your assumptions? (i'll be lurking in\n#git for a while if you want to interactively help ;))\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99561","messageId":"1231319112.8870.506.camel@starfruit","threadId":"16649","inReplyTo":"1231314099.8870.415.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T09:05:12Z","receivedAt":"2009-01-07T09:05:12Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Tue, 2009-01-06 at 23:41 -0800, R. Tyler Ballance wrote:\n\n> I did try to do a git-fsck(1), and this is what I got:\n> tyler@grapefruit:~/jburgess_main> /usr/local/bin/git fsck --full\n> [1] 19381 segmentation fault /usr/local/bin/git fsck --full\n> tyler@grapefruit:~/jburgess_main> \n> > \n\nDisregard this comment, Jan reminded me in IRC of the issue I had with\nthis repository and the stack-size ulimit. A healthy 32M stack-size\nallows for the command to complete:\n\n tyler@grapefruit:~/jburgess_main> /usr/local/bin/git fsck --full\n dangling blob 6e58a06cd0f027fce1fc8a923a8c81d6b55f1705\n dangling blob 5e87e049f69ee06af1f4f92a3d4ddcd912f8535e\n dangling blob acb8e0937cea3f4068c5c67f60d3b97952654fa1\n dangling blob 8bd540d657027ab12228e0522de86a97d3f8a7a9\n dangling blob 90d93096369df4b2151df3e289952297c5f390dd\n dangling blob aaa4a1bf9e6d39991503b7908ff71605d6632fef\n dangling blob b003e20f06de7d0a7e11a1047fbb39e2deb84899\n dangling blob bf207363786f08e888a725cfb8b2fe4bea11ceab\n dangling blob 642493a1bcf09f74551f0ce5b4c3ccc23acaf3c9\n dangling blob ff80b45e48795d4544d0a5b2f18714123ab21746\n dangling blob e3c0e67e23ec588acab733e2069fb09fa38a235d\n dangling blob 62efa65bcbb6d94f26be9b91dc98d7a7f6fbf602\n dangling blob a6a2c717c230fe26263b9ce002f3f82fdab6f727\n dangling blob d32988875aa90ef728dc1af6b71c130f2c8e8b94\n dangling blob c8aad9fc37a27872bae18af78b1623bfb8f9a9f7\n dangling blob 4cdbe97301e333d89037dc9f4a8440dab9e62049\n dangling blob 2819bbcda3b0efe828709f5b22624712cb9ebdae\n dangling blob b156cba3ee89e1506d02fbb845e8dc0889ff4090\n dangling blob ed658b8beca69ebf6e25ab1ed6217881e27d4e9a\n dangling blob d581cb2fb1cf2b4d15f8f9b90b08a3ebc619414f\n dangling blob a85c2e55d27c9fb1f5874f8bd81c65e597327b4f\n dangling blob 548f3e70c06f9306d71f6b8d11ed24ade581fa31\n dangling blob 7a1c8fa8bc59c4e8fb347426cc11fabf8b0d1639\n tyler@grapefruit:~/jburgess_main> \n \nCheers\n\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99563","messageId":"7vaba3a1w4.fsf@gitster.siamese.dyndns.org","threadId":"16649","inReplyTo":"1231317131.8870.471.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T09:42:03Z","receivedAt":"2009-01-07T09:42:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"R. Tyler Ballance\" writes:\n\n> On Wed, 2009-01-07 at 00:16 -0800, Junio C Hamano wrote:\n>> \"R. Tyler Ballance\" writes:\n>> \n>> > Unfortunately it doesn't, what I did notice was this when I did a `git\n>> > status` in the directory right after untarring:\n>> > tyler@grapefruit:~/jburgess_main> git status\n>> > #\n>> > # ---impressive amount of file names fly by---\n>> > # ----snip---\n>> > ...\n>> > Basically, somehow Git thinks that *every* file in the repository is\n>> > deleted at this point.\n>> \n>> That makes me suspect that your .git/index file is corrupt.\n>\n> Would this be tied to the corrupted pack file issue, or separate.\n\nIf you have perfectly good set of packs, if your index is corrupt you may\nsee \"everything deleted\", so in that sense it is independent.\n\nAs Linus's earlier conjecture was that this is related to some sort of\ndisk/cache corruption, I wouldn't be surprised if such a failure hit packs\nand the index file indiscriminatingly. So in that sense they are\nrelated.\n\nI think \"git ls-files\" (before doing anything else, such as resetting, of\ncourse) would report that the index is corrupt, if that is indeed the case.\n"},{"id":"99587","messageId":"alpine.LFD.2.00.0901071008150.26118@xanadu.home","threadId":"16649","inReplyTo":"1231314099.8870.415.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-01-07T15:31:29Z","receivedAt":"2009-01-07T15:31:29Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n\n> On Tue, 2009-01-06 at 20:54 -0800, Linus Torvalds wrote:\n> > \n> > On Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> > > \n> > > I'll back the patch out and redeploy, it's worth mentioning that a\n> > > coworker of mine just got the issue as well (on 1.6.1). He was able to\n> > > `git pull` and the error went away, but I doubt that it \"magically fixed\n> > > itself\"\n> > \n> > Quite frankly, that behaviour sounds like a disk _cache_ corruption issue. \n> > The fact that some corruption \"comes and goes\" and sometimes magically \n> > heals itself sounds very much like some disk cache problem, and then that \n> > particular part of the cache gets replaced and then when re-populated it \n> > is magically correct.\n> > \n> > We had that in one case with a Linux NFS client, where a rename across \n> > directories caused problems.\n> > \n> > This was a networked filesystem on OS X, right? File caching is much more \n> > \"interesting\" in networked filesystems than it is in normal private \n> > on-disk ones.\n> \n> Not quite, what I meant was that some users (not all) who've experienced\n> this issue are using Samba to copy files over directly into the Git\n> repository. I was mentioning this in case somewhere between Finder,\n> Samba, ext3 and Git, some file system change events were pissing Git off\n> and causing it.\n\nAs long as those files are not within the .git directory that should be \nfine.\n\n> I don't think this is the case as the coworker that I\n> mentioned earlier doesn't use Samba and neither do I (we both experience\n> the issue today, mine disappeared by upgrading to 1.6.1, his by `git\n> pull`).\n\nProblem is that none of that \"makes sense\". If a real git corruption \nwas there, it wouldn't disappear without explicit action from your part. \nWhat git v1.6.1 is able to do over earlier versions is to still function \nproperly if some corrupted objects have a redundant copy in the same \nrepository, but it wouldn't stop complaining about the existence of \ncorrupted data. Doing a 'git gc' or 'git repack -a' might get rid of \nthe corruption. And a 'git pull' might hide it if the received pack \nduring the pull operation happens to contain another copy of the object \nthat was corrupted before, but it wouldn't prevent 'git fsck --full' \nfrom seeing it.\n\nThe fact that you cannot reproduce the corruption issues after \nunarchiving a repository that was known to have problems right before it \nwas archived is really really strange. That does not rule out git bugs \nof course, but at least this shows that no actual corruption on disk was \ninitially involved.\n\nAgain, I'd suggest you perform your git usage within a script session so \nto capture the exact operation performed and error messages produced \nwhen/if similar problems do occur again. Otherwise we're only running \nafter our tail.\n\n\nNicolas\n"},{"id":"99592","messageId":"alpine.LFD.2.00.0901070743070.3057@localhost.localdomain","threadId":"16649","inReplyTo":"1231314099.8870.415.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-07T16:07:25Z","receivedAt":"2009-01-07T16:07:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 6 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > The thing to do is\n> > \n> > - untar it on some trusted machine with a local disk and a known-good \n> > filesystem.\n> > \n> > IOW, not that networked samba share.\n> > \n> > - verify that it really does happen on that machine, with that untarred \n> > image. Because maybe it doesn't. \n> \n> Unfortunately it doesn't\n\nWell, that's not necessarily \"unfortunate\". It does actually end up \nshowing that the objects themselves were apparently never really corrupt.\n\nSo there is no fundamental data structure corrupttion - because when you \ncopy the repository, it's all good agin!\n\nIn other words, that's not a worthless piece of information at all, and it \ndoes tell us a lot, namely that the corruption was never real long-term \ndata corruption of the git object archive, but something local and \ntemporary. Again, we're really back to either:\n\n - it could be some _temporary_ git corruption caused internally inside a \n git process - ie a wild pointer, or perhaps a race condition (but we \n don't really use threading in 1.6.0.4 unless you ask for it, and even \n then just for pack-file generation)\n\n - or it's the disk cache corruption, and the tar/untar ended up flushing \n it.\n\nAnd quite frankly, since the corruption seems to be site-specific, I \nreally do suspect the second case. Although it's possible, of course, that \nit could be some compiler issue that makes _your_ binaries have issues \neven when nobody else sees it.\n\n> what I did notice was this when I did a `git\n> status` in the directory right after untarring:\n> tyler@grapefruit:~/jburgess_main> git status\n> #\n> # ---impressive amount of file names fly by---\n> # ----snip---\n> #\n> # Untracked files:\n> # (use \"git add ...\" to include in what will be\n> committed)\n> #\n> # artwork/\n> # bt/\n> # flash/\n\nHmm. That's actually _normal_ under some circumstances. At least with \nolder git versions, or if your .git/index file couldn't be rewritten for \nsome reason - your existing index file contains all the old stat \ninformation, and if git cannot (or, in the case of older git version, just \nwill not) refresh it automatically, it will show all the files as changed, \neven if it's just the inode number that really changed.\n\nA _normal_ git install should have auto-refreshed the index, though. \nUnless the tar archive only contained the \".git\" directory, and not the \ncheckout?\n\n> tyler@grapefruit:~/jburgess_main>\n> \n> Basically, somehow Git thinks that *every* file in the repository is\n> deleted at this point. I went ahead and performed a `git reset --hard`\n> to see if the issue would manifest itself thereafter, but it did not.\n\nThat would be what I'd expect if you had only tarred up .git, although \nthen I wouldn't have expected those \"Untracked files:\". Hmm. Without being \nable to look at the archive, I'm just guessing randomly.\n\n> I did try to do a git-fsck(1), and this is what I got:\n> tyler@grapefruit:~/jburgess_main> /usr/local/bin/git fsck --full\n> [1] 19381 segmentation fault /usr/local/bin/git fsck --full\n> tyler@grapefruit:~/jburgess_main> \n\n.. and that's the unrelated fsck bug that got fixed later.\n\n> > The hope is that you caught the corruption in the cache, and it \n> > actually got written out to the tar-file. But if it _is_ a disk cache \n> > (well, network cache) issue, maybe the IO required to tar everything up \n> > was enough to flush it, and the tar-file actually _works_ because it \n> > got repopulated correctly.\n> \n> When I was working through this with Jan, one of the things that we did\n> was move the actual object file in .git/objects, they existed so maybe I\n> could look into those to check?\n\nYes. If you have any bad loose objects, if you compare them to the good \nobjects with the same name, that's going to be interesting information. \nThe pattern of corruption can be very telling. For example, on Linux, a \ndisk cache corruption would usually be at 4kB block boundaries, because \nthat's the granularity of the cache. While a bit error would be obvious \netc etc.\n\n> I checked with our operations team, and contrary to my suspicion (your\n> NFS comment piqued my curiosity), these disks that are actually on the\n> machines are not NFS mounts but rather local disk arrays.\n\nOk, that generally makes caching much simpler. What filesystem?\n\nIs there anything else that you do that is site-specific and/or slightly \ndifferent? For example, a long time ago we had a bug related to CRLF \nconversion which would cause a use-after-free problem, and that would \ncorrupt the data internally to git.\n\nAnd dobody else saw it than this one person, and it was a total mystery to \neverybody until we realized that he used this one feature that nobody else \nwas using. So as you're on OS X, I assume you don't have CRLF conversion, \nbut maybe you use some other feature that we support but nobody really \nactually uses. Like keyword expansion or something?\n\nOh - that would also explain why you got all those entries in \"git status\" \nthat went away when you did a \"git reset --hard\": if you had some keyword \nexpansion (or CRLF) enabled in the original users \"~/.gitconfig\", that \ncheckout would have had expansion/CRLF/whatever conversion, but then when \nyou tarred/untarred it on another setup, the expansion would be seen as a \ndifference because it wasn't enabled.\n\nHmm?\n\n\t\tLinus\n"},{"id":"99593","messageId":"alpine.LFD.2.00.0901070808000.3057@localhost.localdomain","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901070743070.3057@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-07T16:08:59Z","receivedAt":"2009-01-07T16:08:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Linus Torvalds wrote:\n> \n> And dobody else saw it than this one person, and it was a total mystery to \n> everybody until we realized that he used this one feature that nobody else \n> was using. So as you're on OS X, I assume you don't have CRLF conversion, \n> but maybe you use some other feature that we support but nobody really \n> actually uses. Like keyword expansion or something?\n> \n> Oh - that would also explain why you got all those entries in \"git status\" \n> that went away when you did a \"git reset --hard\": if you had some keyword \n> expansion (or CRLF) enabled in the original users \"~/.gitconfig\", that \n> checkout would have had expansion/CRLF/whatever conversion, but then when \n> you tarred/untarred it on another setup, the expansion would be seen as a \n> difference because it wasn't enabled.\n\nBtw, if you untar it again, and just do a \"git diff\", that should show any \nsuch effects. Rather than showing just that something changed, it should \nshow _how_ it changed.\n\n\t\tLinus\n"},{"id":"99626","messageId":"1231368935.8870.584.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901070743070.3057@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-07T22:55:35Z","receivedAt":"2009-01-07T22:55:35Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 08:07 -0800, Linus Torvalds wrote:\n> Well, that's not necessarily \"unfortunate\". It does actually end up \n> showing that the objects themselves were apparently never really corrupt.\n> \n> So there is no fundamental data structure corrupttion - because when you \n> copy the repository, it's all good agin!\n> - it could be some _temporary_ git corruption caused internally inside a \n> git process - ie a wild pointer, or perhaps a race condition (but we \n> don't really use threading in 1.6.0.4 unless you ask for it, and even \n> then just for pack-file generation)\n\nI have a feeling it's something like this, one of our operations guys\ndid some research while I was looking at code and he came across this:\n\n On Wed, 2009-01-07 at 14:17 -0800, Ken Brownfield wrote:\n git-merge is using too much RAM, and failing to malloc() but\n NOT \n > reporting it. This is all sorts of bad:\n > \n > A) using an unscalable amount of RAM\n > B) failing to detect malloc() failure\n > C) reporting file corruption instead\n > I was able to reproduce this.\n >\n > limit ~1.5GB -> corrupt file\n > limit ~3GB -> magically no longer corrupt.\n >\n > The false fail may be limited to git-merge, but git status also \n > allocates the same amount of RAM.\n > \n > To temporarily work around this problem, issue this once you\n log in to \n > a dev box:\n > \n > tcsh:\n > limit vmemoryuse 3000000\n > bash:\n > ulimit -v 3000000\n > \n > Be gentle.\n \n\n> And quite frankly, since the corruption seems to be site-specific, I \n> really do suspect the second case. Although it's possible, of course, that \n> it could be some compiler issue that makes _your_ binaries have issues \n> even when nobody else sees it.\n\nI think you're correct insofar that our major site-specific alteration\nhas come up on the mailing list before (okay maybe two site-specific\nthings). \n\t* Our Git repo is ~7.1GB\n\t* ulimit -v is set to ~1.5G\n\n\nI think I know how this could be failing and corrupting things (assuming\nit's malloc(2)) related.\n\n\nWhat I'm thinking is that in xmalloc() or one of the other x*)_\nfunctions, the malloc(size) is failing because of the ulimits, and then\nthe potentially somewhere it's silently failing or maybe even\naccidentally returning one of those \"malloc(1)\" pointers?\n\nI've got two new tarred repositories from two developers the issue\nhappened to today, so I'm flush full of sample repositories to try stuff\non :)\n\n\n> \n> Hmm. That's actually _normal_ under some circumstances. At least with \n> older git versions, or if your .git/index file couldn't be rewritten for \n> some reason - your existing index file contains all the old stat \n> information, and if git cannot (or, in the case of older git version, just \n> will not) refresh it automatically, it will show all the files as changed, \n> even if it's just the inode number that really changed.\n> \n> A _normal_ git install should have auto-refreshed the index, though. \n> Unless the tar archive only contained the \".git\" directory, and not the \n> checkout?\n\nI believe the issues I noticed when untarring the repo were a red\nherring, I did the `git diff` after untarring and I noticed that only a\ncertain set of files where changed, I'm willing to go so far as to guess\nthat they were the files affected in the corrupted packs. Of the 32k\nfiles in our repository, 98 were actually different after untarring\n(according to git-diff(1))\n\n> And dobody else saw it than this one person, and it was a total mystery to \n> everybody until we realized that he used this one feature that nobody else \n> was using. So as you're on OS X, I assume you don't have CRLF conversion, \n> but maybe you use some other feature that we support but nobody really \n> actually uses. Like keyword expansion or something?\n\nThe two new folks this happened to today had nothing \"special\" about\nthem other than the ulimit.\n\n\nI've got the script(1) output of performing git-ls-files(1) and some\nother commands that I tried, nothing they output was particular\ninformative or interesting, and I don't think it will help if this\nreally is a memory related issue, that said I'd be more than happy to\nsend it to a couple of you (Junio, Linus, Nico).\n\n\nI'm *so* ready for this bug to die >=\\\n\n\nCheers\n\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99633","messageId":"alpine.LFD.2.00.0901071520330.3057@localhost.localdomain","threadId":"16649","inReplyTo":"1231368935.8870.584.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-07T23:29:26Z","receivedAt":"2009-01-07T23:29:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n>\n> > git process - ie a wild pointer, or perhaps a race condition (but we \n> > don't really use threading in 1.6.0.4 unless you ask for it, and even \n> > then just for pack-file generation)\n> \n> I have a feeling it's something like this, one of our operations guys\n> did some research while I was looking at code and he came across this:\n> \n> On Wed, 2009-01-07 at 14:17 -0800, Ken Brownfield wrote:\n> git-merge is using too much RAM, and failing to malloc() but\n> NOT \n> > reporting it. This is all sorts of bad:\n> > \n> > A) using an unscalable amount of RAM\n> > B) failing to detect malloc() failure\n> > C) reporting file corruption instead\n\nWell, I dont' think that's exactly it. git internally doesn't really use \nmalloc at all, and uses xmalloc() instead which will die() if the malloc \nfails. So there's almost certainly no \"failing to detect failures\"\n\nYes, there's a few places that don't use the wrapper, but they should be \nsafe (eg either they SIGSEGV, or they are like create_delta_index() and \njust create a sub-optimal pack with a warning).\n\nHOWEVER:\n\n> > I was able to reproduce this.\n> >\n> > limit ~1.5GB -> corrupt file\n> > limit ~3GB -> magically no longer corrupt.\n\nThat is interesting, although I also worry that there might be other \nissues going on (ie since you've reported thigns magically fixing \nthemselves, maybe the ulimit tests just _happened_ to show that, even if \nit wasn't the core reason).\n\nBUT! This is definitely worth looking at.\n\nFor example, we do have some cases where we try to do \"mmap()\", and if it \nfails, we try to free some memory and try again. In particular, in \nxmmap(), if an mmap() fails - which may be due to running out of virtual \naddress space - we'll actually try to release some pack-file memory and \ntry again. Maybe there's a bug there - and it would be one that seldom \ntriggers for others.\n\n> I think you're correct insofar that our major site-specific alteration\n> has come up on the mailing list before (okay maybe two site-specific\n> things). \n> \t* Our Git repo is ~7.1GB\n> \t* ulimit -v is set to ~1.5G\n\nIt is certainly possible. It's too bad that it's private, because it makes \nit _much_ harder to try to pinpoint this.\n\n\t\t\t\tLinus\n"},{"id":"99640","messageId":"1231374514.8870.621.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071520330.3057@localhost.localdomain","subject":"Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T00:28:34Z","receivedAt":"2009-01-08T00:28:34Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 15:29 -0800, Linus Torvalds wrote:\n> It is certainly possible. It's too bad that it's private, because it makes \n> it _much_ harder to try to pinpoint this.\n\nMy most esteemed colleague (Ken aka kb) who pointed out the memory issue\nwas on the right path (I think), and I have a reproduction case you can\ntry with your very own Linux kernel tree!\n\nWOO!\n\nI set ulimit -v really low (150M), and the operations I made got an\nmmap(2) fatal error, but there is a sweet spot that I found, see the\ntranscript below. I basically chose an arbitrary revision from a couple\nof weeks ago, and rolled the repository back to that point, then I tried\nwith iterations of ulimit -v 150, 250, 450, and then back down to 350.\n\n tyler@grapefruit:~/source/git/linux-2.6> limit\n cputime unlimited\n filesize unlimited\n datasize unlimited\n stacksize 8MB\n coredumpsize 0kB\n memoryuse 2561MB\n maxproc 24564\n descriptors 1024\n memorylocked 64kB\n addressspace unlimited\n maxfilelocks unlimited\n sigpending 24564\n msgqueue 819200\n nice 0\n rt_priority 0\n tyler@grapefruit:~/source/git/linux-2.6> export\n START=56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n tyler@grapefruit:~/source/git/linux-2.6> git reset --hard $START\n HEAD is now at 56d18e9 Merge branch 'upstream' of\n git://ftp.linux-mips.org/pub/scm/upstream-linus\n tyler@grapefruit:~/source/git/linux-2.6> ulimit -v `echo \"350 *\n 1024\" | bc -l`\n tyler@grapefruit:~/source/git/linux-2.6> git pull\n error: failed to read object\n be1b87c70af69acfadb8a27a7a76dfb61de92643 at offset 1850923\n from .git/objects/pack/pack-dbe154052997a05499eb6b4fd90b924da68e799a.pack\n fatal: object be1b87c70af69acfadb8a27a7a76dfb61de92643 is\n corrupted\n tyler@grapefruit:~/source/git/linux-2.6>\n \nI've tried this a couple of times, and it does seem to be reproducible,\nlet me know if you have any issues reproducing it locally and I'll try\nto dig into it more with valgrind or something a bit more pin-pointing\nthan \"ulimit -v && try, try again\"\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99641","messageId":"alpine.LFD.2.00.0901071621340.3283@localhost.localdomain","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071520330.3057@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T00:37:03Z","receivedAt":"2009-01-08T00:37:03Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Linus Torvalds wrote:\n>\n> > > limit ~1.5GB -> corrupt file\n> > > limit ~3GB -> magically no longer corrupt.\n> \n> That is interesting, although I also worry that there might be other \n> issues going on (ie since you've reported thigns magically fixing \n> themselves, maybe the ulimit tests just _happened_ to show that, even if \n> it wasn't the core reason).\n> \n> BUT! This is definitely worth looking at.\n> \n> For example, we do have some cases where we try to do \"mmap()\", and if it \n> fails, we try to free some memory and try again. In particular, in \n> xmmap(), if an mmap() fails - which may be due to running out of virtual \n> address space - we'll actually try to release some pack-file memory and \n> try again. Maybe there's a bug there - and it would be one that seldom \n> triggers for others.\n\nHo humm. We really do have some interesting things there. \n\nIs this a 64-bit machine? I didn't think OS X did that, but if there is \nsome limited 64-bit support there, maybe \"sizeof(void *)\" is 8, then we \ndefault the default git pack-window to a pretty healthy 1GB.\n\nI could easily see that if you have a virtual memory size limit of 1.5GB, \nand the pack window size is 1GB, we might have trouble. Because we could \nonly keep one such pack window in memory at a time.\n\nI have _not_ looked at the code, though. I'd have expected a SIGSEGV if we \nreally had issues with the window handling.\n\nAnyway, _if_ your system has 64-bit pointers, then _maybe_ something the \ndefault 1GB pack window causes problem.\n\nIf so, then adding a\n\n\t[core]\n\t\tpackedgitwindowsize = 64M\n\nmight make a difference. It would certainly be very interesting to hear if \nthere's any impact.\n\n\t\tLinus\n"},{"id":"99643","messageId":"alpine.LFD.2.00.0901071644330.3283@localhost.localdomain","threadId":"16649","inReplyTo":"1231374514.8870.621.camel@starfruit","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T00:48:42Z","receivedAt":"2009-01-08T00:48:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n>\n> My most esteemed colleague (Ken aka kb) who pointed out the memory issue\n> was on the right path (I think), and I have a reproduction case you can\n> try with your very own Linux kernel tree!\n> \n> WOO!\n> \n> I set ulimit -v really low (150M), and the operations I made got an\n> mmap(2) fatal error, but there is a sweet spot that I found, see the\n> transcript below.\n\nThis is indeed the packfile mapping. The sweet spot you found depends on \nhow big the biggest two pack-files are, I do believe.\n\nAnd if you do that\n\n\t[core]\n\t\tpackedgitwindowsize = 64M\n\nI think you'll find that it works. Of course, with a _really_ low ulimit, \nyou'd need to make it even smaller, but at some point you start hitting \nother problems than the pack-file limits, ie just the simple fact that git \nwants and expects you to have a certain amount of memory available ;)\n\nCan you cnfirm that your \"reproducible\" case starts working with that \naddition to your ~/.gitconfig? If so, the solution is pretty simple: we \nshould just lower the default pack windowsize.\n\n\t\tLinus\n"},{"id":"99644","messageId":"1231375780.8870.629.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071621340.3283@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T00:49:40Z","receivedAt":"2009-01-08T00:49:40Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 16:37 -0800, Linus Torvalds wrote:\n> \n> On Wed, 7 Jan 2009, Linus Torvalds wrote:\n> >\n> > > > limit ~1.5GB -> corrupt file\n> > > > limit ~3GB -> magically no longer corrupt.\n> > \n> > That is interesting, although I also worry that there might be other \n> > issues going on (ie since you've reported thigns magically fixing \n> > themselves, maybe the ulimit tests just _happened_ to show that, even if \n> > it wasn't the core reason).\n> > \n> > BUT! This is definitely worth looking at.\n> > \n> > For example, we do have some cases where we try to do \"mmap()\", and if it \n> > fails, we try to free some memory and try again. In particular, in \n> > xmmap(), if an mmap() fails - which may be due to running out of virtual \n> > address space - we'll actually try to release some pack-file memory and \n> > try again. Maybe there's a bug there - and it would be one that seldom \n> > triggers for others.\n> \n> Ho humm. We really do have some interesting things there. \n\nAlways enjoyable when these mail threads get this deep ;)\n\n> \n> Is this a 64-bit machine? I didn't think OS X did that, but if there is \n> some limited 64-bit support there, maybe \"sizeof(void *)\" is 8, then we \n> default the default git pack-window to a pretty healthy 1GB.\n\nI was only mentioning OS X with regards to the Samba/NFS red herring,\nthe rest of our operations are on 64-bit Linux machines.\n\nThe machine I reproduced this on (\"Public repo case!\") is the following:\n tyler@grapefruit:~> uname -a\n Linux grapefruit.corp.slide.com 2.6.27.7-9-default #1 SMP\n 2008-12-04 18:10:04 +0100 x86_64 x86_64 x86_64 GNU/Linux\n tyler@grapefruit:~> cat /etc/issue\n Welcome to openSUSE 11.1 - Kernel \\r (\\l).\n \nThe machines we're experiencing this issue on \"in the wild\" are:\n xdev3 (master)% uname -a \n Linux xdev3 2.6.24-22-server #1 SMP Mon Nov 24 20:06:28 UTC 2008\n x86_64 GNU/Linux\n xdev3 (master)% cat /etc/issue\n Ubuntu 8.04.1 \\n \\l\n> \n> I could easily see that if you have a virtual memory size limit of 1.5GB, \n> and the pack window size is 1GB, we might have trouble. Because we could \n> only keep one such pack window in memory at a time.\n\nThe DEFAULT_PACKED_GIT_WINDOW_SIZE in our local builds is 256M, FWIW\n\n> \n> I have _not_ looked at the code, though. I'd have expected a SIGSEGV if we \n> really had issues with the window handling.\n> \n> Anyway, _if_ your system has 64-bit pointers, then _maybe_ something the \n> default 1GB pack window causes problem.\n> \n> If so, then adding a\n> \n> \t[core]\n> \t\tpackedgitwindowsize = 64M\n> \n> might make a difference. It would certainly be very interesting to hear if \n> there's any impact.\n\nI can try this still if you'd like, but it doesn't seem like that'd be\nthe issue since we're already lowering the window size system-wide\n\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99646","messageId":"1231376259.8870.633.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071644330.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T00:57:39Z","receivedAt":"2009-01-08T00:57:39Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 16:48 -0800, Linus Torvalds wrote:\n> \n> On Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n> >\n> > My most esteemed colleague (Ken aka kb) who pointed out the memory issue\n> > was on the right path (I think), and I have a reproduction case you can\n> > try with your very own Linux kernel tree!\n> > \n> > WOO!\n> > \n> > I set ulimit -v really low (150M), and the operations I made got an\n> > mmap(2) fatal error, but there is a sweet spot that I found, see the\n> > transcript below.\n> \n> This is indeed the packfile mapping. The sweet spot you found depends on \n> how big the biggest two pack-files are, I do believe.\n> \n> And if you do that\n> \n> \t[core]\n> \t\tpackedgitwindowsize = 64M\n> \n> I think you'll find that it works. Of course, with a _really_ low ulimit, \n> you'd need to make it even smaller, but at some point you start hitting \n> other problems than the pack-file limits, ie just the simple fact that git \n> wants and expects you to have a certain amount of memory available ;)\n> \n> Can you cnfirm that your \"reproducible\" case starts working with that \n> addition to your ~/.gitconfig? If so, the solution is pretty simple: we \n> should just lower the default pack windowsize.\n\nThis certainly corrected the issue, is there some magic\npackedgitwindowsize that i should be looking at my own repository (our\ninternal one) in order to prevent the issue from occurring? \n\nLooking into .git/objects/pack, I think the two biggest pack files are\n3.5G and 177MBG respectively :-!\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99647","messageId":"alpine.LFD.2.00.0901071652490.3283@localhost.localdomain","threadId":"16649","inReplyTo":"1231375780.8870.629.camel@starfruit","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T01:01:53Z","receivedAt":"2009-01-08T01:01:53Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n> \n> I was only mentioning OS X with regards to the Samba/NFS red herring,\n> the rest of our operations are on 64-bit Linux machines.\n\nAhh, ok. Good. \n\n> > I could easily see that if you have a virtual memory size limit of 1.5GB, \n> > and the pack window size is 1GB, we might have trouble. Because we could \n> > only keep one such pack window in memory at a time.\n> \n> The DEFAULT_PACKED_GIT_WINDOW_SIZE in our local builds is 256M, FWIW\n\nInteresting. So you already had to lower it. However, now that you mention \nit, and now that I search for your emails about it on the mailing list (I \ndon't normally read the mailing list except very occasionally), I see your \npatch that does\n\n\t#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0.85\n\t...\n\tpacked_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n\nwhich is actually very bad.\n\nIt's bad for several reasons:\n\n - 85% of the virtual address space is actually pessimal.\n\n You need space for AT LEAST two full-sized windows, so you need less \n than 50%.\n\n - the way that variable is used, it _has_ to be a multiple of the page \n size. In fact, it needs to be a multiple of _twice_ the page size. So \n just doing a random fraction of the rlimit is not correct.\n\nSetting it in the .gitconfig does it right, though.\n\n> > If so, then adding a\n> > \n> > \t[core]\n> > \t\tpackedgitwindowsize = 64M\n> > \n> > might make a difference. It would certainly be very interesting to hear if \n> > there's any impact.\n> \n> I can try this still if you'd like, but it doesn't seem like that'd be\n> the issue since we're already lowering the window size system-wide\n\nPlease do try, at least if your local git changes still match that patch I \nfound, because that patch generates problems.\n\n\t\tLinus\n"},{"id":"99648","messageId":"1231376802.8870.635.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071652490.3283@localhost.localdomain","subject":"Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T01:06:42Z","receivedAt":"2009-01-08T01:06:42Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 17:01 -0800, Linus Torvalds wrote:\n> \n> On Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > I was only mentioning OS X with regards to the Samba/NFS red herring,\n> > the rest of our operations are on 64-bit Linux machines.\n> \n> Ahh, ok. Good. \n> \n> > > I could easily see that if you have a virtual memory size limit of 1.5GB, \n> > > and the pack window size is 1GB, we might have trouble. Because we could \n> > > only keep one such pack window in memory at a time.\n> > \n> > The DEFAULT_PACKED_GIT_WINDOW_SIZE in our local builds is 256M, FWIW\n> \n> Interesting. So you already had to lower it. However, now that you mention \n> it, and now that I search for your emails about it on the mailing list (I \n> don't normally read the mailing list except very occasionally), I see your \n> patch that does\n> \n> \t#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0.85\n> \t...\n> \tpacked_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n> \n> which is actually very bad.\n> \n> It's bad for several reasons:\n> \n> - 85% of the virtual address space is actually pessimal.\n> \n> You need space for AT LEAST two full-sized windows, so you need less \n> than 50%.\n> \n> - the way that variable is used, it _has_ to be a multiple of the page \n> size. In fact, it needs to be a multiple of _twice_ the page size. So \n> just doing a random fraction of the rlimit is not correct.\n\nThis patch never made it into any of our Git builds because my flight\nlanded and it wasn't stable enough (and as you pointed out, it sucks ;))\n\n\n\n> \n> Setting it in the .gitconfig does it right, though.\n> \n> > > If so, then adding a\n> > > \n> > > \t[core]\n> > > \t\tpackedgitwindowsize = 64M\n> > > \n> > > might make a difference. It would certainly be very interesting to hear if \n> > > there's any impact.\n> > \n> > I can try this still if you'd like, but it doesn't seem like that'd be\n> > the issue since we're already lowering the window size system-wide\n> \n> Please do try, at least if your local git changes still match that patch I \n> found, because that patch generates problems.\n\nSee my prior reply in \"Public repo case!\" sent at 4:57PST\n\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99649","messageId":"alpine.LFD.2.00.0901071702190.3283@localhost.localdomain","threadId":"16649","inReplyTo":"1231376259.8870.633.camel@starfruit","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T01:08:28Z","receivedAt":"2009-01-08T01:08:28Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > Can you cnfirm that your \"reproducible\" case starts working with that \n> > addition to your ~/.gitconfig? If so, the solution is pretty simple: we \n> > should just lower the default pack windowsize.\n> \n> This certainly corrected the issue, is there some magic\n> packedgitwindowsize that i should be looking at my own repository (our\n> internal one) in order to prevent the issue from occurring? \n> \n> Looking into .git/objects/pack, I think the two biggest pack files are\n> 3.5G and 177MBG respectively :-!\n\nSo there's a few rules to packedgitwindowsize:\n\n - we need to be able to have at least two windows open at a time, in \n addition to all the \"normal\" memory git needs just for objects, of \n course. And quite frankly, you'd be better off with a few more windows, \n even if that obviously implies smaller windows.\n\n - the window size really wants to be a round power-of-two number, and at \n _least_ it wants to be a nice multiple of the 2*page size.\n\nSo if you have a virtual memory limit of 1.5GB, I'd hesitate to make the \npack window size less than 512M, and 256M is probably better. That way, \nI'd expect you to be able to always have at least four windows open \n(assuming a reasonably generous half a gigabyte for \"other stuff\").\n\nAnd quite frankly, there's not a huge downside to making them smaller. At \n\"just\" 32MB, you'll still fit plenty of data in one pack window, and while \nit will cost you a few mmap/unmap's to switch windows around, most \noperations simply will not likely ever notice. At least not under Linux, \nwhere mmap/munmap is pretty cheap.\n\n\t\tLinus\n"},{"id":"99651","messageId":"alpine.LFD.2.00.0901071726020.3283@localhost.localdomain","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071702190.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T01:29:16Z","receivedAt":"2009-01-08T01:29:16Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Linus Torvalds wrote:\n> \n> So there's a few rules to packedgitwindowsize:\n> \n> - we need to be able to have at least two windows open at a time, in \n> addition to all the \"normal\" memory git needs just for objects, of \n> course. And quite frankly, you'd be better off with a few more windows, \n> even if that obviously implies smaller windows.\n\nBtw, I'm not 100% certain of this. Somebody should double-check me. Maybe \nthere are cases where we want more than two windows alive. And maybe there \naren't even that, and we can always make do with just one.\n\nSo I will _not_ guarantee that \"at least two pack windows\" is necessarily \nthe right answer. The windowing code was mostly other people doing it. I \nthink Shawn and Nico.\n\n\t\t\tLinus\n"},{"id":"99652","messageId":"20090108014646.GD10790@spearce.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071726020.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T01:46:46Z","receivedAt":"2009-01-08T01:46:46Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> On Wed, 7 Jan 2009, Linus Torvalds wrote:\n> > \n> > So there's a few rules to packedgitwindowsize:\n> > \n> > - we need to be able to have at least two windows open at a time, in \n> > addition to all the \"normal\" memory git needs just for objects, of \n> > course. And quite frankly, you'd be better off with a few more windows, \n> > even if that obviously implies smaller windows.\n> \n> Btw, I'm not 100% certain of this. Somebody should double-check me. Maybe \n> there are cases where we want more than two windows alive. And maybe there \n> aren't even that, and we can always make do with just one.\n> \n> So I will _not_ guarantee that \"at least two pack windows\" is necessarily \n> the right answer. The windowing code was mostly other people doing it. I \n> think Shawn and Nico.\n\nI was fairly certain we needed at least two windows open at once,\nbut reviewing the code in sha1_file.c I don't see a reason for that\nrestriction anymore.\n\nI think it used to have to do with the delta reconstruction; to\nunpack a delta we would read the delta header from one window,\nbut we may need base data from another. The delta unpack code\nkept the delta window pinned in use, so we couldn't replace it\nto access base data from elsewhere, hence we needed two windows.\nThinking about it now I don't recall how we handled the recusion\non a delta chain longer than 2. ;-)\n\nBut looking at the code we have long since refactored it so this\nisn't an issue anymore. We release the window between reading\nthe delta header and reading the base, so the delta window can be\nreplaced if necessary. I think the \"2 window minimum\" is just a\nperformance suggestion, not a requirement.\n\n-- \nShawn.\n"},{"id":"99656","messageId":"885649360901071821t2ea481b5k83ab800f6aeb897@mail.gmail.com","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071644330.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2009-01-08T02:21:18Z","receivedAt":"2009-01-08T02:21:18Z","isPatch":true,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Wed, Jan 7, 2009, Linus Torvalds wrote:\n> Can you cnfirm that your \"reproducible\" case starts working with that\n> addition to your ~/.gitconfig? If so, the solution is pretty simple: we\n> should just lower the default pack windowsize.\n\nUmm... isn't that more of a workaround than a solution? I.e., if you lower\nthe default pack windowsize, couldn't the corruption still happen under the\nright conditions?\n\nJames\n"},{"id":"99660","messageId":"20090108024325.GE10790@spearce.org","threadId":"16649","inReplyTo":"885649360901071821t2ea481b5k83ab800f6aeb897@mail.gmail.com","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T02:43:25Z","receivedAt":"2009-01-08T02:43:25Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"James Pickens wrote:\n> On Wed, Jan 7, 2009, Linus Torvalds wrote:\n> > Can you cnfirm that your \"reproducible\" case starts working with that\n> > addition to your ~/.gitconfig? If so, the solution is pretty simple: we\n> > should just lower the default pack windowsize.\n> \n> Umm... isn't that more of a workaround than a solution? I.e., if you lower\n> the default pack windowsize, couldn't the corruption still happen under the\n> right conditions?\n\nUhm, yea. So I managed to reproduce it on a Linux system here.\nDifferent object ids than R. Tyler's case, but I'm going to try\nto debug it and see why we are getting these.\n\nFor those following along at home, Linus' 2.6 tree:\n\n$ ulimit -v `echo '150 * 1024'|bc -l`\n$ git co 56d18e9932ebf4e8eca42d2ce509450e6c9c1666\nHEAD is now at 56d18e9... Merge branch 'upstream' of git://ftp.linux-mips.org/pub/scm/upstream-linus\n$ git merge 9e42d0cf5020aaf217433cad1a224745241d212a\nUpdating 56d18e9..9e42d0c\nerror: failed to read delta base object ef135b90084f3c54fccea4e273aeff029db2d873 at offset 48342508 from .git/objects/pack/pack-edb47354be787909e05c15bd1d9eb8b4684d2e4d.pack\nerror: failed to read delta base object c4e828b71d96622bb258938d69aab9cec53d5cae at offset 128427683 from .git/objects/pack/pack-edb47354be787909e05c15bd1d9eb8b4684d2e4d.pack\nerror: failed to read object 3cd5a6463cfd9306095bf6312a9b7ab09d4f2f5d at offset 128427777 from .git/objects/pack/pack-edb47354be787909e05c15bd1d9eb8b4684d2e4d.pack\nfatal: object 3cd5a6463cfd9306095bf6312a9b7ab09d4f2f5d is corrupted\n\nNo, the repository is not corrupt. We f'd up our memory management\nsomewhere.\n\n-- \nShawn.\n"},{"id":"99662","messageId":"alpine.LFD.2.00.0901071836290.3283@localhost.localdomain","threadId":"16649","inReplyTo":"1231374514.8870.621.camel@starfruit","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T02:52:15Z","receivedAt":"2009-01-08T02:52:15Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n>\n> tyler@grapefruit:~/source/git/linux-2.6> git pull\n> error: failed to read object be1b87c70af69acfadb8a27a7a76dfb61de92643 at offset 1850923\n> from .git/objects/pack/pack-dbe154052997a05499eb6b4fd90b924da68e799a.pack\n> fatal: object be1b87c70af69acfadb8a27a7a76dfb61de92643 is corrupted\n\nBtw, this is an interesting error message, mostly because of what is \n_not_ there.\n\nIn particular, it doesn't report any reason _why_ it failed to read the \nobject, which as far as I can tell can happen for only one reason: \nunpack_compressed_entry() returns NULL, and that path is the only thing \nthat can do so without a message.\n\nAnd it only does it if zlib fails.\n\nNow, zlib can fail because the unpacking fails, but it can fail for other \ncases too.\n\nAnd the thing is, we don't check/report those kinds of failure cases very \nwell. Which really bit us here, because if we had checked the return value \nof inflateInit(), we'd almost certainly would have gotten a nice big \"you \nran out of memory\" thing, and we wouldn't have been chasing this down as a \ncorruption issue.\n\nWe probably should wrap all the \"inflateInit()\" calls, and do something \nlike\n\n\tstatic void xinflateInit(z_streamp strm)\n\t{\n\t\tconst char *err;\n\n\t\tswitch (inflateInit(strm)) {\n\t\tcase Z_OK:\n\t\t\treturn;\n\t\tcase Z_MEM_ERROR:\n\t\t\terr = \"out of memory\";\n\t\t\tbreak;\n\t\tcase Z_VERSION_ERROR:\n\t\t\terr = \"wrong version\";\n\t\t\tbreak;\n\t\tdefault:\n\t\t\terr = \"error\";\n\t\t}\n\t\tdie(\"inflateInit: %s (%s)\", err,\n\t\t\tstrm->msg ? strm->msg : \"no message\");\n\t}\n\nor similar. That way we'd get good error reports when we run out of \nmemory, rather than consider it to be a corruption issue.\n\nWe could also try to make a few of these wrappers actually release some of \nthe memory (the way xmmap() does), but there are likely diminishing \nreturns. And the much more important issue is the proper error reporting: \nif we had reported \"out of memory\", we'd not have spent so much time \nchasing disk corruption etc.\n\n\t\t\tLinus\n"},{"id":"99661","messageId":"200901072053.00492.bss@iguanasuicide.net","threadId":"16649","inReplyTo":"885649360901071821t2ea481b5k83ab800f6aeb897@mail.gmail.com","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Boyd Stephen Smith Jr.","fromEmail":"bss@iguanasuicide.net","sentAt":"2009-01-08T02:52:54Z","receivedAt":"2009-01-08T02:52:54Z","isPatch":true,"sender":{"key":"bss@iguanasuicide.net","avatar":"https://gravatar.com/avatar/84b95eeff194b816c1568b1339e63e4b229825298664a9037b9f1ec713ead1e3?d=mp&s=160"},"body":"On Wednesday 2009 January 07 20:21:18 James Pickens wrote:\n>On Wed, Jan 7, 2009, Linus Torvalds wrote:\n>> Can you cnfirm that your \"reproducible\" case starts working with that\n>> addition to your ~/.gitconfig? If so, the solution is pretty simple: we\n>> should just lower the default pack windowsize.\n>\n>Umm... isn't that more of a workaround than a solution? I.e., if you lower\n>the default pack windowsize, couldn't the corruption still happen under the\n>right conditions?\n\nIMHO:\nI agree, somewhat. I'm fine with a \"die()\" message when there's not enough \nmemory, but either corruption or just a spurious, but scary \" is \ncorrupt\" messages should be fixed.\n-- \nBoyd Stephen Smith Jr. ,= ,-_-. =. \nbss@iguanasuicide.net ((_/)o o(\\_))\nICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-' \nhttp://iguanasuicide.net/ \\_/ \n"},{"id":"99663","messageId":"20090108030115.GF10790@spearce.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071836290.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T03:01:15Z","receivedAt":"2009-01-08T03:01:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> On Wed, 7 Jan 2009, R. Tyler Ballance wrote:\n> >\n> > tyler@grapefruit:~/source/git/linux-2.6> git pull\n> > error: failed to read object be1b87c70af69acfadb8a27a7a76dfb61de92643 at offset 1850923\n> > from .git/objects/pack/pack-dbe154052997a05499eb6b4fd90b924da68e799a.pack\n> > fatal: object be1b87c70af69acfadb8a27a7a76dfb61de92643 is corrupted\n> \n> Btw, this is an interesting error message, mostly because of what is \n> _not_ there.\n> \n> In particular, it doesn't report any reason _why_ it failed to read the \n> object, which as far as I can tell can happen for only one reason: \n> unpack_compressed_entry() returns NULL, and that path is the only thing \n> that can do so without a message.\n> \n> And it only does it if zlib fails.\n\nOk, well, in this case I've been able to reproduce a zlib inflate\nfailure on the base object in a 2 deep delta chain. We got back:\n\n #define Z_STREAM_ERROR (-2)\n\nthis causes the buffer to be freed and NULL to come back out of\nunpack_compressed_entry(), and then everything is corrupt...\n\n-- \nShawn.\n"},{"id":"99665","messageId":"alpine.LFD.2.00.0901071904380.3283@localhost.localdomain","threadId":"16649","inReplyTo":"20090108030115.GF10790@spearce.org","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T03:06:42Z","receivedAt":"2009-01-08T03:06:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Shawn O. Pearce wrote:\n> \n> Ok, well, in this case I've been able to reproduce a zlib inflate\n> failure on the base object in a 2 deep delta chain. We got back:\n> \n> #define Z_STREAM_ERROR (-2)\n> \n> this causes the buffer to be freed and NULL to come back out of\n> unpack_compressed_entry(), and then everything is corrupt...\n\nI bet you actually got an earlier error already from the inflateInit. \n\nThe Z_STREAM_ERROR probably comes from inflate() itself - and could very \neasily be due to a allocation error in inflateInit leaving the stream data \nincomplete.\n\nLet me try wrapping that dang thing and send a patch. \n\n\t\tLinus\n"},{"id":"99666","messageId":"20090108031314.GG10790@spearce.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071904380.3283@localhost.localdomain","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T03:13:14Z","receivedAt":"2009-01-08T03:13:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> The Z_STREAM_ERROR probably comes from inflate() itself - and could very \n> easily be due to a allocation error in inflateInit leaving the stream data \n> incomplete.\n> \n> Let me try wrapping that dang thing and send a patch. \n\nYup. I'm actually doing the same thing...\n\n-- \nShawn.\n"},{"id":"99667","messageId":"20090108031655.GH10790@spearce.org","threadId":"16649","inReplyTo":"20090108031314.GG10790@spearce.org","subject":"[PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T03:16:55Z","receivedAt":"2009-01-08T03:16:55Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we are running low on virtual memory we should release pack\nwindows if zlib's inflateInit fails due to an out of memory error.\nIt may be that we are running under a low ulimit and are getting\ntight on address space. Shedding unused windows may get us\nsufficient working space to continue.\n\nSuggested-by: Linus Torvalds \nSigned-off-by: Shawn O. Pearce \n---\n \"Shawn O. Pearce\" wrote:\n > Linus Torvalds wrote:\n > > The Z_STREAM_ERROR probably comes from inflate() itself - and could very \n > > easily be due to a allocation error in inflateInit leaving the stream data \n > > incomplete.\n > > \n > > Let me try wrapping that dang thing and send a patch. \n > \n > Yup. I'm actually doing the same thing...\n\n builtin-apply.c | 2 +-\n builtin-pack-objects.c | 2 +-\n builtin-unpack-objects.c | 2 +-\n cache.h | 1 +\n http-push.c | 4 ++--\n http-walker.c | 4 ++--\n index-pack.c | 4 ++--\n sha1_file.c | 8 ++++----\n wrapper.c | 20 ++++++++++++++++++++\n 9 files changed, 34 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex af25ee9..cb2663e 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1258,7 +1258,7 @@ static char *inflate_it(const void *data, unsigned long size,\n \tstream.avail_in = size;\n \tstream.next_out = out = xmalloc(inflated_size);\n \tstream.avail_out = inflated_size;\n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \tst = inflate(&stream, Z_FINISH);\n \tif ((st != Z_STREAM_END) || stream.total_out != inflated_size) {\n \t\tfree(out);\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex e851534..09576c6 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -195,7 +195,7 @@ static int check_pack_inflate(struct packed_git *p,\n \tint st;\n \n \tmemset(&stream, 0, sizeof(stream));\n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, offset, &stream.avail_in);\n \t\tstream.next_in = in;\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 47ed610..cb9edac 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -99,7 +99,7 @@ static void *get_data(unsigned long size)\n \tstream.avail_out = size;\n \tstream.next_in = fill(1);\n \tstream.avail_in = len;\n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \n \tfor (;;) {\n \t\tint ret = inflate(&stream, 0);\ndiff --git a/cache.h b/cache.h\nindex 231c06d..7d5c38d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -17,6 +17,7 @@\n #if defined(NO_DEFLATE_BOUND) || ZLIB_VERNUM < 0x1200\n #define deflateBound(c,s) ((s) + (((s) + 7) >> 3) + (((s) + 63) >> 6) + 11)\n #endif\n+extern void xinflateInit(z_stream *stream);\n \n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\ndiff --git a/http-push.c b/http-push.c\nindex a4b7d08..906ca48 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -269,7 +269,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \n \tmemset(&request->stream, 0, sizeof(request->stream));\n \n-\tinflateInit(&request->stream);\n+\txinflateInit(&request->stream);\n \n \tgit_SHA1_Init(&request->c);\n \n@@ -310,7 +310,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n \t\tmemset(&request->stream, 0, sizeof(request->stream));\n-\t\tinflateInit(&request->stream);\n+\t\txinflateInit(&request->stream);\n \t\tgit_SHA1_Init(&request->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\ndiff --git a/http-walker.c b/http-walker.c\nindex 7271c7d..6aa8486 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -142,7 +142,7 @@ static void start_object_request(struct walker *walker,\n \n \tmemset(&obj_req->stream, 0, sizeof(obj_req->stream));\n \n-\tinflateInit(&obj_req->stream);\n+\txinflateInit(&obj_req->stream);\n \n \tgit_SHA1_Init(&obj_req->c);\n \n@@ -183,7 +183,7 @@ static void start_object_request(struct walker *walker,\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n \t\tmemset(&obj_req->stream, 0, sizeof(obj_req->stream));\n-\t\tinflateInit(&obj_req->stream);\n+\t\txinflateInit(&obj_req->stream);\n \t\tgit_SHA1_Init(&obj_req->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\ndiff --git a/index-pack.c b/index-pack.c\nindex 2931511..c6bfc12 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -275,7 +275,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)\n \tstream.avail_out = size;\n \tstream.next_in = fill(1);\n \tstream.avail_in = input_len;\n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \n \tfor (;;) {\n \t\tint ret = inflate(&stream, 0);\n@@ -382,7 +382,7 @@ static void *get_data_from_pack(struct object_entry *obj)\n \tstream.avail_out = obj->size;\n \tstream.next_in = src;\n \tstream.avail_in = len;\n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \twhile ((st = inflate(&stream, Z_FINISH)) == Z_OK);\n \tinflateEnd(&stream);\n \tif (st != Z_STREAM_END || stream.total_out != obj->size)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 52d1ead..9aabae2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1196,7 +1196,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \tstream->avail_out = bufsiz;\n \n \tif (legacy_loose_object(map)) {\n-\t\tinflateInit(stream);\n+\t\txinflateInit(stream);\n \t\treturn inflate(stream, 0);\n \t}\n \n@@ -1217,7 +1217,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \t/* Set up the stream for the rest.. */\n \tstream->next_in = map;\n \tstream->avail_in = mapsize;\n-\tinflateInit(stream);\n+\txinflateInit(stream);\n \n \t/* And generate the fake traditional header */\n \tstream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n@@ -1348,7 +1348,7 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tstream.next_out = delta_head;\n \tstream.avail_out = sizeof(delta_head);\n \n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n@@ -1585,7 +1585,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tstream.next_out = buffer;\n \tstream.avail_out = size;\n \n-\tinflateInit(&stream);\n+\txinflateInit(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\ndiff --git a/wrapper.c b/wrapper.c\nindex 93562f0..f255eef 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -196,3 +196,23 @@ int xmkstemp(char *template)\n \t\tdie(\"Unable to create temporary file: %s\", strerror(errno));\n \treturn fd;\n }\n+\n+void xinflateInit(z_stream *stream)\n+{\n+\tswitch (inflateInit(stream)) {\n+\tcase Z_OK:\n+\t\treturn;\n+\n+\tcase Z_MEM_ERROR:\n+\t\trelease_pack_memory(128 * 1024, -1);\n+\t\tif (inflateInit(stream) == Z_OK)\n+\t\t\treturn;\n+\t\tdie(\"Out of memory? inflateInit failed\");\n+\n+\tcase Z_VERSION_ERROR:\n+\t\tdie(\"Wrong zlib version? inflateInit failed\");\n+\n+\tdefault:\n+\t\tdie(\"Unknown inflateInit failure\");\n+\t}\n+}\n-- \n1.6.1.141.gfe98e\n"},{"id":"99669","messageId":"alpine.LFD.2.00.0901071941210.3283@localhost.localdomain","threadId":"16649","inReplyTo":"20090108031655.GH10790@spearce.org","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T03:54:47Z","receivedAt":"2009-01-08T03:54:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Shawn O. Pearce wrote:\n>\n> If we are running low on virtual memory we should release pack\n> windows if zlib's inflateInit fails due to an out of memory error.\n> It may be that we are running under a low ulimit and are getting\n> tight on address space. Shedding unused windows may get us\n> sufficient working space to continue.\n\nLet's do this (more complete) wrapping instead, ok?\n\nThis one _just_ wraps things, btw - it doesn't do the \"retry on low memory \nerror\" part, at least not yet. I think that's an independent issue from \nthe reporting.\n\nHmm? \n\nTyler - does this make the corruption errors go away, and be replaced by \nhard failures with \"out of memory\" reporting?\n\nThis patch is potentially pretty noisy, on purpose. I didn't remove the \nreporting from places that already do so - some of them have stricter \nerrors than this.\n\nFor example: Z_BUF_ERROR can be valid depending on circumstance, so the \nwrapper doesn't complain about it, but the caller may not accept it. \n\n\t\tLinus\n\n---\n builtin-apply.c | 5 ++-\n builtin-pack-objects.c | 6 ++--\n builtin-unpack-objects.c | 6 ++--\n cache.h | 4 +++\n http-push.c | 8 +++---\n http-walker.c | 8 +++---\n index-pack.c | 12 ++++----\n sha1_file.c | 24 +++++++++---------\n wrapper.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 99 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 07244b0..ed02b6d 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1253,8 +1253,9 @@ static char *inflate_it(const void *data, unsigned long size,\n \tstream.avail_in = size;\n \tstream.next_out = out = xmalloc(inflated_size);\n \tstream.avail_out = inflated_size;\n-\tinflateInit(&stream);\n-\tst = inflate(&stream, Z_FINISH);\n+\tgit_inflate_init(&stream);\n+\tst = git_inflate(&stream, Z_FINISH);\n+\tgit_inflate_end(&stream);\n \tif ((st != Z_STREAM_END) || stream.total_out != inflated_size) {\n \t\tfree(out);\n \t\treturn NULL;\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex e851534..cb51916 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -195,16 +195,16 @@ static int check_pack_inflate(struct packed_git *p,\n \tint st;\n \n \tmemset(&stream, 0, sizeof(stream));\n-\tinflateInit(&stream);\n+\tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, offset, &stream.avail_in);\n \t\tstream.next_in = in;\n \t\tstream.next_out = fakebuf;\n \t\tstream.avail_out = sizeof(fakebuf);\n-\t\tst = inflate(&stream, Z_FINISH);\n+\t\tst = git_inflate(&stream, Z_FINISH);\n \t\toffset += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \treturn (st == Z_STREAM_END &&\n \t\tstream.total_out == expect &&\n \t\tstream.total_in == len) ? 0 : -1;\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 47ed610..9a77323 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -99,10 +99,10 @@ static void *get_data(unsigned long size)\n \tstream.avail_out = size;\n \tstream.next_in = fill(1);\n \tstream.avail_in = len;\n-\tinflateInit(&stream);\n+\tgit_inflate_init(&stream);\n \n \tfor (;;) {\n-\t\tint ret = inflate(&stream, 0);\n+\t\tint ret = git_inflate(&stream, 0);\n \t\tuse(len - stream.avail_in);\n \t\tif (stream.total_out == size && ret == Z_STREAM_END)\n \t\t\tbreak;\n@@ -118,7 +118,7 @@ static void *get_data(unsigned long size)\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = len;\n \t}\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \treturn buf;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 231c06d..49e54fb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -18,6 +18,10 @@\n #define deflateBound(c,s) ((s) + (((s) + 7) >> 3) + (((s) + 63) >> 6) + 11)\n #endif\n \n+void git_inflate_init(z_streamp strm);\n+void git_inflate_end(z_streamp strm);\n+int git_inflate(z_streamp strm, int flush);\n+\n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\n #else\ndiff --git a/http-push.c b/http-push.c\nindex 7c64609..809002b 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -208,7 +208,7 @@ static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,\n \tdo {\n \t\trequest->stream.next_out = expn;\n \t\trequest->stream.avail_out = sizeof(expn);\n-\t\trequest->zret = inflate(&request->stream, Z_SYNC_FLUSH);\n+\t\trequest->zret = git_inflate(&request->stream, Z_SYNC_FLUSH);\n \t\tgit_SHA1_Update(&request->c, expn,\n \t\t\t sizeof(expn) - request->stream.avail_out);\n \t} while (request->stream.avail_in && request->zret == Z_OK);\n@@ -268,7 +268,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \n \tmemset(&request->stream, 0, sizeof(request->stream));\n \n-\tinflateInit(&request->stream);\n+\tgit_inflate_init(&request->stream);\n \n \tgit_SHA1_Init(&request->c);\n \n@@ -309,7 +309,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n \t\tmemset(&request->stream, 0, sizeof(request->stream));\n-\t\tinflateInit(&request->stream);\n+\t\tgit_inflate_init(&request->stream);\n \t\tgit_SHA1_Init(&request->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n@@ -741,7 +741,7 @@ static void finish_request(struct transfer_request *request)\n \t\t\tif (request->http_code == 416)\n \t\t\t\tfprintf(stderr, \"Warning: requested range invalid; we may already have all the data.\\n\");\n \n-\t\t\tinflateEnd(&request->stream);\n+\t\t\tgit_inflate_end(&request->stream);\n \t\t\tgit_SHA1_Final(request->real_sha1, &request->c);\n \t\t\tif (request->zret != Z_STREAM_END) {\n \t\t\t\tunlink(request->tmpfile);\ndiff --git a/http-walker.c b/http-walker.c\nindex 7271c7d..0dbad3c 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -82,7 +82,7 @@ static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,\n \tdo {\n \t\tobj_req->stream.next_out = expn;\n \t\tobj_req->stream.avail_out = sizeof(expn);\n-\t\tobj_req->zret = inflate(&obj_req->stream, Z_SYNC_FLUSH);\n+\t\tobj_req->zret = git_inflate(&obj_req->stream, Z_SYNC_FLUSH);\n \t\tgit_SHA1_Update(&obj_req->c, expn,\n \t\t\t sizeof(expn) - obj_req->stream.avail_out);\n \t} while (obj_req->stream.avail_in && obj_req->zret == Z_OK);\n@@ -142,7 +142,7 @@ static void start_object_request(struct walker *walker,\n \n \tmemset(&obj_req->stream, 0, sizeof(obj_req->stream));\n \n-\tinflateInit(&obj_req->stream);\n+\tgit_inflate_init(&obj_req->stream);\n \n \tgit_SHA1_Init(&obj_req->c);\n \n@@ -183,7 +183,7 @@ static void start_object_request(struct walker *walker,\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n \t\tmemset(&obj_req->stream, 0, sizeof(obj_req->stream));\n-\t\tinflateInit(&obj_req->stream);\n+\t\tgit_inflate_init(&obj_req->stream);\n \t\tgit_SHA1_Init(&obj_req->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n@@ -243,7 +243,7 @@ static void finish_object_request(struct object_request *obj_req)\n \t\treturn;\n \t}\n \n-\tinflateEnd(&obj_req->stream);\n+\tgit_inflate_end(&obj_req->stream);\n \tgit_SHA1_Final(obj_req->real_sha1, &obj_req->c);\n \tif (obj_req->zret != Z_STREAM_END) {\n \t\tunlink(obj_req->tmpfile);\ndiff --git a/index-pack.c b/index-pack.c\nindex 60ed41a..c0a3d97 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -275,10 +275,10 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)\n \tstream.avail_out = size;\n \tstream.next_in = fill(1);\n \tstream.avail_in = input_len;\n-\tinflateInit(&stream);\n+\tgit_inflate_init(&stream);\n \n \tfor (;;) {\n-\t\tint ret = inflate(&stream, 0);\n+\t\tint ret = git_inflate(&stream, 0);\n \t\tuse(input_len - stream.avail_in);\n \t\tif (stream.total_out == size && ret == Z_STREAM_END)\n \t\t\tbreak;\n@@ -287,7 +287,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = input_len;\n \t}\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \treturn buf;\n }\n \n@@ -382,9 +382,9 @@ static void *get_data_from_pack(struct object_entry *obj)\n \tstream.avail_out = obj->size;\n \tstream.next_in = src;\n \tstream.avail_in = len;\n-\tinflateInit(&stream);\n-\twhile ((st = inflate(&stream, Z_FINISH)) == Z_OK);\n-\tinflateEnd(&stream);\n+\tgit_inflate_init(&stream);\n+\twhile ((st = git_inflate(&stream, Z_FINISH)) == Z_OK);\n+\tgit_inflate_end(&stream);\n \tif (st != Z_STREAM_END || stream.total_out != obj->size)\n \t\tdie(\"serious inflate inconsistency\");\n \tfree(src);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 52d1ead..8600b04 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1196,8 +1196,8 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \tstream->avail_out = bufsiz;\n \n \tif (legacy_loose_object(map)) {\n-\t\tinflateInit(stream);\n-\t\treturn inflate(stream, 0);\n+\t\tgit_inflate_init(stream);\n+\t\treturn git_inflate(stream, 0);\n \t}\n \n \n@@ -1217,7 +1217,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \t/* Set up the stream for the rest.. */\n \tstream->next_in = map;\n \tstream->avail_in = mapsize;\n-\tinflateInit(stream);\n+\tgit_inflate_init(stream);\n \n \t/* And generate the fake traditional header */\n \tstream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n@@ -1254,11 +1254,11 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size\n \t\tstream->next_out = buf + bytes;\n \t\tstream->avail_out = size - bytes;\n \t\twhile (status == Z_OK)\n-\t\t\tstatus = inflate(stream, Z_FINISH);\n+\t\t\tstatus = git_inflate(stream, Z_FINISH);\n \t}\n \tbuf[size] = 0;\n \tif (status == Z_STREAM_END && !stream->avail_in) {\n-\t\tinflateEnd(stream);\n+\t\tgit_inflate_end(stream);\n \t\treturn buf;\n \t}\n \n@@ -1348,15 +1348,15 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tstream.next_out = delta_head;\n \tstream.avail_out = sizeof(delta_head);\n \n-\tinflateInit(&stream);\n+\tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n-\t\tst = inflate(&stream, Z_FINISH);\n+\t\tst = git_inflate(&stream, Z_FINISH);\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \tif ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {\n \t\terror(\"delta data unpack-initial failed\");\n \t\treturn 0;\n@@ -1585,14 +1585,14 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tstream.next_out = buffer;\n \tstream.avail_out = size;\n \n-\tinflateInit(&stream);\n+\tgit_inflate_init(&stream);\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n-\t\tst = inflate(&stream, Z_FINISH);\n+\t\tst = git_inflate(&stream, Z_FINISH);\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \tif ((st != Z_STREAM_END) || stream.total_out != size) {\n \t\tfree(buffer);\n \t\treturn NULL;\n@@ -2017,7 +2017,7 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n \telse if (sizep)\n \t\t*sizep = size;\n-\tinflateEnd(&stream);\n+\tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n \treturn status;\n }\ndiff --git a/wrapper.c b/wrapper.c\nindex 93562f0..29afa96 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -196,3 +196,63 @@ int xmkstemp(char *template)\n \t\tdie(\"Unable to create temporary file: %s\", strerror(errno));\n \treturn fd;\n }\n+\n+/*\n+ * zlib wrappers to make sure we don't silently miss errors\n+ * at init time.\n+ */\n+void git_inflate_init(z_streamp strm)\n+{\n+\tconst char *err;\n+\n+\tswitch (inflateInit(strm)) {\n+\tcase Z_OK:\n+\t\treturn;\n+\n+\tcase Z_MEM_ERROR:\n+\t\terr = \"out of memory\";\n+\t\tbreak;\n+\tcase Z_VERSION_ERROR:\n+\t\terr = \"wrong version\";\n+\t\tbreak;\n+\tdefault:\n+\t\terr = \"error\";\n+\t}\n+\tdie(\"inflateInit: %s (%s)\", err, strm->msg ? strm->msg : \"no message\");\n+}\n+\n+void git_inflate_end(z_streamp strm)\n+{\n+\tif (inflateEnd(strm) != Z_OK)\n+\t\terror(\"inflateEnd: %s\", strm->msg ? strm->msg : \"failed\");\n+}\n+\n+int git_inflate(z_streamp strm, int flush)\n+{\n+\tint ret = inflate(strm, flush);\n+\tconst char *err;\n+\n+\tswitch (ret) {\n+\t/* Out of memory is fatal. */\n+\tcase Z_MEM_ERROR:\n+\t\tdie(\"inflate: out of memory\");\n+\n+\t/* Data corruption errors: we may want to recover from them (fsck) */\n+\tcase Z_NEED_DICT:\n+\t\terr = \"needs dictionary\"; break;\n+\tcase Z_DATA_ERROR:\n+\t\terr = \"data stream error\"; break;\n+\tcase Z_STREAM_ERROR:\n+\t\terr = \"stream consistency error\"; break;\n+\tdefault:\n+\t\terr = \"unknown error\"; break;\n+\n+\t/* Z_BUF_ERROR: normal, needs a buffer output buffer */\n+\tcase Z_BUF_ERROR:\n+\tcase Z_OK:\n+\tcase Z_STREAM_END:\n+\t\treturn ret;\n+\t}\n+\terror(\"inflate: %s (%s)\", err, strm->msg ? strm->msg : \"no message\");\n+\treturn ret;\n+}\n"},{"id":"99672","messageId":"7vbpui8j6f.fsf@gitster.siamese.dyndns.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071941210.3283@localhost.localdomain","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-08T05:23:52Z","receivedAt":"2009-01-08T05:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> +int git_inflate(z_streamp strm, int flush)\n> +{\n> +...\n> +\t/* Z_BUF_ERROR: normal, needs a buffer output buffer */\n> +\tcase Z_BUF_ERROR:\n\nThanks, but \"needs a buffer output buffer\" made me scratch my head\nsomewhat.\n\n ... Z_BUF_ERROR if no progress is possible or if there was not enough\n room in the output buffer when Z_FINISH is used. Note that Z_BUF_ERROR\n is not fatal, and inflate() can be called again with more input and more\n output space to continue decompressing.\n"},{"id":"99673","messageId":"7v4p0a8if2.fsf@gitster.siamese.dyndns.org","threadId":"16649","inReplyTo":"20090108024325.GE10790@spearce.org","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-08T05:40:17Z","receivedAt":"2009-01-08T05:40:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> For those following along at home, Linus' 2.6 tree:\n>\n> $ ulimit -v `echo '150 * 1024'|bc -l`\n> $ git co 56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n\nHmm, without any \"wrap zlib to die on error\" patch, this step already\nfails with:\n\n $ git checkout 56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n fatal: Out of memory? mmap failed: Cannot allocate memory\n\nI guess that is because our test repositories are packed differently.\nI'll retry after repacking..\n"},{"id":"99674","messageId":"20090108060413.GA17728@spearce.org","threadId":"16649","inReplyTo":"7v4p0a8if2.fsf@gitster.siamese.dyndns.org","subject":"Re: Public repro case! Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T06:04:13Z","receivedAt":"2009-01-08T06:04:13Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> > For those following along at home, Linus' 2.6 tree:\n> >\n> > $ ulimit -v `echo '150 * 1024'|bc -l`\n> > $ git co 56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n> \n> Hmm, without any \"wrap zlib to die on error\" patch, this step already\n> fails with:\n> \n> $ git checkout 56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n> fatal: Out of memory? mmap failed: Cannot allocate memory\n> \n> I guess that is because our test repositories are packed differently.\n> I'll retry after repacking..\n\nYup. I actually did something more like this to get the test\nrepository:\n\n git clone git://android.git.kernel.org/kernel/common.git\n git fetch git://kernel.org/pub/.../torvalds/linux-2.6.git master\n\nThe android kernel repository I had handy on my local system was\nquite a bit away from Linus' so I wound up with two different but\nsizable packs. I thought android was closer to upstream, but its\napparently not. I started from there because it was local and I\nthought it would be a quick way to get a test environment, but\nsadly it didn't even have the base 56d18e we were talking about.\n\n-- \nShawn.\n"},{"id":"99707","messageId":"20090108153410.GB16840@spearce.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071941210.3283@localhost.localdomain","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-08T15:34:10Z","receivedAt":"2009-01-08T15:34:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> Let's do this (more complete) wrapping instead, ok?\n\nAck.\n \n> This one _just_ wraps things, btw - it doesn't do the \"retry on low memory \n> error\" part, at least not yet. I think that's an independent issue from \n> the reporting.\n\nI still think we should try to reduce pack memory usage when we get\noom from zlib and retry the current operation once. We do it almost\neverywhere else and it works relatively well.\n\nWe may also want to consider dropping (e.g. halving) the window\nsize and/or limit when we run out of memory. We'll run slower but\nif the OS has denied us further resources it may be a ulimit thing\non a shared system, and we should try harder to work with what we\nhave available to us.\n\n-- \nShawn.\n"},{"id":"99708","messageId":"alpine.LFD.2.00.0901080734400.3283@localhost.localdomain","threadId":"16649","inReplyTo":"7vbpui8j6f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T15:35:55Z","receivedAt":"2009-01-08T15:35:55Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 7 Jan 2009, Junio C Hamano wrote:\n> \n> Thanks, but \"needs a buffer output buffer\" made me scratch my head\n> somewhat.\n\nThat was just me editing it.\n\nIt was originally \"needs an output buffer\" and then I was supposed to edit \nit to \"needs a buffer\" (because it can be either output or input, and in \nthe case of git it's usually actually the input that was partial).\n\nAnd then I messed up, and it became that.\n\n\t\tLinus\n"},{"id":"99713","messageId":"alpine.LFD.2.00.0901080813291.3283@localhost.localdomain","threadId":"16649","inReplyTo":"20090108153410.GB16840@spearce.org","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T16:14:58Z","receivedAt":"2009-01-08T16:14:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 8 Jan 2009, Shawn O. Pearce wrote:\n> \n> I still think we should try to reduce pack memory usage when we get\n> oom from zlib and retry the current operation once. We do it almost\n> everywhere else and it works relatively well.\n\nOh, I agree.\n\nIt's just that I wanted to verify that people who see this problem \nactually see the message, and that we confirm that it is due to this and \nnothing else.\n\n\t\t\tLinus\n"},{"id":"99728","messageId":"1231438552.8870.645.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901071941210.3283@localhost.localdomain","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T18:15:52Z","receivedAt":"2009-01-08T18:15:52Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Wed, 2009-01-07 at 19:54 -0800, Linus Torvalds wrote:\n> \n> On Wed, 7 Jan 2009, Shawn O. Pearce wrote:\n> >\n> > If we are running low on virtual memory we should release pack\n> > windows if zlib's inflateInit fails due to an out of memory error.\n> > It may be that we are running under a low ulimit and are getting\n> > tight on address space. Shedding unused windows may get us\n> > sufficient working space to continue.\n> \n> Let's do this (more complete) wrapping instead, ok?\n> \n> This one _just_ wraps things, btw - it doesn't do the \"retry on low memory \n> error\" part, at least not yet. I think that's an independent issue from \n> the reporting.\n> \n> Hmm? \n> \n> Tyler - does this make the corruption errors go away, and be replaced by \n> hard failures with \"out of memory\" reporting?\n\nYeah, looks like it:\n\n\n tyler@grapefruit:~/source/git/linux-2.6> export\n START=56d18e9932ebf4e8eca42d2ce509450e6c9c1666\n tyler@grapefruit:~/source/git/linux-2.6> git reset --hard\n HEAD is now at 9e42d0c Merge\n git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-2.6\n tyler@grapefruit:~/source/git/linux-2.6> git reset --hard $START\n HEAD is now at 56d18e9 Merge branch 'upstream' of\n git://ftp.linux-mips.org/pub/scm/upstream-linus\n tyler@grapefruit:~/source/git/linux-2.6> ulimit -v `echo \"350 *\n 1024\" | bc -l`\n tyler@grapefruit:~/source/git/linux-2.6> limit\n cputime unlimited\n filesize unlimited\n datasize unlimited\n stacksize 8MB\n coredumpsize 0kB\n memoryuse 2561MB\n maxproc 24564\n descriptors 1024\n memorylocked 64kB\n addressspace 350MB\n maxfilelocks unlimited\n sigpending 24564\n msgqueue 819200\n nice 0\n rt_priority 0\n tyler@grapefruit:~/source/git/linux-2.6> git pull\n Updating 56d18e9..9e42d0c\n fatal: Out of memory? inflateInit failed\n tyler@grapefruit:~/source/git/linux-2.6> which git\n /home/tyler/bin/git\n tyler@grapefruit:~/source/git/linux-2.6> \n\n\n\n> \n> This patch is potentially pretty noisy, on purpose. I didn't remove the \n> reporting from places that already do so - some of them have stricter \n> errors than this.\n\nI'm assuming this patch is going to be reworked, if so, I'll back it out\nof our internal 1.6.1 build and anxiously await The Real Deal(tm)\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99738","messageId":"alpine.LFD.2.00.0901081216060.3283@localhost.localdomain","threadId":"16649","inReplyTo":"1231438552.8870.645.camel@starfruit","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-08T20:22:22Z","receivedAt":"2009-01-08T20:22:22Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 8 Jan 2009, R. Tyler Ballance wrote:\n> > \n> > Tyler - does this make the corruption errors go away, and be replaced by \n> > hard failures with \"out of memory\" reporting?\n> \n> Yeah, looks like it:\n\nWell, I was hoping that you'd have a confirmation from your own huge repo, \nbut I do suspect it's all the same thing, so I guess this counts as \nconfirmation too.\n\n> > This patch is potentially pretty noisy, on purpose. I didn't remove the \n> > reporting from places that already do so - some of them have stricter \n> > errors than this.\n> \n> I'm assuming this patch is going to be reworked, if so, I'll back it out\n> of our internal 1.6.1 build and anxiously await The Real Deal(tm)\n\nOh, it shouldn't be any noisier under _normal_ load - it's more that \ncertain real corruption cases will now report the error twice. That said, \nthe new errors should actually be more informative than the old ones, so \neven that isn't necessarily all bad.\n\nJunio - I think we should apply this, and likely to the stable branch too. \nAdd the re-trying the inflateInit() after shrinking pack windows on top of \nit.\n\n\t\t\tLinus\n"},{"id":"99742","messageId":"1231447053.8870.665.camel@starfruit","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901081216060.3283@localhost.localdomain","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2009-01-08T20:37:33Z","receivedAt":"2009-01-08T20:37:33Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Thu, 2009-01-08 at 12:22 -0800, Linus Torvalds wrote:\n> \n> On Thu, 8 Jan 2009, R. Tyler Ballance wrote:\n> > > \n> > > Tyler - does this make the corruption errors go away, and be replaced by \n> > > hard failures with \"out of memory\" reporting?\n> > \n> > Yeah, looks like it:\n> \n> Well, I was hoping that you'd have a confirmation from your own huge repo, \n> but I do suspect it's all the same thing, so I guess this counts as \n> confirmation too.\n\nI never got a real solid \"consistent\" reproduction case with our\nrepository, just a lot of users that experienced the issue. I think the\nLinux repro case is a far better example, and yeah, it's sorta\nconfirmation (waiting for operations here to deploy the patched 1.6.1 to\ndev machines).\n\n> \n> > > This patch is potentially pretty noisy, on purpose. I didn't remove the \n> > > reporting from places that already do so - some of them have stricter \n> > > errors than this.\n> > \n> > I'm assuming this patch is going to be reworked, if so, I'll back it out\n> > of our internal 1.6.1 build and anxiously await The Real Deal(tm)\n> \n> Oh, it shouldn't be any noisier under _normal_ load - it's more that \n> certain real corruption cases will now report the error twice. That said, \n> the new errors should actually be more informative than the old ones, so \n> even that isn't necessarily all bad.\n> \n> Junio - I think we should apply this, and likely to the stable branch too. \n> Add the re-trying the inflateInit() after shrinking pack windows on top of \n> it.\n\nI really appreciate this guys, this is one of the longer threads I've\nparticipated (spanning over a month) and I'm glad you guys were finally\nable to track the issue down.\n\nFrom now moving forward, I'll try to get a reproduction case with the\nkernel tree or something equally big since I know it's frustrating to\nplay the game of telephone with a proprietary code base (\"try this? what\ndoes that do? okay, then this?\").\n\nLinus, I'll have a chance to look at your comments on my \"variable\npacked git window size\" patch this weekend, and I'll follow-up in the\nappropriate thread.\n\n\nI'm relatively certain that after this witch hunt, I can get Slide to\ncover a round of beers at LinuxWorld or the nearest GitTogether ;)\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"99759","messageId":"7vtz892qzx.fsf@gitster.siamese.dyndns.org","threadId":"16649","inReplyTo":"alpine.LFD.2.00.0901081216060.3283@localhost.localdomain","subject":"Re: [PATCH] Wrap inflateInit to retry allocation after releasing pack memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-09T01:43:46Z","receivedAt":"2009-01-09T01:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Junio - I think we should apply this, and likely to the stable branch too. \n\nYeah, I didn't lose the patch.\n\n> Add the re-trying the inflateInit() after shrinking pack windows on top of \n> it.\n\nThat too.\n"}]}