From patchwork Wed Nov 10 11:40:54 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 12611925 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F34B3C433F5 for ; Wed, 10 Nov 2021 11:41:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D480B611BF for ; Wed, 10 Nov 2021 11:41:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231325AbhKJLoI (ORCPT ); Wed, 10 Nov 2021 06:44:08 -0500 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:59013 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230440AbhKJLoG (ORCPT ); Wed, 10 Nov 2021 06:44:06 -0500 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 6816F5C020F; Wed, 10 Nov 2021 06:41:18 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Wed, 10 Nov 2021 06:41:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=MShJwPBeKt0Sc41wjqCPpBdkteI dXABJdu8HFg3o2UI=; b=HDZ56wgIrPMc/d2xEc+hKNES9XjhQ+6cBikl36wJIqV MKC3rJIE9w/49vzzq12rvwQHLfwB7xOHP8QDG8+uJ+T8HyhpdDTyudCx5WJ2K67u 9TmVVN9cFan5Vju5ZijwX1V4cOU4LQBIDa3nQy+SAzU90b26NF7lic8gqtGT0bDZ kPRY55VzM6aqVbIg2A4gd41oWGOnA4Fchrdrb6Jgi3mGsessXqUs7xZRzQXmGpSi wlS69hvIQXnApyD9wji2keVYzBVQ0+fTFGjO73U7bgwPTR0Xr2wfbOVbq7XhuoAj DoifLQ3HqBqoWMQgCNKd4WUklgUAhgV9xfpv61fLJSA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=MShJwP BeKt0Sc41wjqCPpBdkteIdXABJdu8HFg3o2UI=; b=ZiMMw0QDLeIcstSvCCKmdS TwnRhAmZ0gqv8EN2ZnEWPyYP04JpbvzCiQ16hSA5rUCtVZ1vG4CzhseeQpp4dS6G jMgCetjx1KzkdrrW/0dz3gNmwIPOc+Qmq9O42aRleZaCfP6fSTOdzN0wag9wDCTp eXQt7QwSW8h+cuqECjcG0fhuRsUKY9jBuV4bMjAUbtSIDWeJSxA5odgBYkc8BY0I X50AQ0xcoz1L9QmP1wIXCnlRCN5MMUpaIT+oCC6cfBipqbghfX7eRZqa8X46LxfA w/Yjkw3b/otF4T6IucMh0BW3jigRdgv/CXWdUK6vxWambmbVK6TS70QgmlhkRocQ == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrudejgdeftdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfhnecu vehluhhsthgvrhfuihiivgepudenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkh hsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 10 Nov 2021 06:41:17 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id ed12fa1f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 10 Nov 2021 13:26:27 +0000 (UTC) Date: Wed, 10 Nov 2021 12:40:54 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Jeff King , Johannes Schindelin , =?iso-8859-1?q?=C6var_Arn?= =?iso-8859-1?q?fj=F6r=F0?= Bjarmason , Junio C Hamano , Eric Wong , "Neeraj K. Singh" Subject: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()` Message-ID: <0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org While we already have a wrapper around `git_fsync()`, this wrapper doesn't handle looping around `EINTR`. Convert it to do so such that callers don't have to remember doing this everywhere. Signed-off-by: Patrick Steinhardt --- wrapper.c | 9 ++++++++- write-or-die.c | 6 ++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/wrapper.c b/wrapper.c index ece3d2ca10..e20df4f3a6 100644 --- a/wrapper.c +++ b/wrapper.c @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode) return fd; } -int git_fsync(int fd, enum fsync_action action) +static int git_fsync_once(int fd, enum fsync_action action) { switch (action) { case FSYNC_WRITEOUT_ONLY: @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action) default: BUG("unexpected git_fsync(%d) call", action); } +} +int git_fsync(int fd, enum fsync_action action) +{ + while (git_fsync_once(fd, action) < 0) + if (errno != EINTR) + return -1; + return 0; } static int warn_if_unremovable(const char *op, const char *file, int rc) diff --git a/write-or-die.c b/write-or-die.c index cc8291d979..e61220aa2d 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) void fsync_or_die(int fd, const char *msg) { - while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) { - if (errno != EINTR) - die_errno("fsync error on '%s'", msg); - } + if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) + die_errno("fsync error on '%s'", msg); } void write_or_die(int fd, const void *buf, size_t count) From patchwork Wed Nov 10 11:40:59 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 12611927 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7DD1C433FE for ; Wed, 10 Nov 2021 11:41:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D278261152 for ; Wed, 10 Nov 2021 11:41:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231338AbhKJLoL (ORCPT ); Wed, 10 Nov 2021 06:44:11 -0500 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:35795 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230440AbhKJLoK (ORCPT ); Wed, 10 Nov 2021 06:44:10 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id E8E5C5C014F; Wed, 10 Nov 2021 06:41:22 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Wed, 10 Nov 2021 06:41:22 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=6WTQzSMvK+xPUV3rDpoZhrJ1O3M 7bH99gMwKIaED3NY=; b=CWmRUoHFDOsrlrr8NhLYuKm57ovAIao/1iABGhYjhTF y5v4sZPl4/RmmmK0moyV8wwqxojtG4l3C61+T+JlR2zzyczHPCsTBLZb+cT3SC3g ml9IeLpDNmL7tWt0rtTP5AlzIh1+7xAU7ClbnNYym/VNegUrOVOm1Z7Lv9Ho69Rw jwklk7YWu+RKLs6FqDdvFEZkruqygL/czZbtEckARCmQns7MsqNhRn9ET+mthb/U wW9VKO7lsGEVHkwg01CGuILthYezLmCKBU/d0z3OWqdwcyIgCIUndxFXcoyENUcz 2VMdpRjxvs8xlwb8is1l4BWWeshUXv+xEa+f510i/bg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=6WTQzS MvK+xPUV3rDpoZhrJ1O3M7bH99gMwKIaED3NY=; b=UB4HJ+ehXwdTkr89MxW9oV lmxfA4ElLDHhBhQXQqdZXowUYyDVNW89/ztIfT09aruuYi8tSmA6CqQYrLpHxY+A BlOgZ53A9tSypnzIHQp4qn2t4gNHxm0Ar3d3IwSojojXB9woGaV72VmU4A9CO/He QpHU0Q5K0/1GRhraRAwJCycLh1s7+vNTOcYYQ+zamWbhQrjXsf3O27W2R1ZY2PQg 4RUh0zuU9FrnsxbCmTWL1lAWvAVfLGefIdn1qwM15SbzaweyRN+hY4QoHKrKHAZ5 AloA7w4VchA3nrx1Kwufxm5zbpyw1zlXnaCgsatUqdVIKY1ovTCSF4ftxVHL/wcw == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrudejgdeftdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfhnecu vehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkh hsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 10 Nov 2021 06:41:21 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 723f5b5f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 10 Nov 2021 13:26:32 +0000 (UTC) Date: Wed, 10 Nov 2021 12:40:59 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Jeff King , Johannes Schindelin , =?iso-8859-1?q?=C6var_Arn?= =?iso-8859-1?q?fj=F6r=F0?= Bjarmason , Junio C Hamano , Eric Wong , "Neeraj K. Singh" Subject: [PATCH v2 2/3] wrapper: provide function to sync directories Message-ID: <3ac9d4d7abd224a4c0991f1036f2d95eedb9ceac.1636544377.git.ps@pks.im> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04), we have introduced batched syncing of object files. This mode works by only requesting a writeback of the page cache backing the file on written files, followed by a single hardware-flush via a temporary file created in the directory we want to flush. Given modern journaling file systems, this pattern is expected to be durable. While it's possible to reuse the `git_fsync()` helper to synchronize the page cache only, there is no helper which would allow for doing a hardware flush of a directory by creating a temporary file. Other callers which want to follow the same pattern would thus have to repeat this logic. Extract a new helper `git_fsync_dir()` from the object files code which neatly encapsulates this logic such that it can be reused. Signed-off-by: Patrick Steinhardt --- bulk-checkin.c | 13 +++---------- git-compat-util.h | 7 +++++++ wrapper.c | 21 +++++++++++++++++++++ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/bulk-checkin.c b/bulk-checkin.c index 4deee1af46..e6ebdd1db5 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -98,16 +98,9 @@ static void do_batch_fsync(void) * hardware. */ - if (needs_batch_fsync) { - struct strbuf temp_path = STRBUF_INIT; - struct tempfile *temp; - - strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory()); - temp = xmks_tempfile(temp_path.buf); - fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp)); - delete_tempfile(&temp); - strbuf_release(&temp_path); - } + if (needs_batch_fsync && + git_fsync_dir(get_object_directory()) < 0) + die_errno("fsyncing object directory"); if (bulk_fsync_objdir) tmp_objdir_migrate(bulk_fsync_objdir); diff --git a/git-compat-util.h b/git-compat-util.h index 97f97178e7..f890bd07fd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1221,6 +1221,13 @@ enum fsync_action { int git_fsync(int fd, enum fsync_action action); +/* + * Issue a full hardware flush against a temporary file in the given directory + * to ensure that all files inside that directory are durable before any renames + * occur. + */ +int git_fsync_dir(const char *path); + /* * Preserves errno, prints a message, but gives no warning for ENOENT. * Returns 0 on success, which includes trying to unlink an object that does diff --git a/wrapper.c b/wrapper.c index e20df4f3a6..6c6cc8b74f 100644 --- a/wrapper.c +++ b/wrapper.c @@ -3,6 +3,7 @@ */ #include "cache.h" #include "config.h" +#include "tempfile.h" static int memory_limit_check(size_t size, int gentle) { @@ -601,6 +602,26 @@ int git_fsync(int fd, enum fsync_action action) return 0; } +int git_fsync_dir(const char *path) +{ + struct strbuf temp_path = STRBUF_INIT; + struct tempfile *temp; + + strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", path); + + temp = mks_tempfile(temp_path.buf); + if (!temp) + return -1; + + if (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0) + return -1; + + delete_tempfile(&temp); + strbuf_release(&temp_path); + + return 0; +} + static int warn_if_unremovable(const char *op, const char *file, int rc) { int err; From patchwork Wed Nov 10 11:41:03 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 12611929 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A665BC433F5 for ; Wed, 10 Nov 2021 11:41:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 82EC161152 for ; Wed, 10 Nov 2021 11:41:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231455AbhKJLoP (ORCPT ); Wed, 10 Nov 2021 06:44:15 -0500 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:34915 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231441AbhKJLoO (ORCPT ); Wed, 10 Nov 2021 06:44:14 -0500 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 946B05C01F6; Wed, 10 Nov 2021 06:41:26 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Wed, 10 Nov 2021 06:41:26 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=vLnUgeSoTGuUePgu5o/SjOJAMTm SUgNbIeT6STEMCOw=; b=GYUKsxe4ca4iTQZHfay0sz0UXHLDjI6AFjE6j2hC6TB aaDZ0busr6jUr1Jzecb/Wav+OxI6jmMT1aK7lVSeBLcxWw2H+VHaMYlOGOkakCX/ mkNV5yVNSBOkPcsmgf6M04hcOT+uFJMBuGygrTS2VZO3TDqHJOmKp6+vUeoYiK38 AHyjo1Hv0EKc0wsb1hdYVcjgl6oUKxJICMEb3/wY7clH4K+pE4evvgEBuJ3xO89c aC9lPQKDa3zSwz/+xH+DUcgTsyYm4zJ575+4fZ8TTDiSQ3FgruYinfIGn+6bAtBT +HmrpJiMd/JcaFCNdsk/WgZCslsFk+wLa+Zd1OHoQ6g== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=vLnUge SoTGuUePgu5o/SjOJAMTmSUgNbIeT6STEMCOw=; b=JngnjlMHMy5QGrISK5VWAK 7WSnZKkJ5E8mczpr65SOnFI2PJOmvsWDutkhvWtdn6lzJNbayUA887aYjv34hB7d fR9KtYWPQriadnNAkuSrueAG65gRS9QFPjayYxjZA2296PB6UFVjVk3hSp+mtKJm tlQC3xvGmhiK0393lNTix1w3cUGpEgdZYy8WXHnE6IAhLZLt5Q73MoZcNLJOe1Ud Dup8CKzUsWhsFN0N9gVKOSWwvPngll5BB2asGJmFrwGits3rcVkILgLnG2chY60U ShNHW6dpVs4bH4WV6/09NzmT2rsHL3IBklCIoImwkZzhJLtXfikTy5p+REOq0Jcg == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrudejgddvlecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epiedvjeefheeiteekgeejveefffdtvedvudfgvdeuheeffeejfeetudeutdefgfegnecu ffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghrufhiiigvpedtnecurf grrhgrmhepmhgrihhlfhhrohhmpehpshesphhkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 10 Nov 2021 06:41:25 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id c351727d (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 10 Nov 2021 13:26:36 +0000 (UTC) Date: Wed, 10 Nov 2021 12:41:03 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Jeff King , Johannes Schindelin , =?iso-8859-1?q?=C6var_Arn?= =?iso-8859-1?q?fj=F6r=F0?= Bjarmason , Junio C Hamano , Eric Wong , "Neeraj K. Singh" Subject: [PATCH v2 3/3] refs: add configuration to enable flushing of refs Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org When writing loose refs, we first create a lockfile, write the new ref into that lockfile, close it and then rename the lockfile into place such that the actual update is atomic for that single ref. While this works as intended under normal circumstences, at GitLab we infrequently encounter corrupt loose refs in repositories after a machine encountered a hard reset. The corruption is always of the same type: the ref has been committed into place, but it is completely empty. The root cause of this is likely that we don't sync contents of the lockfile to disk before renaming it into place. As a result, it's not guaranteed that the contents are properly persisted and one may observe weird in-between states on hard resets. Quoting ext4 documentation [1]: Many broken applications don't use fsync() when replacing existing files via patterns such as fd = open("foo.new")/write(fd,..)/close(fd)/ rename("foo.new", "foo"), or worse yet, fd = open("foo", O_TRUNC)/write(fd,..)/close(fd). If auto_da_alloc is enabled, ext4 will detect the replace-via-rename and replace-via-truncate patterns and force that any delayed allocation blocks are allocated such that at the next journal commit, in the default data=ordered mode, the data blocks of the new file are forced to disk before the rename() operation is committed. This provides roughly the same level of guarantees as ext3, and avoids the "zero-length" problem that can happen when a system crashes before the delayed allocation blocks are forced to disk. This explicitly points out that one must call fsync(3P) before doing the rename(3P) call, or otherwise data may not be correctly persisted to disk. Fix this by introducing a new configuration "core.fsyncRefFiles". This config matches behaviour of "core.fsyncObjectFiles" in that it provides three different modes: - "off" disables calling fsync on ref files. This is the default behaviour previous to this change and remains the default after this change. - "on" enables calling fsync on ref files, where each reference is flushed to disk before it is being committed. This is the safest setting, but may incur significant performance overhead. - "batch" will flush the page cache of each file as it is written to ensure its data is persisted. After all refs have been written, the directories which host refs are flushed. With this change in place and when "core.fsyncRefFiles" is set to either "on" or "batch", this kind of corruption shouldn't happen anymore. [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt Signed-off-by: Patrick Steinhardt --- Documentation/config/core.txt | 7 ++++ cache.h | 7 ++++ config.c | 10 ++++++ environment.c | 1 + refs/files-backend.c | 62 ++++++++++++++++++++++++++++++++++- 5 files changed, 86 insertions(+), 1 deletion(-) diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index 200b4d9f06..e2fd0d8c90 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -572,6 +572,13 @@ core.fsyncObjectFiles:: stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or ReFS. +core.fsyncRefFiles:: + A value indicating the level of effort Git will expend in trying to make + refs added to the repo durable in the event of an unclean system + shutdown. This setting currently only controls loose refs in the object + store, so updates to packed refs may not be equally durable. Takes the + same parameters as `core.fsyncObjectFiles`. + core.preloadIndex:: Enable parallel index preload for operations like 'git diff' + diff --git a/cache.h b/cache.h index 6d6e6770ec..14c8fab6b4 100644 --- a/cache.h +++ b/cache.h @@ -991,7 +991,14 @@ enum fsync_object_files_mode { FSYNC_OBJECT_FILES_BATCH }; +enum fsync_ref_files_mode { + FSYNC_REF_FILES_OFF, + FSYNC_REF_FILES_ON, + FSYNC_REF_FILES_BATCH +}; + extern enum fsync_object_files_mode fsync_object_files; +extern enum fsync_ref_files_mode fsync_ref_files; extern int core_preload_index; extern int precomposed_unicode; extern int protect_hfs; diff --git a/config.c b/config.c index 5eb36ecd77..4cbad5a29d 100644 --- a/config.c +++ b/config.c @@ -1500,6 +1500,16 @@ static int git_default_core_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "core.fsyncreffiles")) { + if (value && !strcmp(value, "batch")) + fsync_ref_files = FSYNC_REF_FILES_BATCH; + else if (git_config_bool(var, value)) + fsync_ref_files = FSYNC_REF_FILES_ON; + else + fsync_ref_files = FSYNC_REF_FILES_OFF; + return 0; + } + if (!strcmp(var, "core.preloadindex")) { core_preload_index = git_config_bool(var, value); return 0; diff --git a/environment.c b/environment.c index aeafe80235..1514ac9384 100644 --- a/environment.c +++ b/environment.c @@ -43,6 +43,7 @@ const char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; enum fsync_object_files_mode fsync_object_files; +enum fsync_ref_files_mode fsync_ref_files; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 96 * 1024 * 1024; diff --git a/refs/files-backend.c b/refs/files-backend.c index 4b14f30d48..31d7456266 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1360,6 +1360,57 @@ static int commit_ref_update(struct files_ref_store *refs, const struct object_id *oid, const char *logmsg, struct strbuf *err); +static int sync_loose_ref(int fd) +{ + switch (fsync_ref_files) { + case FSYNC_REF_FILES_OFF: + return 0; + case FSYNC_REF_FILES_ON: + return git_fsync(fd, FSYNC_HARDWARE_FLUSH); + case FSYNC_REF_FILES_BATCH: + return git_fsync(fd, FSYNC_WRITEOUT_ONLY); + default: + BUG("invalid fsync mode %d", fsync_ref_files); + } +} + +#define SYNC_LOOSE_REF_GITDIR (1 << 0) +#define SYNC_LOOSE_REF_COMMONDIR (1 << 1) + +static int sync_loose_refs_flags(const char *refname) +{ + switch (ref_type(refname)) { + case REF_TYPE_PER_WORKTREE: + case REF_TYPE_PSEUDOREF: + return SYNC_LOOSE_REF_GITDIR; + case REF_TYPE_MAIN_PSEUDOREF: + case REF_TYPE_OTHER_PSEUDOREF: + case REF_TYPE_NORMAL: + return SYNC_LOOSE_REF_COMMONDIR; + default: + BUG("unknown ref type %d of ref %s", + ref_type(refname), refname); + } +} + +static int sync_loose_refs(struct files_ref_store *refs, + int flags, + struct strbuf *err) +{ + if (fsync_ref_files != FSYNC_REF_FILES_BATCH) + return 0; + + if ((flags & SYNC_LOOSE_REF_GITDIR) && + git_fsync_dir(refs->base.gitdir) < 0) + return -1; + + if ((flags & SYNC_LOOSE_REF_COMMONDIR) && + git_fsync_dir(refs->gitcommondir) < 0) + return -1; + + return 0; +} + /* * Emit a better error message than lockfile.c's * unable_to_lock_message() would in case there is a D/F conflict with @@ -1502,6 +1553,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, oidcpy(&lock->old_oid, &orig_oid); if (write_ref_to_lockfile(lock, &orig_oid, &err) || + sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) || commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) { error("unable to write current sha1 into %s: %s", newrefname, err.buf); strbuf_release(&err); @@ -1522,6 +1574,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, flag = log_all_ref_updates; log_all_ref_updates = LOG_REFS_NONE; if (write_ref_to_lockfile(lock, &orig_oid, &err) || + sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) || commit_ref_update(refs, lock, &orig_oid, NULL, &err)) { error("unable to write current sha1 into %s: %s", oldrefname, err.buf); strbuf_release(&err); @@ -1781,6 +1834,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, fd = get_lock_file_fd(&lock->lk); if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 || write_in_full(fd, &term, 1) < 0 || + sync_loose_ref(fd) < 0 || close_ref_gently(lock) < 0) { strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(&lock->lk)); @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, files_downcast(ref_store, REF_STORE_WRITE, "ref_transaction_prepare"); size_t i; - int ret = 0; + int ret = 0, sync_flags = 0; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; char *head_ref = NULL; int head_type; @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store, &update->new_oid, NULL, NULL); } + + sync_flags |= sync_loose_refs_flags(update->refname); } + ret = sync_loose_refs(refs, sync_flags, err); + if (ret) + goto cleanup; + if (packed_transaction) { if (packed_refs_lock(refs->packed_ref_store, 0, err)) { ret = TRANSACTION_GENERIC_ERROR;