From e5db474ad271a721021b4ec741c91946086188a0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 24 Apr 2015 13:35:49 +0200 Subject: ref_transaction_commit(): only keep one lockfile open at a time 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 cause file descriptor exhaustion. But there is no reason to keep the lockfiles open after the first loop: * For references being deleted or verified, we don't have to write anything into the lockfile, so we can close it immediately. * For references being updated, we can write the new SHA-1 into the lockfile immediately and close the lockfile without renaming it into place. In the second loop, the already-written contents can be renamed into place using commit_ref_update(). 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. This change fixes two tests in t1400. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 68 +++++++++++++++++++++++++++++++++++++-------------- t/t1400-update-ref.sh | 4 +-- 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/refs.c b/refs.c index 782e777580..2bdd93c4a8 100644 --- a/refs.c +++ b/refs.c @@ -36,25 +36,31 @@ static unsigned char refname_disposition[256] = { * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken * refs (i.e., because the reference is about to be deleted anyway). */ -#define REF_DELETING 0x02 +#define REF_DELETING 0x02 /* * Used as a flag in ref_update::flags when a loose ref is being * pruned. */ -#define REF_ISPRUNING 0x04 +#define REF_ISPRUNING 0x04 /* * Used as a flag in ref_update::flags when the reference should be * updated to new_sha1. */ -#define REF_HAVE_NEW 0x08 +#define REF_HAVE_NEW 0x08 /* * Used as a flag in ref_update::flags when old_sha1 should be * checked. */ -#define REF_HAVE_OLD 0x10 +#define REF_HAVE_OLD 0x10 + +/* + * Used as a flag in ref_update::flags when the lockfile needs to be + * committed. + */ +#define REF_NEEDS_COMMIT 0x20 /* * Try to read one refname component from the front of refname. @@ -3749,7 +3755,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]; @@ -3770,13 +3781,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, 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_HAVE_NEW) && !is_null_sha1(update->new_sha1)) { + if ((update->flags & REF_HAVE_NEW) && !(update->flags & REF_DELETING)) { int overwriting_symref = ((update->type & REF_ISSYMREF) && (update->flags & REF_NODEREF)); @@ -3786,14 +3791,39 @@ int ref_transaction_commit(struct ref_transaction *transaction, * The reference already has the desired * value, so we don't need to write it. */ - unlock_ref(update->lock); - 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 */ + update->new_sha1)) { + update->lock = NULL; /* freed by the above call */ + strbuf_addf(err, "Cannot update the ref '%s'.", + update->refname); + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } else { + update->flags |= REF_NEEDS_COMMIT; + } + } + if (!(update->flags & REF_NEEDS_COMMIT)) { + /* + * 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_NEEDS_COMMIT) { + if (commit_ref_update(update->lock, + update->new_sha1, + update->msg)) { + update->lock = NULL; /* freed by the above call */ strbuf_addf(err, "Cannot update the ref '%s'.", update->refname); ret = TRANSACTION_GENERIC_ERROR; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7a69f1a083..636d3a167c 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1071,7 +1071,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 @@ -1082,7 +1082,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 -- cgit v1.2.1