{"thread":{"id":"45307","subject":"[PATCH] repack: Add options to preserve and prune old pack files","startedAt":"2017-03-07T16:42:38Z","lastAt":"2017-03-10T21:52:16Z","messageCount":5,"participants":["James Melvin","Junio C Hamano","jmelvin@codeaurora.org","Martin Fick"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313433","messageId":"20170307164035.27866-1-jmelvin@codeaurora.org","threadId":"45307","inReplyTo":null,"subject":"[PATCH] repack: Add options to preserve and prune old pack files","fromName":"James Melvin","fromEmail":"jmelvin@codeaurora.org","sentAt":"2017-03-07T16:40:35Z","receivedAt":"2017-03-07T16:42:38Z","isPatch":true,"sender":{"key":"jmelvin@codeaurora.org","avatar":null},"body":"The new --preserve-oldpacks option moves old pack files into the\npreserved subdirectory instead of deleting them after repacking.\n\nThe new --prune-preserved option prunes old pack files from the\npreserved subdirectory after repacking, but before potentially\nmoving the latest old packfiles to this subdirectory.\n\nThese options are 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 <jmelvin@codeaurora.org>\n---\n Documentation/git-repack.txt |  9 +++++++++\n builtin/repack.c             | 38 ++++++++++++++++++++++++++++++++++++--\n 2 files changed, 45 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 26afe6ed5..0b19b761f 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -143,6 +143,15 @@ 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-oldpacks::\n+\t Move old pack files into the preserved subdirectory instead\n+\t of deleting them after repacking.\n+\n+--prune-preserved::\n+\t Prune old pack files from the preserved subdirectory after\n+\t repacking, but before potentially moving the latest old\n+\t packfiles to this subdirectory\n+\n Configuration\n -------------\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 677bc7c81..f1a0c97f3 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -10,8 +10,10 @@\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n+static int preserve_oldpacks = 0;\n+static int prune_preserved = 0;\n static int write_bitmaps;\n-static char *packdir, *packtmp;\n+static char *packdir, *packtmp, *preservedir;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack [<options>]\"),\n@@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\n \tclosedir(dir);\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+\tif (mkdir(preservedir, 0700) && errno != EEXIST)\n+\t\terror(_(\"failed to create preserve directory\"));\n+\n+\tfname_old = mkpathdup(\"%s/%s.old-%s\", preservedir, file_name, ++file_ext);\n+\trename(file_path, fname_old);\n+\n+\tfree(fname_old);\n+}\n+\n+static void remove_preserved_dir(void) {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&buf, preservedir);\n+\tremove_dir_recursively(&buf, 0);\n+\tstrbuf_release(&buf);\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 +144,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_oldpacks)\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 +220,10 @@ 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-oldpacks\", &preserve_oldpacks,\n+\t\t\t\tN_(\"move old pack files into the preserved subdirectory\")),\n+\t\tOPT_BOOL(0, \"prune-preserved\", &prune_preserved,\n+\t\t\t\tN_(\"prune old pack files from the preserved subdirectory after repacking\")),\n \t\tOPT_END()\n \t};\n \n@@ -217,6 +247,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\tpreservedir = mkpathdup(\"%s/preserved\", packdir);\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n@@ -404,6 +435,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t/* End of pack replacement. */\n \n+\tif (prune_preserved)\n+\t\tremove_preserved_dir();\n+\n \tif (delete_redundant) {\n \t\tint opts = 0;\n \t\tstring_list_sort(&names);\n-- \n2.12.0.190.gab6997d48.dirty\n\n"},{"id":"313449","messageId":"xmqq4lz4968p.fsf@gitster.mtv.corp.google.com","threadId":"45307","inReplyTo":"20170307164035.27866-1-jmelvin@codeaurora.org","subject":"Re: [PATCH] repack: Add options to preserve and prune old pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-07T20:33:42Z","receivedAt":"2017-03-07T20:34:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Melvin <jmelvin@codeaurora.org> writes:\n\n> These options are 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\nI find it a very sensible strategy to work around NFS, but it does\nnot explain why the directory the old ones are moved to need to be\nconfigurable.  It feels to me that a boolean that causes the old\nones renamed s/^pack-/^old-&/ in the same directory (instead of\npruning them right away) would risk less chances of mistakes (e.g.\nmaking \"preserved\" subdirectory on a separate device mounted there\nin a hope to reduce disk usage of the primary repository, which\nmay defeat the whole point of moving the still-active file around\ninstead of removing them).\n\nAnd if we make \"preserve-old\" a boolean, perhaps the presence of\n\"prune-preserved\" would serve as a substitute for it, iow, perhaps\nwe may only need --prune-preserved option (and repack.prunePreserved\nconfiguration variable)?\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 677bc7c81..f1a0c97f3 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -10,8 +10,10 @@\n>  \n>  static int delta_base_offset = 1;\n>  static int pack_kept_objects = -1;\n> +static int preserve_oldpacks = 0;\n> +static int prune_preserved = 0;\n\nWe avoid initializing statics to 0 or NULL and instead let BSS take\ncare of them...\n\n>  static int write_bitmaps;\n> -static char *packdir, *packtmp;\n> +static char *packdir, *packtmp, *preservedir;\n\n... just like what you did here.\n\n> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s\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> +\tif (mkdir(preservedir, 0700) && errno != EEXIST)\n> +\t\terror(_(\"failed to create preserve directory\"));\n\nYou do not want to do the rest of this function after issuing this\nerror, no?  Because ...\n\n> +\n> +\tfname_old = mkpathdup(\"%s/%s.old-%s\", preservedir, file_name, ++file_ext);\n> +\trename(file_path, fname_old);\n\n... this rename(2) would fail, whose error return you would catch\nand act on.\n\n> +\tfree(fname_old);\n> +}\n> +\n> +static void remove_preserved_dir(void) {\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_addstr(&buf, preservedir);\n> +\tremove_dir_recursively(&buf, 0);\n\nThis is a wrong helper function to use on files and directories\ninside .git/; the function is about removing paths in the working\ntree.\n\n> @@ -121,7 +144,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_oldpacks)\n> +\t\t\tpreserve_pack(buf.buf, base_name, exts[i]);\n> +\t\telse\n> +\t\t\tunlink(buf.buf);\n\nOK.\n\n> @@ -194,6 +220,10 @@ 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-oldpacks\", &preserve_oldpacks,\n> +\t\t\t\tN_(\"move old pack files into the preserved subdirectory\")),\n> +\t\tOPT_BOOL(0, \"prune-preserved\", &prune_preserved,\n> +\t\t\t\tN_(\"prune old pack files from the preserved subdirectory after repacking\")),\n>  \t\tOPT_END()\n>  \t};\n>  \n> @@ -217,6 +247,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n>  \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n> +\tpreservedir = mkpathdup(\"%s/preserved\", packdir);\n>  \n>  \tsigchain_push_common(remove_pack_on_signal);\n>  \n> @@ -404,6 +435,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \t/* End of pack replacement. */\n>  \n> +\tif (prune_preserved)\n> +\t\tremove_preserved_dir();\n\nI am not sure if I understand your design.  Your model looks to me\nlike there are two modes of operation.  #1 uses \"--preserve-old\" and\nsends old ones to purgatory instead of removing them and #2 uses\n\"--prune-preserved\" to remove all the ones in the purgatory\nimmediately.  A few things that come to my mind immediately:\n\n * When \"--prune-preseved\" is used, it removes both ancient ones and\n   more recent ones indiscriminately.  Would it make more sense to\n   \"expire\" only the older ones while keeping the more recent ones?\n\n * It appears that the main reason you would want --prune-preserved\n   in this design is after running with \"--preserve-old\" number of\n   times, you want to remove really old ones that have accumulated,\n   and I would imagine that at that point of time, you are only\n   interested in repack, but the code structure tells me that this\n   will force the users to first run a repack before pruning.\n\nI suspect that a design that is simpler to explain to the users may\nbe to add a command line option \"--preserve-pruned=<expiration>\" and\na matching configuration variable repack.preservePruned, which\ndefaults to \"immediate\" (i.e. no preserving), and\n\n - When the value of preserve_pruned is not \"immediate\", use\n   preserve_pack() instead of unlink();\n\n - At the end, find preserved packs that are older than the value in\n   preserve_pruned and unlink() them.\n\nIt also may make sense to add another command line option\n\"--prune-preserved-packs-only\" (without matching configuration\nvariable) that _ONLY_ does the \"find older preserved packs and\nunlink them\" part, without doing any repack.\n"},{"id":"313653","messageId":"1d816bbb08b228ece9a74ffcdfb7a5b1@codeaurora.org","threadId":"45307","inReplyTo":"xmqq4lz4968p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] repack: Add options to preserve and prune old pack files","fromName":"","fromEmail":"jmelvin@codeaurora.org","sentAt":"2017-03-09T17:50:21Z","receivedAt":"2017-03-09T17:50:39Z","isPatch":true,"sender":{"key":"jmelvin@codeaurora.org","avatar":null},"body":"On 2017-03-07 13:33, Junio C Hamano wrote:\n> James Melvin <jmelvin@codeaurora.org> writes:\n> \n>> These options are 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> I find it a very sensible strategy to work around NFS, but it does\n> not explain why the directory the old ones are moved to need to be\n> configurable.  It feels to me that a boolean that causes the old\n> ones renamed s/^pack-/^old-&/ in the same directory (instead of\n> pruning them right away) would risk less chances of mistakes (e.g.\n> making \"preserved\" subdirectory on a separate device mounted there\n> in a hope to reduce disk usage of the primary repository, which\n> may defeat the whole point of moving the still-active file around\n> instead of removing them).\n\nMoving the preserved pack files to a separate directory only helped make \nthe pack directory cleaner, but I agree that having the old* pack files \nin the same directory is a better approach as it would ensure that it's \nstill on the same mounted device. I'll update the logic to reflect that.\n\nAs for the naming convention of the preserved pack files, there is \nalready some logic to remove \"old-\" files in repack. Currently this is \nthe naming convention I have for them:\n\npack-<sha1>.old-<ext>\npack-7412ee739b8a20941aa1c2fd03abcc7336b330ba.old-pack\n\nOne advantage of that is the extension is no longer an expected one, \ndifferentiating it from current pack files.\n\nThat said, if that is not a concern, I could prefix them with \n\"preserved\" instead of \"old\" to differentiate them from the other logic \nthat cleans up \"old-*\". What are your thoughts on that?\n\npreserved-<sha1>.<ext>\npreserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.pack\n\n\n> And if we make \"preserve-old\" a boolean, perhaps the presence of\n> \"prune-preserved\" would serve as a substitute for it, iow, perhaps\n> we may only need --prune-preserved option (and repack.prunePreserved\n> configuration variable)?\n> \n>> diff --git a/builtin/repack.c b/builtin/repack.c\n>> index 677bc7c81..f1a0c97f3 100644\n>> --- a/builtin/repack.c\n>> +++ b/builtin/repack.c\n>> @@ -10,8 +10,10 @@\n>> \n>>  static int delta_base_offset = 1;\n>>  static int pack_kept_objects = -1;\n>> +static int preserve_oldpacks = 0;\n>> +static int prune_preserved = 0;\n> \n> We avoid initializing statics to 0 or NULL and instead let BSS take\n> care of them...\n\nWill do\n\n>>  static int write_bitmaps;\n>> -static char *packdir, *packtmp;\n>> +static char *packdir, *packtmp, *preservedir;\n> \n> ... just like what you did here.\n> \n>> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s\n>> ...\n>> +static void preserve_pack(const char *file_path, const char \n>> *file_name,  const char *file_ext)\n>> +{\n>> +\tchar *fname_old;\n>> +\n>> +\tif (mkdir(preservedir, 0700) && errno != EEXIST)\n>> +\t\terror(_(\"failed to create preserve directory\"));\n> \n> You do not want to do the rest of this function after issuing this\n> error, no?  Because ...\n\nAgreed, I'll update.\n\n>> +\n>> +\tfname_old = mkpathdup(\"%s/%s.old-%s\", preservedir, file_name, \n>> ++file_ext);\n>> +\trename(file_path, fname_old);\n> \n> ... this rename(2) would fail, whose error return you would catch\n> and act on.\n> \n>> +\tfree(fname_old);\n>> +}\n>> +\n>> +static void remove_preserved_dir(void) {\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\n>> +\tstrbuf_addstr(&buf, preservedir);\n>> +\tremove_dir_recursively(&buf, 0);\n> \n> This is a wrong helper function to use on files and directories\n> inside .git/; the function is about removing paths in the working\n> tree.\n\nWill update.\n\n-- \nQualcomm Innovation Center, Inc.\nThe Qualcomm Innovation Center, Inc. is a member of the Code Aurora \nForum,\na Linux Foundation Collaborative Project\n"},{"id":"313665","messageId":"4065744.f4TKuYAukl@mfick1-lnx","threadId":"45307","inReplyTo":"1d816bbb08b228ece9a74ffcdfb7a5b1@codeaurora.org","subject":"Re: [PATCH] repack: Add options to preserve and prune old pack files","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2017-03-09T18:45:06Z","receivedAt":"2017-03-09T18:45:13Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Thursday, March 09, 2017 10:50:21 AM \njmelvin@codeaurora.org wrote:\n> On 2017-03-07 13:33, Junio C Hamano wrote:\n> > James Melvin <jmelvin@codeaurora.org> writes:\n> >> These options are designed to prevent stale file handle\n> >> exceptions during git operations which can happen on\n> >> users of NFS repos when repacking is done on them. The\n> >> strategy is to preserve old pack files around until\n> >> the next repack with the hopes that they will become\n> >> unreferenced by then and not cause any exceptions to\n> >> running processes when they are finally deleted\n> >> (pruned).\n> > \n> > I find it a very sensible strategy to work around NFS,\n> > but it does not explain why the directory the old ones\n> > are moved to need to be configurable.  It feels to me\n> > that a boolean that causes the old ones renamed\n> > s/^pack-/^old-&/ in the same directory (instead of\n> > pruning them right away) would risk less chances of\n> > mistakes (e.g. making \"preserved\" subdirectory on a\n> > separate device mounted there in a hope to reduce disk\n> > usage of the primary repository, which may defeat the\n> > whole point of moving the still-active file around\n> > instead of removing them).\n> \n> Moving the preserved pack files to a separate directory\n> only helped make the pack directory cleaner, but I agree\n> that having the old* pack files in the same directory is\n> a better approach as it would ensure that it's still on\n> the same mounted device. I'll update the logic to reflect\n> that.\n> \n> As for the naming convention of the preserved pack files,\n> there is already some logic to remove \"old-\" files in\n> repack. Currently this is the naming convention I have\n> for them:\n> \n> pack-<sha1>.old-<ext>\n> pack-7412ee739b8a20941aa1c2fd03abcc7336b330ba.old-pack\n> \n> One advantage of that is the extension is no longer an\n> expected one, differentiating it from current pack files.\n> \n> That said, if that is not a concern, I could prefix them\n> with \"preserved\" instead of \"old\" to differentiate them\n> from the other logic that cleans up \"old-*\". What are\n> your thoughts on that?\n> \n> preserved-<sha1>.<ext>\n> preserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.pack\n\nSome other proposals so that the preserved files do not get \nreturned by naive finds based on their extensions,\n\n preserved-<sha1>.<ext>-preserved\n preserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.pack-\npreserved\n\nor:\n\n preserved-<sha1>.preserved-<ext>\n preserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.preserved-\npack\n\nor maybe even just:\n\n preserved-<ext>-<sha1>\n preserved-pack-7412ee739b8a20941aa1c2fd03abcc7336b330ba\n\n\n-Martin\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n"},{"id":"313802","messageId":"5b578a236338771fbb08007417b26438@codeaurora.org","threadId":"45307","inReplyTo":"xmqq4lz4968p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] repack: Add options to preserve and prune old pack files","fromName":"","fromEmail":"jmelvin@codeaurora.org","sentAt":"2017-03-10T21:52:10Z","receivedAt":"2017-03-10T21:52:16Z","isPatch":true,"sender":{"key":"jmelvin@codeaurora.org","avatar":null},"body":"On 2017-03-07 13:33, Junio C Hamano wrote:\n> James Melvin <jmelvin@codeaurora.org> writes:\n> \n...\n> \n> I am not sure if I understand your design.  Your model looks to me\n> like there are two modes of operation.  #1 uses \"--preserve-old\" and\n> sends old ones to purgatory instead of removing them and #2 uses\n> \"--prune-preserved\" to remove all the ones in the purgatory\n> immediately.  A few things that come to my mind immediately:\n> \n>  * When \"--prune-preseved\" is used, it removes both ancient ones and\n>    more recent ones indiscriminately.  Would it make more sense to\n>    \"expire\" only the older ones while keeping the more recent ones?\n> \n>  * It appears that the main reason you would want --prune-preserved\n>    in this design is after running with \"--preserve-old\" number of\n>    times, you want to remove really old ones that have accumulated,\n>    and I would imagine that at that point of time, you are only\n>    interested in repack, but the code structure tells me that this\n>    will force the users to first run a repack before pruning.\n> \n> I suspect that a design that is simpler to explain to the users may\n> be to add a command line option \"--preserve-pruned=<expiration>\" and\n> a matching configuration variable repack.preservePruned, which\n> defaults to \"immediate\" (i.e. no preserving), and\n> \n>  - When the value of preserve_pruned is not \"immediate\", use\n>    preserve_pack() instead of unlink();\n> \n>  - At the end, find preserved packs that are older than the value in\n>    preserve_pruned and unlink() them.\n> \n> It also may make sense to add another command line option\n> \"--prune-preserved-packs-only\" (without matching configuration\n> variable) that _ONLY_ does the \"find older preserved packs and\n> unlink them\" part, without doing any repack.\n\nI like the idea of only having a single option to do both the preserving \nand the pruning. It makes the operation more cycle based, which is more \nclosely tied to the use case this is intended for. Ie. Run repack while \npreserving old pack files so that any open file handles to those pack \nfiles will continue to work, while the next time repack is run it is \nhighly likely those will no longer be needed, so they can be cleaned up. \nObviously this depends on how frequent repack is run, but something an \nAdministrator can determine.\n\nI'll push a patch that combines that functionality into a single option \nto review.\n\n\n-James\n\n-- \nQualcomm Innovation Center, Inc.\nThe Qualcomm Innovation Center, Inc. is a member of the Code Aurora \nForum,\na Linux Foundation Collaborative Project\n"}]}