From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ronnie Sahlberg Subject: [PATCH v11 26/41] walker.c: use ref transaction for ref updates Date: Tue, 27 May 2014 13:25:45 -0700 Message-ID: <1401222360-21175-27-git-send-email-sahlberg@google.com> References: <1401222360-21175-1-git-send-email-sahlberg@google.com> Cc: mhagger@alum.mit.edu, Ronnie Sahlberg To: git@vger.kernel.org X-From: git-owner@vger.kernel.org Tue May 27 22:26:44 2014 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1WpNxJ-0002RS-Du for gcvg-git-2@plane.gmane.org; Tue, 27 May 2014 22:26:41 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753299AbaE0U0h (ORCPT ); Tue, 27 May 2014 16:26:37 -0400 Received: from mail-yh0-f74.google.com ([209.85.213.74]:55739 "EHLO mail-yh0-f74.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752981AbaE0U0F (ORCPT ); Tue, 27 May 2014 16:26:05 -0400 Received: by mail-yh0-f74.google.com with SMTP id 29so1645979yhl.5 for ; Tue, 27 May 2014 13:26:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=dF9KQcOJPCzlK/MZYZCcn2gVzP+APxWpJhet6//hC/w=; b=TZnyjrNKCh0TW8T8UKz04O3I+wopjlHYGqPl5uwVSHSxv5jl8kfQAASZla/2cjrWo6 gtKT8EZfXRxulQWz7f4JH14VgFDR6l+FFRkRYib5kPSs6X0cBjSx853F9zFwwhoruMtN sA++xMJnHbGp3t6bfsDzEAxmGx4ipT/IZiRA8gAGrjs0N4BxSIF0erSOrnSuwn074bNh P4URX0hUzRAFdUflMTImxYL2qErzcxRiW25l25EILWOCSCYa9gL4avEKARJTxQzBOykH 2nm0qwRxreWk3+uUiqaVasmXuCJFq91mU1G5o3ftfSBqccWzyXkd2BV60eB0NAZEkgGg eF7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=dF9KQcOJPCzlK/MZYZCcn2gVzP+APxWpJhet6//hC/w=; b=iOaI3dlOM6cV2FpO9+7/V4yxk7br46KWYmtztMcB3VnpcPR5tT48/VfkASKT8SpZjx AX7gj8flOINssQWj3wPL1HedXVSYq9E2o8eqEdNREroPlF1fncP3zwo4IGtY99sU1LmS /uQZrNS7RE4F5/94g2fTW0mMhi70orxxHZxeMoHYJ9vNfwgx+/OSp0W4L/BVbEQ2tct7 srFzVV/xyf4uzGTlLyT5j5bbhk0jY9MJI4co/NZTd7xMJtNd5gLw42VEjdBiAMUCoXnn eexJQqV9047XLcq+vmxCVE0I8ZMU1RPBsCWINYJT0WS8gNCIzfCTzWYVCP2vcAVKLPKT khfg== X-Gm-Message-State: ALoCoQkGaRhbNRfP6jboVN8PMTmk06Khi6ks7rmmRcjZQSLDVmNmUQGYgk/vd/M1xzR54kNAo2bl X-Received: by 10.52.142.72 with SMTP id ru8mr12483971vdb.0.1401222365062; Tue, 27 May 2014 13:26:05 -0700 (PDT) Received: from corp2gmr1-2.hot.corp.google.com (corp2gmr1-2.hot.corp.google.com [172.24.189.93]) by gmr-mx.google.com with ESMTPS id i65si1283653yhg.2.2014.05.27.13.26.05 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 27 May 2014 13:26:05 -0700 (PDT) Received: from sahlberg1.mtv.corp.google.com (sahlberg1.mtv.corp.google.com [172.27.69.52]) by corp2gmr1-2.hot.corp.google.com (Postfix) with ESMTP id C15222F4AB9; Tue, 27 May 2014 13:26:04 -0700 (PDT) Received: by sahlberg1.mtv.corp.google.com (Postfix, from userid 177442) id 9F6B7E0DE4; Tue, 27 May 2014 13:26:04 -0700 (PDT) X-Mailer: git-send-email 2.0.0.rc3.474.g0203784 In-Reply-To: <1401222360-21175-1-git-send-email-sahlberg@google.com> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: Switch to using ref transactions in walker_fetch(). As part of the refactoring to use ref transactions we also fix a potential memory leak where in the original code if write_ref_sha1() would fail we would end up returning from the function without free()ing the msg string. This changes the locking slightly for walker_fetch. Previously the code would lock all refs before writing them but now we do not lock the refs until the commit stage. There is thus a very short window where changes could be done locally during the fetch which would be overwritten when the fetch completes and commits its transaction. But this window should be reasonably short. Even if this race does trigger, since both the old code and the new code just overwrites the refs to the new values without checking or comparing them with the previous value, this is not too dissimilar to a similar scenario where you first do a ref change locally and then later do a fetch that overwrites the local change. With this in mind I do not see the change in locking semantics to be critical. Note that this function is only called when fetching from a remote HTTP repository onto the local (most of the time single-user) repository which likely means that the type of collissions that the previous locking would protect against and cause the fetch to fail for to be even more rare. Signed-off-by: Ronnie Sahlberg --- walker.c | 56 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/walker.c b/walker.c index 1dd86b8..51ce1c6 100644 --- a/walker.c +++ b/walker.c @@ -251,39 +251,37 @@ void walker_targets_free(int targets, char **target, const char **write_ref) int walker_fetch(struct walker *walker, int targets, char **target, const char **write_ref, const char *write_ref_log_details) { - struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *)); + struct strbuf ref_name = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; unsigned char *sha1 = xmalloc(targets * 20); char *msg; - int ret; int i; save_commit_buffer = 0; - for (i = 0; i < targets; i++) { - if (!write_ref || !write_ref[i]) - continue; - - lock[i] = lock_ref_sha1(write_ref[i], NULL); - if (!lock[i]) { - error("Can't lock ref %s", write_ref[i]); - goto unlock_and_fail; + if (write_ref) { + transaction = ref_transaction_begin(&err); + if (!transaction) { + error("%s", err.buf); + strbuf_release(&err); + return -1; } } - if (!walker->get_recover) for_each_ref(mark_complete, NULL); for (i = 0; i < targets; i++) { if (interpret_target(walker, target[i], &sha1[20 * i])) { error("Could not interpret response from server '%s' as something to pull", target[i]); - goto unlock_and_fail; + goto rollback_and_fail; } if (process(walker, lookup_unknown_object(&sha1[20 * i]))) - goto unlock_and_fail; + goto rollback_and_fail; } if (loop(walker)) - goto unlock_and_fail; + goto rollback_and_fail; if (write_ref_log_details) { msg = xmalloc(strlen(write_ref_log_details) + 12); @@ -294,19 +292,31 @@ int walker_fetch(struct walker *walker, int targets, char **target, for (i = 0; i < targets; i++) { if (!write_ref || !write_ref[i]) continue; - ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)"); - lock[i] = NULL; - if (ret) - goto unlock_and_fail; + strbuf_reset(&ref_name); + strbuf_addf(&ref_name, "refs/%s", write_ref[i]); + if (ref_transaction_update(transaction, ref_name.buf, + &sha1[20 * i], NULL, 0, 0, + &err)) + break; + } + if (write_ref) { + if (ref_transaction_commit(transaction, + msg ? msg : "fetch (unknown)", + &err)) { + error("%s", err.buf); + ref_transaction_free(transaction); + goto rollback_and_fail; + } + ref_transaction_free(transaction); } - free(msg); + free(msg); return 0; -unlock_and_fail: - for (i = 0; i < targets; i++) - if (lock[i]) - unlock_ref(lock[i]); +rollback_and_fail: + free(msg); + strbuf_release(&err); + strbuf_release(&ref_name); return -1; } -- 2.0.0.rc3.474.g0203784