{"thread":{"id":"51504","subject":"[PATCH v1 0/1] pack-refs: pack expired loose refs to packed_refs","startedAt":"2019-07-21T18:18:01Z","lastAt":"2019-08-20T15:14:55Z","messageCount":8,"participants":["16657101987@163.com","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"379137","messageId":"20190721181739.81110-1-16657101987@163.com","threadId":"51504","inReplyTo":null,"subject":"[PATCH v1 0/1] pack-refs: pack expired loose refs to packed_refs","fromName":"","fromEmail":"16657101987@163.com","sentAt":"2019-07-21T18:17:38Z","receivedAt":"2019-07-21T18:18:01Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <sunchao9@huawei.com>\n\nWhen a packed ref is deleted, the whole packed-refs file is\nrewrite and omit the ref that no longer exists. However if\nanother gc command is running and call `pack-refs --all`\nsimultaneously, there is a change that a ref just updated\nwill lost the newly commits.\n\nThere are two valid methods to avoid losting commit of ref:\n  - force `update-ref -d` to update the snapshot before\n    rewrite packed-refs.\n  - do not pack a recently updated ref, where *recently*\n    could be set by *pack.looserefsexpire* option.\n\nI prefer **do not pack a recently updated ref**, here is the\nreasons:\n\n  1. It could avoid losting the newly commit of a ref which I\n     described upon.\n\n  2. Sometime, the git server will do `pack-refs --all` and\n     `update-ref` the same time, and the two commands have\n     chances to trying lock the same ref such as master, if\n     this happends one command will fail with such error:\n\n     **cannot lock ref 'refs/heads/master'**\n\n     This could happen if a ref is updated frequently, and\n     avoid pack the ref which is update recently could avoid\n     this error most of the time.\n\nSun Chao (1):\n  pack-refs: pack expired loose refs to packed_refs\n\n builtin/pack-refs.c       | 13 ++++++++++++-\n refs.c                    |  4 ++--\n refs.h                    |  2 +-\n refs/files-backend.c      | 18 +++++++++++++++++-\n refs/packed-backend.c     |  2 +-\n refs/refs-internal.h      |  2 +-\n t/helper/test-ref-store.c |  2 +-\n 7 files changed, 35 insertions(+), 8 deletions(-)\n\n-- \n2.22.0.214.g8dca754b1e\n\n\n"},{"id":"379138","messageId":"20190721181739.81110-2-16657101987@163.com","threadId":"51504","inReplyTo":"20190721181739.81110-1-16657101987@163.com","subject":"[PATCH v1 1/1] pack-refs: pack expired loose refs to packed_refs","fromName":"","fromEmail":"16657101987@163.com","sentAt":"2019-07-21T18:17:39Z","receivedAt":"2019-07-21T18:18:10Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <sunchao9@huawei.com>\n\nWhen a packed ref is deleted, the whole packed-refs file is\nrewrite and omit the ref that no longer exists. However if\nanother gc command is running and call `pack-refs --all`\nsimultaneously, there is a change that a ref just updated\nwill lost the newly commits.\n\nThrough these steps, losting commits of newly updated refs\ncould be demonstrated:\n\n  # step 1: compile git without `USE_NSEC` option\n  Some kernerl releases does enable it by default while some\n  does not. And if we compile git without `USE_NSEC`, it\n  will be easier demonstrated by the following steps.\n\n  # step 2: setup a bare repository and clone it as child\n  git init --bare parent &&\n  (cd parent && git config core.logallrefupdates true) &&\n  git clone parent child\n\n  # step 3: in one terminal, repack the bare refs repeatedly\n  cd parent &&\n  while true; do\n    git pack-refs --all\n  done\n\n  # step 4: in another terminal, simultaneously update the\n  # master with update-ref, and create and delete an\n  # unrelated ref also with update-ref\n  cd child &&\n  while true; do\n    git commit --allow-empty -m foo &&\n    us=`git rev-parse master` &&\n    pushd ../parent &&\n      git fetch ../child/.git master &&\n      git update-ref refs/heads/newbranch $us &&\n      git update-ref refs/heads/master $us &&\n      git update-ref -d refs/heads/newbranch &&\n      them=`git rev-parse master` &&\n      if test \"$them\" != \"$us\"; then\n        echo >&2 \"lost commit: $us\"\n        exit 1\n      fi\n    popd\n  done\n\nThough we have the packed-refs lock file and loose refs lock\nfiles to avoid updating conflicts, a ref will lost its newly\ncommits if the situation which is described as racy-git by\n`Documentation/technical/racy-git.txt` happens, it comes like\nthis:\n\n  1. Call `pack-refs --all` to pack all the loose refs to\n     packed-refs, and let say the modify time of the\n     packed-refs is DATE_M.\n\n  2. Call `update-ref` to update a new commit to master while\n     it is already packed.  the old value (let us call it\n     OID_A) remains in the packed-refs file and write the new\n     value (let us call it OID_B) to $GIT_DIR/refs/heads/master.\n\n  3. Call `update-ref -d` within the same DATE_M from the 1th\n     step to delete a different ref newbranch which is packed\n     in the packed-refs file. It check newbranch's oid from\n     packed-refs file without locking it.\n\n     Meanwhile it keeps a snapshot of the packed-refs file in\n     memory and record the file's timestamp with the snapshot.\n     The oid of master in the packed-refs's snapshot is OID_A.\n\n  4. Redo the 1th step, after `pack-refs --all` finished, the\n     oid of master in packe-refs file is OID_B, and the loose\n     refs $GIT_DIR/refs/heads/master is removed. Let's say\n     the `pack-refs --all` is very quickly done and the new\n     packed-refs file's modify time is stille DATE_M.\n\n  5. 3th step now going on, after checking the newbranch, it\n     begin to rewrite the packed-refs file, after get the lock\n     file of packed-ref file, it checks the timestamp of it's\n     snapshot in memory with the packed-refs file's time,\n     they are both the same DATE_M, so the snapshot is not\n     refreshed.\n\n     Because the loose ref of master is removed by 4th step,\n     `update-ref -d` will updates the new packed-ref to disk\n     which contains master with the oid OID-A. So now the\n     newly commit OID-B of master is lost.\n\nThere are two valid methods to avoid losting commit of ref:\n  - force `update-ref -d` to update the snapshot before\n    rewrite packed-refs.\n  - do not pack a recently updated ref, where *recently*\n    could be set by *pack.looserefsexpire* option.\n\nI prefer **do not pack a recently updated ref**, here is the\nreasons:\n\n  1. It could avoid losting the newly commit of a ref which I\n     described upon.\n\n  2. Sometime, the git server will do `pack-refs --all` and\n     `update-ref` the same time, and the two commands have\n     chances to trying lock the same ref such as master, if\n     this happends one command will fail with such error:\n\n     **cannot lock ref 'refs/heads/master'**\n\n     This could happen if a ref is updated frequently, and\n     avoid pack the ref which is update recently could avoid\n     this error most of the time.\n\nSigned-off-by: Sun Chao <sunchao9@huawei.com>\n---\n builtin/pack-refs.c       | 13 ++++++++++++-\n refs.c                    |  4 ++--\n refs.h                    |  2 +-\n refs/files-backend.c      | 18 +++++++++++++++++-\n refs/packed-backend.c     |  2 +-\n refs/refs-internal.h      |  2 +-\n t/helper/test-ref-store.c |  2 +-\n 7 files changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex cfbd5c36c7..7baced5788 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -9,16 +9,27 @@ static char const * const pack_refs_usage[] = {\n \tNULL\n };\n \n+static const char *pack_loose_refs_expire = \"now\";\n+\n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\",   &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\t{ OPTION_STRING, 0, \"expire\", &pack_loose_refs_expire, N_(\"date\"),\n+\t\t\tN_(\"pack unactive loose refs\"),\n+\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)pack_loose_refs_expire },\n \t\tOPT_END(),\n \t};\n+\tstatic timestamp_t expire;\n+\n+\tgit_config_get_expiry(\"pack.looserefsexpire\", &pack_loose_refs_expire);\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n+\n+\tif (parse_expiry_date(pack_loose_refs_expire, &expire))\n+\t\tdie(_(\"failed to parse '%s' value '%s'\"), \"pack.looserefsexpire\", pack_loose_refs_expire);\n+\treturn refs_pack_refs(get_main_ref_store(the_repository), flags, expire);\n }\ndiff --git a/refs.c b/refs.c\nindex cd297ee4bd..e72f4b05dd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1947,9 +1947,9 @@ void base_ref_store_init(struct ref_store *refs,\n }\n \n /* backend functions */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n+int refs_pack_refs(struct ref_store *refs, unsigned int flags, timestamp_t expire)\n {\n-\treturn refs->be->pack_refs(refs, flags);\n+\treturn refs->be->pack_refs(refs, flags, expire);\n }\n \n int refs_peel_ref(struct ref_store *refs, const char *refname,\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..0270aa9efc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -378,7 +378,7 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n  * Write a packed-refs file for the current repository.\n  * flags: Combination of the above PACK_REFS_* flags.\n  */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n+int refs_pack_refs(struct ref_store *refs, unsigned int flags, timestamp_t expire);\n \n /*\n  * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 63e55e6773..7e6676cece 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1144,7 +1144,7 @@ static int should_pack_ref(const char *refname,\n \treturn 1;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int files_pack_refs(struct ref_store *ref_store, unsigned int flags, timestamp_t expire)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1152,13 +1152,19 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct ref_iterator *iter;\n \tint ok;\n \tstruct ref_to_prune *refs_to_prune = NULL;\n+\tstruct stat st;\n+\tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n+\tsize_t path_baselen;\n \n \ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n \tif (!transaction)\n \t\treturn -1;\n \n+\tfiles_ref_path(refs, &path, \"\");\n+\tpath_baselen = path.len;\n+\n \tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);\n \n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n@@ -1172,6 +1178,16 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\t\t     flags))\n \t\t\tcontinue;\n \n+\t\t/*\n+\t\t * If the loose reference is active (not expired), do not pack it.\n+\t\t */\n+\t\tstrbuf_setlen(&path, path_baselen);\n+\t\tstrbuf_addstr(&path, iter->refname);\n+\t\tif (stat(path.buf, &st) == 0) {\n+\t\t\tif (st.st_mtime > expire)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n \t\t/*\n \t\t * Add a reference creation for this reference to the\n \t\t * packed-refs transaction:\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c01c7f5901..2c6e6ee990 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1550,7 +1550,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \treturn ret;\n }\n \n-static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags, timestamp_t expire)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex f2d8c0123a..81f00e4c04 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -530,7 +530,7 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t      struct ref_transaction *transaction,\n \t\t\t\t      struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n+typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags, timestamp_t expire);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t     const char *ref_target,\n \t\t\t     const char *refs_heads_master,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 799fc00aa1..b2be2e311d 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -69,7 +69,7 @@ static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\");\n \n-\treturn refs_pack_refs(refs, flags);\n+\treturn refs_pack_refs(refs, flags, TIME_MAX);\n }\n \n static int cmd_peel_ref(struct ref_store *refs, const char **argv)\n-- \n2.22.0.214.g8dca754b1e\n\n\n"},{"id":"379569","messageId":"20190730063634.GA4901@sigill.intra.peff.net","threadId":"51504","inReplyTo":"20190721181739.81110-2-16657101987@163.com","subject":"Re: [PATCH v1 1/1] pack-refs: pack expired loose refs to packed_refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-30T06:36:35Z","receivedAt":"2019-07-30T06:36:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 22, 2019 at 02:17:39AM +0800, 16657101987@163.com wrote:\n\n> From: Sun Chao <sunchao9@huawei.com>\n> \n> When a packed ref is deleted, the whole packed-refs file is\n> rewrite and omit the ref that no longer exists. However if\n> another gc command is running and call `pack-refs --all`\n> simultaneously, there is a change that a ref just updated\n> will lost the newly commits.\n> \n> Through these steps, losting commits of newly updated refs\n> could be demonstrated:\n\nThanks for the report and an easy-to-follow recipe. I was able to\nreproduce the problem.\n\n>   # step 4: in another terminal, simultaneously update the\n>   # master with update-ref, and create and delete an\n>   # unrelated ref also with update-ref\n>   cd child &&\n>   while true; do\n>     git commit --allow-empty -m foo &&\n>     us=`git rev-parse master` &&\n>     pushd ../parent &&\n>       git fetch ../child/.git master &&\n>       git update-ref refs/heads/newbranch $us &&\n>       git update-ref refs/heads/master $us &&\n>       git update-ref -d refs/heads/newbranch &&\n>       them=`git rev-parse master` &&\n>       if test \"$them\" != \"$us\"; then\n>         echo >&2 \"lost commit: $us\"\n>         exit 1\n>       fi\n>     popd\n>   done\n\nYou can do this step without the fetch, which makes it hit the race more\nquickly. :) Try this:\n\n  # prime it with a single commit\n  git commit --allow-empty -m foo\n  while true; do\n    us=$(git commit-tree -m foo -p HEAD HEAD^{tree}) &&\n    git update-ref refs/heads/newbranch $us &&\n    git update-ref refs/heads/master $us &&\n    git update-ref -d refs/heads/newbranch &&\n    them=$(git rev-parse master) &&\n    if test \"$them\" != \"$us\"; then\n      echo >&2 \"lost commit: $us\"\n      exit 1\n    fi\n    # eye candy\n    printf .\n  done\n\n> Though we have the packed-refs lock file and loose refs lock\n> files to avoid updating conflicts, a ref will lost its newly\n> commits if the situation which is described as racy-git by\n> `Documentation/technical/racy-git.txt` happens, it comes like\n> this:\n\nI don't think this is quite the same as racy-git. There we are comparing\nstat entries for a file X to the timestamp of the index (and we are\nconcerned they were written in the same second).\n\nBut here we have no on-disk stat information to compare to. It's all\nhappening in-process. But you're right that it's a racy stat-validity\nproblem.\n\n>   4. Redo the 1th step, after `pack-refs --all` finished, the\n>      oid of master in packe-refs file is OID_B, and the loose\n>      refs $GIT_DIR/refs/heads/master is removed. Let's say\n>      the `pack-refs --all` is very quickly done and the new\n>      packed-refs file's modify time is stille DATE_M.\n> \n>   5. 3th step now going on, after checking the newbranch, it\n>      begin to rewrite the packed-refs file, after get the lock\n>      file of packed-ref file, it checks the timestamp of it's\n>      snapshot in memory with the packed-refs file's time,\n>      they are both the same DATE_M, so the snapshot is not\n>      refreshed.\n\nThe stat-validity check here is actually more than the timestamp.\nSpecifically it's checking the inode and size. But because of the\nspecific set of operations you're performing, this ends up correlating\nquite often:\n\n  - because our operations involve updating a single ref or\n    adding/deleting another ref, we'll oscillate between two sizes\n    (either one ref or two)\n\n  - likewise if nothing else is happening on the filesystem, pack-refs\n    may flip back and forth between two inodes (not the same one,\n    because our tempfile-and-rename strategy means we're still using the\n    old one while we write the new packed-refs file).\n\nSo I actually find this to be a fairly unlikely case in the real world,\nbut as your script demonstrates, it's not that hard to trigger it if\nyou're trying.\n\n> There are two valid methods to avoid losting commit of ref:\n>   - force `update-ref -d` to update the snapshot before\n>     rewrite packed-refs.\n>   - do not pack a recently updated ref, where *recently*\n>     could be set by *pack.looserefsexpire* option.\n\nI'm not sure the second one actually fixes things entirely. What if I\nhave an older refs/heads/foo, and I do this:\n\n  git pack-refs\n  git pack-refs --all --prune\n\nWe still might hit the race here. The first pack-refs does not pack foo\n(because we didn't say --all), then a simultaneous \"update-ref -d\" opens\n`packed-refs`, then the second pack-refs packs it all in the same\nsecond. Now \"update-ref -d\" uses the old packed-refs file, and we lose\nthe ref.\n\nAdmittedly this is even more unlikely than your original case, because\nit involves quickly running pack-refs in two different modes.\n\nBut I think if we want the same solution as racy-git, the timestamp we\nwant to compare to is not the ref itself, but rather for the\nstat-validity code to see if packed-refs has the same timestamp as the\nmoment when we called stat().\n\nUnfortunately that's hard to do robustly, because the filesystem time\nand the OS clock time do not necessarily match up. I don't know of a way\nto record the current filesystem time without modifying a file.\n\nI do agree that simply removing the stat-validity check and _always_\nre-opening the packed-refs file when we take the lock would work.\nTraditionally we avoided that because refreshing it implied parsing the\nwhole file. But these days we mmap it, so it really is just an extra\nopen()/mmap() and a quick read of the header. That doesn't seem like an\noutrageous cost to pay when we're already taking the lock.\n\nAnother option would be to put an increasing counter into the file\nheader itself. We could then record the old counter instead of the\nstat-validity info, and always re-open(), mmap(), and parse the header.\nBut at that point I don't think it saves anything over just refreshing\nthe file as above.\n\nSo I actually think the best path forward is just always refreshing when\nwe take the lock, something like:\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c01c7f5901..0c8fdce7be 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1019,7 +1019,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t * is still valid. We've just locked the file, but it might\n \t * have changed the moment *before* we locked it.\n \t */\n-\tvalidate_snapshot(refs);\n+\tclear_snapshot(refs);\n \n \t/*\n \t * Now make sure that the packed-refs file as it exists in the\n\nWe could still see an old packed-refs file on reading (somebody packs a\nref and then deletes it, but we still see the old packed-refs), but\nthat's an inherent race (we can't know that somebody didn't update\npacked-refs after we checked its stat, because we're not holding the\nlock). It's also just one of many ways that the filesystem ref storage\nis not atomic (e.g., renaming X to Y, a reader might see neither ref!).\n\nUltimately the best solution there is to move to a better format (like\nthe reftables proposal).\n\n> I prefer **do not pack a recently updated ref**, here is the\n> reasons:\n> \n>   1. It could avoid losting the newly commit of a ref which I\n>      described upon.\n> \n>   2. Sometime, the git server will do `pack-refs --all` and\n>      `update-ref` the same time, and the two commands have\n>      chances to trying lock the same ref such as master, if\n>      this happends one command will fail with such error:\n> \n>      **cannot lock ref 'refs/heads/master'**\n> \n>      This could happen if a ref is updated frequently, and\n>      avoid pack the ref which is update recently could avoid\n>      this error most of the time.\n\nIt can also happen if you simply get unlucky with a ref that isn't\nupdated frequently. We may pack an older ref, but then collide with\nsomebody updating the ref when we take the lock to delete the loose\nversion.\n\n-Peff\n"},{"id":"379701","messageId":"20190731183544.24406-1-16657101987@163.com","threadId":"51504","inReplyTo":"20190730063634.GA4901@sigill.intra.peff.net","subject":"[PATCH v2 0/1] pack-refs: always refreshing after take the lock file","fromName":"","fromEmail":"16657101987@163.com","sentAt":"2019-07-31T18:35:43Z","receivedAt":"2019-07-31T18:36: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\nOn Tue, 30 Jul 2019 02:36:35 -0400, Jeff King wrote:\n\n> You can do this step without the fetch, which makes it hit the race more\n> quickly. :) Try this:\n> \n>   # prime it with a single commit\n>   git commit --allow-empty -m foo\n>   while true; do\n>     us=$(git commit-tree -m foo -p HEAD HEAD^{tree}) &&\n>     git update-ref refs/heads/newbranch $us &&\n>     git update-ref refs/heads/master $us &&\n>     git update-ref -d refs/heads/newbranch &&\n>     them=$(git rev-parse master) &&\n>     if test \"$them\" != \"$us\"; then\n>       echo >&2 \"lost commit: $us\"\n>       exit 1\n>     fi\n>     # eye candy\n>     printf .\n>   done\n\nThanks, this could hit the race more quickly and I update it to the\ncommit log.\n\n> I don't think this is quite the same as racy-git. There we are comparing\n> stat entries for a file X to the timestamp of the index (and we are\n> concerned they were written in the same second).\n> \n> But here we have no on-disk stat information to compare to. It's all\n> happening in-process. But you're right that it's a racy stat-validity\n> problem.\n\nYes, I agree with you.\n\n> The stat-validity check here is actually more than the timestamp.\n> Specifically it's checking the inode and size. But because of the\n> specific set of operations you're performing, this ends up correlating\n> quite often:\n> \n>   - because our operations involve updating a single ref or\n>     adding/deleting another ref, we'll oscillate between two sizes\n>     (either one ref or two)\n> \n>   - likewise if nothing else is happening on the filesystem, pack-refs\n>     may flip back and forth between two inodes (not the same one,\n>     because our tempfile-and-rename strategy means we're still using the\n>     old one while we write the new packed-refs file).\n> \n> So I actually find this to be a fairly unlikely case in the real world,\n> but as your script demonstrates, it's not that hard to trigger it if\n> you're trying.\n>\n> I'm not sure the second one actually fixes things entirely. What if I\n> have an older refs/heads/foo, and I do this:\n> \n>   git pack-refs\n>   git pack-refs --all --prune\n> \n> We still might hit the race here. The first pack-refs does not pack foo\n> (because we didn't say --all), then a simultaneous \"update-ref -d\" opens\n> `packed-refs`, then the second pack-refs packs it all in the same\n> second. Now \"update-ref -d\" uses the old packed-refs file, and we lose\n> the ref.\n\nYes, I agree with you. And in the real word if the git servers has some\n3rd-service which update repositories refs or pack-refs frequently may\nhave this problem, my company's git servers works like this unfortunately.\n\n> So I actually think the best path forward is just always refreshing when\n> we take the lock, something like:\n> \n> Ultimately the best solution there is to move to a better format (like\n> the reftables proposal).\n\nI do not know if we could get the new reftables in the next few versions,\nSo I commit the changes as you suggested, which is also the same as\nanother way I metioned in `PATCH v1`:\n\n**force `update-ref -d` to update the snapshot before rewrite packed-refs.**\n\nBut if the reftables is comeing soon, please just ignore my PATCH :)\n\n**And thank a lot for your reply, it's great to me, because it's my first\nPATCh to git myself :)**\n\nSun Chao (1):\n  pack-refs: always refreshing after take the lock file\n\n refs/packed-backend.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\n-- \n2.22.0.214.g8dca754b1e\n\n\n"},{"id":"379702","messageId":"20190731183544.24406-2-16657101987@163.com","threadId":"51504","inReplyTo":"20190731183544.24406-1-16657101987@163.com","subject":"[PATCH v2 1/1] pack-refs: always refreshing after take the lock file","fromName":"","fromEmail":"16657101987@163.com","sentAt":"2019-07-31T18:35:44Z","receivedAt":"2019-07-31T18:36:18Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <16657101987@163.com>\n\nWhen a packed ref is deleted, the whole packed-refs file is\nrewrite and omit the ref that no longer exists. However if\nanother gc command is running and call `pack-refs --all`\nsimultaneously, there is a chance that a ref just updated\nwill lost the newly commits.\n\nThrough these steps, losting commits of newly updated refs\ncould be demonstrated:\n\n  # step 1: compile git without `USE_NSEC` option\n  Some kernerl releases does enable it by default while some\n  does not. And if we compile git without `USE_NSEC`, it\n  will be easier demonstrated by the following steps.\n\n  # step 2: setup a repository and add first commit\n  git init repo &&\n  (cd repo &&\n   git config core.logallrefupdates true &&\n   git commit --allow-empty -m foo)\n\n  # step 3: in one terminal, repack the refs repeatedly\n  cd repo &&\n  while true; do\n    git pack-refs --all\n  done\n\n  # step 4: in another terminal, simultaneously update the\n  # master with update-ref, and create and delete an\n  # unrelated ref also with update-ref\n  cd repo &&\n  while true; do\n    us=$(git commit-tree -m foo -p HEAD HEAD^{tree}) &&\n    git update-ref refs/heads/newbranch $us &&\n    git update-ref refs/heads/master $us &&\n    git update-ref -d refs/heads/newbranch &&\n    them=$(git rev-parse master) &&\n    if test \"$them\" != \"$us\"; then\n      echo >&2 \"lost commit: $us\"\n      exit 1\n    fi\n    # eye candy\n    printf .\n  done\n\nThough we have the packed-refs lock file and loose refs lock\nfiles to avoid updating conflicts, a ref will lost its newly\ncommits if racy stat-validity of `packed-refs` file happens\n(which is quite same as the racy-git described in\n`Documentation/technical/racy-git.txt`), the following\nspecific set of operations demonstrates the problem:\n\n  1. Call `pack-refs --all` to pack all the loose refs to\n     packed-refs, and let say the modify time of the\n     packed-refs is DATE_M.\n\n  2. Call `update-ref` to update a new commit to master while\n     it is already packed.  the old value (let us call it\n     OID_A) remains in the packed-refs file and write the new\n     value (let us call it OID_B) to $GIT_DIR/refs/heads/master.\n\n  3. Call `update-ref -d` within the same DATE_M from the 1th\n     step to delete a different ref newbranch which is packed\n     in the packed-refs file. It check newbranch's oid from\n     packed-refs file without locking it.\n\n     Meanwhile it keeps a snapshot of the packed-refs file in\n     memory and record the file's attributes with the snapshot.\n     The oid of master in the packed-refs's snapshot is OID_A.\n\n  4. Call a new `pack-refs --all` to pack the loose refs, the\n     oid of master in packe-refs file is OID_B, and the loose\n     refs $GIT_DIR/refs/heads/master is removed. Let's say\n     the `pack-refs --all` is very quickly done and the new\n     packed-refs file's modify time is still DATE_M, and it\n     has the same file size, even the same inode.\n\n  5. 3th step now goes on after checking the newbranch, it\n     begin to rewrite the packed-refs file. After get the\n     lock file of packed-ref file, it checks it's on-disk\n     file attributes with the snapshot, suck as the timestamp,\n     the file size and the inode value. If they are both the\n     same values, and the snapshot is not refreshed.\n\n     Because the loose ref of master is removed by 4th step,\n     `update-ref -d` will updates the new packed-ref to disk\n     which contains master with the oid OID_A. So now the\n     newly commit OID_B of master is lost.\n\nThe best path forward is just always refreshing after take\nthe lock file of `packed-refs` file. Traditionally we avoided\nthat because refreshing it implied parsing the whole file.\nBut these days we mmap it, so it really is just an extra\nopen()/mmap() and a quick read of the header. That doesn't seem\nlike an outrageous cost to pay when we're already taking the lock.\n\nSigned-off-by: Sun Chao <sunchao9@huawei.com>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Sun Chao <sunchao9@huawei.com>\n---\n refs/packed-backend.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c01c7f5901..4458a0f69c 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1012,14 +1012,23 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t}\n \n \t/*\n-\t * Now that we hold the `packed-refs` lock, make sure that our\n-\t * snapshot matches the current version of the file. Normally\n-\t * `get_snapshot()` does that for us, but that function\n-\t * assumes that when the file is locked, any existing snapshot\n-\t * is still valid. We've just locked the file, but it might\n-\t * have changed the moment *before* we locked it.\n+\t * There is a stat-validity problem might cause `update-ref -d`\n+\t * lost the newly commit of a ref, because a new `packed-refs`\n+\t * file might has the same on-disk file attributes such as\n+\t * timestamp, file size and inode value, but has a changed\n+\t * ref value.\n+\t *\n+\t * This could happen with a very small chance when\n+\t * `update-ref -d` is called and at the same time another\n+\t * `pack-refs --all` process is running.\n+\t *\n+\t * Now that we hold the `packed-refs` lock, it is important\n+\t * to make sure we could read the latest version of\n+\t * `packed-refs` file no matter we have just mmap it or not.\n+\t * So what need to do is clear the snapshot if we hold it\n+\t * already.\n \t */\n-\tvalidate_snapshot(refs);\n+\tclear_snapshot(refs);\n \n \t/*\n \t * Now make sure that the packed-refs file as it exists in the\n-- \n2.22.0.214.g8dca754b1e\n\n\n"},{"id":"380551","messageId":"20190816204906.GA29853@sigill.intra.peff.net","threadId":"51504","inReplyTo":"20190731183544.24406-1-16657101987@163.com","subject":"Re: [PATCH v2 0/1] pack-refs: always refreshing after take the lock file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-16T20:49:06Z","receivedAt":"2019-08-16T20:49:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2019 at 02:35:43AM +0800, 16657101987@163.com wrote:\n\n> > So I actually think the best path forward is just always refreshing when\n> > we take the lock, something like:\n> > \n> > Ultimately the best solution there is to move to a better format (like\n> > the reftables proposal).\n> \n> I do not know if we could get the new reftables in the next few versions,\n> So I commit the changes as you suggested, which is also the same as\n> another way I metioned in `PATCH v1`:\n> \n> **force `update-ref -d` to update the snapshot before rewrite packed-refs.**\n> \n> But if the reftables is comeing soon, please just ignore my PATCH :)\n\nI'm undecided on this. I think reftables are still a while off, and even\nonce they are here, many people will still be using the older format. So\nit makes sense to still apply fixes to the old code.\n\nWhat I wonder, though, is whether always refreshing will cause a\nnoticeable performance impact (and that's why I was so slow in\nresponding -- I had hoped to try to come up with some numbers, but I\njust hadn't gotten around to it).\n\nMy gut says it's _probably_ not an issue, but it would be nice to have\nsome data to back it up.\n\n> **And thank a lot for your reply, it's great to me, because it's my first\n> PATCh to git myself :)**\n\nYou're welcome. Thanks for diagnosing a rather tricky case. :)\n\n-Peff\n"},{"id":"380719","messageId":"xmqqr25hxdk6.fsf@gitster-ct.c.googlers.com","threadId":"51504","inReplyTo":"20190816204906.GA29853@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/1] pack-refs: always refreshing after take the lock file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-19T17:36:25Z","receivedAt":"2019-08-19T17:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm undecided on this. I think reftables are still a while off, and even\n> once they are here, many people will still be using the older format. So\n> it makes sense to still apply fixes to the old code.\n\nYeah.\n\n> What I wonder, though, is whether always refreshing will cause a\n> noticeable performance impact (and that's why I was so slow in\n> responding -- I had hoped to try to come up with some numbers, but I\n> just hadn't gotten around to it).\n>\n> My gut says it's _probably_ not an issue, but it would be nice to have\n> some data to back it up.\n\nI am tempted to let correctness (and ease-of-reasoning about the\ncode) take precedence over potential and unknown performance issue,\nat least for now.  A single liner is rather simple to revert (or in\nthe worst case we could add \"allow pack-refs to efficiently lose a\nref to a race\" configuration option) anyway.\n\n"},{"id":"380814","messageId":"20190820151408.12700-1-16657101987@163.com","threadId":"51504","inReplyTo":"xmqqr25hxdk6.fsf@gitster-ct.c.googlers.com","subject":"Re: Re: [PATCH v2 0/1] pack-refs: always refreshing after take the lock file","fromName":"","fromEmail":"16657101987@163.com","sentAt":"2019-08-20T15:14:08Z","receivedAt":"2019-08-20T15:14:55Z","isPatch":true,"sender":{"key":"16657101987@163.com","avatar":"https://avatars.githubusercontent.com/u/192864724?v=4"},"body":"From: Sun Chao <sunchao9@huawei.com>\n\n---\n\nJeff King <peff@peff.net> writes:\n\n> I'm undecided on this. I think reftables are still a while off, and even\n> once they are here, many people will still be using the older format. So\n> it makes sense to still apply fixes to the old code.\n\nGot it, thanks for explainning.\n\n> What I wonder, though, is whether always refreshing will cause a\n> noticeable performance impact (and that's why I was so slow in\n> responding -- I had hoped to try to come up with some numbers, but I\n> just hadn't gotten around to it).\n>\n> My gut says it's _probably_ not an issue, but it would be nice to have\n> some data to back it up.\n\nSorry for responding after 4 days because I have been away on official\nbusiness.\n\nTody I have tryied some tools like trace logs, time, and strace, tring\nto figure out if there are some noticeable numbers. I tried different\nrepositories with different ref numbers and blob numbers, I also can\nnot recognize how much the refreshing impact the performance, perhaps\nI need to find a better computer for benchmark testing.\n\n---\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> I am tempted to let correctness (and ease-of-reasoning about the\n> code) take precedence over potential and unknown performance issue,\n> at least for now.  A single liner is rather simple to revert (or in\n> the worst case we could add \"allow pack-refs to efficiently lose a\n> ref to a race\" configuration option) anyway.\n\nThanks a lot :)\n\n-- \n2.17.2 (Apple Git-113)\n\n\n"}]}