{"thread":{"id":"46639","subject":"[PATCH] Retry acquiring reference locks for 100ms","startedAt":"2017-08-21T11:51:49Z","lastAt":"2017-08-24T14:44:23Z","messageCount":4,"participants":["Michael Haggerty","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"326868","messageId":"030b6bb22973df429ddbb64a079b9cdc1fbcb1b7.1503313472.git.mhagger@alum.mit.edu","threadId":"46639","inReplyTo":null,"subject":"[PATCH] Retry acquiring reference locks for 100ms","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-21T11:51:34Z","receivedAt":"2017-08-21T11:51:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The philosophy of reference locking has been, \"if another process is\nchanging a reference, then whatever I'm trying to do to it will\nprobably fail anyway because my old-SHA-1 value is probably no longer\ncurrent\". But this argument falls down if the other process has locked\nthe reference to do something that doesn't actually change the value\nof the reference, such as `pack-refs` or `reflog expire`. There\nactually *is* a decent chance that a planned reference update will\nstill be able to go through after the other process has released the\nlock.\n\nSo when trying to lock an individual reference (e.g., when creating\n\"refs/heads/master.lock\"), if it is already locked, then retry the\nlock acquisition for approximately 100 ms before giving up. This\nshould eliminate some unnecessary lock conflicts without wasting a lot\nof time.\n\nAdd a configuration setting, `core.filesRefLockTimeout`, to allow this\nsetting to be tweaked.\n\nNote: the function `get_files_ref_lock_timeout_ms()` cannot be private\nto the files backend because it is also used by `write_pseudoref()`\nand `delete_pseudoref()`, which are defined in `refs.c` so that they\ncan be used by other reference backends.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nThis patch applies to master, but (perhaps surprisingly) doesn't\nconflict with the mh/packed-ref-store changes. It can also be obtained\nfrom my Git fork [1] as branch \"ref-lock-retry\".\n\nMichael\n\n[1] https://github.com/mhagger/git\n\n Documentation/config.txt |  6 ++++++\n refs.c                   | 24 +++++++++++++++++++++---\n refs/files-backend.c     |  8 ++++++--\n refs/refs-internal.h     |  6 ++++++\n 4 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d5c9c4cab6..2c04b9dfb4 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -776,6 +776,12 @@ core.commentChar::\n If set to \"auto\", `git-commit` would select a character that is not\n the beginning character of any line in existing commit messages.\n \n+core.filesRefLockTimeout::\n+\tThe length of time, in milliseconds, to retry when trying to\n+\tlock an individual reference. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 100 (i.e.,\n+\tretry for 100ms).\n+\n core.packedRefsTimeout::\n \tThe length of time, in milliseconds, to retry when trying to\n \tlock the `packed-refs` file. Value 0 means not to retry at\ndiff --git a/refs.c b/refs.c\nindex fe4c59aa8b..29dbb9b610 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -561,6 +561,21 @@ enum ref_type ref_type(const char *refname)\n        return REF_TYPE_NORMAL;\n }\n \n+long get_files_ref_lock_timeout_ms(void)\n+{\n+\tstatic int configured = 0;\n+\n+\t/* The default timeout is 100 ms: */\n+\tstatic int timeout_ms = 100;\n+\n+\tif (!configured) {\n+\t\tgit_config_get_int(\"core.filesreflocktimeout\", &timeout_ms);\n+\t\tconfigured = 1;\n+\t}\n+\n+\treturn timeout_ms;\n+}\n+\n static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n \t\t\t   const unsigned char *old_sha1, struct strbuf *err)\n {\n@@ -573,7 +588,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n \tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(sha1));\n \n \tfilename = git_path(\"%s\", pseudoref);\n-\tfd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update_timeout(&lock, filename,\n+\t\t\t\t\t       LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t       get_files_ref_lock_timeout_ms());\n \tif (fd < 0) {\n \t\tstrbuf_addf(err, \"could not open '%s' for writing: %s\",\n \t\t\t    filename, strerror(errno));\n@@ -616,8 +633,9 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1\n \t\tint fd;\n \t\tunsigned char actual_old_sha1[20];\n \n-\t\tfd = hold_lock_file_for_update(&lock, filename,\n-\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\t\tfd = hold_lock_file_for_update_timeout(\n+\t\t\t\t&lock, filename, LOCK_DIE_ON_ERROR,\n+\t\t\t\tget_files_ref_lock_timeout_ms());\n \t\tif (fd < 0)\n \t\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \t\tif (read_ref(pseudoref, actual_old_sha1))\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0404f2c233..d611e0f7d7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -855,7 +855,9 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \tif (!lock->lk)\n \t\tlock->lk = xcalloc(1, sizeof(struct lock_file));\n \n-\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {\n+\tif (hold_lock_file_for_update_timeout(\n+\t\t\t    lock->lk, ref_file.buf, LOCK_NO_DEREF,\n+\t\t\t    get_files_ref_lock_timeout_ms()) < 0) {\n \t\tif (errno == ENOENT && --attempts_remaining > 0) {\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -1181,7 +1183,9 @@ static int create_reflock(const char *path, void *cb)\n {\n \tstruct lock_file *lk = cb;\n \n-\treturn hold_lock_file_for_update(lk, path, LOCK_NO_DEREF) < 0 ? -1 : 0;\n+\treturn hold_lock_file_for_update_timeout(\n+\t\t\tlk, path, LOCK_NO_DEREF,\n+\t\t\tget_files_ref_lock_timeout_ms()) < 0 ? -1 : 0;\n }\n \n /*\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 192f9f85c9..9977fea98b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -61,6 +61,12 @@\n  */\n #define REF_DELETED_LOOSE 0x200\n \n+/*\n+ * Return the length of time to retry acquiring a loose reference lock\n+ * before giving up, in milliseconds:\n+ */\n+long get_files_ref_lock_timeout_ms(void);\n+\n /*\n  * Return true iff refname is minimally safe. \"Safe\" here means that\n  * deleting a loose reference by this name will not do any damage, for\n-- \n2.11.0\n\n"},{"id":"327095","messageId":"xmqqd17mx82h.fsf@gitster.mtv.corp.google.com","threadId":"46639","inReplyTo":"030b6bb22973df429ddbb64a079b9cdc1fbcb1b7.1503313472.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] Retry acquiring reference locks for 100ms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T21:55:50Z","receivedAt":"2017-08-23T21:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The philosophy of reference locking has been, \"if another process is\n> changing a reference, then whatever I'm trying to do to it will\n> probably fail anyway because my old-SHA-1 value is probably no longer\n> current\". But this argument falls down if the other process has locked\n> the reference to do something that doesn't actually change the value\n> of the reference, such as `pack-refs` or `reflog expire`. There\n> actually *is* a decent chance that a planned reference update will\n> still be able to go through after the other process has released the\n> lock.\n\nThe reason why these 'read-only' operations take locks is because\nthey want to ensure that other people will not mess with the\nanchoring points of the history they base their operation on while\nthey do their work, right?\n\n> So when trying to lock an individual reference (e.g., when creating\n> \"refs/heads/master.lock\"), if it is already locked, then retry the\n> lock acquisition for approximately 100 ms before giving up. This\n> should eliminate some unnecessary lock conflicts without wasting a lot\n> of time.\n>\n> Add a configuration setting, `core.filesRefLockTimeout`, to allow this\n> setting to be tweaked.\n\nI suspect that this came from real-life needs of a server operator.\nWhat numbers should I be asking to justify this change? ;-) \n\n\"Without this change, 0.4% of pushes used to fail due to losing the\nrace against periodic GC, but with this, the rate went down to 0.2%,\nwhich is 50% improvement!\" or something like that?\n\n\n\n"},{"id":"327108","messageId":"CAMy9T_FJiDG7uzs8cDExzz_UPiRvAwM61Xfsv=wMUkNJCTghCw@mail.gmail.com","threadId":"46639","inReplyTo":"xmqqd17mx82h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Retry acquiring reference locks for 100ms","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-24T07:01:38Z","receivedAt":"2017-08-24T07:01:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Wed, Aug 23, 2017 at 11:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>> The philosophy of reference locking has been, \"if another process is\n>> changing a reference, then whatever I'm trying to do to it will\n>> probably fail anyway because my old-SHA-1 value is probably no longer\n>> current\". But this argument falls down if the other process has locked\n>> the reference to do something that doesn't actually change the value\n>> of the reference, such as `pack-refs` or `reflog expire`. There\n>> actually *is* a decent chance that a planned reference update will\n>> still be able to go through after the other process has released the\n>> lock.\n>\n> The reason why these 'read-only' operations take locks is because\n> they want to ensure that other people will not mess with the\n> anchoring points of the history they base their operation on while\n> they do their work, right?\n\nIn the case of `pack-refs`, after it makes packed versions of the\nloose references, it needs to lock each loose reference before pruning\nit, so that it can verify in a non-racy way that the loose reference\nstill has the same value as the one it just packed.\n\nIn the case of `reflog expire`, it locks the reference because that\nimplies a lock on the reflog file, which it needs to rewrite. (Reflog\nfiles don't have their own locks.) Otherwise it could inadvertently\noverwrite a new reflog entry that is added by another process while it\nis rewriting the file.\n\n>> So when trying to lock an individual reference (e.g., when creating\n>> \"refs/heads/master.lock\"), if it is already locked, then retry the\n>> lock acquisition for approximately 100 ms before giving up. This\n>> should eliminate some unnecessary lock conflicts without wasting a lot\n>> of time.\n>>\n>> Add a configuration setting, `core.filesRefLockTimeout`, to allow this\n>> setting to be tweaked.\n>\n> I suspect that this came from real-life needs of a server operator.\n> What numbers should I be asking to justify this change? ;-)\n>\n> \"Without this change, 0.4% of pushes used to fail due to losing the\n> race against periodic GC, but with this, the rate went down to 0.2%,\n> which is 50% improvement!\" or something like that?\n\nWe've had a patch like this deployed to our servers for quite some\ntime, so I don't remember too accurately. But I think we were seeing\nmaybe 10-50 such errors every day across our whole infrastructure\n(we're talking like literally one in a million updates). That number\nwent basically to zero after retries were added.\n\nIt's also not particularly serious when the race happens: the\nreference update is rejected, but it is rejected cleanly.\n\nSo it's definitely a rare race, and probably only of any interest at\nall on a high-traffic Git server. OTOH the cure is pretty simple, so\nit seems worth fixing.\n\nMichael\n"},{"id":"327134","messageId":"20170824144403.lxpgenqfgt7r3mrr@sigill.intra.peff.net","threadId":"46639","inReplyTo":"030b6bb22973df429ddbb64a079b9cdc1fbcb1b7.1503313472.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] Retry acquiring reference locks for 100ms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-24T14:44:04Z","receivedAt":"2017-08-24T14:44:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2017 at 01:51:34PM +0200, Michael Haggerty wrote:\n\n> The philosophy of reference locking has been, \"if another process is\n> changing a reference, then whatever I'm trying to do to it will\n> probably fail anyway because my old-SHA-1 value is probably no longer\n> current\". But this argument falls down if the other process has locked\n> the reference to do something that doesn't actually change the value\n> of the reference, such as `pack-refs` or `reflog expire`. There\n> actually *is* a decent chance that a planned reference update will\n> still be able to go through after the other process has released the\n> lock.\n> \n> So when trying to lock an individual reference (e.g., when creating\n> \"refs/heads/master.lock\"), if it is already locked, then retry the\n> lock acquisition for approximately 100 ms before giving up. This\n> should eliminate some unnecessary lock conflicts without wasting a lot\n> of time.\n\nIt will probably comes as little surprise to others on the list that\nI agree with the intent of this patch. This version is cleaned up a\nlittle from what we're running at GitHub right now, so I'll try to\nreview with fresh eyes.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index d5c9c4cab6..2c04b9dfb4 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -776,6 +776,12 @@ core.commentChar::\n>  If set to \"auto\", `git-commit` would select a character that is not\n>  the beginning character of any line in existing commit messages.\n>  \n> +core.filesRefLockTimeout::\n> +\tThe length of time, in milliseconds, to retry when trying to\n> +\tlock an individual reference. Value 0 means not to retry at\n> +\tall; -1 means to try indefinitely. Default is 100 (i.e.,\n> +\tretry for 100ms).\n> +\n>  core.packedRefsTimeout::\n>  \tThe length of time, in milliseconds, to retry when trying to\n>  \tlock the `packed-refs` file. Value 0 means not to retry at\n\nDo we need a separate config from packedRefsTimeout that is in the\ncontext? I guess so, since the default values are different. And\nrightfully so, I think, since writing a packed-refs file is potentially\na much larger operation (being O(n) in the number of refs rather than a\nconstant 40 bytes).\n\nIt probably doesn't matter all that much either way, as I wouldn't\nexpect people to need to tweak either of these in practice.\n\nAt some point when we have another ref backend that needs to take a\nglobal lock, we'd probably have a third timeout. If we were starting\nfrom scratch, I'd suggest that these might be part of refstorage.files.*\ninstead of core.*. And then eventually we might have\nrefstorage.reftable.timeout, refstorage.lmdb.timeout, etc.\n\nBut since core.packedRefsTimeout has already sailed, I'm not sure it's\nworth caring about.\n\n> diff --git a/refs.c b/refs.c\n> index fe4c59aa8b..29dbb9b610 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -561,6 +561,21 @@ enum ref_type ref_type(const char *refname)\n>         return REF_TYPE_NORMAL;\n>  }\n>  \n> +long get_files_ref_lock_timeout_ms(void)\n> +{\n> +\tstatic int configured = 0;\n> +\n> +\t/* The default timeout is 100 ms: */\n> +\tstatic int timeout_ms = 100;\n> +\n> +\tif (!configured) {\n> +\t\tgit_config_get_int(\"core.filesreflocktimeout\", &timeout_ms);\n> +\t\tconfigured = 1;\n> +\t}\n> +\n> +\treturn timeout_ms;\n> +}\n\nThis reads the config value into a static local that gets cached for the\nrest of the program run, and there's no way to invalidate the cache. I\nthink that should be OK, as we would never hit the ref code until we've\nactually initialized the repository, at which point we're fairly well\ncommitted.\n\n(I'm a little gun-shy because I used a similar lazy-load pattern for\ncore.sharedrepository a while back, and those corner cases bit us. But\nthere it was heavily used by git-init, which wants to \"switch\" repos\nafter having loaded some values).\n\n> @@ -573,7 +588,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n>  \tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(sha1));\n>  \n>  \tfilename = git_path(\"%s\", pseudoref);\n> -\tfd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);\n> +\tfd = hold_lock_file_for_update_timeout(&lock, filename,\n> +\t\t\t\t\t       LOCK_DIE_ON_ERROR,\n> +\t\t\t\t\t       get_files_ref_lock_timeout_ms());\n>  \tif (fd < 0) {\n>  \t\tstrbuf_addf(err, \"could not open '%s' for writing: %s\",\n>  \t\t\t    filename, strerror(errno));\n\nThe rest of the patch looks obviously correct to me. The one thing I\ndidn't audit for was whether there were any calls that _should_ be\nchanged that you missed. But I'm pretty sure based on our previous\noff-list discussions that you just did that audit.\n\nObviously new ref-locking calls would want to use the timeout variant\nhere, too, and we need to remember to do so. But I don't think there\nshould be many, as most sites should be going through create_reflock()\nor lock_raw_ref(), which you touch here.\n\n-Peff\n"}]}