* [PATCH v2 1/2] lockfile: allow file locking to be retried with a timeout
2015-05-11 10:35 [PATCH v2 0/2] Retry attempts to acquire the packed-refs lock Michael Haggerty
@ 2015-05-11 10:35 ` Michael Haggerty
2015-05-11 10:35 ` [PATCH v2 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock Michael Haggerty
1 sibling, 0 replies; 3+ messages in thread
From: Michael Haggerty @ 2015-05-11 10:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, Johannes Sixt, git, Michael Haggerty
Currently, there is only one attempt to lock a file. If it fails, the
whole operation fails.
But it might sometimes be advantageous to try acquiring a file lock a
few times before giving up. So add a new function,
hold_lock_file_for_update_timeout(), that allows a timeout to be
specified. Make hold_lock_file_for_update() a thin wrapper around the
new function.
If timeout_ms is positive, then retry for at least that many
milliseconds to acquire the lock. On each failed attempt, use select()
to wait for a backoff time that increases quadratically (capped at 1
second) and has a random component to prevent two processes from
getting synchronized. If timeout_ms is negative, retry indefinitely.
In a moment we will switch to using the new function when locking
packed-refs.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Notes (discussion):
It would be easy to also add a hold_lock_file_for_append_timeout(),
but I can't yet think of an application for it.
lockfile.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
lockfile.h | 16 +++++++++++--
2 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9889277..30e65e9 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,6 +157,80 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
return lk->fd;
}
+static int sleep_microseconds(long us)
+{
+ struct timeval tv;
+ tv.tv_sec = 0;
+ tv.tv_usec = us;
+ return select(0, NULL, NULL, NULL, &tv);
+}
+
+/*
+ * Constants defining the gaps between attempts to lock a file. The
+ * first backoff period is approximately INITIAL_BACKOFF_MS
+ * milliseconds. The longest backoff period is approximately
+ * (BACKOFF_MAX_MULTIPLIER * INITIAL_BACKOFF_MS) milliseconds.
+ */
+#define INITIAL_BACKOFF_MS 1L
+#define BACKOFF_MAX_MULTIPLIER 1000
+
+/*
+ * Try locking path, retrying with quadratic backoff for at least
+ * timeout_ms milliseconds. If timeout_ms is 0, try locking the file
+ * exactly once. If timeout_ms is -1, try indefinitely.
+ */
+static int lock_file_timeout(struct lock_file *lk, const char *path,
+ int flags, long timeout_ms)
+{
+ int n = 1;
+ int multiplier = 1;
+ long remaining_us = 0;
+ static int random_initialized = 0;
+
+ if (timeout_ms == 0)
+ return lock_file(lk, path, flags);
+
+ if (!random_initialized) {
+ srandom((unsigned int)getpid());
+ random_initialized = 1;
+ }
+
+ if (timeout_ms > 0) {
+ /* avoid overflow */
+ if (timeout_ms <= LONG_MAX / 1000)
+ remaining_us = timeout_ms * 1000;
+ else
+ remaining_us = LONG_MAX;
+ }
+
+ while (1) {
+ long backoff_ms, wait_us;
+ int fd;
+
+ fd = lock_file(lk, path, flags);
+
+ if (fd >= 0)
+ return fd; /* success */
+ else if (errno != EEXIST)
+ return -1; /* failure other than lock held */
+ else if (timeout_ms > 0 && remaining_us <= 0)
+ return -1; /* failure due to timeout */
+
+ backoff_ms = multiplier * INITIAL_BACKOFF_MS;
+ /* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
+ wait_us = (750 + random() % 500) * backoff_ms;
+ sleep_microseconds(wait_us);
+ remaining_us -= wait_us;
+
+ /* Recursion: (n+1)^2 = n^2 + 2n + 1 */
+ multiplier += 2*n + 1;
+ if (multiplier > BACKOFF_MAX_MULTIPLIER)
+ multiplier = BACKOFF_MAX_MULTIPLIER;
+ else
+ n++;
+ }
+}
+
void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
{
if (err == EEXIST) {
@@ -179,9 +253,10 @@ NORETURN void unable_to_lock_die(const char *path, int err)
}
/* This should return a meaningful errno on failure */
-int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
+int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
+ int flags, long timeout_ms)
{
- int fd = lock_file(lk, path, flags);
+ int fd = lock_file_timeout(lk, path, flags, timeout_ms);
if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
unable_to_lock_die(path, errno);
return fd;
diff --git a/lockfile.h b/lockfile.h
index cd2ec95..b4abc61 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -74,8 +74,20 @@ struct lock_file {
extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_die(const char *path, int err);
-extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
+extern int hold_lock_file_for_update_timeout(
+ struct lock_file *lk, const char *path,
+ int flags, long timeout_ms);
+
+static inline int hold_lock_file_for_update(
+ struct lock_file *lk, const char *path,
+ int flags)
+{
+ return hold_lock_file_for_update_timeout(lk, path, flags, 0);
+}
+
+extern int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+ int flags);
+
extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
extern char *get_locked_file_path(struct lock_file *);
extern int commit_lock_file_to(struct lock_file *, const char *path);
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock
2015-05-11 10:35 [PATCH v2 0/2] Retry attempts to acquire the packed-refs lock Michael Haggerty
2015-05-11 10:35 ` [PATCH v2 1/2] lockfile: allow file locking to be retried with a timeout Michael Haggerty
@ 2015-05-11 10:35 ` Michael Haggerty
1 sibling, 0 replies; 3+ messages in thread
From: Michael Haggerty @ 2015-05-11 10:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, Johannes Sixt, git, Michael Haggerty
Currently, there is only one attempt to acquire any lockfile, and if
the lock is held by another process, the locking attempt fails
immediately.
This is not such a limitation for loose reference files. First, they
don't take long to rewrite. Second, most reference updates have a
known "old" value, so if another process is updating a reference at
the same moment that we are trying to lock it, then probably the
expected "old" value will not longer be valid, and the update will
fail anyway.
But these arguments do not hold for packed-refs:
* The packed-refs file can be large and take significant time to
rewrite.
* Many references are stored in a single packed-refs file, so it could
be that the other process was changing a different reference than
the one that we are interested in.
Therefore, it is much more likely for there to be spurious lock
conflicts in connection to the packed-refs file, resulting in
unnecessary command failures.
So, if the first attempt to lock the packed-refs file fails, continue
retrying for a configurable length of time before giving up. The
default timeout is 1 second.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Documentation/config.txt | 6 ++++++
refs.c | 12 +++++++++++-
t/t3210-pack-refs.sh | 17 +++++++++++++++++
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..3c24e10 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -622,6 +622,12 @@ core.commentChar::
If set to "auto", `git-commit` would select a character that is not
the beginning character of any line in existing commit messages.
+core.packedRefsTimeout::
+ The length of time, in milliseconds, to retry when trying to
+ lock the `packed-refs` file. Value 0 means not to retry at
+ all; -1 means to try indefinitely. Default is 1000 (i.e.,
+ retry for 1 second).
+
sequence.editor::
Text editor used by `git rebase -i` for editing the rebase instruction file.
The value is meant to be interpreted by the shell when it is used.
diff --git a/refs.c b/refs.c
index 47e4e53..3f8ac63 100644
--- a/refs.c
+++ b/refs.c
@@ -2413,9 +2413,19 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
/* This should return a meaningful errno on failure */
int lock_packed_refs(int flags)
{
+ static int timeout_configured = 0;
+ static int timeout_value = 1000;
+
struct packed_ref_cache *packed_ref_cache;
- if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
+ if (!timeout_configured) {
+ git_config_get_int("core.packedrefstimeout", &timeout_value);
+ timeout_configured = 1;
+ }
+
+ if (hold_lock_file_for_update_timeout(
+ &packlock, git_path("packed-refs"),
+ flags, timeout_value) < 0)
return -1;
/*
* Get the current packed-refs while holding the lock. If the
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index aa9eb3a..e5e3204 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -187,4 +187,21 @@ test_expect_success 'notice d/f conflict with existing ref' '
test_must_fail git branch foo/bar/baz/lots/of/extra/components
'
+test_expect_success 'timeout if packed-refs.lock exists' '
+ LOCK=.git/packed-refs.lock &&
+ >$LOCK &&
+ test_when_finished "rm -f $LOCK" &&
+ test_must_fail git pack-refs --all --prune
+'
+
+test_expect_success 'retry acquiring packed-refs.lock' '
+ LOCK=.git/packed-refs.lock &&
+ >$LOCK &&
+ test_when_finished "wait; rm -f $LOCK" &&
+ {
+ ( sleep 1 ; rm -f $LOCK ) &
+ } &&
+ git -c core.packedrefstimeout=3000 pack-refs --all --prune
+'
+
test_done
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread