ref_transaction_commit(): fix atomicity and avoid fd exhaustion

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Haggerty committed May 10, 2015 at 04:45 UTC 6c34492ab4f260a4f32968e235da9badb22b56b4
2 files changed +47 -12
refs.c
+45 -10
@@ -30,6 +30,13 @@ static unsigned char refname_disposition[256] = {
30 * pruned.
31 */
32 #define REF_ISPRUNING 0x0100
33 +
34 +/*
35 + * Used as a flag in ref_update::flags when the lockfile needs to be
36 + * committed.
37 + */
38 +#define REF_NEEDS_COMMIT 0x0200
39 +
40 /*
41 * Try to read one refname component from the front of refname.
42 * 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,
3806 goto cleanup;
3807 }
3808
3802 - /* Acquire all locks while verifying old values */
3809 + /*
3810 + * Acquire all locks, verify old values if provided, check
3811 + * that new values are valid, and write new values to the
3812 + * lockfiles, ready to be activated. Only keep one lockfile
3813 + * open at a time to avoid running out of file descriptors.
3814 + */
3815 for (i = 0; i < n; i++) {
3816 struct ref_update *update = updates[i];
3817
@@ -3820,26 +3832,49 @@ int ref_transaction_commit(struct ref_transaction *transaction,
3832 update->refname);
3833 goto cleanup;
3834 }
3835 + if (!(update->flags & REF_DELETING) &&
3836 + (update->lock->force_write ||
3837 + hashcmp(update->lock->old_sha1, update->new_sha1))) {
3838 + if (write_ref_to_lockfile(update->lock, update->new_sha1)) {
3839 + /*
3840 + * The lock was freed upon failure of
3841 + * write_ref_to_lockfile():
3842 + */
3843 + update->lock = NULL;
3844 + strbuf_addf(err, "Cannot update the ref '%s'.",
3845 + update->refname);
3846 + ret = TRANSACTION_GENERIC_ERROR;
3847 + goto cleanup;
3848 + }
3849 + update->flags |= REF_NEEDS_COMMIT;
3850 + } else {
3851 + /*
3852 + * We didn't have to write anything to the lockfile.
3853 + * Close it to free up the file descriptor:
3854 + */
3855 + if (close_ref(update->lock)) {
3856 + strbuf_addf(err, "Couldn't close %s.lock",
3857 + update->refname);
3858 + goto cleanup;
3859 + }
3860 + }
3861 }
3862
3863 /* Perform updates first so live commits remain referenced */
3864 for (i = 0; i < n; i++) {
3865 struct ref_update *update = updates[i];
3866
3829 - if (!(update->flags & REF_DELETING)) {
3830 - if (!update->lock->force_write &&
3831 - !hashcmp(update->lock->old_sha1, update->new_sha1)) {
3832 - unlock_ref(update->lock);
3867 + if (update->flags & REF_NEEDS_COMMIT) {
3868 + if (commit_ref_update(update->lock,
3869 + update->new_sha1, update->msg)) {
3870 + /* The lock was freed by commit_ref_update(): */
3871 update->lock = NULL;
3834 - } else if (write_ref_to_lockfile(update->lock, update->new_sha1) ||
3835 - commit_ref_update(update->lock, update->new_sha1, update->msg)) {
3836 - update->lock = NULL; /* freed by the above calls */
3872 strbuf_addf(err, "Cannot update the ref '%s'.",
3873 update->refname);
3874 ret = TRANSACTION_GENERIC_ERROR;
3875 goto cleanup;
3876 } else {
3842 - /* freed by the above calls: */
3877 + /* freed by the above call: */
3878 update->lock = NULL;
3879 }
3880 }
@@ -3849,7 +3884,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
3884 for (i = 0; i < n; i++) {
3885 struct ref_update *update = updates[i];
3886
3852 - if (update->lock) {
3887 + if (update->flags & REF_DELETING) {
3888 if (delete_ref_loose(update->lock, update->type, err)) {
3889 ret = TRANSACTION_GENERIC_ERROR;
3890 goto cleanup;
t/t1400-update-ref.sh
+2 -2
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
979
980 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
981
982 -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
982 +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
983 (
984 for i in $(test_seq 33)
985 do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
990 )
991 '
992
993 -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
993 +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
994 (
995 for i in $(test_seq 33)
996 do