{"thread":{"id":"55327","subject":"[PATCH] builtin/fetch.c: clean tmp pack after receive signal","startedAt":"2021-03-16T02:54:19Z","lastAt":"2021-03-17T18:16:00Z","messageCount":3,"participants":["SURA via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"419264","messageId":"pull.890.git.1615863217226.gitgitgadget@gmail.com","threadId":"55327","inReplyTo":null,"subject":"[PATCH] builtin/fetch.c: clean tmp pack after receive signal","fromName":"SURA via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-16T02:53:36Z","receivedAt":"2021-03-16T02:54:19Z","isPatch":true,"sender":{"key":"surak8806@gmail.com","avatar":null},"body":"From: SURA <sura907@hotmail.com>\n\nIn Gitee.com, I often use scripts to start a time-limited\n\"git fetch\". When the execution time of'git fetch' is too\nlong, send a signal (such as SIGINT) to the'git fetch' process\n\nBut I found that after the process exits, there are some\nfiles in the format of 'objects/pack/tmp_pack_XXXXXX' on the disk.\nThey are usually very large (some of them exceed 5G), and'git gc'\nhas no effect on them.\n\nThese fetch_tmp_pack files will gradually fill up the disk.\nFor a long time, I set F_WRLCK to the file to confirm whether the\nfile is opened by other processes, and delete the file if it is not opened\n\nObviously this is only a temporary solution, I think it should be fixed from git\n\nI found fetch will start a 'index-pack' sub-process, this sub-process\ncreate the tmp_pack\n  1. make 'index-pack' unlink tmp_pack when get signal\n  2. make 'fetch' send signal to 'index-pack' when get signal\n\nSigned-off-by: SURA <sura907@hotmail.com>\n---\n    builtin/fetch.c: clean tmp pack after receive signal\n    \n    Clean tmp-pack after 'git fetch' process get signal(SIGINT, SIGHUP,\n    SIGTERM, SIGQUIT or SIGPIPE)\n    \n    Only for operations using ‘wire protocol version 2’ and ssh\n    \n    Signed-off-by: SURA sura907@hotmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-890%2FSURA907%2Fgit-fetch-tmp-pack-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-890/SURA907/git-fetch-tmp-pack-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/890\n\n builtin/fetch-pack.c |  3 ++-\n builtin/fetch.c      | 10 ++++++++++\n builtin/index-pack.c | 18 ++++++++++++++++++\n fetch-pack.c         |  7 +++++--\n fetch-pack.h         |  3 ++-\n transport.c          |  3 ++-\n transport.h          |  6 ++++++\n 7 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89ab..49a3e0bf2ae3 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -56,6 +56,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct packet_reader reader;\n \tenum protocol_version version;\n+\tpid_t index_pack_pid = -1;\n \n \tfetch_if_missing = 0;\n \n@@ -232,7 +233,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, ref, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfiles_ptr, version);\n+\t\t\t &shallow, pack_lockfiles_ptr, version, &index_pack_pid);\n \tif (pack_lockfiles.nr) {\n \t\tint i;\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0b90de87c7a2..a87efa23ceb5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -224,8 +224,18 @@ static void unlock_pack(void)\n \t\ttransport_unlock_pack(gsecondary);\n }\n \n+static void send_signo_to_index_pack(int signo)\n+{\n+\tif (gtransport && gtransport->index_pack_pid > 0)\n+\t\tkill(gtransport->index_pack_pid, signo);\n+\n+\tif (gsecondary && gsecondary->index_pack_pid > 0)\n+\t\tkill(gsecondary->index_pack_pid, signo);\n+}\n+\n static void unlock_pack_on_signal(int signo)\n {\n+\tsend_signo_to_index_pack(signo);\n \tunlock_pack();\n \tsigchain_pop(signo);\n \traise(signo);\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex bad57488079c..27d1ebba746e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -15,10 +15,13 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n+#include \"sigchain.h\"\n \n static const char index_pack_usage[] =\n \"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])\";\n \n+static const char *tmp_pack_name;\n+\n struct object_entry {\n \tstruct pack_idx_entry idx;\n \tunsigned long size;\n@@ -336,6 +339,7 @@ static const char *open_pack_file(const char *pack_name)\n \t\t\toutput_fd = odb_mkstemp(&tmp_file,\n \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n \t\t\tpack_name = strbuf_detach(&tmp_file, NULL);\n+\t\t\ttmp_pack_name = pack_name;\n \t\t} else {\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n \t\t\tif (output_fd < 0)\n@@ -353,6 +357,19 @@ static const char *open_pack_file(const char *pack_name)\n \treturn pack_name;\n }\n \n+static void remove_tmp_pack(void)\n+{\n+\tif (tmp_pack_name) \n+\t\tunlink_or_warn(tmp_pack_name);\n+}\n+\n+static void remove_tmp_pack_on_signal(int signo)\n+{\n+\tremove_tmp_pack();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n static void parse_pack_header(void)\n {\n \tstruct pack_header *hdr = fill(sizeof(struct pack_header));\n@@ -1911,6 +1928,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tcurr_pack = open_pack_file(pack_name);\n+\tsigchain_push_common(remove_tmp_pack_on_signal);\n \tparse_pack_header();\n \tobjects = xcalloc(st_add(nr_objects, 1), sizeof(struct object_entry));\n \tif (show_stat)\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6a61a464283e..bedbf69710f6 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -40,6 +40,7 @@ static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n+static pid_t *curr_index_pack_pid;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -944,6 +945,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n+\t*curr_index_pack_pid = cmd.pid;\n \tif (do_keep && (pack_lockfiles || fsck_objects)) {\n \t\tint is_well_formed;\n \t\tchar *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);\n@@ -1945,12 +1947,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       struct ref **sought, int nr_sought,\n \t\t       struct oid_array *shallow,\n \t\t       struct string_list *pack_lockfiles,\n-\t\t       enum protocol_version version)\n+\t\t       enum protocol_version version,\n+\t\t       pid_t *index_pack_pid)\n {\n \tstruct ref *ref_cpy;\n \tstruct shallow_info si;\n \tstruct oid_array shallows_scratch = OID_ARRAY_INIT;\n-\n+\tcurr_index_pack_pid = index_pack_pid;\n \tfetch_pack_setup();\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 736a3dae467a..c05a2544bb06 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -78,7 +78,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       int nr_sought,\n \t\t       struct oid_array *shallow,\n \t\t       struct string_list *pack_lockfiles,\n-\t\t       enum protocol_version version);\n+\t\t       enum protocol_version version,\n+\t\t\t   pid_t *index_pack_pid);\n \n /*\n  * Print an appropriate error message for each sought ref that wasn't\ndiff --git a/transport.c b/transport.c\nindex b13fab5dc3b1..b18f62e6bd23 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -391,7 +391,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \trefs = fetch_pack(&args, data->fd,\n \t\t\t  refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t  to_fetch, nr_heads, &data->shallow,\n-\t\t\t  &transport->pack_lockfiles, data->version);\n+\t\t\t  &transport->pack_lockfiles, data->version, &transport->index_pack_pid);\n \n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n@@ -1095,6 +1095,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t}\n \n \tret->hash_algo = &hash_algos[GIT_HASH_SHA1];\n+\tret->index_pack_pid = -1;\n \n \treturn ret;\n }\ndiff --git a/transport.h b/transport.h\nindex 24e15799e714..d25d76125784 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -118,6 +118,12 @@ struct transport {\n \tenum transport_family family;\n \n \tconst struct git_hash_algo *hash_algo;\n+\n+\t/*\n+\t * Record the pid of the index_pack sub-process\n+\t * used to send the signal to this sub-process after the main fetch process receives the signal\n+\t */\n+\tpid_t index_pack_pid;\n };\n \n #define TRANSPORT_PUSH_ALL\t\t\t(1<<0)\n\nbase-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945\n-- \ngitgitgadget\n"},{"id":"419440","messageId":"YFEpGGLBgLSdR40V@coredump.intra.peff.net","threadId":"55327","inReplyTo":"pull.890.git.1615863217226.gitgitgadget@gmail.com","subject":"Re: [PATCH] builtin/fetch.c: clean tmp pack after receive signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-16T21:54:32Z","receivedAt":"2021-03-16T21:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 16, 2021 at 02:53:36AM +0000, SURA via GitGitGadget wrote:\n\n> In Gitee.com, I often use scripts to start a time-limited\n\nNot related to your patch, but I think this name falls afoul of Git's\ntrademark policy. See:\n\n  https://git-scm.com/trademark\n\nThere's also some discussion in this thread:\n\n  https://lore.kernel.org/git/20170202022655.2jwvudhvo4hmueaw@sigill.intra.peff.net/\n\n> \"git fetch\". When the execution time of'git fetch' is too\n> long, send a signal (such as SIGINT) to the'git fetch' process\n> \n> But I found that after the process exits, there are some\n> files in the format of 'objects/pack/tmp_pack_XXXXXX' on the disk.\n> They are usually very large (some of them exceed 5G), and'git gc'\n> has no effect on them.\n\nThis isn't quite true. \"git gc\" will clean up the temporary files, but\nonly if the mtime is sufficiently old. The purpose here is to give a\ngrace period to avoid deleting a file that is actively being written to.\nHowever, we use the same grace period that we use for deleting\nunreachable objects, which is absurdly long for this purpose: 2 weeks.\nProbably something like an hour would be more appropriate (since the\nmtime is updated on each write, this would imply a process not making\nforward progress).\n\nThat said...\n\n> Obviously this is only a temporary solution, I think it should be fixed from git\n> \n> I found fetch will start a 'index-pack' sub-process, this sub-process\n> create the tmp_pack\n>   1. make 'index-pack' unlink tmp_pack when get signal\n>   2. make 'fetch' send signal to 'index-pack' when get signal\n\nI do think this is worth doing. One of the reasons we haven't\ntraditionally cleaned up temporary packfiles is that the failed state is\nsometimes useful for analyzing what went wrong, or even recovering\npartial data. But that probably should not be the default mode of\noperation (i.e., a config option or environment variable should probably\nbe able to turn it on for debugging).\n\nLooking at the implementation itself...\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 0b90de87c7a2..a87efa23ceb5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -224,8 +224,18 @@ static void unlock_pack(void)\n>  \t\ttransport_unlock_pack(gsecondary);\n>  }\n>  \n> +static void send_signo_to_index_pack(int signo)\n> +{\n> +\tif (gtransport && gtransport->index_pack_pid > 0)\n> +\t\tkill(gtransport->index_pack_pid, signo);\n> +\n> +\tif (gsecondary && gsecondary->index_pack_pid > 0)\n> +\t\tkill(gsecondary->index_pack_pid, signo);\n> +}\n> +\n>  static void unlock_pack_on_signal(int signo)\n>  {\n> +\tsend_signo_to_index_pack(signo);\n>  \tunlock_pack();\n>  \tsigchain_pop(signo);\n>  \traise(signo);\n\nWe'd probably want to also trigger this if we just hit die(), I'd think.\nWe have a system for killing process children when we exit unexpectedly.\nI think just setting the \"clean_on_exit\" flag on the index-pack\nchild_process struct would turn this into a one-liner.\n\nLikewise, we'd probably want to do this from receive-pack, too (so that\npushes don't accumulate temporary packfiles on the receiving side).\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index bad57488079c..27d1ebba746e 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> [...]\n> +static void remove_tmp_pack(void)\n> +{\n> +\tif (tmp_pack_name) \n> +\t\tunlink_or_warn(tmp_pack_name);\n> +}\n> +\n> +static void remove_tmp_pack_on_signal(int signo)\n> +{\n> +\tremove_tmp_pack();\n> +\tsigchain_pop(signo);\n> +\traise(signo);\n> +}\n\nLikewise, we have a tempfile cleanup system already.\n\nI think this hunk:\n\n> @@ -336,6 +339,7 @@ static const char *open_pack_file(const char *pack_name)\n>  \t\t\toutput_fd = odb_mkstemp(&tmp_file,\n>  \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n>  \t\t\tpack_name = strbuf_detach(&tmp_file, NULL);\n> +\t\t\ttmp_pack_name = pack_name;\n\n...can just call register_tempfile(). It should also record the result\nso that we don't try to unlink() it after we've already moved it away\nfrom its temporary name (though it's fairly unlikely for somebody else\nto have used the name in the interim).\n\nI think you'd want to do the same for the tmp_idx_* files, too. Likewise\nfor \".rev\" files we create starting in v2.31.\n\nI think it would also make sense in create_tmp_packfile(), which is used\nduring repacking (a different problem space, but really the same thing:\nif repacking fails for some reason, we probably shouldn't leave a\nuseless gigantic half-finished packfile on disk).\n\nWe should possibly also do so for tmp_obj_* files. Those can be written\nfor a fetch or push via unpack-objects (as well as normal local\ncommands). They're not usually as big as a pack, obviously, but I think\nthe same principle applies.\n\n> [...]\n\nIt would be nice to see some tests covering this functionality, too.\nReproducing it with signals is likely to be racy and not worth it. But I\nthink that right now index-pack reading a bogus pack (say, one that\nfails fsck checks) will leave the tmp_pack_* on disk. And it would not\nif we cleanup tempfiles (again, this would be on any exit, not just\nsignal death, but I think that is what we'd want, and also what\nregister_tempfile() will do).\n\n-Peff\n"},{"id":"419498","messageId":"xmqqsg4twst0.fsf@gitster.g","threadId":"55327","inReplyTo":"YFEpGGLBgLSdR40V@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/fetch.c: clean tmp pack after receive signal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-17T18:15:07Z","receivedAt":"2021-03-17T18:16:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 16, 2021 at 02:53:36AM +0000, SURA via GitGitGadget wrote:\n>\n>> In Gitee.com, I often use scripts to start a time-limited\n>\n> Not related to your patch, but I think this name falls afoul of Git's\n> trademark policy. See:\n>\n>   https://git-scm.com/trademark\n>\n> There's also some discussion in this thread:\n>\n>   https://lore.kernel.org/git/20170202022655.2jwvudhvo4hmueaw@sigill.intra.peff.net/\n\nThanks.  On somewhat related to this patch, we also ask contributors\nto use their real names so that we do not render the Signed-off-by:\nprocedure meaningless.\n\n> This isn't quite true. \"git gc\" will clean up the temporary files, but\n> only if the mtime is sufficiently old. The purpose here is to give a\n> grace period to avoid deleting a file that is actively being written to.\n> However, we use the same grace period that we use for deleting\n> unreachable objects, which is absurdly long for this purpose: 2 weeks.\n> Probably something like an hour would be more appropriate (since the\n> mtime is updated on each write, this would imply a process not making\n> forward progress).\n\nI agree that for temporaries the two-week default is way too long,\nand I am OK if we decide to shorten the expiration for them\nseparately from the known-to-be-good-but-unreferenced objects.\n\n> Likewise, we have a tempfile cleanup system already.\n>\n> I think this hunk:\n>\n>> @@ -336,6 +339,7 @@ static const char *open_pack_file(const char *pack_name)\n>>  \t\t\toutput_fd = odb_mkstemp(&tmp_file,\n>>  \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n>>  \t\t\tpack_name = strbuf_detach(&tmp_file, NULL);\n>> +\t\t\ttmp_pack_name = pack_name;\n>\n> ...can just call register_tempfile(). It should also record the result\n> so that we don't try to unlink() it after we've already moved it away\n> from its temporary name (though it's fairly unlikely for somebody else\n> to have used the name in the interim).\n>\n> I think you'd want to do the same for the tmp_idx_* files, too. Likewise\n> for \".rev\" files we create starting in v2.31.\n>\n> I think it would also make sense in create_tmp_packfile(), which is used\n> during repacking (a different problem space, but really the same thing:\n> if repacking fails for some reason, we probably shouldn't leave a\n> useless gigantic half-finished packfile on disk).\n>\n> We should possibly also do so for tmp_obj_* files. Those can be written\n> for a fetch or push via unpack-objects (as well as normal local\n> commands). They're not usually as big as a pack, obviously, but I think\n> the same principle applies.\n>\n>> [...]\n>\n> It would be nice to see some tests covering this functionality, too.\n> Reproducing it with signals is likely to be racy and not worth it. But I\n> think that right now index-pack reading a bogus pack (say, one that\n> fails fsck checks) will leave the tmp_pack_* on disk. And it would not\n> if we cleanup tempfiles (again, this would be on any exit, not just\n> signal death, but I think that is what we'd want, and also what\n> register_tempfile() will do).\n\nSounds like a good medium difficulty leftover bit.\n\nThanks.\n"}]}