{"thread":{"id":"45348","subject":"[PATCH v2] repack: Add option to preserve and prune old pack files","startedAt":"2017-03-10T22:00:42Z","lastAt":"2017-03-13T16:40:21Z","messageCount":6,"participants":["James Melvin","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King","Martin Fick"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"313803","messageId":"20170310220020.2666-1-jmelvin@codeaurora.org","threadId":"45348","inReplyTo":null,"subject":"[PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"James Melvin","fromEmail":"jmelvin@codeaurora.org","sentAt":"2017-03-10T22:00:20Z","receivedAt":"2017-03-10T22:00:42Z","isPatch":true,"sender":{"key":"jmelvin@codeaurora.org","avatar":null},"body":"The new --preserve-and-prune option renames old pack files\ninstead of deleting them after repacking and prunes previously\npreserved pack files.\n\nThis option is designed to prevent stale file handle exceptions\nduring git operations which can happen on users of NFS repos when\nrepacking is done on them. The strategy is to preserve old pack files\naround until the next repack with the hopes that they will become\nunreferenced by then and not cause any exceptions to running processes\nwhen they are finally deleted (pruned).\n\nSigned-off-by: James Melvin \n---\n Documentation/git-repack.txt | 4 +++\n builtin/repack.c | 66 +++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 26afe6ed5..effd98a43 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -143,6 +143,10 @@ other objects in that pack they already have locally.\n \tbeing removed. In addition, any unreachable loose objects will\n \tbe packed (and their loose counterparts removed).\n \n+--preserve-and-prune::\n+\t Preserve old pack files by renaming them instead of deleting. Prune any\n+\t previously preserved pack files before preserving new ones.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 677bc7c81..78fad8f1a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -10,6 +10,7 @@\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n+static int preserve_and_prune;\n static int write_bitmaps;\n static char *packdir, *packtmp;\n \n@@ -108,6 +109,60 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\n \tclosedir(dir);\n }\n \n+/*\n+ * Adds all preserved packs hex strings to the fname list\n+ */\n+static void get_preserved_pack_filenames(struct string_list *fname_list)\n+{\n+\tDIR *dir;\n+\tstruct dirent *e;\n+\tchar *fname;\n+\n+\tif (!(dir = opendir(packdir)))\n+\t\treturn;\n+\n+\twhile ((e = readdir(dir)) != NULL) {\n+\t\tsize_t len;\n+\t\tif (!strip_suffix(e->d_name, \".old-pack\", &len))\n+\t\t\tcontinue;\n+\n+\t\tfname = xmemdupz(e->d_name, len);\n+\t\tstring_list_append_nodup(fname_list, fname);\n+\t}\n+\tclosedir(dir);\n+}\n+\n+static void remove_preserved_packs(void) {\n+\tconst char *exts[] = {\"pack\", \"idx\", \"keep\", \"bitmap\"};\n+\tint i;\n+\tstruct string_list names = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n+\n+\tget_preserved_pack_filenames(&names);\n+\n+\tfor_each_string_list_item(item, &names) {\n+\t\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\t\tchar *fname;\n+\t\t\tfname = mkpathdup(\"%s/%s.old-%s\",\n+\t\t\t\t\t packdir,\n+\t\t\t\t\t item->string,\n+\t\t\t\t\t exts[i]);\n+\t\t\tif (remove_path(fname))\n+\t\t\t\twarning(_(\"failed to remove '%s'\"), fname);\n+\t\t\tfree(fname);\n+\t\t}\n+\t}\n+}\n+\n+static void preserve_pack(const char *file_path, const char *file_name, const char *file_ext)\n+{\n+\tchar *fname_old;\n+\n+\tfname_old = mkpathdup(\"%s/%s.old-%s\", packdir, file_name, ++file_ext);\n+\trename(file_path, fname_old);\n+\tfree(fname_old);\n+}\n+\n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n \tconst char *exts[] = {\".pack\", \".idx\", \".keep\", \".bitmap\"};\n@@ -121,7 +176,10 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)\n \tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n \t\tstrbuf_setlen(&buf, plen);\n \t\tstrbuf_addstr(&buf, exts[i]);\n-\t\tunlink(buf.buf);\n+\t\tif (preserve_and_prune)\n+\t\t\tpreserve_pack(buf.buf, base_name, exts[i]);\n+\t\telse\n+\t\t\tunlink(buf.buf);\n \t}\n \tstrbuf_release(&buf);\n }\n@@ -194,6 +252,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n+\t\tOPT_BOOL(0, \"preserve-and-prune\", &preserve_and_prune,\n+\t\t\t\tN_(\"preserve old pack files by renaming them instead of deleting, prune any \"\n+\t\t\t\t\t\t\"previously preserved pack files before preserving new ones\")),\n \t\tOPT_END()\n \t};\n \n@@ -404,6 +465,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t/* End of pack replacement. */\n \n+\tif (preserve_and_prune)\n+\t\tremove_preserved_packs();\n+\n \tif (delete_redundant) {\n \t\tint opts = 0;\n \t\tstring_list_sort(&names);\n-- \n2.12.0.191.ga15447d9f\n\n"},{"id":"313809","messageId":"xmqqmvcswvdc.fsf@gitster.mtv.corp.google.com","threadId":"45348","inReplyTo":"20170310220020.2666-1-jmelvin@codeaurora.org","subject":"Re: [PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-10T23:43:43Z","receivedAt":"2017-03-10T23:43:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Melvin writes:\n\n> The new --preserve-and-prune option renames old pack files\n> instead of deleting them after repacking and prunes previously\n> preserved pack files.\n>\n> This option is designed to prevent stale file handle exceptions\n> during git operations which can happen on users of NFS repos when\n> repacking is done on them. The strategy is to preserve old pack files\n> around until the next repack with the hopes that they will become\n> unreferenced by then and not cause any exceptions to running processes\n> when they are finally deleted (pruned).\n\nThis certainly is simpler than the previous one, but if you run\n\n\tgit repack --preserve-and-prune \n\tsleep for N minutes\n\tgit repack --preserve-and-prune \n\nthe second \"repack\" will drop the obsoleted packs that were\npreserved by the first one no matter how short the value of N is,\nno?\n\nI suspect that users would be more comfortable with something based\non expiration timestamp that gives them a guaranteed grace period,\ne.g. \"--preserve-expire=8.hours\", like how we expire reflog entries\nand loose objects.\n\nPerhaps builtin/prune.c can be a source of inspiration?\n"},{"id":"313825","messageId":"CACBZZX4guVXWDHgAotdNa6CqTSv3iNSWpdCbexJQ_Y3AXU+AUQ@mail.gmail.com","threadId":"45348","inReplyTo":"20170310220020.2666-1-jmelvin@codeaurora.org","subject":"Re: [PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-11T12:38:00Z","receivedAt":"2017-03-11T12:38:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 10, 2017 at 11:00 PM, James Melvin wrote:\n> The new --preserve-and-prune option renames old pack files\n> instead of deleting them after repacking and prunes previously\n> preserved pack files.\n\nI think some of this rationale...\n\n> This option is designed to prevent stale file handle exceptions\n> during git operations which can happen on users of NFS repos when\n> repacking is done on them. The strategy is to preserve old pack files\n> around until the next repack with the hopes that they will become\n> unreferenced by then and not cause any exceptions to running processes\n> when they are finally deleted (pruned).\n\n...belongs in the actual docs, i.e. here:\n\n> +--preserve-and-prune::\n> + Preserve old pack files by renaming them instead of deleting. Prune any\n> + previously preserved pack files before preserving new ones.\n> +\n\nThis is a really obscure option with an obscure use-case, let's\nexplain that in the docs, and also mention NFS & what problems it\nsolves there, so someone having the same issue can find the solution\nmore easily.\n"},{"id":"313852","messageId":"20170312122418.e544nrmqniycqouc@sigill.intra.peff.net","threadId":"45348","inReplyTo":"xmqqmvcswvdc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T12:24:18Z","receivedAt":"2017-03-12T12:24:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 03:43:43PM -0800, Junio C Hamano wrote:\n\n> James Melvin writes:\n> \n> > The new --preserve-and-prune option renames old pack files\n> > instead of deleting them after repacking and prunes previously\n> > preserved pack files.\n> >\n> > This option is designed to prevent stale file handle exceptions\n> > during git operations which can happen on users of NFS repos when\n> > repacking is done on them. The strategy is to preserve old pack files\n> > around until the next repack with the hopes that they will become\n> > unreferenced by then and not cause any exceptions to running processes\n> > when they are finally deleted (pruned).\n> \n> This certainly is simpler than the previous one, but if you run\n> \n> \tgit repack --preserve-and-prune \n> \tsleep for N minutes\n> \tgit repack --preserve-and-prune \n> \n> the second \"repack\" will drop the obsoleted packs that were\n> preserved by the first one no matter how short the value of N is,\n> no?\n> \n> I suspect that users would be more comfortable with something based\n> on expiration timestamp that gives them a guaranteed grace period,\n> e.g. \"--preserve-expire=8.hours\", like how we expire reflog entries\n> and loose objects.\n> \n> Perhaps builtin/prune.c can be a source of inspiration?\n\nI have been a bit slow to read this series, but FWIW I had the exact\nsame thought while reading up to this point. There should be an\nexpiration, and that expiration time corresponds roughly to \"how long do\nyou expect a long-running git operation to last\".\n\nYou'd probably want \"--preserve-expire\" as an option to repack, and then\na \"gc.preservePackExpire\" option so that \"git gc\" triggers it reliably.\n\nI can think of one downside of a time-based solution, though: if you run\nmultiple gc's during the time period, you may end up using a lot of disk\nspace (one repo's worth per gc). But that's a fundamental tension in the\nproblem space; the whole point is to waste disk to keep helping old\nprocesses. But you may want a knob that lets you slide between those two\nthings. For instance, if you kept a sliding window of N sets of\npreserved packs, and ejected from one end of the window (regardless of\ntime), while inserting into the other end. James' existing patch is that\nsame strategy with a hardcoded window of \"1\".\n\nThe other variable you can manipulate is whether to gc in the first\nplace. E.g., don't gc if there are N preserved sets (or sets consuming\nmore than N bytes, or whatever). You could do that check outside of git\nentirely (or in an auto-gc hook, if you're using it).\n\nNote that something like a sliding window gets complicated pretty\nquickly. I'm not really proposing it as a direction; I'm just trying to\nthink of ways the time-based system could go wrong. IMHO it would\nprobably be fine to just ignore the problem and assume that repacking\ndoesn't happen often enough for it to come up in practice.\n\n-Peff\n"},{"id":"313864","messageId":"xmqqbmt6v0cf.fsf@gitster.mtv.corp.google.com","threadId":"45348","inReplyTo":"20170312122418.e544nrmqniycqouc@sigill.intra.peff.net","subject":"Re: [PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T18:03:44Z","receivedAt":"2017-03-12T18:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I can think of one downside of a time-based solution, though: if you run\n> multiple gc's during the time period, you may end up using a lot of disk\n> space (one repo's worth per gc). But that's a fundamental tension in the\n> problem space; the whole point is to waste disk to keep helping old\n> processes.\n\nYes. If you want to help a process that mmap's a packfile and wants\nto keep using it for N seconds, no matter how many times somebody\nelse ran \"git repack\" while you are doing your work within that\ntimeframe, you somehow need to make sure the NFS server knows the\nfile is still in use for that N seconds.\n\n> But you may want a knob that lets you slide between those two\n> things. For instance, if you kept a sliding window of N sets of\n> preserved packs, and ejected from one end of the window (regardless of\n> time), while inserting into the other end. James' existing patch is that\n> same strategy with a hardcoded window of \"1\".\n\nAgain, yes. But then the user does not get any guarantee of how\nlong-living a process the user can have without getting broken by\nthe NFS server losing track of a packfile that is still in use. My\nsuggestion for the \"expiry\" based approach is essentially that I do\nnot see a useful tradeoff afforded by having such a knob.\n\n> The other variable you can manipulate is whether to gc in the first\n> place. E.g., don't gc if there are N preserved sets (or sets consuming\n> more than N bytes, or whatever). You could do that check outside of git\n> entirely (or in an auto-gc hook, if you're using it).\n\nYes, \"don't gc/repack more than once within N seconds\" may also be\nan alternative and may generally be more useful by covering general\nsource of wastage coming from doing gc too frequently, not necessarily\nlimited to preserved pack accumulation.\n\n\n"},{"id":"313897","messageId":"2062422.zvngdQjy4d@mfick1-lnx","threadId":"45348","inReplyTo":"xmqqbmt6v0cf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] repack: Add option to preserve and prune old pack files","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2017-03-13T16:39:37Z","receivedAt":"2017-03-13T16:40:21Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Sunday, March 12, 2017 11:03:44 AM Junio C Hamano wrote:\n> Jeff King writes:\n> > I can think of one downside of a time-based solution,\n> > though: if you run multiple gc's during the time\n> > period, you may end up using a lot of disk space (one\n> > repo's worth per gc). But that's a fundamental tension\n> > in the problem space; the whole point is to waste disk\n> > to keep helping old processes.\n> \n> Yes. If you want to help a process that mmap's a packfile\n> and wants to keep using it for N seconds, no matter how\n> many times somebody else ran \"git repack\" while you are\n> doing your work within that timeframe, you somehow need\n> to make sure the NFS server knows the file is still in\n> use for that N seconds.\n> \n> > But you may want a knob that lets you slide between\n> > those two things. For instance, if you kept a sliding\n> > window of N sets of preserved packs, and ejected from\n> > one end of the window (regardless of time), while\n> > inserting into the other end. James' existing patch is\n> > that same strategy with a hardcoded window of \"1\".\n> \n> Again, yes. But then the user does not get any guarantee\n> of how long-living a process the user can have without\n> getting broken by the NFS server losing track of a\n> packfile that is still in use. My suggestion for the\n> \"expiry\" based approach is essentially that I do not see\n> a useful tradeoff afforded by having such a knob.\n> > The other variable you can manipulate is whether to gc\n> > in the first place. E.g., don't gc if there are N\n> > preserved sets (or sets consuming more than N bytes, or\n> > whatever). You could do that check outside of git\n> > entirely (or in an auto-gc hook, if you're using it).\n> Yes, \"don't gc/repack more than once within N seconds\" may\n> also be an alternative and may generally be more useful\n> by covering general source of wastage coming from doing\n> gc too frequently, not necessarily limited to preserved\n> pack accumulation.\n\nAs someone who helps manage a Gerrit server for several \nthousand repos, all on the same NFS disks, a time based \nexpiry seems unpractical, and not something that I am very \ninterested in having. I favor the simpler (single for now) \nrepacking cycle approach, and it is what we have been using \nfor almost 6 months now successfully, without suffering any \nmore stale file handle exceptions.\n\nWhile time is indeed the factor that is going to determine \nwhether someone is going to see the stale file handles or \nnot, on a server (which is what this feature is aimed at), \nthis is secondary in my mind to predictability about space \nutilization. I have no specific minimum time that I can \nreason about, i.e. I cannot reasonably say \"I want all \noperations that last less than 1 hour, 1 min, or 1 second... \nto succeed\". I don't really want ANY failures, and I am \nwilling to sacrifice some disk space to prevent as many as \npossible. So the question to me is \"How much disk space am \nI willing to sacrifice?\", not \"How long do I want operations \nto be able to last?\". The only way that time enters my \nequation is to compare it to how long repacking takes, i.e. \nI want the preserved files cleaned up on the next repack. \nSo effectively I am choosing a repacking cycle based \napproach, so that I can reasonably predict the extra disk \nspace that I need to reserve for my collection of repos. \nWith a single cycle, I am effectively doubling the \"static\" \nsize of repos. \n\nTo achieve this predictability with a time based approach \nrequires coordination between the expiry setting and the \nrepacking time cycle. This coordination is extra effort for \nme, with no apparent gain. It is also an additional risk \nthat I don't want to have. If I decide to bump up how often \nI run repacking, and I forget to reduce the expiry time, my \ndisk utilization will grow and potentially cause serious \nissues for all my repositories (since they share the same \nvolume). This problem is even more difficult if I decide to \nuse a usage (instead of time) based algorithm to determine \nwhen I repack.\n\nAdmittedly, a repacking cycle based approach happens to be \nvery easy and practical when it is a \"single\" cycle. If I \ndetermine eventually empirically that a single cycle is not \nlong enough for my server, I don't know what I will do? \nPerhaps I would then want a switch that preserves the repos \nfor another cycle? Maybe it could work the way that log \nrotation works, add a number to the end of each file name for \neach preserved cycle? This option seems preferable to me \nthan a time based approach because it makes it more obvious \nwhat the impact on disk utilization will be. However, so \nfar in practice, this does not seem necessary.\n\nI don't really see a good use case for a time based expiry \n(other than \"this is how it was done for other things in \ngit\"). Of course, that doesn't mean such a use case doesn't \nexist, but I don't support adding a feature unless I really \nunderstand why and how someone would want to use it. I \nthink that a time based expiry should only be added if \nsomeone has a specific use case they expect to achieve with \nit, and they actually plan to use it that way, not just for \nuniformity. \n\nOne might even eventually decide that some of the other \ncurrent use cases for time based expiries should be \nconverted to cycle based expiries; I suspect server admins \nwill have fewer surprises that way,\n\n-Martin\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n"}]}