{"thread":{"id":"56088","subject":"[PATCH] packfile: enhance the mtime of packfile by idx file","startedAt":"2021-07-10T19:01:32Z","lastAt":"2021-08-15T17:08:55Z","messageCount":34,"participants":["Sun Chao via GitGitGadget","Ævar Arnfjörð Bjarmason","Sun Chao","Taylor Blau","Martin Fick","Junio C Hamano","Son Luong Ngoc"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429637","messageId":"pull.1043.git.git.1625943685565.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":null,"subject":"[PATCH] packfile: enhance the mtime of packfile by idx file","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-10T19:01:25Z","receivedAt":"2021-07-10T19:01:32Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nCommit 33d4221c79 (write_sha1_file: freshen existing objects,\n2014-10-15) avoid writing existing objects by freshen their\nmtime (especially the packfiles contains them) in order to\naid the correct caching, and some process like find_lru_pack\ncan make good decision. However, this is unfriendly to\nincremental backup jobs or services rely on file system\ncache when there are large '.pack' files exists.\n\nFor example, after packed all objects, use 'write-tree' to\ncreate same commit with the same tree and same environments\nsuch like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\nnotice the '.pack' file's mtime changed, but '.idx' file not.\n\nSo if we update the mtime of packfile by updating the '.idx'\nfile instead of '.pack' file, when we check the mtime\nof packfile, get it from '.idx' file instead. Large git\nrepository may contains large '.pack' files, but '.idx'\nfiles are smaller enough, this can avoid file system cache\nreload the large files again and speed up git commands.\n\nSigned-off-by: Sun Chao <16657101987@163.com>\n---\n packfile: enhance the mtime of packfile by idx file\n \n Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n 2014-10-15) avoid writing existing objects by freshen their mtime\n (especially the packfiles contains them) in order to aid the correct\n caching, and some process like find_lru_pack can make good decision.\n However, this is unfriendly to incremental backup jobs or services rely\n on file system cache when there are large '.pack' files exists.\n \n For example, after packed all objects, use 'write-tree' to create same\n commit with the same tree and same environments such like\n GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can notice the '.pack' file's\n mtime changed, but '.idx' file not.\n \n So if we update the mtime of packfile by updating the '.idx' file\n instead of '.pack' file, when we check the mtime of packfile, get it\n from '.idx' file instead. Large git repository may contains large\n '.pack' files, but '.idx' files are smaller enough, this can avoid file\n system cache reload the large files again and speed up git commands.\n \n Signed-off-by: Sun Chao 16657101987@163.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1043%2Fsunchao9%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1043/sunchao9/master-v1\nPull-Request: https://github.com/git/git/pull/1043\n\n builtin/index-pack.c | 19 +++----------------\n object-file.c | 7 ++++++-\n packfile.c | 19 ++++++++++++++++++-\n packfile.h | 7 +++++++\n t/t7701-repack-unpack-unreachable.sh | 2 +-\n 5 files changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3fbc5d70777..60bacc8ee7f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1437,19 +1437,6 @@ static void fix_unresolved_deltas(struct hashfile *f)\n \tfree(sorted_by_pos);\n }\n \n-static const char *derive_filename(const char *pack_name, const char *strip,\n-\t\t\t\t const char *suffix, struct strbuf *buf)\n-{\n-\tsize_t len;\n-\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n-\t pack_name[len - 1] != '.')\n-\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n-\t\t pack_name, strip);\n-\tstrbuf_add(buf, pack_name, len);\n-\tstrbuf_addstr(buf, suffix);\n-\treturn buf->buf;\n-}\n-\n static void write_special_file(const char *suffix, const char *msg,\n \t\t\t const char *pack_name, const unsigned char *hash,\n \t\t\t const char **report)\n@@ -1460,7 +1447,7 @@ static void write_special_file(const char *suffix, const char *msg,\n \tint msg_len = strlen(msg);\n \n \tif (pack_name)\n-\t\tfilename = derive_filename(pack_name, \"pack\", suffix, &name_buf);\n+\t\tfilename = derive_pack_filename(pack_name, \"pack\", suffix, &name_buf);\n \telse\n \t\tfilename = odb_pack_name(&name_buf, hash, suffix);\n \n@@ -1855,13 +1842,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (from_stdin && hash_algo)\n \t\tdie(_(\"--object-format cannot be used with --stdin\"));\n \tif (!index_name && pack_name)\n-\t\tindex_name = derive_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n+\t\tindex_name = derive_pack_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n \n \topts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);\n \tif (rev_index) {\n \t\topts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;\n \t\tif (index_name)\n-\t\t\trev_index_name = derive_filename(index_name,\n+\t\t\trev_index_name = derive_pack_filename(index_name,\n \t\t\t\t\t\t\t \"idx\", \"rev\",\n \t\t\t\t\t\t\t &rev_index_name_buf);\n \t}\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..068ef99d461 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1974,11 +1974,16 @@ static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n+\tconst char *filename;\n+\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n \t\treturn 1;\n-\tif (!freshen_file(e.p->pack_name))\n+\n+\tfilename = derive_pack_filename(e.p->pack_name, \"pack\", \"idx\", &name_buf);\n+\tif (!freshen_file(filename))\n \t\treturn 0;\n \te.p->freshened = 1;\n \treturn 1;\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..46f8fb22462 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -40,6 +40,19 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n \treturn odb_pack_name(&buf, sha1, \"idx\");\n }\n \n+const char *derive_pack_filename(const char *pack_name, const char *strip,\n+\t\t\t\tconst char *suffix, struct strbuf *buf)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n+\t pack_name[len - 1] != '.')\n+\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n+\t\t pack_name, strip);\n+\tstrbuf_add(buf, pack_name, len);\n+\tstrbuf_addstr(buf, suffix);\n+\treturn buf->buf;\n+}\n+\n static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\n@@ -693,6 +706,10 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \tsize_t alloc;\n \tstruct packed_git *p;\n \n+\tif (stat(path, &st) || !S_ISREG(st.st_mode)) {\n+\t\treturn NULL;\n+\t}\n+\n \t/*\n \t * Make sure a corresponding .pack file exists and that\n \t * the index looks sane.\n@@ -707,6 +724,7 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \talloc = st_add3(path_len, strlen(\".promisor\"), 1);\n \tp = alloc_packed_git(alloc);\n \tmemcpy(p->pack_name, path, path_len);\n+\tp->mtime = st.st_mtime;\n \n \txsnprintf(p->pack_name + path_len, alloc - path_len, \".keep\");\n \tif (!access(p->pack_name, F_OK))\n@@ -727,7 +745,6 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \t */\n \tp->pack_size = st.st_size;\n \tp->pack_local = local;\n-\tp->mtime = st.st_mtime;\n \tif (path_len < the_hash_algo->hexsz ||\n \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n \t\thashclr(p->hash);\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8aef..ff702b22e6a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,6 +31,13 @@ char *sha1_pack_name(const unsigned char *sha1);\n */\n char *sha1_pack_index_name(const unsigned char *sha1);\n \n+/*\n+ * Return the corresponding filename with given suffix from \"file_name\"\n+ * which must has \"strip\" suffix.\n+ */\n+const char *derive_pack_filename(const char *file_name, const char *strip,\n+\t\tconst char *suffix, struct strbuf *buf);\n+\n /*\n * Return the basename of the packfile, omitting any containing directory\n * (e.g., \"pack-1234abcd[...].pack\").\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex 937f89ee8c8..51c1afcbe2c 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -106,7 +106,7 @@ test_expect_success 'do not bother loosening old objects' '\n \tgit prune-packed &&\n \tgit cat-file -p $obj1 &&\n \tgit cat-file -p $obj2 &&\n-\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.idx &&\n \tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n \tgit cat-file -p $obj1 &&\n \ttest_must_fail git cat-file -p $obj2\n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\n-- \ngitgitgadget\n"},{"id":"429715","messageId":"874kd04dgo.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"pull.1043.git.git.1625943685565.gitgitgadget@gmail.com","subject":"Re: [PATCH] packfile: enhance the mtime of packfile by idx file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-11T23:44:15Z","receivedAt":"2021-07-11T23:50:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 10 2021, Sun Chao via GitGitGadget wrote:\n\n> From: Sun Chao <16657101987@163.com>\n>\n> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n> 2014-10-15) avoid writing existing objects by freshen their\n> mtime (especially the packfiles contains them) in order to\n> aid the correct caching, and some process like find_lru_pack\n> can make good decision. However, this is unfriendly to\n> incremental backup jobs or services rely on file system\n> cache when there are large '.pack' files exists.\n>\n> For example, after packed all objects, use 'write-tree' to\n> create same commit with the same tree and same environments\n> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n> notice the '.pack' file's mtime changed, but '.idx' file not.\n>\n> So if we update the mtime of packfile by updating the '.idx'\n> file instead of '.pack' file, when we check the mtime\n> of packfile, get it from '.idx' file instead. Large git\n> repository may contains large '.pack' files, but '.idx'\n> files are smaller enough, this can avoid file system cache\n> reload the large files again and speed up git commands.\n>\n> Signed-off-by: Sun Chao <16657101987@163.com>\n\nDoes this have the unstated trade-off that in a mixed-version\nenvironment (say two git versions coordinating writes to an NFS share)\nwhere one is old and thinks *.pack needs updating, and the other is new\nand thinks *.idx is what should be checked, that until both are upgraded\nwe're effectively back to pre-33d4221c79.\n\nI don't think it's a dealbreaker, just wondering if I've got that right\n& if it is's a trade-off you thought about, maybe we should check the\nmtime of both. The stat() is cheap, it's the re-sync that matters for\nyou.\n\nBut just to run with that thought, wouldn't it be even more helpful to\nyou to have say a config setting to create a *.bump file next to the\n*.{idx,pack}.\n\nThen you'd have an empty file (the *.idx is smaller, but still not\nempty), and as a patch it seems relatively simple, i.e. some core.* or\ngc.* or pack.* setting changing what we touch/stat().\n"},{"id":"429760","messageId":"9FCDF81A-F466-44F2-8B70-543748E91CB9@163.com","threadId":"56088","inReplyTo":"874kd04dgo.fsf@evledraar.gmail.com","subject":"Re: [PATCH] packfile: enhance the mtime of packfile by idx file","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-12T16:17:15Z","receivedAt":"2021-07-12T16:17:42Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月12日 07:44，Ævar Arnfjörð Bjarmason 写道：\n> \n> \n> On Sat, Jul 10 2021, Sun Chao via GitGitGadget wrote:\n> \n>> From: Sun Chao <16657101987@163.com>\n>> \n>> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n>> 2014-10-15) avoid writing existing objects by freshen their\n>> mtime (especially the packfiles contains them) in order to\n>> aid the correct caching, and some process like find_lru_pack\n>> can make good decision. However, this is unfriendly to\n>> incremental backup jobs or services rely on file system\n>> cache when there are large '.pack' files exists.\n>> \n>> For example, after packed all objects, use 'write-tree' to\n>> create same commit with the same tree and same environments\n>> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n>> notice the '.pack' file's mtime changed, but '.idx' file not.\n>> \n>> So if we update the mtime of packfile by updating the '.idx'\n>> file instead of '.pack' file, when we check the mtime\n>> of packfile, get it from '.idx' file instead. Large git\n>> repository may contains large '.pack' files, but '.idx'\n>> files are smaller enough, this can avoid file system cache\n>> reload the large files again and speed up git commands.\n>> \n>> Signed-off-by: Sun Chao <16657101987@163.com>\n> \n> Does this have the unstated trade-off that in a mixed-version\n> environment (say two git versions coordinating writes to an NFS share)\n> where one is old and thinks *.pack needs updating, and the other is new\n> and thinks *.idx is what should be checked, that until both are upgraded\n> we're effectively back to pre-33d4221c79.\n> \nThanks for your reply, I can not agree with you more.\n\n> I don't think it's a dealbreaker, just wondering if I've got that right\n> & if it is's a trade-off you thought about, maybe we should check the\n> mtime of both. The stat() is cheap, it's the re-sync that matters for\n> you.\n> \n> But just to run with that thought, wouldn't it be even more helpful to\n> you to have say a config setting to create a *.bump file next to the\n> *.{idx,pack}.\n> \n> Then you'd have an empty file (the *.idx is smaller, but still not\n> empty), and as a patch it seems relatively simple, i.e. some core.* or\n> gc.* or pack.* setting changing what we touch/stat().\n\nYes, thanks. This is a good idea, let me try this way.\n\n\n"},{"id":"429989","messageId":"pull.1043.v2.git.git.1626226114067.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":"pull.1043.git.git.1625943685565.gitgitgadget@gmail.com","subject":"[PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-14T01:28:33Z","receivedAt":"2021-07-14T01:28:39Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nCommit 33d4221c79 (write_sha1_file: freshen existing objects,\n2014-10-15) avoid writing existing objects by freshen their\nmtime (especially the packfiles contains them) in order to\naid the correct caching, and some process like find_lru_pack\ncan make good decision. However, this is unfriendly to\nincremental backup jobs or services rely on file system\ncache when there are large '.pack' files exists.\n\nFor example, after packed all objects, use 'write-tree' to\ncreate same commit with the same tree and same environments\nsuch like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\nnotice the '.pack' file's mtime changed, and '.idx' file not.\n\nIf we freshen the mtime of packfile by updating another\nfile instead of '.pack' file e.g. a empty '.bump' file,\nwhen we need to check the mtime of packfile, get it from\nanother file instead. Large git repository may contains\nlarge '.pack' files, and we can use smaller files even empty\nfile to do the mtime get/set operation, this can avoid\nfile system cache re-sync large '.pack' files again and\nthen speed up most git commands.\n\nSigned-off-by: Sun Chao <16657101987@163.com>\n---\n packfile: freshen the mtime of packfile by configuration\n \n Commit 33d4221 (write_sha1_file: freshen existing objects, 2014-10-15)\n avoid writing existing objects by freshen their mtime (especially the\n packfiles contains them) in order to aid the correct caching, and some\n process like find_lru_pack can make good decision. However, this is\n unfriendly to incremental backup jobs or services rely on file system\n cache when there are large '.pack' files exists.\n \n For example, after packed all objects, use 'write-tree' to create same\n commit with the same tree and same environments such like\n GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can notice the '.pack' file's\n mtime changed, and '.idx' file not.\n \n If we freshen the mtime of packfile by updating another file instead of\n '.pack' file e.g. a empty '.bump' file, when we need to check the mtime\n of packfile, get it from another file instead. Large git repository may\n contains large '.pack' files, and we can use smaller files even empty\n file to do the mtime get/set operation, this can avoid file system cache\n re-sync large '.pack' files again and then speed up most git commands.\n \n Signed-off-by: Sun Chao 16657101987@163.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1043%2Fsunchao9%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1043/sunchao9/master-v2\nPull-Request: https://github.com/git/git/pull/1043\n\nRange-diff vs v1:\n\n 1: f6adbb4e90f ! 1: 943e31e8587 packfile: enhance the mtime of packfile by idx file\n @@ Metadata\n Author: Sun Chao <16657101987@163.com>\n \n ## Commit message ##\n - packfile: enhance the mtime of packfile by idx file\n + packfile: freshen the mtime of packfile by configuration\n \n Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n 2014-10-15) avoid writing existing objects by freshen their\n @@ Commit message\n For example, after packed all objects, use 'write-tree' to\n create same commit with the same tree and same environments\n such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n - notice the '.pack' file's mtime changed, but '.idx' file not.\n + notice the '.pack' file's mtime changed, and '.idx' file not.\n \n - So if we update the mtime of packfile by updating the '.idx'\n - file instead of '.pack' file, when we check the mtime\n - of packfile, get it from '.idx' file instead. Large git\n - repository may contains large '.pack' files, but '.idx'\n - files are smaller enough, this can avoid file system cache\n - reload the large files again and speed up git commands.\n + If we freshen the mtime of packfile by updating another\n + file instead of '.pack' file e.g. a empty '.bump' file,\n + when we need to check the mtime of packfile, get it from\n + another file instead. Large git repository may contains\n + large '.pack' files, and we can use smaller files even empty\n + file to do the mtime get/set operation, this can avoid\n + file system cache re-sync large '.pack' files again and\n + then speed up most git commands.\n \n Signed-off-by: Sun Chao <16657101987@163.com>\n \n + ## Documentation/config/core.txt ##\n +@@ Documentation/config/core.txt: the largest projects. You probably do not need to adjust this value.\n + +\n + Common unit suffixes of 'k', 'm', or 'g' are supported.\n + \n ++core.packMtimeSuffix::\n ++\tNormally we avoid writing existing object by freshening the mtime\n ++\tof the *.pack file which contains it in order to aid some processes\n ++\tsuch like prune. Use different file instead of *.pack file will\n ++\tavoid file system cache re-sync the large packfiles, and consequently\n ++\tmake git commands faster.\n +++\n ++The default is 'pack' which means the *.pack file will be freshened by\n ++default. You can configure a different suffix to use, the file with the\n ++suffix will be created automatically, it's better not using any known\n ++suffix such like 'idx', 'keep', 'promisor'.\n ++\n + core.deltaBaseCacheLimit::\n + \tMaximum number of bytes per thread to reserve for caching base objects\n + \tthat may be referenced by multiple deltified objects. By storing the\n +\n ## builtin/index-pack.c ##\n @@ builtin/index-pack.c: static void fix_unresolved_deltas(struct hashfile *f)\n \tfree(sorted_by_pos);\n @@ builtin/index-pack.c: int cmd_index_pack(int argc, const char **argv, const char\n \t\t\t\t\t\t\t &rev_index_name_buf);\n \t}\n \n + ## cache.h ##\n +@@ cache.h: extern size_t packed_git_limit;\n + extern size_t delta_base_cache_limit;\n + extern unsigned long big_file_threshold;\n + extern unsigned long pack_size_limit_cfg;\n ++extern const char *pack_mtime_suffix;\n + \n + /*\n + * Accessors for the core.sharedrepository config which lazy-load the value\n +\n + ## config.c ##\n +@@ config.c: static int git_default_core_config(const char *var, const char *value, void *cb)\n + \t\treturn 0;\n + \t}\n + \n ++\tif (!strcmp(var, \"core.packmtimesuffix\")) {\n ++\t\treturn git_config_string(&pack_mtime_suffix, var, value);\n ++\t}\n ++\n + \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n + \t\tdelta_base_cache_limit = git_config_ulong(var, value);\n + \t\treturn 0;\n +\n + ## environment.c ##\n +@@ environment.c: const char *git_hooks_path;\n + int zlib_compression_level = Z_BEST_SPEED;\n + int core_compression_level;\n + int pack_compression_level = Z_DEFAULT_COMPRESSION;\n ++const char *pack_mtime_suffix = \"pack\";\n + int fsync_object_files;\n + size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n + size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n +\n ## object-file.c ##\n @@ object-file.c: static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n ++\tstruct stat st;\n +\tstruct strbuf name_buf = STRBUF_INIT;\n +\tconst char *filename;\n +\n @@ object-file.c: static int freshen_loose_object(const struct object_id *oid)\n \tif (e.p->freshened)\n \t\treturn 1;\n -\tif (!freshen_file(e.p->pack_name))\n +-\t\treturn 0;\n ++\n ++\tfilename = e.p->pack_name;\n ++\tif (!strcasecmp(pack_mtime_suffix, \"pack\")) {\n ++\t\tif (!freshen_file(filename))\n ++\t\t\treturn 0;\n ++\t\te.p->freshened = 1;\n ++\t\treturn 1;\n ++\t}\n ++\n ++\t/* If we want to freshen different file instead of .pack file, we need\n ++\t * to make sure the file exists and create it if needed.\n ++\t */\n ++\tfilename = derive_pack_filename(filename, \"pack\", pack_mtime_suffix, &name_buf);\n ++\tif (lstat(filename, &st) < 0) {\n ++\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n ++\t\tif (fd < 0) {\n ++\t\t\t// here we need to check it again because other git process may created it\n ++\t\t\tif (lstat(filename, &st) < 0)\n ++\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n ++\t\t} else {\n ++\t\t\tclose(fd);\n ++\t\t}\n ++\t} else {\n ++\t\tif (!freshen_file(filename))\n ++\t\t\treturn 0;\n ++\t}\n +\n -+\tfilename = derive_pack_filename(e.p->pack_name, \"pack\", \"idx\", &name_buf);\n -+\tif (!freshen_file(filename))\n - \t\treturn 0;\n \te.p->freshened = 1;\n \treturn 1;\n + }\n \n ## packfile.c ##\n @@ packfile.c: char *sha1_pack_index_name(const unsigned char *sha1)\n @@ packfile.c: char *sha1_pack_index_name(const unsigned char *sha1)\n static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\n -@@ packfile.c: struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n - \tsize_t alloc;\n - \tstruct packed_git *p;\n - \n -+\tif (stat(path, &st) || !S_ISREG(st.st_mode)) {\n -+\t\treturn NULL;\n -+\t}\n -+\n - \t/*\n - \t * Make sure a corresponding .pack file exists and that\n - \t * the index looks sane.\n -@@ packfile.c: struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n - \talloc = st_add3(path_len, strlen(\".promisor\"), 1);\n - \tp = alloc_packed_git(alloc);\n - \tmemcpy(p->pack_name, path, path_len);\n -+\tp->mtime = st.st_mtime;\n - \n - \txsnprintf(p->pack_name + path_len, alloc - path_len, \".keep\");\n - \tif (!access(p->pack_name, F_OK))\n @@ packfile.c: struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \t */\n \tp->pack_size = st.st_size;\n \tp->pack_local = local;\n --\tp->mtime = st.st_mtime;\n ++\n ++\t/* If we have different file used to freshen the mtime, we should\n ++\t * use it at a higher priority.\n ++\t */\n ++\tif (!!strcasecmp(pack_mtime_suffix, \"pack\")) {\n ++\t\tstruct strbuf name_buf = STRBUF_INIT;\n ++\t\tconst char *filename;\n ++\n ++\t\tfilename = derive_pack_filename(path, \"idx\", pack_mtime_suffix, &name_buf);\n ++\t\tstat(filename, &st);\n ++\t}\n + \tp->mtime = st.st_mtime;\n \tif (path_len < the_hash_algo->hexsz ||\n \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n - \t\thashclr(p->hash);\n \n ## packfile.h ##\n @@ packfile.h: char *sha1_pack_name(const unsigned char *sha1);\n @@ packfile.h: char *sha1_pack_name(const unsigned char *sha1);\n \n ## t/t7701-repack-unpack-unreachable.sh ##\n @@ t/t7701-repack-unpack-unreachable.sh: test_expect_success 'do not bother loosening old objects' '\n - \tgit prune-packed &&\n - \tgit cat-file -p $obj1 &&\n - \tgit cat-file -p $obj2 &&\n --\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n -+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.idx &&\n - \tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n - \tgit cat-file -p $obj1 &&\n \ttest_must_fail git cat-file -p $obj2\n + '\n + \n ++test_expect_success 'do not bother loosening old objects with core.packmtimesuffix config' '\n ++\tobj1=$(echo three | git hash-object -w --stdin) &&\n ++\tobj2=$(echo four | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n ++\tgit -c core.packmtimesuffix=bump prune-packed &&\n ++\tgit cat-file -p $obj1 &&\n ++\tgit cat-file -p $obj2 &&\n ++\ttouch .git/objects/pack/pack-$pack2.bump &&\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n ++\tgit -c core.packmtimesuffix=bump repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\tgit cat-file -p $obj1 &&\n ++\ttest_must_fail git cat-file -p $obj2\n ++'\n ++\n + test_expect_success 'keep packed objects found only in index' '\n + \techo my-unique-content >file &&\n + \tgit add file &&\n\n\n Documentation/config/core.txt | 12 ++++++++++\n builtin/index-pack.c | 19 +++-------------\n cache.h | 1 +\n config.c | 4 ++++\n environment.c | 1 +\n object-file.c | 33 ++++++++++++++++++++++++++--\n packfile.c | 24 ++++++++++++++++++++\n packfile.h | 7 ++++++\n t/t7701-repack-unpack-unreachable.sh | 15 +++++++++++++\n 9 files changed, 98 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex c04f62a54a1..9a256992d8c 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -398,6 +398,18 @@ the largest projects. You probably do not need to adjust this value.\n +\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \n+core.packMtimeSuffix::\n+\tNormally we avoid writing existing object by freshening the mtime\n+\tof the *.pack file which contains it in order to aid some processes\n+\tsuch like prune. Use different file instead of *.pack file will\n+\tavoid file system cache re-sync the large packfiles, and consequently\n+\tmake git commands faster.\n++\n+The default is 'pack' which means the *.pack file will be freshened by\n+default. You can configure a different suffix to use, the file with the\n+suffix will be created automatically, it's better not using any known\n+suffix such like 'idx', 'keep', 'promisor'.\n+\n core.deltaBaseCacheLimit::\n \tMaximum number of bytes per thread to reserve for caching base objects\n \tthat may be referenced by multiple deltified objects. By storing the\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3fbc5d70777..60bacc8ee7f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1437,19 +1437,6 @@ static void fix_unresolved_deltas(struct hashfile *f)\n \tfree(sorted_by_pos);\n }\n \n-static const char *derive_filename(const char *pack_name, const char *strip,\n-\t\t\t\t const char *suffix, struct strbuf *buf)\n-{\n-\tsize_t len;\n-\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n-\t pack_name[len - 1] != '.')\n-\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n-\t\t pack_name, strip);\n-\tstrbuf_add(buf, pack_name, len);\n-\tstrbuf_addstr(buf, suffix);\n-\treturn buf->buf;\n-}\n-\n static void write_special_file(const char *suffix, const char *msg,\n \t\t\t const char *pack_name, const unsigned char *hash,\n \t\t\t const char **report)\n@@ -1460,7 +1447,7 @@ static void write_special_file(const char *suffix, const char *msg,\n \tint msg_len = strlen(msg);\n \n \tif (pack_name)\n-\t\tfilename = derive_filename(pack_name, \"pack\", suffix, &name_buf);\n+\t\tfilename = derive_pack_filename(pack_name, \"pack\", suffix, &name_buf);\n \telse\n \t\tfilename = odb_pack_name(&name_buf, hash, suffix);\n \n@@ -1855,13 +1842,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (from_stdin && hash_algo)\n \t\tdie(_(\"--object-format cannot be used with --stdin\"));\n \tif (!index_name && pack_name)\n-\t\tindex_name = derive_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n+\t\tindex_name = derive_pack_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n \n \topts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);\n \tif (rev_index) {\n \t\topts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;\n \t\tif (index_name)\n-\t\t\trev_index_name = derive_filename(index_name,\n+\t\t\trev_index_name = derive_pack_filename(index_name,\n \t\t\t\t\t\t\t \"idx\", \"rev\",\n \t\t\t\t\t\t\t &rev_index_name_buf);\n \t}\ndiff --git a/cache.h b/cache.h\nindex ba04ff8bd36..d95b3d127e0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -956,6 +956,7 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern unsigned long big_file_threshold;\n extern unsigned long pack_size_limit_cfg;\n+extern const char *pack_mtime_suffix;\n \n /*\n * Accessors for the core.sharedrepository config which lazy-load the value\ndiff --git a/config.c b/config.c\nindex f9c400ad306..27a6b619ed8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1431,6 +1431,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.packmtimesuffix\")) {\n+\t\treturn git_config_string(&pack_mtime_suffix, var, value);\n+\t}\n+\n \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n \t\tdelta_base_cache_limit = git_config_ulong(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 2f27008424a..07c21e1a934 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -43,6 +43,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n+const char *pack_mtime_suffix = \"pack\";\n int fsync_object_files;\n size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..b3e77213c42 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1974,12 +1974,41 @@ static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n+\tstruct stat st;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n+\tconst char *filename;\n+\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n \t\treturn 1;\n-\tif (!freshen_file(e.p->pack_name))\n-\t\treturn 0;\n+\n+\tfilename = e.p->pack_name;\n+\tif (!strcasecmp(pack_mtime_suffix, \"pack\")) {\n+\t\tif (!freshen_file(filename))\n+\t\t\treturn 0;\n+\t\te.p->freshened = 1;\n+\t\treturn 1;\n+\t}\n+\n+\t/* If we want to freshen different file instead of .pack file, we need\n+\t * to make sure the file exists and create it if needed.\n+\t */\n+\tfilename = derive_pack_filename(filename, \"pack\", pack_mtime_suffix, &name_buf);\n+\tif (lstat(filename, &st) < 0) {\n+\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n+\t\tif (fd < 0) {\n+\t\t\t// here we need to check it again because other git process may created it\n+\t\t\tif (lstat(filename, &st) < 0)\n+\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n+\t\t} else {\n+\t\t\tclose(fd);\n+\t\t}\n+\t} else {\n+\t\tif (!freshen_file(filename))\n+\t\t\treturn 0;\n+\t}\n+\n \te.p->freshened = 1;\n \treturn 1;\n }\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..a607dda4e25 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -40,6 +40,19 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n \treturn odb_pack_name(&buf, sha1, \"idx\");\n }\n \n+const char *derive_pack_filename(const char *pack_name, const char *strip,\n+\t\t\t\tconst char *suffix, struct strbuf *buf)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n+\t pack_name[len - 1] != '.')\n+\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n+\t\t pack_name, strip);\n+\tstrbuf_add(buf, pack_name, len);\n+\tstrbuf_addstr(buf, suffix);\n+\treturn buf->buf;\n+}\n+\n static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\n@@ -727,6 +740,17 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \t */\n \tp->pack_size = st.st_size;\n \tp->pack_local = local;\n+\n+\t/* If we have different file used to freshen the mtime, we should\n+\t * use it at a higher priority.\n+\t */\n+\tif (!!strcasecmp(pack_mtime_suffix, \"pack\")) {\n+\t\tstruct strbuf name_buf = STRBUF_INIT;\n+\t\tconst char *filename;\n+\n+\t\tfilename = derive_pack_filename(path, \"idx\", pack_mtime_suffix, &name_buf);\n+\t\tstat(filename, &st);\n+\t}\n \tp->mtime = st.st_mtime;\n \tif (path_len < the_hash_algo->hexsz ||\n \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8aef..ff702b22e6a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,6 +31,13 @@ char *sha1_pack_name(const unsigned char *sha1);\n */\n char *sha1_pack_index_name(const unsigned char *sha1);\n \n+/*\n+ * Return the corresponding filename with given suffix from \"file_name\"\n+ * which must has \"strip\" suffix.\n+ */\n+const char *derive_pack_filename(const char *file_name, const char *strip,\n+\t\tconst char *suffix, struct strbuf *buf);\n+\n /*\n * Return the basename of the packfile, omitting any containing directory\n * (e.g., \"pack-1234abcd[...].pack\").\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex 937f89ee8c8..15828a318c4 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -112,6 +112,21 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'do not bother loosening old objects with core.packmtimesuffix config' '\n+\tobj1=$(echo three | git hash-object -w --stdin) &&\n+\tobj2=$(echo four | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n+\tgit -c core.packmtimesuffix=bump prune-packed &&\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\ttouch .git/objects/pack/pack-$pack2.bump &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n+\tgit -c core.packmtimesuffix=bump repack -A -d --unpack-unreachable=1.hour.ago &&\n+\tgit cat-file -p $obj1 &&\n+\ttest_must_fail git cat-file -p $obj2\n+'\n+\n test_expect_success 'keep packed objects found only in index' '\n \techo my-unique-content >file &&\n \tgit add file &&\n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\n-- \ngitgitgadget\n"},{"id":"429990","messageId":"87wnpt1wwc.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"pull.1043.v2.git.git.1626226114067.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T01:39:18Z","receivedAt":"2021-07-14T01:56:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Sun Chao via GitGitGadget wrote:\n\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index c04f62a54a1..9a256992d8c 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -398,6 +398,18 @@ the largest projects. You probably do not need to adjust this value.\n> +\n> Common unit suffixes of 'k', 'm', or 'g' are supported.\n> \n> +core.packMtimeSuffix::\n> +\tNormally we avoid writing existing object by freshening the mtime\n> +\tof the *.pack file which contains it in order to aid some processes\n> +\tsuch like prune. Use different file instead of *.pack file will\n> +\tavoid file system cache re-sync the large packfiles, and consequently\n> +\tmake git commands faster.\n> ++\n> +The default is 'pack' which means the *.pack file will be freshened by\n> +default. You can configure a different suffix to use, the file with the\n> +suffix will be created automatically, it's better not using any known\n> +suffix such like 'idx', 'keep', 'promisor'.\n> +\n\nHrm, per my v1 feedback (and I'm not sure if my suggestion is even good\nhere, there's others more familiar with this area than I am), I was\nthinking of something like a *.bump file written via:\n\n core.packUseBumpFiles=bool\n\nOr something like that, anyway, the edge case in allowing the user to\npick arbitrary suffixes is that we'd get in-the-wild user arbitrary\nconfiguration squatting on a relatively sensitive part of the object\nstore.\n\nE.g. we recently added *.rev files to go with\n*.{pack,idx,bitmap,keep,promisor} (and I'm probably forgetting some\nsuffix). What if before that a user had set:\n\n core.packMtimeSuffix=rev\n\nIn practice it's probably too obscure to worry about, but I think it's\nstill worth it to only strictly write things we decide to write into the\nobject store.\n> core.deltaBaseCacheLimit::\n> \tMaximum number of bytes per thread to reserve for caching base objects\n> \tthat may be referenced by multiple deltified objects. By storing the\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 3fbc5d70777..60bacc8ee7f 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1437,19 +1437,6 @@ static void fix_unresolved_deltas(struct hashfile *f)\n> \tfree(sorted_by_pos);\n> }\n> \n> -static const char *derive_filename(const char *pack_name, const char *strip,\n> -\t\t\t\t const char *suffix, struct strbuf *buf)\n> -{\n> -\tsize_t len;\n> -\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n> -\t pack_name[len - 1] != '.')\n> -\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n> -\t\t pack_name, strip);\n> -\tstrbuf_add(buf, pack_name, len);\n> -\tstrbuf_addstr(buf, suffix);\n> -\treturn buf->buf;\n> -}\n\n\nWould be more readable to split this series up into at least two\npatches, starting with just moving this function as-is to a the new\nlocation (just renaming it & moving it), and then using it. I don't\nthink there's changes to it, but right now I'm just eyeballing the\ndiff. It's more obvious if it's split up.\n\n> +\tif (!strcmp(var, \"core.packmtimesuffix\")) {\n> +\t\treturn git_config_string(&pack_mtime_suffix, var, value);\n\nCan drop the {} braces here, per Documentation/CodingGuidelines\n\n> +const char *pack_mtime_suffix = \"pack\";\n\nI can see how having a configurable suffix made the implementation\neasier, perhaps that's how it started?\n\n\n> int fsync_object_files;\n> size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n> size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n> diff --git a/object-file.c b/object-file.c\n> index f233b440b22..b3e77213c42 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1974,12 +1974,41 @@ static int freshen_loose_object(const struct object_id *oid)\n> static int freshen_packed_object(const struct object_id *oid)\n> {\n> \tstruct pack_entry e;\n> +\tstruct stat st;\n> +\tstruct strbuf name_buf = STRBUF_INIT;\n> +\tconst char *filename;\n> +\n> \tif (!find_pack_entry(the_repository, oid, &e))\n> \t\treturn 0;\n> \tif (e.p->freshened)\n> \t\treturn 1;\n> -\tif (!freshen_file(e.p->pack_name))\n> -\t\treturn 0;\n> +\n> +\tfilename = e.p->pack_name;\n> +\tif (!strcasecmp(pack_mtime_suffix, \"pack\")) {\n> +\t\tif (!freshen_file(filename))\n> +\t\t\treturn 0;\n> +\t\te.p->freshened = 1;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\t/* If we want to freshen different file instead of .pack file, we need\n> +\t * to make sure the file exists and create it if needed.\n> +\t */\n> +\tfilename = derive_pack_filename(filename, \"pack\", pack_mtime_suffix, &name_buf);\n\nYou populate name_buf here, but don't strbuf_release(&name_buf) it at the end of this function.\n\n> +\tif (lstat(filename, &st) < 0) {\n> +\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n> +\t\tif (fd < 0) {\n> +\t\t\t// here we need to check it again because other git process may created it\n\n/* */ comments, not //, if it's needed at all. Covered in CodingGuidelines\n\n> +\t\t\tif (lstat(filename, &st) < 0)\n> +\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n\nIf we can't create this specific file here shouldn't we just continue\nsilently at this point? Surely if this process is screwed we're just\nabout to die on something more important?\n\nAnd lstat() can also return transitory errors that don't indicate\n\"unable to create\", e.g. maybe we can out of memory the kernel is\nwilling to give us or something (just skimming the lstat manpage).\n\n> +\t\t} else {\n> +\t\t\tclose(fd);\n> +\t\t}\n> +\t} else {\n> +\t\tif (!freshen_file(filename))\n> +\t\t\treturn 0;\n\nStyle/indentatino: just do \"} else if (!freshen...\" ?\n\n> +\t}\n> +\n> \te.p->freshened = 1;\n> \treturn 1;\n> }\n> diff --git a/packfile.c b/packfile.c\n> index 755aa7aec5e..a607dda4e25 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -40,6 +40,19 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n> \treturn odb_pack_name(&buf, sha1, \"idx\");\n> }\n> \n> +const char *derive_pack_filename(const char *pack_name, const char *strip,\n> +\t\t\t\tconst char *suffix, struct strbuf *buf)\n> +{\n> +\tsize_t len;\n> +\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n> +\t pack_name[len - 1] != '.')\n> +\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n> +\t\t pack_name, strip);\n> +\tstrbuf_add(buf, pack_name, len);\n> +\tstrbuf_addstr(buf, suffix);\n> +\treturn buf->buf;\n> +}\n\nJust have this return void?\n\n> static unsigned int pack_used_ctr;\n> static unsigned int pack_mmap_calls;\n> static unsigned int peak_pack_open_windows;\n> @@ -727,6 +740,17 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n> \t */\n> \tp->pack_size = st.st_size;\n> \tp->pack_local = local;\n> +\n> +\t/* If we have different file used to freshen the mtime, we should\n> +\t * use it at a higher priority.\n> +\t */\n> +\tif (!!strcasecmp(pack_mtime_suffix, \"pack\")) {\n> +\t\tstruct strbuf name_buf = STRBUF_INIT;\n> +\t\tconst char *filename;\n> +\n> +\t\tfilename = derive_pack_filename(path, \"idx\", pack_mtime_suffix, &name_buf);\n> +\t\tstat(filename, &st);\n\nI.e. the \"filename\" here isn't needed, just call derive_pack_filename()\nand use name.buf.buf to stat.\n\nAlso: We should check the stat return value here & report errno if\nneeded, no?\n\n> +test_expect_success 'do not bother loosening old objects with core.packmtimesuffix config' '\n> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n> +\tpack1=$(echo $obj1 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n> +\tpack2=$(echo $obj2 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n> +\tgit -c core.packmtimesuffix=bump prune-packed &&\n> +\tgit cat-file -p $obj1 &&\n> +\tgit cat-file -p $obj2 &&\n> +\ttouch .git/objects/pack/pack-$pack2.bump &&\n> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n> +\tgit -c core.packmtimesuffix=bump repack -A -d --unpack-unreachable=1.hour.ago &&\n\nOn command-lines we can spell it camel-cased, e.g. -c\ncore.packMtimeSuffix[...].\n"},{"id":"429993","messageId":"YO5RZ0Wix/K5q53Z@nand.local","threadId":"56088","inReplyTo":"87wnpt1wwc.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-14T02:52:23Z","receivedAt":"2021-07-14T02:52:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 14, 2021 at 03:39:18AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Hrm, per my v1 feedback (and I'm not sure if my suggestion is even good\n> here, there's others more familiar with this area than I am), I was\n> thinking of something like a *.bump file written via:\n>\n> core.packUseBumpFiles=bool\n>\n> Or something like that, anyway, the edge case in allowing the user to\n> pick arbitrary suffixes is that we'd get in-the-wild user arbitrary\n> configuration squatting on a relatively sensitive part of the object\n> store.\n>\n> E.g. we recently added *.rev files to go with\n> *.{pack,idx,bitmap,keep,promisor} (and I'm probably forgetting some\n> suffix). What if before that a user had set:\n>\n> core.packMtimeSuffix=rev\n\nI think making the suffix configurable is probably a mistake. It seems\nlike an unnecessary detail to expose, but it also forces us to think\nabout cases like these where the configured suffix is already used for\nsome other purpose.\n\nI don't think that a new \".bump\" file is a bad idea, but it does seem\nlike we have a lot of files that represent a relatively little amount of\nthe state that a pack can be in. The \".promisor\" and \".keep\" files both\ncome to mind here. Some thoughts in this direction:\n\n - Combining *all* of the pack-related files (including the index,\n reverse-index, bitmap, and so on) into a single \"pack-meta\" file\n seems like a mistake for caching reasons.\n\n - But a meta file that contains just the small state (like promisor\n information and whether or not the pack is \"kept\") seems like it\n could be OK. On the other hand, being able to tweak the kept state\n by touching or deleting a file is convenient (and having to rewrite\n a meta file containing other information is much less so).\n\nBut a \".bump\" file does seem like an awkward way to not rely on the\nmtime of the pack itself. And I do think it runs into compatibility\nissues like Ævar mentioned. Any version of Git that includes a\nhypothetical .bump file (or something like it) needs to also update the\npack's mtime, too, so that old versions of Git can understand it. (Of\ncourse, that could be configurable, but that seems far too obscure to\nme).\n\nStepping back, I'm not sure I understand why freshening a pack is so\nslow for you. freshen_file() just calls utime(2), and any sync back to\nthe disk shouldn't need to update the pack itself, just a couple of\nfields in its inode. Maybe you could help explain further.\n\nIn any case, I couldn't find a spot in your patch that updates the\npacked_git's 'mtime' field, which is used to (a) sort packs in the\nlinked list of packs, and (b) for determining the least-recently used\npack if it has individual windows mmapped.\n\nThanks,\nTaylor\n"},{"id":"430065","messageId":"D87972E0-BF82-402B-9531-81B50531438C@163.com","threadId":"56088","inReplyTo":"87wnpt1wwc.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-14T16:11:42Z","receivedAt":"2021-07-14T16:12:59Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月14日 09:39，Ævar Arnfjörð Bjarmason 写道：\n> \n>> +The default is 'pack' which means the *.pack file will be freshened by\n>> +default. You can configure a different suffix to use, the file with the\n>> +suffix will be created automatically, it's better not using any known\n>> +suffix such like 'idx', 'keep', 'promisor'.\n>> +\n> \n> Hrm, per my v1 feedback (and I'm not sure if my suggestion is even good\n> here, there's others more familiar with this area than I am), I was\n> thinking of something like a *.bump file written via:\n> \n> core.packUseBumpFiles=bool\n> \n> Or something like that, anyway, the edge case in allowing the user to\n> pick arbitrary suffixes is that we'd get in-the-wild user arbitrary\n> configuration squatting on a relatively sensitive part of the object\n> store.\n> \n> E.g. we recently added *.rev files to go with\n> *.{pack,idx,bitmap,keep,promisor} (and I'm probably forgetting some\n> suffix). What if before that a user had set:\n> \n> core.packMtimeSuffix=rev\n> \n> In practice it's probably too obscure to worry about, but I think it's\n> still worth it to only strictly write things we decide to write into the\n> object store.\n\nThanks, this makes sense, allowing user to pick arbitrary suffixes may cause\nsome unpredictable problems, E.g. users who don’t know about the *.keep files\nbut setting core.packMtimeSuffix=keep, so I think it’s right to restrict the\nsuffix here.\n\n>> core.deltaBaseCacheLimit::\n>> \tMaximum number of bytes per thread to reserve for caching base objects\n>> \tthat may be referenced by multiple deltified objects. By storing the\n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n>> index 3fbc5d70777..60bacc8ee7f 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -1437,19 +1437,6 @@ static void fix_unresolved_deltas(struct hashfile *f)\n>> \tfree(sorted_by_pos);\n>> }\n>> \n>> -static const char *derive_filename(const char *pack_name, const char *strip,\n>> -\t\t\t\t const char *suffix, struct strbuf *buf)\n>> -{\n>> -\tsize_t len;\n>> -\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n>> -\t pack_name[len - 1] != '.')\n>> -\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n>> -\t\t pack_name, strip);\n>> -\tstrbuf_add(buf, pack_name, len);\n>> -\tstrbuf_addstr(buf, suffix);\n>> -\treturn buf->buf;\n>> -}\n> \n> \n> Would be more readable to split this series up into at least two\n> patches, starting with just moving this function as-is to a the new\n> location (just renaming it & moving it), and then using it. I don't\n> think there's changes to it, but right now I'm just eyeballing the\n> diff. It's more obvious if it's split up.\n\nThanks, I will do it.\n\n> \n>> +\tif (!strcmp(var, \"core.packmtimesuffix\")) {\n>> +\t\treturn git_config_string(&pack_mtime_suffix, var, value);\n> \n> Can drop the {} braces here, per Documentation/CodingGuidelines\n\nThanks, I will read the CodingGuidelines again and fix the issue.\n\n> \n>> +const char *pack_mtime_suffix = \"pack\";\n> \n> I can see how having a configurable suffix made the implementation\n> easier, perhaps that's how it started?\n> \n\nyes, here is the default value.\n\n> \n>> int fsync_object_files;\n>> size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n>> size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n>> diff --git a/object-file.c b/object-file.c\n>> index f233b440b22..b3e77213c42 100644\n>> --- a/object-file.c\n>> +++ b/object-file.c\n>> @@ -1974,12 +1974,41 @@ static int freshen_loose_object(const struct object_id *oid)\n>> static int freshen_packed_object(const struct object_id *oid)\n>> {\n>> \tstruct pack_entry e;\n>> +\tstruct stat st;\n>> +\tstruct strbuf name_buf = STRBUF_INIT;\n>> +\tconst char *filename;\n>> +\n>> \tif (!find_pack_entry(the_repository, oid, &e))\n>> \t\treturn 0;\n>> \tif (e.p->freshened)\n>> \t\treturn 1;\n>> -\tif (!freshen_file(e.p->pack_name))\n>> -\t\treturn 0;\n>> +\n>> +\tfilename = e.p->pack_name;\n>> +\tif (!strcasecmp(pack_mtime_suffix, \"pack\")) {\n>> +\t\tif (!freshen_file(filename))\n>> +\t\t\treturn 0;\n>> +\t\te.p->freshened = 1;\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\t/* If we want to freshen different file instead of .pack file, we need\n>> +\t * to make sure the file exists and create it if needed.\n>> +\t */\n>> +\tfilename = derive_pack_filename(filename, \"pack\", pack_mtime_suffix, &name_buf);\n> \n> You populate name_buf here, but don't strbuf_release(&name_buf) it at the end of this function.\n\nWill fix it.\n\n> \n>> +\tif (lstat(filename, &st) < 0) {\n>> +\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n>> +\t\tif (fd < 0) {\n>> +\t\t\t// here we need to check it again because other git process may created it\n> \n> /* */ comments, not //, if it's needed at all. Covered in CodingGuidelines\n\nWill fix it.\n\n> \n>> +\t\t\tif (lstat(filename, &st) < 0)\n>> +\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n> \n> If we can't create this specific file here shouldn't we just continue\n> silently at this point? Surely if this process is screwed we're just\n> about to die on something more important?\n\nYes, because here we have the *.pack file exists, we can step back to freshen\nthe *.pack file if the specific file cannot be created. And when we want to get\nthe mtime, we can also read mtime from *.pack file either if the specific\nfile does not exists. \n\n> \n> And lstat() can also return transitory errors that don't indicate\n> \"unable to create\", e.g. maybe we can out of memory the kernel is\n> willing to give us or something (just skimming the lstat manpage).\n\nThanks, I will think about it.\n\n> \n>> +\t\t} else {\n>> +\t\t\tclose(fd);\n>> +\t\t}\n>> +\t} else {\n>> +\t\tif (!freshen_file(filename))\n>> +\t\t\treturn 0;\n> \n> Style/indentatino: just do \"} else if (!freshen...\" ?\n\nWill fix it.\n> \n>> +\t}\n>> +\n>> \te.p->freshened = 1;\n>> \treturn 1;\n>> }\n>> diff --git a/packfile.c b/packfile.c\n>> index 755aa7aec5e..a607dda4e25 100644\n>> --- a/packfile.c\n>> +++ b/packfile.c\n>> @@ -40,6 +40,19 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n>> \treturn odb_pack_name(&buf, sha1, \"idx\");\n>> }\n>> \n>> +const char *derive_pack_filename(const char *pack_name, const char *strip,\n>> +\t\t\t\tconst char *suffix, struct strbuf *buf)\n>> +{\n>> +\tsize_t len;\n>> +\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n>> +\t pack_name[len - 1] != '.')\n>> +\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n>> +\t\t pack_name, strip);\n>> +\tstrbuf_add(buf, pack_name, len);\n>> +\tstrbuf_addstr(buf, suffix);\n>> +\treturn buf->buf;\n>> +}\n> \n> Just have this return void?\n\nI renamed the original 'derive_filename' here with new name 'derive_pack_filename',\nwhen it is used like this:\n\n filename = derive_filename(pack_name, \"pack\", suffix, &name_buf);\n\nand so it looks like a more convenient way to use the strbuf object, so I decided to\nkeep it the old way.\n\n> \n>> static unsigned int pack_used_ctr;\n>> static unsigned int pack_mmap_calls;\n>> static unsigned int peak_pack_open_windows;\n>> @@ -727,6 +740,17 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n>> \t */\n>> \tp->pack_size = st.st_size;\n>> \tp->pack_local = local;\n>> +\n>> +\t/* If we have different file used to freshen the mtime, we should\n>> +\t * use it at a higher priority.\n>> +\t */\n>> +\tif (!!strcasecmp(pack_mtime_suffix, \"pack\")) {\n>> +\t\tstruct strbuf name_buf = STRBUF_INIT;\n>> +\t\tconst char *filename;\n>> +\n>> +\t\tfilename = derive_pack_filename(path, \"idx\", pack_mtime_suffix, &name_buf);\n>> +\t\tstat(filename, &st);\n> \n> I.e. the \"filename\" here isn't needed, just call derive_pack_filename()\n> and use name.buf.buf to stat.\n\nLet me thinks about it, it's a good idea then.\n\n> \n> Also: We should check the stat return value here & report errno if\n> needed, no?\n\nHere I think we should step back to use the mtime of *.pack file instead, so if the specific file does\nnot exists, there are some reasons like (a) the *.pack is just created and mtime of it has not freshened\nagain or (b) we can not create the specific file for some reason, and in this case we will freshen the\n*.pack file instead. (c) someone or some process just delete it, we should try to go futher.\n\nI don't know if it's right to do this, but I think we should avoid die here.\n> \n>> +test_expect_success 'do not bother loosening old objects with core.packmtimesuffix config' '\n>> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n>> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n>> +\tpack1=$(echo $obj1 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n>> +\tpack2=$(echo $obj2 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n>> +\tgit -c core.packmtimesuffix=bump prune-packed &&\n>> +\tgit cat-file -p $obj1 &&\n>> +\tgit cat-file -p $obj2 &&\n>> +\ttouch .git/objects/pack/pack-$pack2.bump &&\n>> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n>> +\tgit -c core.packmtimesuffix=bump repack -A -d --unpack-unreachable=1.hour.ago &&\n> \n> On command-lines we can spell it camel-cased, e.g. -c\n> core.packMtimeSuffix[...].\n> \n\nThanks, will do it.\n"},{"id":"430075","messageId":"ACE7ECBE-0D7A-4FB8-B4F9-F9E32BE2234C@163.com","threadId":"56088","inReplyTo":"YO5RZ0Wix/K5q53Z@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-14T16:46:47Z","receivedAt":"2021-07-14T16:47:01Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月14日 10:52，Taylor Blau 写道：\n> \n> On Wed, Jul 14, 2021 at 03:39:18AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Hrm, per my v1 feedback (and I'm not sure if my suggestion is even good\n>> here, there's others more familiar with this area than I am), I was\n>> thinking of something like a *.bump file written via:\n>> \n>> core.packUseBumpFiles=bool\n>> \n>> Or something like that, anyway, the edge case in allowing the user to\n>> pick arbitrary suffixes is that we'd get in-the-wild user arbitrary\n>> configuration squatting on a relatively sensitive part of the object\n>> store.\n>> \n>> E.g. we recently added *.rev files to go with\n>> *.{pack,idx,bitmap,keep,promisor} (and I'm probably forgetting some\n>> suffix). What if before that a user had set:\n>> \n>> core.packMtimeSuffix=rev\n> \n> I think making the suffix configurable is probably a mistake. It seems\n> like an unnecessary detail to expose, but it also forces us to think\n> about cases like these where the configured suffix is already used for\n> some other purpose.\n\nThanks, I agree with you and will fix it, such like the *.keep file, we\ndo not use the suffix configuration to create keep files.\n\n> \n> I don't think that a new \".bump\" file is a bad idea, but it does seem\n> like we have a lot of files that represent a relatively little amount of\n> the state that a pack can be in. The \".promisor\" and \".keep\" files both\n> come to mind here. Some thoughts in this direction:\n> \n> - Combining *all* of the pack-related files (including the index,\n> reverse-index, bitmap, and so on) into a single \"pack-meta\" file\n> seems like a mistake for caching reasons.\n> \n> - But a meta file that contains just the small state (like promisor\n> information and whether or not the pack is \"kept\") seems like it\n> could be OK. On the other hand, being able to tweak the kept state\n> by touching or deleting a file is convenient (and having to rewrite\n> a meta file containing other information is much less so).\n\nYes, read and rewrite a meta file also means we need do lock/unlock, which\nmay cause inconvenient operations.\n\n> \n> But a \".bump\" file does seem like an awkward way to not rely on the\n> mtime of the pack itself. And I do think it runs into compatibility\n> issues like Ævar mentioned. Any version of Git that includes a\n> hypothetical .bump file (or something like it) needs to also update the\n> pack's mtime, too, so that old versions of Git can understand it. (Of\n> course, that could be configurable, but that seems far too obscure to\n> me).\n\nHere we will have a configuration and default is backward compatiblity,\nand if user decide to use the '.bump' file, which means he can not use\nthe old versions of Git, like the Repository Format Version, it is limited.\n\n> \n> Stepping back, I'm not sure I understand why freshening a pack is so\n> slow for you. freshen_file() just calls utime(2), and any sync back to\n> the disk shouldn't need to update the pack itself, just a couple of\n> fields in its inode. Maybe you could help explain further.\n> \n> In any case, I couldn't find a spot in your patch that updates the\n> packed_git's 'mtime' field, which is used to (a) sort packs in the\n> linked list of packs, and (b) for determining the least-recently used\n> pack if it has individual windows mmapped.\n\nThe reason why we want to avoid freshen the mtime of \".pack\" file is to\nimprove the reading speed of Git Servers.\n\nWe have some large repositories in our Git Severs (some are bigger than 10GB),\nand we created '.keep' files for large \".pack\" files, we want the big files\nunchanged to speed up git upload-pack, because in our mind the file system\ncache will reduce the disk IO if a file does not changed.\n\nHowever we find the mtime of \".pack\" files changes over time which makes the\nfile system always reload the big files, that takes a lot of IO time and result\nin lower speed of git upload-pack and even further the disk IOPS is exhausted.\n\n> \n> Thanks,\n> Taylor\n> \n\n\n"},{"id":"430077","messageId":"YO8XrOChAtxhpuxS@nand.local","threadId":"56088","inReplyTo":"ACE7ECBE-0D7A-4FB8-B4F9-F9E32BE2234C@163.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2021-07-14T17:04:03Z","receivedAt":"2021-07-14T17:04:06Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Thu, Jul 15, 2021 at 12:46:47AM +0800, Sun Chao wrote:\n> > Stepping back, I'm not sure I understand why freshening a pack is so\n> > slow for you. freshen_file() just calls utime(2), and any sync back to\n> > the disk shouldn't need to update the pack itself, just a couple of\n> > fields in its inode. Maybe you could help explain further.\n> >\n> > [ ... ]\n>\n> The reason why we want to avoid freshen the mtime of \".pack\" file is to\n> improve the reading speed of Git Servers.\n>\n> We have some large repositories in our Git Severs (some are bigger than 10GB),\n> and we created '.keep' files for large \".pack\" files, we want the big files\n> unchanged to speed up git upload-pack, because in our mind the file system\n> cache will reduce the disk IO if a file does not changed.\n>\n> However we find the mtime of \".pack\" files changes over time which makes the\n> file system always reload the big files, that takes a lot of IO time and result\n> in lower speed of git upload-pack and even further the disk IOPS is exhausted.\n\nThat's surprising behavior to me. Are you saying that calling utime(2)\ncauses the *page* cache to be invalidated and that most reads are\ncache-misses lowering overall IOPS?\n\nIf so, then I am quite surprised ;). The only state that should be\ndirtied by calling utime(2) is the inode itself, so the blocks referred\nto by the inode corresponding to a pack should be left in-tact.\n\nIf you're on Linux, you can try observing the behavior of evicting\ninodes, blocks, or both from the disk cache by changing \"2\" in the\nfollowing:\n\n hyperfine 'git pack-objects --all --stdout --delta-base-offset >/dev/null'\n --prepare='sync; echo 2 | sudo tee /proc/sys/vm/drop_caches'\n\nwhere \"1\" drops the page cache, \"2\" drops the inodes, and \"3\" evicts\nboth.\n\nI wonder if you could share the results of running the above varying\nthe value of \"1\", \"2\", and \"3\", as well as swapping the `--prepare` for\n`--warmup=3` to warm your caches (and give us an idea of what your\nexpected performance is probably like).\n\nThanks,\nTaylor\n"},{"id":"430099","messageId":"877dhs20x3.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"YO8XrOChAtxhpuxS@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T18:19:15Z","receivedAt":"2021-07-14T18:41:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Taylor Blau wrote:\n\n> On Thu, Jul 15, 2021 at 12:46:47AM +0800, Sun Chao wrote:\n>> > Stepping back, I'm not sure I understand why freshening a pack is so\n>> > slow for you. freshen_file() just calls utime(2), and any sync back to\n>> > the disk shouldn't need to update the pack itself, just a couple of\n>> > fields in its inode. Maybe you could help explain further.\n>> >\n>> > [ ... ]\n>>\n>> The reason why we want to avoid freshen the mtime of \".pack\" file is to\n>> improve the reading speed of Git Servers.\n>>\n>> We have some large repositories in our Git Severs (some are bigger than 10GB),\n>> and we created '.keep' files for large \".pack\" files, we want the big files\n>> unchanged to speed up git upload-pack, because in our mind the file system\n>> cache will reduce the disk IO if a file does not changed.\n>>\n>> However we find the mtime of \".pack\" files changes over time which makes the\n>> file system always reload the big files, that takes a lot of IO time and result\n>> in lower speed of git upload-pack and even further the disk IOPS is exhausted.\n>\n> That's surprising behavior to me. Are you saying that calling utime(2)\n> causes the *page* cache to be invalidated and that most reads are\n> cache-misses lowering overall IOPS?\n>\n> If so, then I am quite surprised ;). The only state that should be\n> dirtied by calling utime(2) is the inode itself, so the blocks referred\n> to by the inode corresponding to a pack should be left in-tact.\n>\n> If you're on Linux, you can try observing the behavior of evicting\n> inodes, blocks, or both from the disk cache by changing \"2\" in the\n> following:\n>\n> hyperfine 'git pack-objects --all --stdout --delta-base-offset >/dev/null'\n> --prepare='sync; echo 2 | sudo tee /proc/sys/vm/drop_caches'\n>\n> where \"1\" drops the page cache, \"2\" drops the inodes, and \"3\" evicts\n> both.\n>\n> I wonder if you could share the results of running the above varying\n> the value of \"1\", \"2\", and \"3\", as well as swapping the `--prepare` for\n> `--warmup=3` to warm your caches (and give us an idea of what your\n> expected performance is probably like).\n\nI think you may be right narrowly, but wrong in this context :)\n\nI.e. my understanding of this problem is that they have some incremental\nbackup job, e.g. rsync without --checksum (not that doing that would\nhelp, chicken & egg issue)..\n\nSo by changing the mtime you cause the file to be re-synced.\n\nYes Linux (or hopefully any modern OS) isn't so dumb as to evict your FS\ncache because of such a metadata change, but that's besides the point.\n\nIf you have a backup job like that your FS cache will get evicted or be\nsubject to churn anyway, because you'll shortly be having to deal with\nthe \"rsync\" job that's noticed the changed mtime competing for caching\nresources with \"real\" traffic.\n\nSun: Does that summarize the problem you're having?\n\n\n\nSun, also: Note that in general doing backups of live git repositories\nwith rsync is a bad idea, and will lead to corruption.\n\nThe most common cause of such corruption is that a tool like \"rsync\"\nwill iterate recursively through say \"objects\" followed by \"refs\".\n\nSo by the time it gets to the latter (or is doing a deep iteration\nwithin those dirs) git's state has changed in such a way as to yield an\nrsync backup in a state that the repository was never in.\n\n(As an aside, I've often wondered what it is about git exactly makes\npeople who'd never think of doing the same thing with the FS part of an\nRDMBS's data store think that implementing such an ad-hoc backup\nsolution for git would be a good idea, but I digress. Perhaps we need\nmore scarier looking BerkeleyDB-looking names in the .git directory :)\n\nEven if you do FS snapshots of live git repositories you're likely to\nget corruption, search this mailing list for references to fsync(),\ne.g. [1].\n\nIn short, git's historically (and still) been sloppy about rsync, and\nrelied on non-standard behavior such as \"if I do N updates for N=1..100,\nand fsync just \"100\", then I can assume 1..99 are fsynced (spoiler: you\ncan't assume that).\n\nOur use of fsync is still broken in that sense today, git is not a safe\nplace to store your data in the POSIXLY pedantic sense (and no, I don't\njust mean that core.fsyncObjectFiles is `false` by default, it only\ncovers a small part of this, e.g. we don't fsync dir entries even with\nthat).\n\nOn a real live filesystem this is usually not an issue, because if\nyou're not dealing with yanked power cords (and even then, journals\nmight save you), then even if you fsync a file but don't fsync the dir\nentry it's in, the FS is usually forgiving about such cases.\n\nI.e. if someone does a concurrent request for the could-be-outdated dir\nentry they'll service the up-to-date one, even without that having been\nfsync'd, because the VFS layer isn't going to the synced disk, it's\nchecking it's current state and servicing your request from that.\n\nBut at least some FS snapshot implementations have a habit of exposing\nthe most pedantic interpretation possible of FS semantics, and one that\nyou wouldn't ever get on a live FS. I.e. you might be hooking into the\nequivalent of the order in which things are written to disk, and end up\nwith a state that would never have been exposed to a running program\n(there would be a 1=1 correspondence if we fsync'd properly, which we\ndon't).\n\nThe best way to get backups of git repositories you know are correct are\nis to use git's own transport mechanisms, i.e. fetch/pull the data, or\ncreate bundles from it. This would be the case even if we fixed all our\nfsync issues, because doing so wouldn't help you in the case of a\nbit-flip, but an \"index-pack\" on the other end will spot such issues.\n\n1. https://lore.kernel.org/git/20200917112830.26606-2-avarab@gmail.com/\n"},{"id":"430103","messageId":"12435060.NHVMl2pYiE@mfick-lnx","threadId":"56088","inReplyTo":"877dhs20x3.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2021-07-14T19:11:21Z","receivedAt":"2021-07-14T19:11:31Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Wednesday, July 14, 2021 8:19:15 PM MDT Ævar Arnfjörð Bjarmason wrote:\n> The best way to get backups of git repositories you know are correct are\n> is to use git's own transport mechanisms, i.e. fetch/pull the data, or\n> create bundles from it. \n\nI don't think this is a fair recommendation since unfortunately, this cannot \nbe used to create a full backup. This can be used to back up the version \ncontrolled data, but not the repositories meta-data, i.e. configs, reflogs, \nalternate setups...\n\n-Martin\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n"},{"id":"430107","messageId":"YO87ax2JpLndc5Ly@nand.local","threadId":"56088","inReplyTo":"877dhs20x3.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-14T19:30:51Z","receivedAt":"2021-07-14T19:30:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 14, 2021 at 08:19:15PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> The reason why we want to avoid freshen the mtime of \".pack\" file is to\n> >> improve the reading speed of Git Servers.\n> >\n> > That's surprising behavior to me. Are you saying that calling utime(2)\n> > causes the *page* cache to be invalidated and that most reads are\n> > cache-misses lowering overall IOPS?\n>\n> I think you may be right narrowly, but wrong in this context :)\n>\n> I.e. my understanding of this problem is that they have some incremental\n> backup job, e.g. rsync without --checksum (not that doing that would\n> help, chicken & egg issue)..\n\nAh, thanks for explaining. That's helpful, and changes my thinking.\n\nIdeally, Sun would be able to use --checksum (if they are using rsync)\nor some equivalent (if they are not). In other words, this seems like a\nproblem that Git shouldn't be bending over backwards for.\n\nBut if that isn't possible, then I find introducing a new file to\nredefine the pack's mtime just to accommodate a backup system that\ndoesn't know better to be a poor justification for adding this\ncomplexity. Especially since we agree that rsync-ing live Git\nrepositories is a bad idea in the first place ;).\n\nIf it were me, I would probably stop here and avoid pursuing this\nfurther. But an OK middle ground might be core.freshenPackfiles=\nto indicate whether or not packs can be freshened, or the objects\ncontained within them should just be rewritten loose.\n\nSun could then set this configuration to \"false\", implying:\n\n - That they would have more random loose objects, leading to some\n redundant work by their backup system.\n - But they wouldn't have to resync their huge packfiles.\n\n...and we wouldn't have to introduce any new formats/file types to do\nit. To me, that seems like a net-positive outcome.\n\nThanks,\nTaylor\n"},{"id":"430112","messageId":"87y2a8zntw.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"YO87ax2JpLndc5Ly@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T19:32:26Z","receivedAt":"2021-07-14T19:44:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Taylor Blau wrote:\n\n> On Wed, Jul 14, 2021 at 08:19:15PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> >> The reason why we want to avoid freshen the mtime of \".pack\" file is to\n>> >> improve the reading speed of Git Servers.\n>> >\n>> > That's surprising behavior to me. Are you saying that calling utime(2)\n>> > causes the *page* cache to be invalidated and that most reads are\n>> > cache-misses lowering overall IOPS?\n>>\n>> I think you may be right narrowly, but wrong in this context :)\n>>\n>> I.e. my understanding of this problem is that they have some incremental\n>> backup job, e.g. rsync without --checksum (not that doing that would\n>> help, chicken & egg issue)..\n>\n> Ah, thanks for explaining. That's helpful, and changes my thinking.\n>\n> Ideally, Sun would be able to use --checksum (if they are using rsync)\n> or some equivalent (if they are not). In other words, this seems like a\n> problem that Git shouldn't be bending over backwards for.\n\nEven with my strong opinions about rsync being bad for this use-case, in\npractice it does work for a lot of people, e.g. with nightly jobs\netc. Not everyone's repository is insanely busy, where I've mainly seen\nthis sort of corruption.\n\nIn that case making people use --checksum is borderline inhumane :)\n\n> But if that isn't possible, then I find introducing a new file to\n> redefine the pack's mtime just to accommodate a backup system that\n> doesn't know better to be a poor justification for adding this\n> complexity. Especially since we agree that rsync-ing live Git\n> repositories is a bad idea in the first place ;).\n>\n> If it were me, I would probably stop here and avoid pursuing this\n> further. But an OK middle ground might be core.freshenPackfiles=\n> to indicate whether or not packs can be freshened, or the objects\n> contained within them should just be rewritten loose.\n>\n> Sun could then set this configuration to \"false\", implying:\n>\n> - That they would have more random loose objects, leading to some\n> redundant work by their backup system.\n> - But they wouldn't have to resync their huge packfiles.\n>\n> ...and we wouldn't have to introduce any new formats/file types to do\n> it. To me, that seems like a net-positive outcome.\n\nThis approach is getting quite close to my core.checkCollisions patch,\nto the point of perhaps being indistinguishable in practice:\nhttps://lore.kernel.org/git/20181028225023.26427-5-avarab@gmail.com/\n\nI.e. if you're happy to re-write out duplicate objects then you're going\nto be ignoring the collision check and don't need to do it. It's not the\nsame in that you might skip writing objects you know are reachable, and\nwith the collisions check off and not-so-thin packs you will/might get\nmore redundancy than you asked for.\n\nBut in practice with modern clients mostly/entirely sending you just the\nthings you need in the common case it might be close enough.\n\nI mean it addresses the expiry race that a unreachable-becomes-reachable\nagain race would be \"solved\" by just re-writing that data, hrm, but\nthere's probably aspects of that race I'm not considering.\n\nAnyway, in the sense that for a lot of systems syncing file additions is\na lot cheaper than rewrites it might get you what you want...\n"},{"id":"430114","messageId":"YO9AeudYPmWRnRNb@nand.local","threadId":"56088","inReplyTo":"87y2a8zntw.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-14T19:52:26Z","receivedAt":"2021-07-14T19:59:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 14, 2021 at 09:32:26PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > But if that isn't possible, then I find introducing a new file to\n> > redefine the pack's mtime just to accommodate a backup system that\n> > doesn't know better to be a poor justification for adding this\n> > complexity. Especially since we agree that rsync-ing live Git\n> > repositories is a bad idea in the first place ;).\n> >\n> > If it were me, I would probably stop here and avoid pursuing this\n> > further. But an OK middle ground might be core.freshenPackfiles=\n> > to indicate whether or not packs can be freshened, or the objects\n> > contained within them should just be rewritten loose.\n> >\n> > Sun could then set this configuration to \"false\", implying:\n> >\n> > - That they would have more random loose objects, leading to some\n> > redundant work by their backup system.\n> > - But they wouldn't have to resync their huge packfiles.\n> >\n> > ...and we wouldn't have to introduce any new formats/file types to do\n> > it. To me, that seems like a net-positive outcome.\n>\n> This approach is getting quite close to my core.checkCollisions patch,\n> to the point of perhaps being indistinguishable in practice:\n> https://lore.kernel.org/git/20181028225023.26427-5-avarab@gmail.com/\n\nHmm, I'm not sure if I understand. That collision check is only done\nduring index-pack, and reading builtin/index-pack.c:check_collision(),\nit looks like we only do it for large blobs anyway.\n\n> I.e. if you're happy to re-write out duplicate objects then you're going\n> to be ignoring the collision check and don't need to do it. It's not the\n> same in that you might skip writing objects you know are reachable, and\n> with the collisions check off and not-so-thin packs you will/might get\n> more redundancy than you asked for.\n\nWe may be talking about different things, but if users are concerned\nabout SHA-1 collisions, then they should still be able to build with\nDC_SHA1=YesPlease to catch shattered-style collisions.\n\nAnyway, I think we may be a little in the weeds for what we are trying\nto accomplish here. I'm thinking something along the lines of the\nfollowing (sans documentation and tests, of course ;)).\n\n--- >8 ---\n\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b2..87c9238365 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1971,9 +1971,22 @@ static int freshen_loose_object(const struct object_id *oid)\n \treturn check_and_freshen(oid, 1);\n }\n\n+static int can_freshen_packs = -1;\n+static int get_can_freshen_packs(void)\n+{\n+\t if (can_freshen_packs < 0) {\n+\t\tif (git_config_get_bool(\"core.freshenpackfiles\",\n+\t\t\t\t\t&can_freshen_packs))\n+\t\t\tcan_freshen_packs = 1;\n+\t }\n+\t return can_freshen_packs;\n+}\n+\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n+\tif (!get_can_freshen_packs())\n+\t\treturn 0;\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n"},{"id":"430115","messageId":"87v95czn7q.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"12435060.NHVMl2pYiE@mfick-lnx","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T19:41:42Z","receivedAt":"2021-07-14T20:00:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Martin Fick wrote:\n\n> On Wednesday, July 14, 2021 8:19:15 PM MDT Ævar Arnfjörð Bjarmason wrote:\n>> The best way to get backups of git repositories you know are correct are\n>> is to use git's own transport mechanisms, i.e. fetch/pull the data, or\n>> create bundles from it. \n>\n> I don't think this is a fair recommendation since unfortunately, this cannot \n> be used to create a full backup. This can be used to back up the version \n> controlled data, but not the repositories meta-data, i.e. configs, reflogs, \n> alternate setups...\n\n*nod*\n\nFWIW at an ex-job I helped systems administrators who'd produced such a\nbroken backup-via-rsync create a hybrid version as an interim\nsolution. I.e. it would sync the objects via git transport, and do an\nrsync on a whitelist (or blacklist), so pickup config, but exclude\nobjects.\n\n\"Hybrid\" because it was in a state of needing to deal with manual\ntweaking of config.\n\nBut usually someone who's needing to thoroughly solve this backup\nproblem will inevitably end up with wanting to drive everything that's\nnot in the object or refstore from some external system, i.e. have\nconfig be generated from puppet, a database etc., ditto for alternates\netc.\n\nBut even if you can't get to that point (or don't want to) I'd say aim\nfor the hybrid system.\n\nThis isn't some purely theoretical concern b.t.w., the system using\nrsync like this was producing repos that wouldn't fsck all the time, and\nit wasn't such a busy site.\n\nI suspect (but haven't tried) that for someone who can't easily change\ntheir backup solution they'd get most of the benefits of git-native\ntransport by having their \"rsync\" sync refs, then objects, not the other\nway around. Glob order dictates that most backup systems will do\nobjects, then refs (which will of course, at that point, refer to\nnonexisting objects).\n\nIt's still not safe, you'll still be subject to races, but probably a\nlot better in practice.\n"},{"id":"430119","messageId":"3112447.ymCj9SdLpg@mfick-lnx","threadId":"56088","inReplyTo":"87v95czn7q.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2021-07-14T20:20:31Z","receivedAt":"2021-07-14T20:20:42Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Wednesday, July 14, 2021 9:41:42 PM MDT you wrote:\n> On Wed, Jul 14 2021, Martin Fick wrote:\n> > On Wednesday, July 14, 2021 8:19:15 PM MDT Ævar Arnfjörð Bjarmason wrote:\n> >> The best way to get backups of git repositories you know are correct are\n> >> is to use git's own transport mechanisms, i.e. fetch/pull the data, or\n> >> create bundles from it.\n> > \n> > I don't think this is a fair recommendation since unfortunately, this\n> > cannot be used to create a full backup. This can be used to back up the\n> > version controlled data, but not the repositories meta-data, i.e.\n> > configs, reflogs, alternate setups...\n> \n> *nod*\n> \n> FWIW at an ex-job I helped systems administrators who'd produced such a\n> broken backup-via-rsync create a hybrid version as an interim\n> solution. I.e. it would sync the objects via git transport, and do an\n> rsync on a whitelist (or blacklist), so pickup config, but exclude\n> objects.\n> \n> \"Hybrid\" because it was in a state of needing to deal with manual\n> tweaking of config.\n> \n> But usually someone who's needing to thoroughly solve this backup\n> problem will inevitably end up with wanting to drive everything that's\n> not in the object or refstore from some external system, i.e. have\n> config be generated from puppet, a database etc., ditto for alternates\n> etc.\n> \n> But even if you can't get to that point (or don't want to) I'd say aim\n> for the hybrid system.\n> \n> This isn't some purely theoretical concern b.t.w., the system using\n> rsync like this was producing repos that wouldn't fsck all the time, and\n> it wasn't such a busy site.\n> \n> I suspect (but haven't tried) that for someone who can't easily change\n> their backup solution they'd get most of the benefits of git-native\n> transport by having their \"rsync\" sync refs, then objects, not the other\n> way around. Glob order dictates that most backup systems will do\n> objects, then refs (which will of course, at that point, refer to\n> nonexisting objects).\n> \n> It's still not safe, you'll still be subject to races, but probably a\n> lot better in practice.\n\nIt would be great if git provided a command to do a reliable incremental \nbackup, maybe it could copy things in the order that you mention?\n\nHowever, most people will want to use the backup system they have and not a \nspecial git tool. Maybe git fsck should gain a switch that would rewind any \nrefs to an older point that is no broken (using reflogs)? That way, most \nbackups would just work and be rewound to the point at which the backup \nstarted?\n\n-Martin\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n\n-Martin\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n"},{"id":"430134","messageId":"xmqq5yxcvak7.fsf@gitster.g","threadId":"56088","inReplyTo":"YO87ax2JpLndc5Ly@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-14T21:40:24Z","receivedAt":"2021-07-14T21:40:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But if that isn't possible, then I find introducing a new file to\n> redefine the pack's mtime just to accommodate a backup system that\n> doesn't know better to be a poor justification for adding this\n> complexity. Especially since we agree that rsync-ing live Git\n> repositories is a bad idea in the first place ;).\n>\n> If it were me, I would probably stop here and avoid pursuing this\n> further.\n\n;-).\n"},{"id":"430187","messageId":"CAL3xRKee3YmOrV_-4Tu6FmJyRnS2y-tdiAmXp5TjzL_WxQNrtw@mail.gmail.com","threadId":"56088","inReplyTo":"87v95czn7q.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2021-07-15T08:23:04Z","receivedAt":"2021-07-15T08:23:19Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi folks,\n\nOn Wed, Jul 14, 2021 at 10:03 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> *nod*\n>\n> FWIW at an ex-job I helped systems administrators who'd produced such a\n> broken backup-via-rsync create a hybrid version as an interim\n> solution. I.e. it would sync the objects via git transport, and do an\n> rsync on a whitelist (or blacklist), so pickup config, but exclude\n> objects.\n>\n> \"Hybrid\" because it was in a state of needing to deal with manual\n> tweaking of config.\n>\n> But usually someone who's needing to thoroughly solve this backup\n> problem will inevitably end up with wanting to drive everything that's\n> not in the object or refstore from some external system, i.e. have\n> config be generated from puppet, a database etc., ditto for alternates\n> etc.\n>\n> But even if you can't get to that point (or don't want to) I'd say aim\n> for the hybrid system.\n\nFWIW, we are running our repo on top of a some-what flickery DRBD setup and\nwe decided to use both\n\n git clone --upload-pack 'git -c transfer.hiderefs=\"!refs\"\nupload-pack' --mirror`\n\nand\n\n `tar`\n\nto create 2 separate snapshots for backup in parallel (full backup,\nnot incremental).\n\nIn case of recovery (manual), we first rely on the git snapshot and if\nthere is any\nmissing objects/refs, we will try to get it from the tarball.\n\n>\n> This isn't some purely theoretical concern b.t.w., the system using\n> rsync like this was producing repos that wouldn't fsck all the time, and\n> it wasn't such a busy site.\n>\n> I suspect (but haven't tried) that for someone who can't easily change\n> their backup solution they'd get most of the benefits of git-native\n> transport by having their \"rsync\" sync refs, then objects, not the other\n> way around. Glob order dictates that most backup systems will do\n> objects, then refs (which will of course, at that point, refer to\n> nonexisting objects).\n>\n> It's still not safe, you'll still be subject to races, but probably a\n> lot better in practice.\n\nI would love to get some guidance in official documentation on what is the best\npractice around handling git data on the server side.\n\nIs git-clone + git-bundle the go-to solution?\nShould tar/rsync not be used completely or is there a trade-off?\n\nThanks,\nSon Luong.\n"},{"id":"430235","messageId":"YPBlbNRoupMtT2dg@nand.local","threadId":"56088","inReplyTo":"ACFA1FCF-3F24-470D-A3AE-DBAA269E9E2C@163.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-15T16:42:20Z","receivedAt":"2021-07-15T16:42:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 16, 2021 at 12:30:18AM +0800, Sun Chao wrote:\n> I'm sorry to reply so late, I work long hours during the day, and the\n> company network can not send external mail, so I can only go home late\n> at night to reply to you.\n\nThere's no need to apologize :-).\n\n> Thanks for your reply again, My explaination for 'why the mtime is so\n> important' lost some informations and it is not clear enough, I will\n> tell the details here:\n\nLet me see if I can summarize here. Basically:\n\n - You have a number of servers that have NFS mounts which hold large\n repositories with packs in excess of 10 GB in size.\n - You have a lot of clients that are fetching, and a smaller number of\n clients that are pushing, some of which happen to freshen the mtimes\n of the packs.\n\n...and the mtimes being updated cause the disk cache to be invalidated?\n\nIt's the last part that is so surprising to me. Ævar and I discussed\nearlier in the thread that their understanding was that you had a backup\nsystem which had to resynchronize an unchanged file because its metadata\nhad changed.\n\nBut this is different than that. If I understand what you're saying\ncorrectly, then you're saying that the disk caches themselves are\ninvalidated by changing the mtime.\n\nThat is highly surprising to me, since the block cache should only be\ninvalidated if the *blocks* change, not metadata in the inode. It would\nbe good to confirm that this is actually what's happening.\n\nThanks,\nTaylor\n"},{"id":"430237","messageId":"1B70F549-6A11-4FC7-B21F-C7FB014820CB@163.com","threadId":"56088","inReplyTo":"YPBlbNRoupMtT2dg@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-15T16:48:54Z","receivedAt":"2021-07-15T16:50:13Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月16日 00:42，Taylor Blau 写道：\n> \n> On Fri, Jul 16, 2021 at 12:30:18AM +0800, Sun Chao wrote:\n>> I'm sorry to reply so late, I work long hours during the day, and the\n>> company network can not send external mail, so I can only go home late\n>> at night to reply to you.\n> \n> There's no need to apologize :-).\n> \n>> Thanks for your reply again, My explaination for 'why the mtime is so\n>> important' lost some informations and it is not clear enough, I will\n>> tell the details here:\n> \n> Let me see if I can summarize here. Basically:\n> \n> - You have a number of servers that have NFS mounts which hold large\n> repositories with packs in excess of 10 GB in size.\n> - You have a lot of clients that are fetching, and a smaller number of\n> clients that are pushing, some of which happen to freshen the mtimes\n> of the packs.\n> \n> ...and the mtimes being updated cause the disk cache to be invalidated?\n> \n> It's the last part that is so surprising to me. Ævar and I discussed\n> earlier in the thread that their understanding was that you had a backup\n> system which had to resynchronize an unchanged file because its metadata\n> had changed.\n> \n> But this is different than that. If I understand what you're saying\n> correctly, then you're saying that the disk caches themselves are\n> invalidated by changing the mtime.\n> \n> That is highly surprising to me, since the block cache should only be\n> invalidated if the *blocks* change, not metadata in the inode. It would\n> be good to confirm that this is actually what's happening.\n> \n> Thanks,\n> Taylor\n\nOh, Maybe I didn't understand caching well enough, let me check it again,\nand thanks for your and Ævar's answers, they are really helpful.\n\n\n"},{"id":"430246","messageId":"ACFA1FCF-3F24-470D-A3AE-DBAA269E9E2C@163.com","threadId":"56088","inReplyTo":"YO8XrOChAtxhpuxS@nand.local","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-15T16:30:18Z","receivedAt":"2021-07-15T17:19:36Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月15日 01:04，Taylor Blau 写道：\n> \n> [...]\n>> \n>> However we find the mtime of \".pack\" files changes over time which makes the\n>> file system always reload the big files, that takes a lot of IO time and result\n>> in lower speed of git upload-pack and even further the disk IOPS is exhausted.\n> \n> That's surprising behavior to me. Are you saying that calling utime(2)\n> causes the *page* cache to be invalidated and that most reads are\n> cache-misses lowering overall IOPS?\n> \n> If so, then I am quite surprised ;). The only state that should be\n> dirtied by calling utime(2) is the inode itself, so the blocks referred\n> to by the inode corresponding to a pack should be left in-tact.\n> \n> If you're on Linux, you can try observing the behavior of evicting\n> inodes, blocks, or both from the disk cache by changing \"2\" in the\n> following:\n> \n> hyperfine 'git pack-objects --all --stdout --delta-base-offset >/dev/null'\n> --prepare='sync; echo 2 | sudo tee /proc/sys/vm/drop_caches'\n> \n> where \"1\" drops the page cache, \"2\" drops the inodes, and \"3\" evicts\n> both.\n> \n> I wonder if you could share the results of running the above varying\n> the value of \"1\", \"2\", and \"3\", as well as swapping the `--prepare` for\n> `--warmup=3` to warm your caches (and give us an idea of what your\n> expected performance is probably like).\n> \n> Thanks,\n> Taylor\n\nI'm sorry to reply so late, I work long hours during the day, and the company\nnetwork can not send external mail, so I can only go home late at night to reply to you.\n\nThanks for your reply again, My explaination for 'why the mtime is so important' lost some\ninformations and it is not clear enough, I will tell the details here:\n\nServers:\n- We maintain a number of servers, each mounting some NFS disks that hold our git\n repositories, some of them are so large (cannot reduce the size now), they are > 10GB\n- There are too many objects and large files in the git history which result in some\n large '.pack' files in the '.git/objects/pack' directires\n- We created the '.keep' files for each large '.pack' file, wish the disk cache can reduce\n the NFS IOPS and just load contents from caches.\n\nClients:\n- There are too many CI systems are keep downloading the git repositories in a very\n high frequency, e.g. we find different CI systems make 600 download requests in a short\n period of time by 'git fetch'.\n- Some developers are doing 'git push' at the same time, create Pull Requests after that\n (which trigger the CI then), so git servers will do some update tasks which may cause the\n mtime of '.pack' file freshend.\n\nSo, in this case there will be many 'git-upload-pack' processes running on the git servers,\nthey all need to load the big '.pack' files. The 'git-upload-pack' will be faster if the\ndisk cache is warmed up and the NFS server will be not so busy.\n\nHowever we find the IOPS of the NFS server always be exhausted and the 'git-upload-pack' will\nruns for a very long time. We noticed the mtime of '.pack' changes over time, one of my\ncolleagues who is familiar with the file system tell me it's the mtime who invalidate the\ndisk caches.\n\nSo we want the caches to be valid for a long time which can speed up the 'git-upload-pack'\nprocesses.\n\nI don't known if the `/proc/sys/vm/drop_caches` can help or not, but thanks for your tips,\nI will try to check them and see if there are some differences.\n"},{"id":"430543","messageId":"pull.1043.v3.git.git.1626724399377.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":"pull.1043.v2.git.git.1626226114067.gitgitgadget@gmail.com","subject":"[PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-19T19:53:19Z","receivedAt":"2021-07-19T22:58:16Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nCommit 33d4221c79 (write_sha1_file: freshen existing objects,\n2014-10-15) avoid writing existing objects by freshen their\nmtime (especially the packfiles contains them) in order to\naid the correct caching, and some process like find_lru_pack\ncan make good decision. However, this is unfriendly to\nincremental backup jobs or services rely on cached file system\nwhen there are large '.pack' files exists.\n\nFor example, after packed all objects, use 'write-tree' to\ncreate same commit with the same tree and same environments\nsuch like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\nnotice the '.pack' file's mtime changed. Git servers\nthat mount the same NFS disk will re-sync the '.pack' files\nto cached file system which will slow the git commands.\n\nSo if add core.freshenPackfiles to indicate whether or not\npacks can be freshened, turning off this option on some\nservers can speed up the execution of some commands on servers\nwhich use NFS disk instead of local disk.\n\nSigned-off-by: Sun Chao <16657101987@163.com>\n---\n packfile: freshen the mtime of packfile by configuration\n \n packfile: freshen the mtime of packfile by configuration\n \n Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n 2014-10-15) avoid writing existing objects by freshen their mtime\n (especially the packfiles contains them) in order to aid the correct\n caching, and some process like find_lru_pack can make good decision.\n However, this is unfriendly to incremental backup jobs or services rely\n on cached file system when there are large '.pack' files exists.\n \n For example, after packed all objects, use 'write-tree' to create same\n commit with the same tree and same environments such like\n GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can notice the '.pack' file's\n mtime changed. Git servers that mount the same NFS disk will re-sync the\n '.pack' files to cached file system which will slow the git commands.\n \n Here we can find the description of the cached file system for NFS\n Client from\n https://www.ibm.com/docs/en/aix/7.2?topic=performance-cache-file-system:\n \n 3. To ensure that the cached directories and files are kept up to date, \n CacheFS periodically checks the consistency of files stored in the cache.\n It does this by comparing the current modification time to the previous\n modification time.\n \n 4. If the modification times are different, all data and attributes\n for the directory or file are purged from the cache, and new data and\n attributes are retrieved from the back file system.\n \n \n So if add core.freshenPackfiles to indicate whether or not packs can be\n freshened, turning off this option on some servers can speed up the\n execution of some commands on servers which use NFS disk instead of\n local disk.\n \n Signed-off-by: Sun Chao 16657101987@163.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1043%2Fsunchao9%2Fmaster-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1043/sunchao9/master-v3\nPull-Request: https://github.com/git/git/pull/1043\n\nRange-diff vs v2:\n\n 1: 943e31e8587 ! 1: 16c68923bea packfile: freshen the mtime of packfile by configuration\n @@ Commit message\n mtime (especially the packfiles contains them) in order to\n aid the correct caching, and some process like find_lru_pack\n can make good decision. However, this is unfriendly to\n - incremental backup jobs or services rely on file system\n - cache when there are large '.pack' files exists.\n + incremental backup jobs or services rely on cached file system\n + when there are large '.pack' files exists.\n \n For example, after packed all objects, use 'write-tree' to\n create same commit with the same tree and same environments\n such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n - notice the '.pack' file's mtime changed, and '.idx' file not.\n + notice the '.pack' file's mtime changed. Git servers\n + that mount the same NFS disk will re-sync the '.pack' files\n + to cached file system which will slow the git commands.\n \n - If we freshen the mtime of packfile by updating another\n - file instead of '.pack' file e.g. a empty '.bump' file,\n - when we need to check the mtime of packfile, get it from\n - another file instead. Large git repository may contains\n - large '.pack' files, and we can use smaller files even empty\n - file to do the mtime get/set operation, this can avoid\n - file system cache re-sync large '.pack' files again and\n - then speed up most git commands.\n + So if add core.freshenPackfiles to indicate whether or not\n + packs can be freshened, turning off this option on some\n + servers can speed up the execution of some commands on servers\n + which use NFS disk instead of local disk.\n \n Signed-off-by: Sun Chao <16657101987@163.com>\n \n @@ Documentation/config/core.txt: the largest projects. You probably do not need t\n +\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \n -+core.packMtimeSuffix::\n ++core.freshenPackFiles::\n +\tNormally we avoid writing existing object by freshening the mtime\n +\tof the *.pack file which contains it in order to aid some processes\n -+\tsuch like prune. Use different file instead of *.pack file will\n -+\tavoid file system cache re-sync the large packfiles, and consequently\n -+\tmake git commands faster.\n ++\tsuch like prune. Turning off this option on some servers can speed\n ++\tup the execution of some commands like 'git-upload-pack'(e.g. some\n ++\tservers that mount the same NFS disk will re-sync the *.pack files\n ++\tto cached file system if the mtime cahnges).\n ++\n -+The default is 'pack' which means the *.pack file will be freshened by\n -+default. You can configure a different suffix to use, the file with the\n -+suffix will be created automatically, it's better not using any known\n -+suffix such like 'idx', 'keep', 'promisor'.\n ++The default is true which means the *.pack file will be freshened if we\n ++want to write a existing object whthin it.\n +\n core.deltaBaseCacheLimit::\n \tMaximum number of bytes per thread to reserve for caching base objects\n \tthat may be referenced by multiple deltified objects. By storing the\n \n - ## builtin/index-pack.c ##\n -@@ builtin/index-pack.c: static void fix_unresolved_deltas(struct hashfile *f)\n - \tfree(sorted_by_pos);\n - }\n - \n --static const char *derive_filename(const char *pack_name, const char *strip,\n --\t\t\t\t const char *suffix, struct strbuf *buf)\n --{\n --\tsize_t len;\n --\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n --\t pack_name[len - 1] != '.')\n --\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n --\t\t pack_name, strip);\n --\tstrbuf_add(buf, pack_name, len);\n --\tstrbuf_addstr(buf, suffix);\n --\treturn buf->buf;\n --}\n --\n - static void write_special_file(const char *suffix, const char *msg,\n - \t\t\t const char *pack_name, const unsigned char *hash,\n - \t\t\t const char **report)\n -@@ builtin/index-pack.c: static void write_special_file(const char *suffix, const char *msg,\n - \tint msg_len = strlen(msg);\n - \n - \tif (pack_name)\n --\t\tfilename = derive_filename(pack_name, \"pack\", suffix, &name_buf);\n -+\t\tfilename = derive_pack_filename(pack_name, \"pack\", suffix, &name_buf);\n - \telse\n - \t\tfilename = odb_pack_name(&name_buf, hash, suffix);\n - \n -@@ builtin/index-pack.c: int cmd_index_pack(int argc, const char **argv, const char *prefix)\n - \tif (from_stdin && hash_algo)\n - \t\tdie(_(\"--object-format cannot be used with --stdin\"));\n - \tif (!index_name && pack_name)\n --\t\tindex_name = derive_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n -+\t\tindex_name = derive_pack_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n - \n - \topts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);\n - \tif (rev_index) {\n - \t\topts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;\n - \t\tif (index_name)\n --\t\t\trev_index_name = derive_filename(index_name,\n -+\t\t\trev_index_name = derive_pack_filename(index_name,\n - \t\t\t\t\t\t\t \"idx\", \"rev\",\n - \t\t\t\t\t\t\t &rev_index_name_buf);\n - \t}\n -\n ## cache.h ##\n @@ cache.h: extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern unsigned long big_file_threshold;\n extern unsigned long pack_size_limit_cfg;\n -+extern const char *pack_mtime_suffix;\n ++extern int core_freshen_packfiles;\n \n /*\n * Accessors for the core.sharedrepository config which lazy-load the value\n @@ config.c: static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n -+\tif (!strcmp(var, \"core.packmtimesuffix\")) {\n -+\t\treturn git_config_string(&pack_mtime_suffix, var, value);\n ++\tif (!strcmp(var, \"core.freshenpackfiles\")) {\n ++\t\tcore_freshen_packfiles = git_config_bool(var, value);\n +\t}\n +\n \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n @@ config.c: static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \n ## environment.c ##\n -@@ environment.c: const char *git_hooks_path;\n - int zlib_compression_level = Z_BEST_SPEED;\n - int core_compression_level;\n - int pack_compression_level = Z_DEFAULT_COMPRESSION;\n -+const char *pack_mtime_suffix = \"pack\";\n - int fsync_object_files;\n - size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n - size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n +@@ environment.c: int core_sparse_checkout_cone;\n + int merge_log_config = -1;\n + int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n + unsigned long pack_size_limit_cfg;\n ++int core_freshen_packfiles = 1;\n + enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n + \n + #ifndef PROTECT_HFS_DEFAULT\n \n ## object-file.c ##\n @@ object-file.c: static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n -+\tstruct stat st;\n -+\tstruct strbuf name_buf = STRBUF_INIT;\n -+\tconst char *filename;\n ++\n ++\tif (!core_freshen_packfiles)\n ++\t\treturn 1;\n +\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n - \t\treturn 1;\n --\tif (!freshen_file(e.p->pack_name))\n --\t\treturn 0;\n -+\n -+\tfilename = e.p->pack_name;\n -+\tif (!strcasecmp(pack_mtime_suffix, \"pack\")) {\n -+\t\tif (!freshen_file(filename))\n -+\t\t\treturn 0;\n -+\t\te.p->freshened = 1;\n -+\t\treturn 1;\n -+\t}\n -+\n -+\t/* If we want to freshen different file instead of .pack file, we need\n -+\t * to make sure the file exists and create it if needed.\n -+\t */\n -+\tfilename = derive_pack_filename(filename, \"pack\", pack_mtime_suffix, &name_buf);\n -+\tif (lstat(filename, &st) < 0) {\n -+\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n -+\t\tif (fd < 0) {\n -+\t\t\t// here we need to check it again because other git process may created it\n -+\t\t\tif (lstat(filename, &st) < 0)\n -+\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n -+\t\t} else {\n -+\t\t\tclose(fd);\n -+\t\t}\n -+\t} else {\n -+\t\tif (!freshen_file(filename))\n -+\t\t\treturn 0;\n -+\t}\n -+\n - \te.p->freshened = 1;\n - \treturn 1;\n - }\n -\n - ## packfile.c ##\n -@@ packfile.c: char *sha1_pack_index_name(const unsigned char *sha1)\n - \treturn odb_pack_name(&buf, sha1, \"idx\");\n - }\n - \n -+const char *derive_pack_filename(const char *pack_name, const char *strip,\n -+\t\t\t\tconst char *suffix, struct strbuf *buf)\n -+{\n -+\tsize_t len;\n -+\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n -+\t pack_name[len - 1] != '.')\n -+\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n -+\t\t pack_name, strip);\n -+\tstrbuf_add(buf, pack_name, len);\n -+\tstrbuf_addstr(buf, suffix);\n -+\treturn buf->buf;\n -+}\n -+\n - static unsigned int pack_used_ctr;\n - static unsigned int pack_mmap_calls;\n - static unsigned int peak_pack_open_windows;\n -@@ packfile.c: struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n - \t */\n - \tp->pack_size = st.st_size;\n - \tp->pack_local = local;\n -+\n -+\t/* If we have different file used to freshen the mtime, we should\n -+\t * use it at a higher priority.\n -+\t */\n -+\tif (!!strcasecmp(pack_mtime_suffix, \"pack\")) {\n -+\t\tstruct strbuf name_buf = STRBUF_INIT;\n -+\t\tconst char *filename;\n -+\n -+\t\tfilename = derive_pack_filename(path, \"idx\", pack_mtime_suffix, &name_buf);\n -+\t\tstat(filename, &st);\n -+\t}\n - \tp->mtime = st.st_mtime;\n - \tif (path_len < the_hash_algo->hexsz ||\n - \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n -\n - ## packfile.h ##\n -@@ packfile.h: char *sha1_pack_name(const unsigned char *sha1);\n - */\n - char *sha1_pack_index_name(const unsigned char *sha1);\n - \n -+/*\n -+ * Return the corresponding filename with given suffix from \"file_name\"\n -+ * which must has \"strip\" suffix.\n -+ */\n -+const char *derive_pack_filename(const char *file_name, const char *strip,\n -+\t\tconst char *suffix, struct strbuf *buf);\n -+\n - /*\n - * Return the basename of the packfile, omitting any containing directory\n - * (e.g., \"pack-1234abcd[...].pack\").\n \n ## t/t7701-repack-unpack-unreachable.sh ##\n @@ t/t7701-repack-unpack-unreachable.sh: test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n -+test_expect_success 'do not bother loosening old objects with core.packmtimesuffix config' '\n ++test_expect_success 'do not bother loosening old objects without freshen pack time' '\n +\tobj1=$(echo three | git hash-object -w --stdin) &&\n +\tobj2=$(echo four | git hash-object -w --stdin) &&\n -+\tpack1=$(echo $obj1 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n -+\tpack2=$(echo $obj2 | git -c core.packmtimesuffix=bump pack-objects .git/objects/pack/pack) &&\n -+\tgit -c core.packmtimesuffix=bump prune-packed &&\n ++\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n ++\tgit -c core.freshenPackFiles=false prune-packed &&\n +\tgit cat-file -p $obj1 &&\n +\tgit cat-file -p $obj2 &&\n -+\ttouch .git/objects/pack/pack-$pack2.bump &&\n -+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n -+\tgit -c core.packmtimesuffix=bump repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n ++\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n +\tgit cat-file -p $obj1 &&\n +\ttest_must_fail git cat-file -p $obj2\n +'\n\n\n Documentation/config/core.txt | 11 +++++++++++\n cache.h | 1 +\n config.c | 4 ++++\n environment.c | 1 +\n object-file.c | 4 ++++\n t/t7701-repack-unpack-unreachable.sh | 14 ++++++++++++++\n 6 files changed, 35 insertions(+)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex c04f62a54a1..1e7cf366628 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -398,6 +398,17 @@ the largest projects. You probably do not need to adjust this value.\n +\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \n+core.freshenPackFiles::\n+\tNormally we avoid writing existing object by freshening the mtime\n+\tof the *.pack file which contains it in order to aid some processes\n+\tsuch like prune. Turning off this option on some servers can speed\n+\tup the execution of some commands like 'git-upload-pack'(e.g. some\n+\tservers that mount the same NFS disk will re-sync the *.pack files\n+\tto cached file system if the mtime cahnges).\n++\n+The default is true which means the *.pack file will be freshened if we\n+want to write a existing object whthin it.\n+\n core.deltaBaseCacheLimit::\n \tMaximum number of bytes per thread to reserve for caching base objects\n \tthat may be referenced by multiple deltified objects. By storing the\ndiff --git a/cache.h b/cache.h\nindex ba04ff8bd36..46126c6977c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -956,6 +956,7 @@ extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n extern unsigned long big_file_threshold;\n extern unsigned long pack_size_limit_cfg;\n+extern int core_freshen_packfiles;\n \n /*\n * Accessors for the core.sharedrepository config which lazy-load the value\ndiff --git a/config.c b/config.c\nindex f9c400ad306..02dcc8a028e 100644\n--- a/config.c\n+++ b/config.c\n@@ -1431,6 +1431,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.freshenpackfiles\")) {\n+\t\tcore_freshen_packfiles = git_config_bool(var, value);\n+\t}\n+\n \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n \t\tdelta_base_cache_limit = git_config_ulong(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 2f27008424a..397525609a8 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -73,6 +73,7 @@ int core_sparse_checkout_cone;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n+int core_freshen_packfiles = 1;\n enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n \n #ifndef PROTECT_HFS_DEFAULT\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..884c3e92c38 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1974,6 +1974,10 @@ static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n+\n+\tif (!core_freshen_packfiles)\n+\t\treturn 1;\n+\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex 937f89ee8c8..b6a0b6c9695 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -112,6 +112,20 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'do not bother loosening old objects without freshen pack time' '\n+\tobj1=$(echo three | git hash-object -w --stdin) &&\n+\tobj2=$(echo four | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n+\tgit -c core.freshenPackFiles=false prune-packed &&\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n+\tgit cat-file -p $obj1 &&\n+\ttest_must_fail git cat-file -p $obj2\n+'\n+\n test_expect_success 'keep packed objects found only in index' '\n \techo my-unique-content >file &&\n \tgit add file &&\n\nbase-commit: 75ae10bc75336db031ee58d13c5037b929235912\n-- \ngitgitgadget\n"},{"id":"430548","messageId":"YPXluqywHs3u4Qr+@nand.local","threadId":"56088","inReplyTo":"pull.1043.v3.git.git.1626724399377.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T20:51:06Z","receivedAt":"2021-07-19T23:11:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 19, 2021 at 07:53:19PM +0000, Sun Chao via GitGitGadget wrote:\n> From: Sun Chao <16657101987@163.com>\n>\n> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n> 2014-10-15) avoid writing existing objects by freshen their\n> mtime (especially the packfiles contains them) in order to\n> aid the correct caching, and some process like find_lru_pack\n> can make good decision. However, this is unfriendly to\n> incremental backup jobs or services rely on cached file system\n> when there are large '.pack' files exists.\n>\n> For example, after packed all objects, use 'write-tree' to\n> create same commit with the same tree and same environments\n> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n> notice the '.pack' file's mtime changed. Git servers\n> that mount the same NFS disk will re-sync the '.pack' files\n> to cached file system which will slow the git commands.\n>\n> So if add core.freshenPackfiles to indicate whether or not\n> packs can be freshened, turning off this option on some\n> servers can speed up the execution of some commands on servers\n> which use NFS disk instead of local disk.\n\nHmm. I'm still quite unconvinced that we should be taking this direction\nwithout better motivation. We talked about your assumption that NFS\nseems to be invalidating the block cache when updating the inodes that\npoint at those blocks, but I don't recall seeing further evidence.\n\nRegardless, a couple of idle thoughts:\n\n> +\tif (!core_freshen_packfiles)\n> +\t\treturn 1;\n\nIt is important to still freshen the object mtimes even when we cannot\nupdate the pack mtimes. That's why we return 0 when \"freshen_file\"\nreturned 0: even if there was an error calling utime, we should still\nfreshen the object. This is important because it impacts when\nunreachable objects are pruned.\n\nSo I would have assumed that if a user set \"core.freshenPackfiles=false\"\nthat they would still want their object mtimes updated, in which case\nthe only option we have is to write those objects out loose.\n\n...and that happens by the caller of freshen_packed_object (either\nwrite_object_file() or hash_object_file_literally()) then calling\nwrite_loose_object() if freshen_packed_object() failed. So I would have\nexpected to see a \"return 0\" in the case that packfile freshening was\ndisabled.\n\nBut that leads us to an interesting problem: how many redundant objects\ndo we expect to see on the server? It may be a lot, in which case you\nmay end up having the same IO problems for a different reason. Peff\nmentioned to me off-list that he suspected write-tree was overeager in\nhow many trees it would try to write out. I'm not sure.\n\n> +test_expect_success 'do not bother loosening old objects without freshen pack time' '\n> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n> +\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n> +\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n> +\tgit -c core.freshenPackFiles=false prune-packed &&\n> +\tgit cat-file -p $obj1 &&\n> +\tgit cat-file -p $obj2 &&\n> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n> +\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n> +\tgit cat-file -p $obj1 &&\n> +\ttest_must_fail git cat-file -p $obj2\n> +'\n\nI had a little bit of a hard time following this test. AFAICT, it\nproceeds as follows:\n\n - Write two packs, each containing a unique unreachable blob.\n - Call 'git prune-packed' with packfile freshening disabled, then\n check that the object survived.\n - Then repack while in a state where one of the pack's contents would\n be pruned.\n - Make sure that one object survives and the other doesn't.\n\nThis doesn't really seem to be testing the behavior of disabling\npackfile freshening so much as it's testing prune-packed, and repack's\n`--unpack-unreachable` option. I would probably have expected to see\nsomething more along the lines of:\n\n - Write an unreachable object, pack it, and then remove the loose copy\n you wrote in the first place.\n - Then roll the pack's mtime to some fixed value in the past.\n - Try to write the same object again with packfile freshening\n disabled, and verify that:\n - the pack's mtime was unchanged,\n - the object exists loose again\n\nBut I'd really like to get some other opinions (especially from Peff,\nwho brought up the potential concerns with write-tree) as to whether or\nnot this is a direction worth pursuing.\n\nMy opinion is that it is not, and that the bizarre caching behavior you\nare seeing is out of Git's control.\n\nThanks,\nTaylor\n"},{"id":"430562","messageId":"xmqqlf61j19i.fsf@gitster.g","threadId":"56088","inReplyTo":"YPXluqywHs3u4Qr+@nand.local","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-20T00:07:53Z","receivedAt":"2021-07-20T02:08:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Hmm. I'm still quite unconvinced that we should be taking this direction\n> without better motivation. We talked about your assumption that NFS\n> seems to be invalidating the block cache when updating the inodes that\n> point at those blocks, but I don't recall seeing further evidence.\n\nMe neither. Not touching the pack and not updating the \"most\nrecently used\" time of individual objects smells like a recipe\nfor repository corruption.\n\n> My opinion is that it is not, and that the bizarre caching behavior you\n> are seeing is out of Git's control.\n\n"},{"id":"430564","messageId":"87bl6xwlaz.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"YPXluqywHs3u4Qr+@nand.local","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T06:19:17Z","receivedAt":"2021-07-20T06:28:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 19 2021, Taylor Blau wrote:\n\n> On Mon, Jul 19, 2021 at 07:53:19PM +0000, Sun Chao via GitGitGadget wrote:\n>> From: Sun Chao <16657101987@163.com>\n>>\n>> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n>> 2014-10-15) avoid writing existing objects by freshen their\n>> mtime (especially the packfiles contains them) in order to\n>> aid the correct caching, and some process like find_lru_pack\n>> can make good decision. However, this is unfriendly to\n>> incremental backup jobs or services rely on cached file system\n>> when there are large '.pack' files exists.\n>>\n>> For example, after packed all objects, use 'write-tree' to\n>> create same commit with the same tree and same environments\n>> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n>> notice the '.pack' file's mtime changed. Git servers\n>> that mount the same NFS disk will re-sync the '.pack' files\n>> to cached file system which will slow the git commands.\n>>\n>> So if add core.freshenPackfiles to indicate whether or not\n>> packs can be freshened, turning off this option on some\n>> servers can speed up the execution of some commands on servers\n>> which use NFS disk instead of local disk.\n>\n> Hmm. I'm still quite unconvinced that we should be taking this direction\n> without better motivation. We talked about your assumption that NFS\n> seems to be invalidating the block cache when updating the inodes that\n> point at those blocks, but I don't recall seeing further evidence.\n\nI don't know about Sun's setup, but what he's describing is consistent\nwith how NFS works, or can commonly be made to work.\n\nSee e.g. \"lookupcache\" in nfs(5) on Linux, but also a lot of people use\nsome random vendor's proprietary NFS implementation, and commonly tweak\nvarious options that make it anywhere between \"I guess that's not too\ncrazy\" and \"are you kidding me?\" levels of non-POSIX compliant.\n\n> Regardless, a couple of idle thoughts:\n>\n>> +\tif (!core_freshen_packfiles)\n>> +\t\treturn 1;\n>\n> It is important to still freshen the object mtimes even when we cannot\n> update the pack mtimes. That's why we return 0 when \"freshen_file\"\n> returned 0: even if there was an error calling utime, we should still\n> freshen the object. This is important because it impacts when\n> unreachable objects are pruned.\n>\n> So I would have assumed that if a user set \"core.freshenPackfiles=false\"\n> that they would still want their object mtimes updated, in which case\n> the only option we have is to write those objects out loose.\n>\n> ...and that happens by the caller of freshen_packed_object (either\n> write_object_file() or hash_object_file_literally()) then calling\n> write_loose_object() if freshen_packed_object() failed. So I would have\n> expected to see a \"return 0\" in the case that packfile freshening was\n> disabled.\n>\n> But that leads us to an interesting problem: how many redundant objects\n> do we expect to see on the server? It may be a lot, in which case you\n> may end up having the same IO problems for a different reason. Peff\n> mentioned to me off-list that he suspected write-tree was overeager in\n> how many trees it would try to write out. I'm not sure.\n\nIn my experience with NFS the thing that kills you is anything that\nneeds to do iterations, i.e. recursive readdir() and the like, or to\nread a lot of data, throughput was excellent. It's why I hacked core\nthat core.checkCollisions patch.\n\nJeff improved the situation I was mainly trying to fix with with the\nloose objects cache. I never got around to benchmarking the two in\nproduction, and now that setup is at an ex-job...\n\n>> +test_expect_success 'do not bother loosening old objects without freshen pack time' '\n>> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n>> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n>> +\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>> +\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>> +\tgit -c core.freshenPackFiles=false prune-packed &&\n>> +\tgit cat-file -p $obj1 &&\n>> +\tgit cat-file -p $obj2 &&\n>> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n>> +\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n>> +\tgit cat-file -p $obj1 &&\n>> +\ttest_must_fail git cat-file -p $obj2\n>> +'\n>\n> I had a little bit of a hard time following this test. AFAICT, it\n> proceeds as follows:\n>\n> - Write two packs, each containing a unique unreachable blob.\n> - Call 'git prune-packed' with packfile freshening disabled, then\n> check that the object survived.\n> - Then repack while in a state where one of the pack's contents would\n> be pruned.\n> - Make sure that one object survives and the other doesn't.\n>\n> This doesn't really seem to be testing the behavior of disabling\n> packfile freshening so much as it's testing prune-packed, and repack's\n> `--unpack-unreachable` option. I would probably have expected to see\n> something more along the lines of:\n>\n> - Write an unreachable object, pack it, and then remove the loose copy\n> you wrote in the first place.\n> - Then roll the pack's mtime to some fixed value in the past.\n> - Try to write the same object again with packfile freshening\n> disabled, and verify that:\n> - the pack's mtime was unchanged,\n> - the object exists loose again\n>\n> But I'd really like to get some other opinions (especially from Peff,\n> who brought up the potential concerns with write-tree) as to whether or\n> not this is a direction worth pursuing.\n>\n> My opinion is that it is not, and that the bizarre caching behavior you\n> are seeing is out of Git's control.\n\nThanks for this, I found the test hard to follow too, but didn't have\ntime to really think about it, this makes sense.\n\nBack to the topic: I share your sentiment of trying to avoid complexity\nin this area.\n\nSun: Have you considered --keep-unreachable to \"git repack\"? It's\northagonal to what you're trying here, but I wonder if being more\naggressive about keeping objects + some impromevents to perhaps skip\nthis \"touch\" at all if we have that in effect wouldn't be more viable &\nsomething e.g. Taylor would be more comforable having part of git.git.\n"},{"id":"430566","messageId":"878s21wl4z.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"CAL3xRKee3YmOrV_-4Tu6FmJyRnS2y-tdiAmXp5TjzL_WxQNrtw@mail.gmail.com","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T06:29:17Z","receivedAt":"2021-07-20T06:32:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 15 2021, Son Luong Ngoc wrote:\n\n> Hi folks,\n>\n> On Wed, Jul 14, 2021 at 10:03 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> *nod*\n>>\n>> FWIW at an ex-job I helped systems administrators who'd produced such a\n>> broken backup-via-rsync create a hybrid version as an interim\n>> solution. I.e. it would sync the objects via git transport, and do an\n>> rsync on a whitelist (or blacklist), so pickup config, but exclude\n>> objects.\n>>\n>> \"Hybrid\" because it was in a state of needing to deal with manual\n>> tweaking of config.\n>>\n>> But usually someone who's needing to thoroughly solve this backup\n>> problem will inevitably end up with wanting to drive everything that's\n>> not in the object or refstore from some external system, i.e. have\n>> config be generated from puppet, a database etc., ditto for alternates\n>> etc.\n>>\n>> But even if you can't get to that point (or don't want to) I'd say aim\n>> for the hybrid system.\n>\n> FWIW, we are running our repo on top of a some-what flickery DRBD setup and\n> we decided to use both\n>\n> git clone --upload-pack 'git -c transfer.hiderefs=\"!refs\"\n> upload-pack' --mirror`\n>\n> and\n>\n> `tar`\n>\n> to create 2 separate snapshots for backup in parallel (full backup,\n> not incremental).\n>\n> In case of recovery (manual), we first rely on the git snapshot and if\n> there is any\n> missing objects/refs, we will try to get it from the tarball.\n\nThat sounds good, and similar to what I described with that \"hybrid\"\nsetup.\n\n>>\n>> This isn't some purely theoretical concern b.t.w., the system using\n>> rsync like this was producing repos that wouldn't fsck all the time, and\n>> it wasn't such a busy site.\n>>\n>> I suspect (but haven't tried) that for someone who can't easily change\n>> their backup solution they'd get most of the benefits of git-native\n>> transport by having their \"rsync\" sync refs, then objects, not the other\n>> way around. Glob order dictates that most backup systems will do\n>> objects, then refs (which will of course, at that point, refer to\n>> nonexisting objects).\n>>\n>> It's still not safe, you'll still be subject to races, but probably a\n>> lot better in practice.\n>\n> I would love to get some guidance in official documentation on what is the best\n> practice around handling git data on the server side.\n>\n> Is git-clone + git-bundle the go-to solution?\n> Should tar/rsync not be used completely or is there a trade-off?\n\nI should have tempered some of those comments, it's perfectly fine in\ngeneral to use tar+rsync for \"backing up\" git repositories in certain\ncontexts. E.g. when I switch laptops or whatever it's what I do to grab\ndata.\n\nThe problem is when the data isn't at rest, i.e. in the context of an\nactive server.\n\nThere you start moving towards a scale where it goes from \"sure, it's\nfine\" to \"this is such a bad idea that nobody should pursue it\".\n\nIf you're running a setup where you're starting to submit patches to\ngit.git you're probably at the far end of that spectrum.\n\nWhether it's clone, push, fetch, bundle etc. doesn't really matter, the\nimportant part is that you're using git's pack transport mechanism to\nferry updates around, which gives you guarantees rsync+tar can't,\nparticularly in the face of concurrently updated data.\n\n"},{"id":"430568","messageId":"875yx5wkt2.fsf@evledraar.gmail.com","threadId":"56088","inReplyTo":"3112447.ymCj9SdLpg@mfick-lnx","subject":"Re: [PATCH v2] packfile: freshen the mtime of packfile by configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T06:32:35Z","receivedAt":"2021-07-20T06:40:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Martin Fick wrote:\n\n> On Wednesday, July 14, 2021 9:41:42 PM MDT you wrote:\n>> On Wed, Jul 14 2021, Martin Fick wrote:\n>> > On Wednesday, July 14, 2021 8:19:15 PM MDT Ævar Arnfjörð Bjarmason wrote:\n>> >> The best way to get backups of git repositories you know are correct are\n>> >> is to use git's own transport mechanisms, i.e. fetch/pull the data, or\n>> >> create bundles from it.\n>> > \n>> > I don't think this is a fair recommendation since unfortunately, this\n>> > cannot be used to create a full backup. This can be used to back up the\n>> > version controlled data, but not the repositories meta-data, i.e.\n>> > configs, reflogs, alternate setups...\n>> \n>> *nod*\n>> \n>> FWIW at an ex-job I helped systems administrators who'd produced such a\n>> broken backup-via-rsync create a hybrid version as an interim\n>> solution. I.e. it would sync the objects via git transport, and do an\n>> rsync on a whitelist (or blacklist), so pickup config, but exclude\n>> objects.\n>> \n>> \"Hybrid\" because it was in a state of needing to deal with manual\n>> tweaking of config.\n>> \n>> But usually someone who's needing to thoroughly solve this backup\n>> problem will inevitably end up with wanting to drive everything that's\n>> not in the object or refstore from some external system, i.e. have\n>> config be generated from puppet, a database etc., ditto for alternates\n>> etc.\n>> \n>> But even if you can't get to that point (or don't want to) I'd say aim\n>> for the hybrid system.\n>> \n>> This isn't some purely theoretical concern b.t.w., the system using\n>> rsync like this was producing repos that wouldn't fsck all the time, and\n>> it wasn't such a busy site.\n>> \n>> I suspect (but haven't tried) that for someone who can't easily change\n>> their backup solution they'd get most of the benefits of git-native\n>> transport by having their \"rsync\" sync refs, then objects, not the other\n>> way around. Glob order dictates that most backup systems will do\n>> objects, then refs (which will of course, at that point, refer to\n>> nonexisting objects).\n>> \n>> It's still not safe, you'll still be subject to races, but probably a\n>> lot better in practice.\n>\n> It would be great if git provided a command to do a reliable incremental \n> backup, maybe it could copy things in the order that you mention?\n\nI don't think we can or want to support this sort of thing ever, for the\nsame reason that you probably won't convince MySQL,PostgreSQL etc. that\nthey should support \"cp -r\" as a mode for backing up their live database\nservices.\n\nI mean, there is the topic of git being lazy about fsync() etc, but even\nif all of that were 100% solved you'd still get bad things if you picked\nan arbitrary time to snapshot a running git directory, e.g. your\n\"master\" branch might have a \"master.lock\" because it was in the middle\nof an update.\n\nIf you used \"fetch/clone/bundle\" etc. to get the data no problem, but if\nyour snapshot happens then you'd need to manually clean that up, a\nsituation which in practice wouldn't persist, but would be persistent\nwith a snapshot approach.\n\n> However, most people will want to use the backup system they have and not a \n> special git tool. Maybe git fsck should gain a switch that would rewind any \n> refs to an older point that is no broken (using reflogs)? That way, most \n> backups would just work and be rewound to the point at which the backup \n> started?\n\nI think the main problem in the wild is not the inability of using a\nspecial tool, but one of education. Most people wouldn't think of \"cp\n-r\" as a first approach to say backing up a live mysql server, they'd\nuse mysqldump and the like.\n\nBut for some reason git is considered \"not a database\" enough that those\nsame people would just use rsync/tar/whatever, and are then surprised\nwhen their data is corrupt or in some weird or inconsistent state...\n\nAnyway, see also my just-posted:\nhttps://lore.kernel.org/git/878s21wl4z.fsf@evledraar.gmail.com/\n\nI.e. I'm not saying \"never use rsync\", there's cases where that's fine,\nbut for a live \"real\" server I'd say solutions in that class shouldn't\nbe considered/actively migrated away from.\n"},{"id":"430631","messageId":"5D8CDAF1-256C-4CC3-920C-2063CFACE9BD@163.com","threadId":"56088","inReplyTo":"YPXluqywHs3u4Qr+@nand.local","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-20T15:00:18Z","receivedAt":"2021-07-20T15:46:32Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月20日 04:51，Taylor Blau 写道：\n> \n> On Mon, Jul 19, 2021 at 07:53:19PM +0000, Sun Chao via GitGitGadget wrote:\n>> From: Sun Chao <16657101987@163.com>\n>> \n>> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n>> 2014-10-15) avoid writing existing objects by freshen their\n>> mtime (especially the packfiles contains them) in order to\n>> aid the correct caching, and some process like find_lru_pack\n>> can make good decision. However, this is unfriendly to\n>> incremental backup jobs or services rely on cached file system\n>> when there are large '.pack' files exists.\n>> \n>> For example, after packed all objects, use 'write-tree' to\n>> create same commit with the same tree and same environments\n>> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n>> notice the '.pack' file's mtime changed. Git servers\n>> that mount the same NFS disk will re-sync the '.pack' files\n>> to cached file system which will slow the git commands.\n>> \n>> So if add core.freshenPackfiles to indicate whether or not\n>> packs can be freshened, turning off this option on some\n>> servers can speed up the execution of some commands on servers\n>> which use NFS disk instead of local disk.\n> \n> Hmm. I'm still quite unconvinced that we should be taking this direction\n> without better motivation. We talked about your assumption that NFS\n> seems to be invalidating the block cache when updating the inodes that\n> point at those blocks, but I don't recall seeing further evidence.\n\nYes, these days I'm trying to asking help from our SRE to do tests in our\nproduction environments (where we can get the real traffic report of NFS server),\nsuch like:\n\n- setup a repository witch some large packfiles in a NFS disk\n- keep running 'git pack-objects --all --stdout --delta-base-offset >/dev/null' in\nmultiple git servers that mount the same NFS disk above\n- 'touch' the packfiles in another server, and check (a) if the IOPS and IO traffic\nof the NFS server changes and (b) if the IO traffic of network interfaces from\nthe git servers to the NFS server changes\n\nI like to share the data when I receive the reports.\n\nMeanwhile I find the description of the cached file system for NFS Client:\n https://www.ibm.com/docs/en/aix/7.2?topic=performance-cache-file-system\nIt is mentioned that:\n\n 3. To ensure that the cached directories and files are kept up to date, \n CacheFS periodically checks the consistency of files stored in the cache.\n It does this by comparing the current modification time to the previous\n modification time.\n 4. If the modification times are different, all data and attributes\n for the directory or file are purged from the cache, and new data and\n attributes are retrieved from the back file system.\n\nIt looks like reasonable, but perhaps I should check it from my test reports ;)\n\n> \n> Regardless, a couple of idle thoughts:\n> \n>> +\tif (!core_freshen_packfiles)\n>> +\t\treturn 1;\n> \n> It is important to still freshen the object mtimes even when we cannot\n> update the pack mtimes. That's why we return 0 when \"freshen_file\"\n> returned 0: even if there was an error calling utime, we should still\n> freshen the object. This is important because it impacts when\n> unreachable objects are pruned.\n> \n> So I would have assumed that if a user set \"core.freshenPackfiles=false\"\n> that they would still want their object mtimes updated, in which case\n> the only option we have is to write those objects out loose.\n> \n> ...and that happens by the caller of freshen_packed_object (either\n> write_object_file() or hash_object_file_literally()) then calling\n> write_loose_object() if freshen_packed_object() failed. So I would have\n> expected to see a \"return 0\" in the case that packfile freshening was\n> disabled.\n> \n> But that leads us to an interesting problem: how many redundant objects\n> do we expect to see on the server? It may be a lot, in which case you\n> may end up having the same IO problems for a different reason. Peff\n> mentioned to me off-list that he suspected write-tree was overeager in\n> how many trees it would try to write out. I'm not sure.\n\nYou are right, I haven't thought it in details, if we do not update the mtime\nboth of packfiles and loose files, 'prune' may delete them by accident.\n\n> \n>> +test_expect_success 'do not bother loosening old objects without freshen pack time' '\n>> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n>> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n>> +\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>> +\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>> +\tgit -c core.freshenPackFiles=false prune-packed &&\n>> +\tgit cat-file -p $obj1 &&\n>> +\tgit cat-file -p $obj2 &&\n>> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n>> +\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n>> +\tgit cat-file -p $obj1 &&\n>> +\ttest_must_fail git cat-file -p $obj2\n>> +'\n> \n> I had a little bit of a hard time following this test. AFAICT, it\n> proceeds as follows:\n> \n> - Write two packs, each containing a unique unreachable blob.\n> - Call 'git prune-packed' with packfile freshening disabled, then\n> check that the object survived.\n> - Then repack while in a state where one of the pack's contents would\n> be pruned.\n> - Make sure that one object survives and the other doesn't.\n> \n> This doesn't really seem to be testing the behavior of disabling\n> packfile freshening so much as it's testing prune-packed, and repack's\n> `--unpack-unreachable` option. I would probably have expected to see\n> something more along the lines of:\n> \n> - Write an unreachable object, pack it, and then remove the loose copy\n> you wrote in the first place.\n> - Then roll the pack's mtime to some fixed value in the past.\n> - Try to write the same object again with packfile freshening\n> disabled, and verify that:\n> - the pack's mtime was unchanged,\n> - the object exists loose again\n> \n> But I'd really like to get some other opinions (especially from Peff,\n> who brought up the potential concerns with write-tree) as to whether or\n> not this is a direction worth pursuing.\n> \n> My opinion is that it is not, and that the bizarre caching behavior you\n> are seeing is out of Git's control.\nOK, I will try this. And I will try to get the test reports from our SRE and\ncheck how the mtime impacts the caches. \n\n> \n> Thanks,\n> Taylor\n\n"},{"id":"430632","messageId":"29757455-B195-49CD-8185-55B64DCEC1D1@163.com","threadId":"56088","inReplyTo":"87bl6xwlaz.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-20T15:34:25Z","receivedAt":"2021-07-20T15:47:02Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月20日 14:19，Ævar Arnfjörð Bjarmason 写道：\n> \n> \n> On Mon, Jul 19 2021, Taylor Blau wrote:\n> \n>> On Mon, Jul 19, 2021 at 07:53:19PM +0000, Sun Chao via GitGitGadget wrote:\n>>> From: Sun Chao <16657101987@163.com>\n>>> \n>>> Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n>>> 2014-10-15) avoid writing existing objects by freshen their\n>>> mtime (especially the packfiles contains them) in order to\n>>> aid the correct caching, and some process like find_lru_pack\n>>> can make good decision. However, this is unfriendly to\n>>> incremental backup jobs or services rely on cached file system\n>>> when there are large '.pack' files exists.\n>>> \n>>> For example, after packed all objects, use 'write-tree' to\n>>> create same commit with the same tree and same environments\n>>> such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n>>> notice the '.pack' file's mtime changed. Git servers\n>>> that mount the same NFS disk will re-sync the '.pack' files\n>>> to cached file system which will slow the git commands.\n>>> \n>>> So if add core.freshenPackfiles to indicate whether or not\n>>> packs can be freshened, turning off this option on some\n>>> servers can speed up the execution of some commands on servers\n>>> which use NFS disk instead of local disk.\n>> \n>> Hmm. I'm still quite unconvinced that we should be taking this direction\n>> without better motivation. We talked about your assumption that NFS\n>> seems to be invalidating the block cache when updating the inodes that\n>> point at those blocks, but I don't recall seeing further evidence.\n> \n> I don't know about Sun's setup, but what he's describing is consistent\n> with how NFS works, or can commonly be made to work.\n> \n> See e.g. \"lookupcache\" in nfs(5) on Linux, but also a lot of people use\n> some random vendor's proprietary NFS implementation, and commonly tweak\n> various options that make it anywhere between \"I guess that's not too\n> crazy\" and \"are you kidding me?\" levels of non-POSIX compliant.\n> \n>> Regardless, a couple of idle thoughts:\n>> \n>>> +\tif (!core_freshen_packfiles)\n>>> +\t\treturn 1;\n>> \n>> It is important to still freshen the object mtimes even when we cannot\n>> update the pack mtimes. That's why we return 0 when \"freshen_file\"\n>> returned 0: even if there was an error calling utime, we should still\n>> freshen the object. This is important because it impacts when\n>> unreachable objects are pruned.\n>> \n>> So I would have assumed that if a user set \"core.freshenPackfiles=false\"\n>> that they would still want their object mtimes updated, in which case\n>> the only option we have is to write those objects out loose.\n>> \n>> ...and that happens by the caller of freshen_packed_object (either\n>> write_object_file() or hash_object_file_literally()) then calling\n>> write_loose_object() if freshen_packed_object() failed. So I would have\n>> expected to see a \"return 0\" in the case that packfile freshening was\n>> disabled.\n>> \n>> But that leads us to an interesting problem: how many redundant objects\n>> do we expect to see on the server? It may be a lot, in which case you\n>> may end up having the same IO problems for a different reason. Peff\n>> mentioned to me off-list that he suspected write-tree was overeager in\n>> how many trees it would try to write out. I'm not sure.\n> \n> In my experience with NFS the thing that kills you is anything that\n> needs to do iterations, i.e. recursive readdir() and the like, or to\n> read a lot of data, throughput was excellent. It's why I hacked core\n> that core.checkCollisions patch.\nI have read your patch, looks like a good idea to reduce the expensive operations\nlike readdir(). And in my production environments, IOPS stress of the NFS server\nbothers me which make the git commands slow.\n\n> \n> Jeff improved the situation I was mainly trying to fix with with the\n> loose objects cache. I never got around to benchmarking the two in\n> production, and now that setup is at an ex-job...\n> \n>>> +test_expect_success 'do not bother loosening old objects without freshen pack time' '\n>>> +\tobj1=$(echo three | git hash-object -w --stdin) &&\n>>> +\tobj2=$(echo four | git hash-object -w --stdin) &&\n>>> +\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>>> +\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n>>> +\tgit -c core.freshenPackFiles=false prune-packed &&\n>>> +\tgit cat-file -p $obj1 &&\n>>> +\tgit cat-file -p $obj2 &&\n>>> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n>>> +\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n>>> +\tgit cat-file -p $obj1 &&\n>>> +\ttest_must_fail git cat-file -p $obj2\n>>> +'\n>> \n>> I had a little bit of a hard time following this test. AFAICT, it\n>> proceeds as follows:\n>> \n>> - Write two packs, each containing a unique unreachable blob.\n>> - Call 'git prune-packed' with packfile freshening disabled, then\n>> check that the object survived.\n>> - Then repack while in a state where one of the pack's contents would\n>> be pruned.\n>> - Make sure that one object survives and the other doesn't.\n>> \n>> This doesn't really seem to be testing the behavior of disabling\n>> packfile freshening so much as it's testing prune-packed, and repack's\n>> `--unpack-unreachable` option. I would probably have expected to see\n>> something more along the lines of:\n>> \n>> - Write an unreachable object, pack it, and then remove the loose copy\n>> you wrote in the first place.\n>> - Then roll the pack's mtime to some fixed value in the past.\n>> - Try to write the same object again with packfile freshening\n>> disabled, and verify that:\n>> - the pack's mtime was unchanged,\n>> - the object exists loose again\n>> \n>> But I'd really like to get some other opinions (especially from Peff,\n>> who brought up the potential concerns with write-tree) as to whether or\n>> not this is a direction worth pursuing.\n>> \n>> My opinion is that it is not, and that the bizarre caching behavior you\n>> are seeing is out of Git's control.\n> \n> Thanks for this, I found the test hard to follow too, but didn't have\n> time to really think about it, this makes sense.\n> \n> Back to the topic: I share your sentiment of trying to avoid complexity\n> in this area.\n> \n> Sun: Have you considered --keep-unreachable to \"git repack\"? It's\n> orthagonal to what you're trying here, but I wonder if being more\n> aggressive about keeping objects + some impromevents to perhaps skip\n> this \"touch\" at all if we have that in effect wouldn't be more viable &\n> something e.g. Taylor would be more comforable having part of git.git.\n\nYes, I will try to create some more useful test cases with both `--keep-unreachable`\nand `--unpack-unreachable` if I still believe I need to do something with\nthe mtime, whatever I need to get my test reports first, thanks ;)\n\n"},{"id":"430635","messageId":"60EFBDBF-8A9F-4004-82C1-C7C1E9D1778E@163.com","threadId":"56088","inReplyTo":"xmqqlf61j19i.fsf@gitster.g","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao","fromEmail":"16657101987@163.com","sentAt":"2021-07-20T15:07:09Z","receivedAt":"2021-07-20T16:08:59Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"\n\n> 2021年7月20日 08:07，Junio C Hamano 写道：\n> \n> Taylor Blau writes:\n> \n>> Hmm. I'm still quite unconvinced that we should be taking this direction\n>> without better motivation. We talked about your assumption that NFS\n>> seems to be invalidating the block cache when updating the inodes that\n>> point at those blocks, but I don't recall seeing further evidence.\n> \n> Me neither. Not touching the pack and not updating the \"most\n> recently used\" time of individual objects smells like a recipe\n> for repository corruption.\n> \n>> My opinion is that it is not, and that the bizarre caching behavior you\n>> are seeing is out of Git's control.\n> \nThanks Junio, I will try to get a more detial reports of the NFS caches and\nshare it if it is valuable. Not touching the mtime of packfiles really has\npotencial problems just as Taylor said.\n"},{"id":"430638","messageId":"YPb9rXLiLcl04k6d@nand.local","threadId":"56088","inReplyTo":"5D8CDAF1-256C-4CC3-920C-2063CFACE9BD@163.com","subject":"Re: [PATCH v3] packfile: freshen the mtime of packfile by configuration","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2021-07-20T16:53:33Z","receivedAt":"2021-07-20T16:54:12Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Tue, Jul 20, 2021 at 11:00:18PM +0800, Sun Chao wrote:\n> Meanwhile I find the description of the cached file system for NFS Client:\n> https://www.ibm.com/docs/en/aix/7.2?topic=performance-cache-file-system\n> It is mentioned that:\n>\n> 3. To ensure that the cached directories and files are kept up to date,\n> CacheFS periodically checks the consistency of files stored in the cache.\n> It does this by comparing the current modification time to the previous\n> modification time.\n> 4. If the modification times are different, all data and attributes\n> for the directory or file are purged from the cache, and new data and\n> attributes are retrieved from the back file system.\n>\n> It looks like reasonable, but perhaps I should check it from my test reports ;)\n\nThat seems reasonable, assuming that you have CacheFS mounted and that's\nwhat you're interacting with (instead of talking to NFS directly).\n\nIt seems reasonable that CacheFS would also have some way to tune how\noften the \"purge cached blocks because of stale mtimes\" would kick in,\nand what the grace period for determining if an mtime is \"stale\" is. So\nhopefully those values are (a) configurable and (b) you can find values\nthat result in acceptable performance.\n\nThanks,\nTaylor\n"},{"id":"432763","messageId":"81afc69d22c0c782eea80719557161ae19a4f72e.1629047327.git.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":"pull.1043.v4.git.git.1629047327.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] packfile: rename `derive_filename()` to `derive_pack_filename()`","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-15T17:08:46Z","receivedAt":"2021-08-15T17:08:54Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nIn order to allow some function get a new file name from `.pack` file\nwith a new suffix, move `derive_filename()` in `builtin/index-pack.c`\nto `packfile.c` with a new name `derive_pack_filename(), and export\nit from `packfile.h`.\n\nSigned-off-by: Sun Chao <16657101987@163.com>\n---\n builtin/index-pack.c | 19 +++----------------\n packfile.c | 13 +++++++++++++\n packfile.h | 7 +++++++\n 3 files changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8336466865c..3c83789ccef 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1435,19 +1435,6 @@ static void fix_unresolved_deltas(struct hashfile *f)\n \tfree(sorted_by_pos);\n }\n \n-static const char *derive_filename(const char *pack_name, const char *strip,\n-\t\t\t\t const char *suffix, struct strbuf *buf)\n-{\n-\tsize_t len;\n-\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n-\t pack_name[len - 1] != '.')\n-\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n-\t\t pack_name, strip);\n-\tstrbuf_add(buf, pack_name, len);\n-\tstrbuf_addstr(buf, suffix);\n-\treturn buf->buf;\n-}\n-\n static void write_special_file(const char *suffix, const char *msg,\n \t\t\t const char *pack_name, const unsigned char *hash,\n \t\t\t const char **report)\n@@ -1458,7 +1445,7 @@ static void write_special_file(const char *suffix, const char *msg,\n \tint msg_len = strlen(msg);\n \n \tif (pack_name)\n-\t\tfilename = derive_filename(pack_name, \"pack\", suffix, &name_buf);\n+\t\tfilename = derive_pack_filename(pack_name, \"pack\", suffix, &name_buf);\n \telse\n \t\tfilename = odb_pack_name(&name_buf, hash, suffix);\n \n@@ -1853,13 +1840,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (from_stdin && hash_algo)\n \t\tdie(_(\"--object-format cannot be used with --stdin\"));\n \tif (!index_name && pack_name)\n-\t\tindex_name = derive_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n+\t\tindex_name = derive_pack_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n \n \topts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);\n \tif (rev_index) {\n \t\topts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;\n \t\tif (index_name)\n-\t\t\trev_index_name = derive_filename(index_name,\n+\t\t\trev_index_name = derive_pack_filename(index_name,\n \t\t\t\t\t\t\t \"idx\", \"rev\",\n \t\t\t\t\t\t\t &rev_index_name_buf);\n \t}\ndiff --git a/packfile.c b/packfile.c\nindex 9ef6d982928..315c3da259a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -40,6 +40,19 @@ char *sha1_pack_index_name(const unsigned char *sha1)\n \treturn odb_pack_name(&buf, sha1, \"idx\");\n }\n \n+const char *derive_pack_filename(const char *pack_name, const char *strip,\n+\t\t\t\tconst char *suffix, struct strbuf *buf)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(pack_name, strip, &len) || !len ||\n+\t pack_name[len - 1] != '.')\n+\t\tdie(_(\"packfile name '%s' does not end with '.%s'\"),\n+\t\t pack_name, strip);\n+\tstrbuf_add(buf, pack_name, len);\n+\tstrbuf_addstr(buf, suffix);\n+\treturn buf->buf;\n+}\n+\n static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8aef..ff702b22e6a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -31,6 +31,13 @@ char *sha1_pack_name(const unsigned char *sha1);\n */\n char *sha1_pack_index_name(const unsigned char *sha1);\n \n+/*\n+ * Return the corresponding filename with given suffix from \"file_name\"\n+ * which must has \"strip\" suffix.\n+ */\n+const char *derive_pack_filename(const char *file_name, const char *strip,\n+\t\tconst char *suffix, struct strbuf *buf);\n+\n /*\n * Return the basename of the packfile, omitting any containing directory\n * (e.g., \"pack-1234abcd[...].pack\").\n-- \ngitgitgadget\n\n"},{"id":"432764","messageId":"pull.1043.v4.git.git.1629047327.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":"pull.1043.v3.git.git.1626724399377.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] packfile: freshen the mtime of packfile by configuration","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-15T17:08:45Z","receivedAt":"2021-08-15T17:08:54Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"packfile: freshen the mtime of packfile by bump file\n\nWe've talked about the cache reload through earlier patches, and we stopped\nbecause no further evidence can tell NFS client will reload the page caches\nif the file mtime changed. So our team have done these experiments:\n\nStep1: prepare git servers which mount the NFS disk and a big repo\n\nWe prepared 3 vms named c1, s1 and s2, we also have a NFS server named n1.\ns1 and s2 mount the NFS disk from n1 by:\n\n mount -t nfs -o vers=3,timeo=600,nolock,noatime,lookupcache=postive,\\\n actimeo=3 :/repositories /mnt/repositories\n\n\nWe setup git server services on s1 and s2, so we can clone repos from s1 by\ngit commands. Then we created a repository under /mnt/repositories, and\npushed large files to the repository, so we can find a large .pack file in\nthe repository with about 1.2 GB size.\n\nStep2: do first git clone from client after drop caches of s1\n\nFirst we drop the caches from s1 by:\n\n sync; echo 3 > /proc/sys/vm/drop_caches\n\n\nThen we run git command in c1 to clone the huge repository we created in\nStep1, at the same time we run the two commands in s1:\n\n tcpdump -nn host -w 1st_command.pcap\n nfsiostat 1 -p /mnt/repositories\n\n\ntry to get the result and check what happends.\n\nStep3: do new git clones without drop caches of s1\n\nAfter Step2, we called new git clone command in c1 to clone the huge\nrepository for serveral times, and also run the commands at the same time:\n\n tcpdump -nn host -w lots_of_command.pcap\n nfsiostat 1 -p /mnt/repositories\n\n\nStep4: do new git clones with packfile mtime changed\n\nAfter Step2 and Step3, we try to touch all the \".pack\" files from s2, and we\ncall a new git clone in c1 to download the huge repository again, and run\nthe two command in s1 at the same time:\n\n tcpdump -nn host -w mtime_changed_command.pcap\n nfsiostat 1 -p /mnt/repositories\n\n\nResult:\n\nWe got a about 1.4GB big pcap file during Step2 and Step4, we can find lots\nof READ request and response after open it with wireshark. And by\n'nfsiostat' command we can see the 'ops/s' and 'KB/s' of 'read' in the\noutput shows a relatively large value for a while.\n\nBut we got a 4MB pcap file in Step3, and open it with wireshark, we can only\nfind GETATTR and FSSTAT requests and response. And we the 'nfsiostat' always\nshow 0 in 'ops/s' and 'KB/s' of 'read' part in the output.\n\nWe have done Step1 to Step4 serveral times, each time the result are same.\n\nSo we can make sure the NFS client will reload the page cache if other NFS\nclient changes the mtime of the large .pack files. And for git servers which\nuse filesystem like NFS to manage large repositories, reload large files\nthat only have mtime changed result big NFS server IOPS pressure and that\nalso makes the git server slow because the IO is the bottleneck when there\nare too many client requests for the same big repositries.\n\nAnd I do think the team who manage the git servers need a configuration\nchoise which can enhance the mtime of packfile through another file which\nshould be small enough or even empty. It should be backward compatibility\nwhen it is in default value, but just as metioned by Ævar before, maybe\nsomepeople what to use it in mixed-version environment, we should warn them\nin documents, but such configuration do big help for some team who run some\nservers mount the NFS disks.\n\nSun Chao (2):\n packfile: rename `derive_filename()` to `derive_pack_filename()`\n packfile: freshen the mtime of packfile by bump file\n\n Documentation/config/core.txt | 11 +++\n builtin/index-pack.c | 19 +----\n cache.h | 1 +\n config.c | 5 ++\n environment.c | 1 +\n object-file.c | 30 +++++++-\n packfile.c | 25 ++++++-\n packfile.h | 7 ++\n t/t5326-pack-mtime-bumpfiles.sh | 118 ++++++++++++++++++++++++++++++++\n 9 files changed, 198 insertions(+), 19 deletions(-)\n create mode 100755 t/t5326-pack-mtime-bumpfiles.sh\n\n\nbase-commit: 5d213e46bb7b880238ff5ea3914e940a50ae9369\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1043%2Fsunchao9%2Fmaster-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1043/sunchao9/master-v4\nPull-Request: https://github.com/git/git/pull/1043\n\nRange-diff vs v3:\n\n -: ----------- > 1: 81afc69d22c packfile: rename `derive_filename()` to `derive_pack_filename()`\n 1: 16c68923bea ! 2: 7166f776154 packfile: freshen the mtime of packfile by configuration\n @@ Metadata\n Author: Sun Chao <16657101987@163.com>\n \n ## Commit message ##\n - packfile: freshen the mtime of packfile by configuration\n + packfile: freshen the mtime of packfile by bump file\n \n Commit 33d4221c79 (write_sha1_file: freshen existing objects,\n 2014-10-15) avoid writing existing objects by freshen their\n @@ Commit message\n create same commit with the same tree and same environments\n such like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\n notice the '.pack' file's mtime changed. Git servers\n - that mount the same NFS disk will re-sync the '.pack' files\n - to cached file system which will slow the git commands.\n + that use filesystems like NFS will reload the '.pack' files\n + to file system page cache, which will slow the git commands.\n \n - So if add core.freshenPackfiles to indicate whether or not\n - packs can be freshened, turning off this option on some\n - servers can speed up the execution of some commands on servers\n - which use NFS disk instead of local disk.\n + So if we freshen the mtime of packfile by updating a '.bump'\n + file instead, when we check the mtime of packfile, get it from\n + '.bump' file also. Large git repository may contains large\n + '.pack' files, but '.bump' files can be empty. This will avoid\n + file system page caches reload large files from NFS and then\n + make git commands faster.\n \n Signed-off-by: Sun Chao <16657101987@163.com>\n \n @@ Documentation/config/core.txt: the largest projects. You probably do not need t\n +\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \n -+core.freshenPackFiles::\n ++core.packMtimeToBumpFiles::\n +\tNormally we avoid writing existing object by freshening the mtime\n +\tof the *.pack file which contains it in order to aid some processes\n -+\tsuch like prune. Turning off this option on some servers can speed\n -+\tup the execution of some commands like 'git-upload-pack'(e.g. some\n -+\tservers that mount the same NFS disk will re-sync the *.pack files\n -+\tto cached file system if the mtime cahnges).\n ++\tsuch like prune. Use a *.bump file instead of *.pack file will\n ++\tavoid file system cache re-sync the large packfiles on filesystems\n ++\tlike NFS, and consequently make git commands faster.\n ++\n -+The default is true which means the *.pack file will be freshened if we\n -+want to write a existing object whthin it.\n ++The default is 'false' which means the *.pack file will be freshened by\n ++default. If set to 'true', the file with the '.bump' suffix will be\n ++created automatically, and it's mtime will be freshened instead.\n +\n core.deltaBaseCacheLimit::\n \tMaximum number of bytes per thread to reserve for caching base objects\n \tthat may be referenced by multiple deltified objects. By storing the\n \n ## cache.h ##\n -@@ cache.h: extern size_t packed_git_limit;\n +@@ cache.h: extern const char *git_hooks_path;\n + extern int zlib_compression_level;\n + extern int core_compression_level;\n + extern int pack_compression_level;\n ++extern int pack_mtime_to_bumpfiles;\n + extern size_t packed_git_window_size;\n + extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\n - extern unsigned long big_file_threshold;\n - extern unsigned long pack_size_limit_cfg;\n -+extern int core_freshen_packfiles;\n - \n - /*\n - * Accessors for the core.sharedrepository config which lazy-load the value\n \n ## config.c ##\n @@ config.c: static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n -+\tif (!strcmp(var, \"core.freshenpackfiles\")) {\n -+\t\tcore_freshen_packfiles = git_config_bool(var, value);\n ++\tif (!strcmp(var, \"core.packmtimetobumpfiles\")) {\n ++\t\tpack_mtime_to_bumpfiles = git_config_bool(var, value);\n ++\t\treturn 0;\n +\t}\n +\n \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n @@ config.c: static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \n ## environment.c ##\n -@@ environment.c: int core_sparse_checkout_cone;\n - int merge_log_config = -1;\n - int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n - unsigned long pack_size_limit_cfg;\n -+int core_freshen_packfiles = 1;\n - enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n - \n - #ifndef PROTECT_HFS_DEFAULT\n +@@ environment.c: const char *git_hooks_path;\n + int zlib_compression_level = Z_BEST_SPEED;\n + int core_compression_level;\n + int pack_compression_level = Z_DEFAULT_COMPRESSION;\n ++int pack_mtime_to_bumpfiles;\n + int fsync_object_files;\n + size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n + size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n \n ## object-file.c ##\n @@ object-file.c: static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n -+\n -+\tif (!core_freshen_packfiles)\n -+\t\treturn 1;\n ++\tstruct stat st;\n ++\tstruct strbuf name_buf = STRBUF_INIT;\n ++\tconst char *filename;\n +\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n + \t\treturn 1;\n +-\tif (!freshen_file(e.p->pack_name))\n +-\t\treturn 0;\n ++\n ++\tfilename = e.p->pack_name;\n ++\tif (!pack_mtime_to_bumpfiles) {\n ++\t\tif (!freshen_file(filename))\n ++\t\t\treturn 0;\n ++\t\te.p->freshened = 1;\n ++\t\treturn 1;\n ++\t}\n ++\n ++\tfilename = derive_pack_filename(filename, \"pack\", \"bump\", &name_buf);\n ++\tif (lstat(filename, &st) < 0) {\n ++\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n ++\t\tif (fd < 0) {\n ++\t\t\t// here we need to check it again because other git process may created it\n ++\t\t\tif (lstat(filename, &st) < 0)\n ++\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n ++\t\t} else {\n ++\t\t\tclose(fd);\n ++\t\t}\n ++\t} else {\n ++\t\tif (!freshen_file(filename))\n ++\t\t\treturn 0;\n ++\t}\n ++\n + \te.p->freshened = 1;\n + \treturn 1;\n + }\n \n - ## t/t7701-repack-unpack-unreachable.sh ##\n -@@ t/t7701-repack-unpack-unreachable.sh: test_expect_success 'do not bother loosening old objects' '\n - \ttest_must_fail git cat-file -p $obj2\n - '\n + ## packfile.c ##\n +@@ packfile.c: void close_object_store(struct raw_object_store *o)\n \n -+test_expect_success 'do not bother loosening old objects without freshen pack time' '\n -+\tobj1=$(echo three | git hash-object -w --stdin) &&\n -+\tobj2=$(echo four | git hash-object -w --stdin) &&\n -+\tpack1=$(echo $obj1 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n -+\tpack2=$(echo $obj2 | git -c core.freshenPackFiles=false pack-objects .git/objects/pack/pack) &&\n -+\tgit -c core.freshenPackFiles=false prune-packed &&\n + void unlink_pack_path(const char *pack_name, int force_delete)\n + {\n +-\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\"};\n ++\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".bump\"};\n + \tint i;\n + \tstruct strbuf buf = STRBUF_INIT;\n + \tsize_t plen;\n +@@ packfile.c: struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n + \tp->pack_size = st.st_size;\n + \tp->pack_local = local;\n + \tp->mtime = st.st_mtime;\n ++\n ++\tif (pack_mtime_to_bumpfiles) {\n ++\t\tstruct strbuf name_buf = STRBUF_INIT;\n ++\t\tconst char *filename;\n ++\n ++\t\tfilename = derive_pack_filename(path, \"idx\", \"bump\", &name_buf);\n ++\t\tif (!stat(filename, &st)) {\n ++\t\t\tp->mtime = st.st_mtime;\n ++\t\t}\n ++\t}\n + \tif (path_len < the_hash_algo->hexsz ||\n + \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n + \t\thashclr(p->hash);\n +\n + ## t/t5326-pack-mtime-bumpfiles.sh (new) ##\n +@@\n ++#!/bin/sh\n ++\n ++test_description='packfile mtime use bump files'\n ++. ./test-lib.sh\n ++\n ++if stat -c %Y . >/dev/null 2>&1; then\n ++ get_modified_time() { stat -c %Y \"$1\" 2>/dev/null; }\n ++elif stat -f %m . >/dev/null 2>&1; then\n ++ get_modified_time() { stat -f %m \"$1\" 2>/dev/null; }\n ++elif date -r . +%s >/dev/null 2>&1; then\n ++ get_modified_time() { date -r \"$1\" +%s 2>/dev/null; }\n ++else\n ++ echo 'get_modified_time() is unsupported' >&2\n ++ get_modified_time() { printf '%s' 0; }\n ++fi\n ++\n ++test_expect_success 'freshen existing packfile without core.packMtimeToBumpFiles' '\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack1.* &&\n ++\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack2.* &&\n ++\tpack1_mtime=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n ++\tpack2_mtime=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n ++\t(echo one | git hash-object -w --stdin) &&\n ++\t! test_path_exists .git/objects/pack/pack-$pack1.bump &&\n ++\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n ++\tpack1_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n ++\tpack2_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n ++\techo \"$pack1_mtime : $pack1_mtime_new\" &&\n ++\ttest ! \"$pack1_mtime\" = \"$pack1_mtime_new\" &&\n ++\ttest \"$pack2_mtime\" = \"$pack2_mtime_new\"\n ++\n ++'\n ++\n ++test_expect_success 'freshen existing packfile with core.packMtimeToBumpFiles' '\n ++\n ++\trm -rf .git/objects && git init &&\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack1.* &&\n ++\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack2.* &&\n ++\tpack1_mtime=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n ++\tpack2_mtime=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n ++\t(echo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin) &&\n ++\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n ++\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n ++\tpack1_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n ++\tpack2_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n ++\ttest \"$pack1_mtime\" = \"$pack1_mtime_new\" &&\n ++\ttest \"$pack2_mtime\" = \"$pack2_mtime_new\"\n ++\n ++'\n ++\n ++test_expect_success 'repack prune unreachable objects without core.packMtimeToBumpFiles' '\n ++\n ++\trm -rf .git/objects && git init &&\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\techo one | git hash-object -w --stdin &&\n ++\techo two | git hash-object -w --stdin &&\n ++\t! test_path_exists .git/objects/pack/pack-$pack1.bump &&\n ++\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n ++\tgit prune-packed &&\n +\tgit cat-file -p $obj1 &&\n +\tgit cat-file -p $obj2 &&\n +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n -+\tgit -c core.freshenPackFiles=false repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\tgit cat-file -p $obj1 &&\n ++\ttest_must_fail git cat-file -p $obj2\n ++\n ++'\n ++\n ++test_expect_success 'repack prune unreachable objects with core.packMtimeToBumpFiles and bump files' '\n ++\n ++\trm -rf .git/objects && git init &&\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\techo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n ++\techo two | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n ++\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n ++\ttest_path_exists .git/objects/pack/pack-$pack2.bump &&\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n ++\tgit -c core.packMtimeToBumpFiles=true repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\tgit cat-file -p $obj1 &&\n ++\tgit cat-file -p $obj2\n ++\n ++'\n ++\n ++test_expect_success 'repack prune unreachable objects with core.packMtimeToBumpFiles and old bump files' '\n ++\n ++\trm -rf .git/objects && git init &&\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\techo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n ++\techo two | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n ++\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n ++\ttest_path_exists .git/objects/pack/pack-$pack2.bump &&\n ++\tgit prune-packed &&\n ++\tgit cat-file -p $obj1 &&\n ++\tgit cat-file -p $obj2 &&\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n ++\tgit -c core.packMtimeToBumpFiles=true repack -A -d --unpack-unreachable=1.hour.ago &&\n +\tgit cat-file -p $obj1 &&\n +\ttest_must_fail git cat-file -p $obj2\n ++\n +'\n +\n - test_expect_success 'keep packed objects found only in index' '\n - \techo my-unique-content >file &&\n - \tgit add file &&\n ++test_done\n\n-- \ngitgitgadget\n"},{"id":"432765","messageId":"7166f77615442e511159be2d7ad2b3b46f40cbd7.1629047327.git.gitgitgadget@gmail.com","threadId":"56088","inReplyTo":"pull.1043.v4.git.git.1629047327.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] packfile: freshen the mtime of packfile by bump file","fromName":"Sun Chao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-15T17:08:47Z","receivedAt":"2021-08-15T17:08:55Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nCommit 33d4221c79 (write_sha1_file: freshen existing objects,\n2014-10-15) avoid writing existing objects by freshen their\nmtime (especially the packfiles contains them) in order to\naid the correct caching, and some process like find_lru_pack\ncan make good decision. However, this is unfriendly to\nincremental backup jobs or services rely on cached file system\nwhen there are large '.pack' files exists.\n\nFor example, after packed all objects, use 'write-tree' to\ncreate same commit with the same tree and same environments\nsuch like GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, we can\nnotice the '.pack' file's mtime changed. Git servers\nthat use filesystems like NFS will reload the '.pack' files\nto file system page cache, which will slow the git commands.\n\nSo if we freshen the mtime of packfile by updating a '.bump'\nfile instead, when we check the mtime of packfile, get it from\n'.bump' file also. Large git repository may contains large\n'.pack' files, but '.bump' files can be empty. This will avoid\nfile system page caches reload large files from NFS and then\nmake git commands faster.\n\nSigned-off-by: Sun Chao <16657101987@163.com>\n---\n Documentation/config/core.txt | 11 +++\n cache.h | 1 +\n config.c | 5 ++\n environment.c | 1 +\n object-file.c | 30 +++++++-\n packfile.c | 12 +++-\n t/t5326-pack-mtime-bumpfiles.sh | 118 ++++++++++++++++++++++++++++++++\n 7 files changed, 175 insertions(+), 3 deletions(-)\n create mode 100755 t/t5326-pack-mtime-bumpfiles.sh\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex c04f62a54a1..963d1b54e7e 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -398,6 +398,17 @@ the largest projects. You probably do not need to adjust this value.\n +\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \n+core.packMtimeToBumpFiles::\n+\tNormally we avoid writing existing object by freshening the mtime\n+\tof the *.pack file which contains it in order to aid some processes\n+\tsuch like prune. Use a *.bump file instead of *.pack file will\n+\tavoid file system cache re-sync the large packfiles on filesystems\n+\tlike NFS, and consequently make git commands faster.\n++\n+The default is 'false' which means the *.pack file will be freshened by\n+default. If set to 'true', the file with the '.bump' suffix will be\n+created automatically, and it's mtime will be freshened instead.\n+\n core.deltaBaseCacheLimit::\n \tMaximum number of bytes per thread to reserve for caching base objects\n \tthat may be referenced by multiple deltified objects. By storing the\ndiff --git a/cache.h b/cache.h\nindex bd4869beee4..a563cbacfa2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -960,6 +960,7 @@ extern const char *git_hooks_path;\n extern int zlib_compression_level;\n extern int core_compression_level;\n extern int pack_compression_level;\n+extern int pack_mtime_to_bumpfiles;\n extern size_t packed_git_window_size;\n extern size_t packed_git_limit;\n extern size_t delta_base_cache_limit;\ndiff --git a/config.c b/config.c\nindex f33abeab851..10ccf7c5581 100644\n--- a/config.c\n+++ b/config.c\n@@ -1431,6 +1431,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.packmtimetobumpfiles\")) {\n+\t\tpack_mtime_to_bumpfiles = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n \t\tdelta_base_cache_limit = git_config_ulong(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex d6b22ede7ea..5fa26cb3758 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -43,6 +43,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n+int pack_mtime_to_bumpfiles;\n int fsync_object_files;\n size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\ndiff --git a/object-file.c b/object-file.c\nindex a8be8994814..434073c17f1 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1994,12 +1994,38 @@ static int freshen_loose_object(const struct object_id *oid)\n static int freshen_packed_object(const struct object_id *oid)\n {\n \tstruct pack_entry e;\n+\tstruct stat st;\n+\tstruct strbuf name_buf = STRBUF_INIT;\n+\tconst char *filename;\n+\n \tif (!find_pack_entry(the_repository, oid, &e))\n \t\treturn 0;\n \tif (e.p->freshened)\n \t\treturn 1;\n-\tif (!freshen_file(e.p->pack_name))\n-\t\treturn 0;\n+\n+\tfilename = e.p->pack_name;\n+\tif (!pack_mtime_to_bumpfiles) {\n+\t\tif (!freshen_file(filename))\n+\t\t\treturn 0;\n+\t\te.p->freshened = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tfilename = derive_pack_filename(filename, \"pack\", \"bump\", &name_buf);\n+\tif (lstat(filename, &st) < 0) {\n+\t\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0664);\n+\t\tif (fd < 0) {\n+\t\t\t// here we need to check it again because other git process may created it\n+\t\t\tif (lstat(filename, &st) < 0)\n+\t\t\t\tdie_errno(\"unable to create '%s'\", filename);\n+\t\t} else {\n+\t\t\tclose(fd);\n+\t\t}\n+\t} else {\n+\t\tif (!freshen_file(filename))\n+\t\t\treturn 0;\n+\t}\n+\n \te.p->freshened = 1;\n \treturn 1;\n }\ndiff --git a/packfile.c b/packfile.c\nindex 315c3da259a..f5cee440601 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -374,7 +374,7 @@ void close_object_store(struct raw_object_store *o)\n \n void unlink_pack_path(const char *pack_name, int force_delete)\n {\n-\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\"};\n+\tstatic const char *exts[] = {\".pack\", \".idx\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".bump\"};\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t plen;\n@@ -741,6 +741,16 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)\n \tp->pack_size = st.st_size;\n \tp->pack_local = local;\n \tp->mtime = st.st_mtime;\n+\n+\tif (pack_mtime_to_bumpfiles) {\n+\t\tstruct strbuf name_buf = STRBUF_INIT;\n+\t\tconst char *filename;\n+\n+\t\tfilename = derive_pack_filename(path, \"idx\", \"bump\", &name_buf);\n+\t\tif (!stat(filename, &st)) {\n+\t\t\tp->mtime = st.st_mtime;\n+\t\t}\n+\t}\n \tif (path_len < the_hash_algo->hexsz ||\n \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n \t\thashclr(p->hash);\ndiff --git a/t/t5326-pack-mtime-bumpfiles.sh b/t/t5326-pack-mtime-bumpfiles.sh\nnew file mode 100755\nindex 00000000000..d6d9e6dc446\n--- /dev/null\n+++ b/t/t5326-pack-mtime-bumpfiles.sh\n@@ -0,0 +1,118 @@\n+#!/bin/sh\n+\n+test_description='packfile mtime use bump files'\n+. ./test-lib.sh\n+\n+if stat -c %Y . >/dev/null 2>&1; then\n+ get_modified_time() { stat -c %Y \"$1\" 2>/dev/null; }\n+elif stat -f %m . >/dev/null 2>&1; then\n+ get_modified_time() { stat -f %m \"$1\" 2>/dev/null; }\n+elif date -r . +%s >/dev/null 2>&1; then\n+ get_modified_time() { date -r \"$1\" +%s 2>/dev/null; }\n+else\n+ echo 'get_modified_time() is unsupported' >&2\n+ get_modified_time() { printf '%s' 0; }\n+fi\n+\n+test_expect_success 'freshen existing packfile without core.packMtimeToBumpFiles' '\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack1.* &&\n+\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack2.* &&\n+\tpack1_mtime=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n+\tpack2_mtime=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n+\t(echo one | git hash-object -w --stdin) &&\n+\t! test_path_exists .git/objects/pack/pack-$pack1.bump &&\n+\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n+\tpack1_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n+\tpack2_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n+\techo \"$pack1_mtime : $pack1_mtime_new\" &&\n+\ttest ! \"$pack1_mtime\" = \"$pack1_mtime_new\" &&\n+\ttest \"$pack2_mtime\" = \"$pack2_mtime_new\"\n+\n+'\n+\n+test_expect_success 'freshen existing packfile with core.packMtimeToBumpFiles' '\n+\n+\trm -rf .git/objects && git init &&\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack1.* &&\n+\ttest-tool chmtime =-60 .git/objects/pack/pack-$pack2.* &&\n+\tpack1_mtime=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n+\tpack2_mtime=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n+\t(echo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin) &&\n+\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n+\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n+\tpack1_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack1.pack) &&\n+\tpack2_mtime_new=$(get_modified_time .git/objects/pack/pack-$pack2.pack) &&\n+\ttest \"$pack1_mtime\" = \"$pack1_mtime_new\" &&\n+\ttest \"$pack2_mtime\" = \"$pack2_mtime_new\"\n+\n+'\n+\n+test_expect_success 'repack prune unreachable objects without core.packMtimeToBumpFiles' '\n+\n+\trm -rf .git/objects && git init &&\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\techo one | git hash-object -w --stdin &&\n+\techo two | git hash-object -w --stdin &&\n+\t! test_path_exists .git/objects/pack/pack-$pack1.bump &&\n+\t! test_path_exists .git/objects/pack/pack-$pack2.bump &&\n+\tgit prune-packed &&\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n+\tgit cat-file -p $obj1 &&\n+\ttest_must_fail git cat-file -p $obj2\n+\n+'\n+\n+test_expect_success 'repack prune unreachable objects with core.packMtimeToBumpFiles and bump files' '\n+\n+\trm -rf .git/objects && git init &&\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\techo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n+\techo two | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n+\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n+\ttest_path_exists .git/objects/pack/pack-$pack2.bump &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\tgit -c core.packMtimeToBumpFiles=true repack -A -d --unpack-unreachable=1.hour.ago &&\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2\n+\n+'\n+\n+test_expect_success 'repack prune unreachable objects with core.packMtimeToBumpFiles and old bump files' '\n+\n+\trm -rf .git/objects && git init &&\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\techo one | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n+\techo two | git -c core.packMtimeToBumpFiles=true hash-object -w --stdin &&\n+\ttest_path_exists .git/objects/pack/pack-$pack1.bump &&\n+\ttest_path_exists .git/objects/pack/pack-$pack2.bump &&\n+\tgit prune-packed &&\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.bump &&\n+\tgit -c core.packMtimeToBumpFiles=true repack -A -d --unpack-unreachable=1.hour.ago &&\n+\tgit cat-file -p $obj1 &&\n+\ttest_must_fail git cat-file -p $obj2\n+\n+'\n+\n+test_done\n-- \ngitgitgadget\n"}]}