From patchwork Thu Nov 4 12:38:22 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 12603011 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 CBFBBC433F5 for ; Thu, 4 Nov 2021 12:38:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A801461076 for ; Thu, 4 Nov 2021 12:38:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230304AbhKDMlW (ORCPT ); Thu, 4 Nov 2021 08:41:22 -0400 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:54829 "EHLO out2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229809AbhKDMlV (ORCPT ); Thu, 4 Nov 2021 08:41:21 -0400 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 4409A5C0112 for ; Thu, 4 Nov 2021 08:38:43 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Thu, 04 Nov 2021 08:38:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:subject:message-id:mime-version:content-type; s=fm2; bh=rnVl5AYZbiVD9K0V9rbtCRVVBQxgiYdLx4ohVEOpILE=; b=qBiRRVbvdAsK rYwRE6x+++qq6LYbznK7FVtuqkcNMIrc+gViB7SZI2ZB2PYa+r3NbGzKxzOfFnnC cHVMwm2jmE8AWudRQeUWoXJMuzR7M3YI8VJpFc0RkDveVbtz36/0X7xKzRg+EsE6 xaClNkY30IrpYFDsVaKWBeXFdHMUCvUqLojq3dcKEWmOrgpFXllcW1twsg8d3u23 idpbD/GmfYQpp3fx8ZD7fLenG0LpNLd6+/ss8Mznzq0Izj9h9mkJSjKiWnjNtcfW pDusNL2x5lfcA97RD8xgHsIqvWxbSj3Di3q6IKLT8GQjTjdNoKd8b8bxpqVoLgRT 9XsUEhuEDw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=content-type:date:from:message-id :mime-version:subject:to:x-me-proxy:x-me-proxy:x-me-sender :x-me-sender:x-sasl-enc; s=fm1; bh=rnVl5AYZbiVD9K0V9rbtCRVVBQxgi YdLx4ohVEOpILE=; b=kj7hEO3tB/B+azrYDmEo1WruVfo/ekVK1+v0R5kFbyvZe BVVo8XK0kxhScFRIjPLJK9GyRo7+u8tLQeH6S1SxDD5wolnysFSDPqAC05P6ONbd oygujgkng8/8/eT6Fgp5pxgra8AaxupAFCyBnlEby9udgN9/uBuSuVWtkkhlV7Do NtSse95NZTD5THHF4DPd+4XJa0+4FDv+F3AFV0/tu9lNjuTvo1Abzwa3PtT24hAt WHFyt/y88w7TDaVKHjKHN7xU5RqVDXJYs4lkrpkCHpzo/ZxZLRPyK1WSOAzV9kYS snUcCObbSzM8OzkMiYjxTGk+u7/WzrB3KcPJacwdg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrtdeggdefkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecunecujfgurhepfffhvffukfggtggusehgtderredttd dvnecuhfhrohhmpefrrghtrhhitghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdr ihhmqeenucggtffrrghtthgvrhhnpeffjeehvdfgudefffefffelueekgfevieektdejvd dukeeugeekudehtdduffffjeenucffohhmrghinhepkhgvrhhnvghlrdhorhhgnecuvehl uhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrd himh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA for ; Thu, 4 Nov 2021 08:38:42 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 693e8966 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO) for ; Thu, 4 Nov 2021 14:26:49 +0000 (UTC) Date: Thu, 4 Nov 2021 13:38:22 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Subject: [PATCH] refs: sync loose refs to disk before committing them Message-ID: MIME-Version: 1.0 Content-Disposition: inline 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 always flushing refs to disk before committing them into place to avoid this class of corruption. [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt Signed-off-by: Patrick Steinhardt --- refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 151b0056fe..06a3f0bdea 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1749,6 +1749,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 || + fsync(fd) < 0 || close_ref_gently(lock) < 0) { strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(&lock->lk));