git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 8/8] ref_transaction_commit(): fix atomicity and avoid fd exhaustion

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
May 10, 2015, 02:45 UTC
Message-ID
<1431225937-10456-9-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1431225937-10456-1-git-send-email-mhagger@alum.mit.edu>
The old code was roughly
    for update in updates:
        acquire locks and check old_sha
    for update in updates:
        if changing value:
            write_ref_to_lockfile()
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    rewrite packed-refs file
    for update in updates:
        if reference still locked:
            unlock_ref()
This has two problems.

Non-atomic updates ==================

The atomicity of the reference transaction depends on all pre-checks being done in the first loop, before any changes have started being committed in the second loop. The problem is that write_ref_to_lockfile() (previously part of write_ref_sha1()), which is called from the second loop, contains two more checks:

* It verifies that new_sha1 is a valid object
* If the reference being updated is a branch, it verifies that
  new_sha1 points at a commit object (as opposed to a tag, tree, or
  blob).

If either of these checks fails, the "transaction" is aborted during the second loop. But this might happen after some reference updates have already been permanently committed. In other words, the all-or-nothing promise of "git update-ref --stdin" could be violated.

So these checks have to be moved to the first loop.

File descriptor exhaustion ==========================

The old code locked all of the references in the first loop, leaving all of the lockfiles open until later loops. Since we might be updating a lot of references, this could result in file descriptor exhaustion.

The solution ============

After this patch, the code looks like
    for update in updates:
        acquire locks and check old_sha
        if changing value:
            write_ref_to_lockfile()
        else:
            close_ref()
    for update in updates:
        if changing value:
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    rewrite packed-refs file
    for update in updates:
        if reference still locked:
            unlock_ref()
This fixes both problems:
1. The pre-checks in write_ref_to_lockfile() are now done in the first
   loop, before any changes have been committed. If any of the checks
   fails, the whole transaction can now be rolled back correctly.
2. All lockfiles are closed in the first loop immediately after they
   are created (either by write_ref_to_lockfile() or by close_ref()).
   This means that there is never more than one open lockfile at a
   time, preventing file descriptor exhaustion.

To simplify the bookkeeping across loops, add a new REF_NEEDS_COMMIT bit to update->flags, which keeps track of whether the corresponding lockfile needs to be committed, as opposed to just unlocked. (Since "struct ref_update" is internal to the refs module, this change is not visible to external callers.)

This change fixes two tests in t1400.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                | 55 +++++++++++++++++++++++++++++++++++++++++----------
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index 58b1182..85c1dcb 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,13 @@ static unsigned char refname_disposition[256] = {
  * pruned.
  */
 #define REF_ISPRUNING	0x0100
+
+/*
+ * Used as a flag in ref_update::flags when the lockfile needs to be
+ * committed.
+ */
+#define REF_NEEDS_COMMIT 0x0200
+
 /*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
@@ -3799,7 +3806,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		goto cleanup;
 	}
 
-	/* Acquire all locks while verifying old values */
+	/*
+	 * Acquire all locks, verify old values if provided, check
+	 * that new values are valid, and write new values to the
+	 * lockfiles, ready to be activated. Only keep one lockfile
+	 * open at a time to avoid running out of file descriptors.
+	 */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
@@ -3820,26 +3832,49 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		if (!(update->flags & REF_DELETING) &&
+		    (update->lock->force_write ||
+		     hashcmp(update->lock->old_sha1, update->new_sha1))) {
+			if (write_ref_to_lockfile(update->lock, update->new_sha1)) {
+				/*
+				 * The lock was freed upon failure of
+				 * write_ref_to_lockfile():
+				 */
+				update->lock = NULL;
+				strbuf_addf(err, "Cannot update the ref '%s'.",
+					    update->refname);
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
+			update->flags |= REF_NEEDS_COMMIT;
+		} else {
+			/*
+			 * We didn't have to write anything to the lockfile.
+			 * Close it to free up the file descriptor:
+			 */
+			if (close_ref(update->lock)) {
+				strbuf_addf(err, "Couldn't close %s.lock",
+					    update->refname);
+				goto cleanup;
+			}
+		}
 	}
 
 	/* Perform updates first so live commits remain referenced */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!(update->flags & REF_DELETING)) {
-			if (!update->lock->force_write &&
-			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
-				unlock_ref(update->lock);
+		if (update->flags & REF_NEEDS_COMMIT) {
+			if (commit_ref_update(update->lock,
+					      update->new_sha1, update->msg)) {
+				/* The lock was freed by commit_ref_update(): */
 				update->lock = NULL;
-			} else if (write_ref_to_lockfile(update->lock, update->new_sha1) ||
-				   commit_ref_update(update->lock, update->new_sha1, update->msg)) {
-				update->lock = NULL; /* freed by the above calls */
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			} else {
-				/* freed by the above calls: */
+				/* freed by the above call: */
 				update->lock = NULL;
 			}
 		}
@@ -3849,7 +3884,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (update->lock) {
+		if (update->flags & REF_DELETING) {
 			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
 )
 '
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do

-- 
2.1.4
Previous: Michael HaggertyNext: Junio C Hamano
Message 11 of 16 in “Fix atomicity and avoid fd exhaustion in ref transactions”
  1. 0/8 Fix atomicity and avoid fd exhaustion in ref transactionsMichael Haggerty, May 10, 2015
  2. 1/8 update-ref: test handling large transactions properlyMichael Haggerty, May 10, 2015
  3. 2/8 t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZEMichael Haggerty, May 10, 2015
  4. Stefan BellerMay 11, 2015
  5. Junio C HamanoMay 12, 2015
  6. 3/8 write_ref_to_lockfile(): new function, extracted from write_ref_sha1()Michael Haggerty, May 10, 2015
  7. 4/8 commit_ref_update(): new function, extracted from write_ref_sha1()Michael Haggerty, May 10, 2015
  8. 5/8 rename_ref(): inline calls to write_ref_sha1() from this functionMichael Haggerty, May 10, 2015
  9. 6/8 ref_transaction_commit(): inline call to write_ref_sha1()Michael Haggerty, May 10, 2015
  10. 7/8 ref_transaction_commit(): remove the local flags variableMichael Haggerty, May 10, 2015
  11. 8/8 ref_transaction_commit(): fix atomicity and avoid fd exhaustionMichael Haggerty, May 10, 2015
  12. Junio C HamanoMay 11, 2015
  13. Michael HaggertyMay 11, 2015
  14. Stefan BellerMay 11, 2015
  15. Christian CouderMay 12, 2015
  16. Stefan BellerMay 11, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.