{"thread":{"id":"46950","subject":"git repack leaks disk space on ENOSPC","startedAt":"2017-10-11T15:30:27Z","lastAt":"2017-10-12T13:36:43Z","messageCount":6,"participants":["Andreas Krey","Jonathan Nieder","brian m. carlson","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330183","messageId":"20171011150546.GC32090@inner.h.apk.li","threadId":"46950","inReplyTo":null,"subject":"git repack leaks disk space on ENOSPC","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2017-10-11T15:05:46Z","receivedAt":"2017-10-11T15:30:27Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi all,\n\nI observed (again) an annoying behavior of 'git repack':\nWhen the new pack file cannot be fully written because\nthe disk gets full beforehand, the tmp_pack file isn't\ndeleted, meaning the disk stays full:\n\n $ df -h .; git repack -ad; df -h .; ls -lart .git/objects/pack/tmp*; rm .git/objects/pack/tmp*; df -h .\n Filesystem Size Used Avail Use% Mounted on\n /dev/mapper/vg02-localworkspaces 250G 245G 5.1G 98% /workspaces/calvin\n Counting objects: 4715349, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (978051/978051), done.\n fatal: sha1 file '.git/objects/pack/tmp_pack_xB7DMt' write error: No space left on device\n Filesystem Size Used Avail Use% Mounted on\n /dev/mapper/vg02-localworkspaces 250G 250G 20K 100% /workspaces/calvin\n -r--r--r-- 1 andrkrey users 5438435328 Oct 11 17:03 .git/objects/pack/tmp_pack_xB7DMt\n rm: remove write-protected regular file '.git/objects/pack/tmp_pack_xB7DMt'? y\n Filesystem Size Used Avail Use% Mounted on\n /dev/mapper/vg02-localworkspaces 250G 245G 5.1G 98% /workspaces/calvin\n\n- Andreas\n\ngit version 2.15.0.rc0\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds \nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"330231","messageId":"20171012031702.GB155740@aiede.mtv.corp.google.com","threadId":"46950","inReplyTo":"20171011150546.GC32090@inner.h.apk.li","subject":"Re: git repack leaks disk space on ENOSPC","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-12T03:17:03Z","receivedAt":"2017-10-12T03:17:11Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Andreas,\n\nAndreas Krey wrote:\n\n> I observed (again) an annoying behavior of 'git repack':\n\nDo you have context for this 'again'? E.g. was this discussed\npreviously on-list?\n\n> When the new pack file cannot be fully written because\n> the disk gets full beforehand, the tmp_pack file isn't\n> deleted, meaning the disk stays full:\n>\n> $ df -h .; git repack -ad; df -h .; ls -lart .git/objects/pack/tmp*; rm .git/objects/pack/tmp*; df -h .\n> Filesystem Size Used Avail Use% Mounted on\n> /dev/mapper/vg02-localworkspaces 250G 245G 5.1G 98% /workspaces/calvin\n> Counting objects: 4715349, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (978051/978051), done.\n> fatal: sha1 file '.git/objects/pack/tmp_pack_xB7DMt' write error: No space left on device\n> Filesystem Size Used Avail Use% Mounted on\n> /dev/mapper/vg02-localworkspaces 250G 250G 20K 100% /workspaces/calvin\n> -r--r--r-- 1 andrkrey users 5438435328 Oct 11 17:03 .git/objects/pack/tmp_pack_xB7DMt\n> rm: remove write-protected regular file '.git/objects/pack/tmp_pack_xB7DMt'? y\n> Filesystem Size Used Avail Use% Mounted on\n> /dev/mapper/vg02-localworkspaces 250G 245G 5.1G 98% /workspaces/calvin\n>\n> git version 2.15.0.rc0\n\nI can imagine this behavior of retaining tmp_pack being useful for\ndebugging in some circumstances, but I agree with you that it is\ncertainly not a good default.\n\nChasing this down, I find:\n\n pack-write.c::create_tmp_packfile chooses the filename\n builtin/pack-objects.c::write_pack_file writes to it and the .bitmap, calling\n pack-write.c::finish_tmp_packfile to rename it into place\n\nNothing tries to install an atexit handler to do anything special to it\non exit.\n\nThe natural thing, I'd expect, would be for pack-write to use the\ntempfile API (see tempfile.h) to create and finish the file. That way,\nwe'd get such atexit handlers for free. If we want a way to keep temp\nfiles for debugging on abnormal exit, we could set that up separately as\na generic feature of the tempfile API (e.g. an envvar\nGIT_KEEP_TEMPFILES_ON_FAILURE), making that an orthogonal topic.\n\nDoes using create_tempfile there seem like a good path forward to you?\nWould you be interested in working on it (either writing a patch with\nsuch a fix or a test in t/ to make sure it keeps working)?\n\nThanks,\nJonathan\n"},{"id":"330256","messageId":"20171012093439.GD32090@inner.h.apk.li","threadId":"46950","inReplyTo":"20171012031702.GB155740@aiede.mtv.corp.google.com","subject":"Re: git repack leaks disk space on ENOSPC","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2017-10-12T09:34:39Z","receivedAt":"2017-10-12T09:35:01Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"On Wed, 11 Oct 2017 20:17:03 +0000, Jonathan Nieder wrote:\n> Hi Andreas,\n> \n> Andreas Krey wrote:\n> \n> > I observed (again) an annoying behavior of 'git repack':\n> \n> Do you have context for this 'again'? E.g. was this discussed\n> previously on-list?\n\nI think I posted about it, but no discussion. I poked a bit\nat the code, with not much luck back then.\n\n...\n> Does using create_tempfile there seem like a good path forward to you?\n> Would you be interested in working on it (either writing a patch with\n> such a fix or a test in t/ to make sure it keeps working)?\n\nI will look into creating a patch (thanks for the pointers),\nbut I don't see how to make a testcase for this - pre-filling the\ndisk doesn't sound like a good idea. Most people probably won't run in\nthis situation, and then won't have tmp_packs with a dozen GBytes each\nlying around.\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds \nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"330264","messageId":"20171012110158.pxjn6ckgw6z2g3md@genre.crustytoothpaste.net","threadId":"46950","inReplyTo":"20171012093439.GD32090@inner.h.apk.li","subject":"Re: git repack leaks disk space on ENOSPC","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-12T11:01:58Z","receivedAt":"2017-10-12T11:02:12Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Oct 12, 2017 at 11:34:39AM +0200, Andreas Krey wrote:\n> On Wed, 11 Oct 2017 20:17:03 +0000, Jonathan Nieder wrote:\n> > Does using create_tempfile there seem like a good path forward to you?\n> > Would you be interested in working on it (either writing a patch with\n> > such a fix or a test in t/ to make sure it keeps working)?\n> \n> I will look into creating a patch (thanks for the pointers),\n> but I don't see how to make a testcase for this - pre-filling the\n> disk doesn't sound like a good idea. Most people probably won't run in\n> this situation, and then won't have tmp_packs with a dozen GBytes each\n> lying around.\n\nA patch would be very welcome. We have this problem not infrequently at\nwork with development and test VMs, which tend to have a relatively\nsmall amount of disk.\n\nIf you decide that you don't want to create a patch, I'll probably pick\nit up eventually.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"330281","messageId":"20171012132730.bvglyiar4h6win4b@sigill.intra.peff.net","threadId":"46950","inReplyTo":"20171012031702.GB155740@aiede.mtv.corp.google.com","subject":"Re: git repack leaks disk space on ENOSPC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T13:27:30Z","receivedAt":"2017-10-12T13:27:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2017 at 08:17:03PM -0700, Jonathan Nieder wrote:\n\n> I can imagine this behavior of retaining tmp_pack being useful for\n> debugging in some circumstances, but I agree with you that it is\n> certainly not a good default.\n> \n> Chasing this down, I find:\n> \n> pack-write.c::create_tmp_packfile chooses the filename\n> builtin/pack-objects.c::write_pack_file writes to it and the .bitmap, calling\n> pack-write.c::finish_tmp_packfile to rename it into place\n> \n> Nothing tries to install an atexit handler to do anything special to it\n> on exit.\n> \n> The natural thing, I'd expect, would be for pack-write to use the\n> tempfile API (see tempfile.h) to create and finish the file. That way,\n> we'd get such atexit handlers for free. If we want a way to keep temp\n> files for debugging on abnormal exit, we could set that up separately as\n> a generic feature of the tempfile API (e.g. an envvar\n> GIT_KEEP_TEMPFILES_ON_FAILURE), making that an orthogonal topic.\n\nYes, I think this is the right direction. I've had a patch in GitHub's\nfork for years that does so (since otherwise failures can fill up your\ndisk and need manual intervention).\n\nThe main reason that I hadn't submitted it upstream was because of the\n\"you can never free a struct tempfile\" requirement. So my patch just\nleaks the tempfile structs. That's OK for packs, of which we tend to\ncreate only a few in a given process, but doesn't scale for loose\nobjects.\n\nNow that 89563ec379 (Merge branch 'jk/incore-lockfile-removal',\n2017-09-19) has landed, I think it makes sense to pursue that direction.\n\nMy patch roughly looks like:\n\n diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n index 4ff567db47..7f261e56c4 100644\n --- a/builtin/index-pack.c\n +++ b/builtin/index-pack.c\n\n @@ -308,9 +348,11 @@ static const char *open_pack_file(const char *pack_name)\n input_fd = 0;\n if (!pack_name) {\n struct strbuf tmp_file = STRBUF_INIT;\n + struct tempfile *t = xcalloc(1, sizeof(*t));\n output_fd = odb_mkstemp(&tmp_file,\n \"pack/tmp_pack_XXXXXX\");\n pack_name = strbuf_detach(&tmp_file, NULL);\n + register_tempfile(t, pack_name);\n } else {\n output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n if (output_fd < 0)\n\nbut note that's not quite what we'd want. It never closes the tempfile,\nso:\n\n 1. Under the new regime, we'd still leak the struct!\n\n 2. Git will still try to unlink the tempfile on exit, even if we\n successfully moved it into place.\n\nSo I think all the code around open_pack_file() needs to learn to pass\naround the tempfile struct, and eventually use rename_tempfile() to\ncement it in place. I also suspect that odb_mkstemp should just take a\n\"struct tempfile\".\n\n-Peff\n"},{"id":"330282","messageId":"20171012133636.isvg5typrputvxmm@sigill.intra.peff.net","threadId":"46950","inReplyTo":"20171012093439.GD32090@inner.h.apk.li","subject":"Re: git repack leaks disk space on ENOSPC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T13:36:37Z","receivedAt":"2017-10-12T13:36:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 11:34:39AM +0200, Andreas Krey wrote:\n\n> > Does using create_tempfile there seem like a good path forward to you?\n> > Would you be interested in working on it (either writing a patch with\n> > such a fix or a test in t/ to make sure it keeps working)?\n> \n> I will look into creating a patch (thanks for the pointers),\n> but I don't see how to make a testcase for this - pre-filling the\n> disk doesn't sound like a good idea. Most people probably won't run in\n> this situation, and then won't have tmp_packs with a dozen GBytes each\n> lying around.\n\nIt may be easier to trigger a case which rejects the pack for other\nreasons. For an incoming index-pack, turning on transfer.fsckObjects is\nan easy one. For a repack, perhaps corrupting a loose object\nto-be-packed would work.\n\nE.g.:\n\n git init\n echo content >file\n git add file\n git commit -m foo\n\n # corrupt the blob in a subtle way\n obj=.git/objects/$(git rev-parse HEAD:file | sed 's,..,&/,')\n chmod +w $obj\n echo cruft >>$obj\n\nAfter that, I get:\n\n $ git repack -ad\n Counting objects: 3, done.\n error: garbage at end of loose object 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n fatal: loose object d95f3ad14dee633a758d2e331151e950dd13e4ed (stored in .git/objects/d9/5f3ad14dee633a758d2e331151e950dd13e4ed) is corrupt\n\n $ find -type f .git/objects/pack\n .git/objects/pack/tmp_pack_0GaXwk\n\n-Peff\n"}]}