From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id kGRhHTmT815WagAA0tVLHw (envelope-from ) for ; Wed, 24 Jun 2020 17:54:01 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id 4Lk4GTmT8175EAAA1q6Kng (envelope-from ) for ; Wed, 24 Jun 2020 17:54:01 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 753289400B7 for ; Wed, 24 Jun 2020 17:54:00 +0000 (UTC) Received: from localhost ([::1]:57956 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jo9aw-0007wb-G3 for larch@yhetil.org; Wed, 24 Jun 2020 13:53:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43886) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jo9a2-0006gT-EV for guix-patches@gnu.org; Wed, 24 Jun 2020 13:53:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:55543) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jo9a2-0004u5-4R for guix-patches@gnu.org; Wed, 24 Jun 2020 13:53:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jo9a2-0000xJ-2o for guix-patches@gnu.org; Wed, 24 Jun 2020 13:53:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#42023] [PATCH] database: register-items: reduce transaction scope. Resent-From: Caleb Ristvedt Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Wed, 24 Jun 2020 17:53:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 42023 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 42023@debbugs.gnu.org, Christopher Baines Received: via spool by 42023-submit@debbugs.gnu.org id=B42023.15930211243608 (code B ref 42023); Wed, 24 Jun 2020 17:53:02 +0000 Received: (at 42023) by debbugs.gnu.org; 24 Jun 2020 17:52:04 +0000 Received: from localhost ([127.0.0.1]:38856 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jo9Z5-0000w7-EF for submit@debbugs.gnu.org; Wed, 24 Jun 2020 13:52:03 -0400 Received: from mail-io1-f65.google.com ([209.85.166.65]:41167) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jo9Z3-0000vd-Ta for 42023@debbugs.gnu.org; Wed, 24 Jun 2020 13:52:02 -0400 Received: by mail-io1-f65.google.com with SMTP id o5so3121902iow.8 for <42023@debbugs.gnu.org>; Wed, 24 Jun 2020 10:52:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cune-org.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:in-reply-to:references:user-agent:date :message-id:mime-version; bh=qIuaswtnteYHfnXy1La8UAtAcxVT5kzAh7RuCRvzJSA=; b=ash13EVOu3gnfKUCCjXoTNzu7zmxmzmRzD9eDOno9KtaDs0syZZ9NAsbxRmGP7Y+Yy bsgs/hFcf3iBz1OWnpO9VUGJzRWZbzzZY/dy1kKRejXkWSKD/W0yjrRaar5n8INRkvdu 3hX8vX5w31ryDsjLMgt7gI6UFT13FggNU1vvS9nJ0eURFa4uofSKnyUeoEDv/YgH/6XL i8lphhBHbC3bOK+/7H/ez3hTh35CuQZd37mAAUvRWpVm+PaOEyUpw9E1kc7qWGlDFdDP QYZGD4UCXmYX5FDeuv5m5x1uaXZKvZ3/gdM1UmGm5LywxHL1OTuWaUqHZPeiOh0A3PuS ZBbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references :user-agent:date:message-id:mime-version; bh=qIuaswtnteYHfnXy1La8UAtAcxVT5kzAh7RuCRvzJSA=; b=OQaC9bFyxrcvFxvPRBFr6dtswozJhVWOk/MADr2sPQ6JumXMmyAZVizM+s0efK65pU 4tVduZjOxQ3VEEYAOa3voiN3A5oXem6A1X4qy/VUEuTQy3cdySw73w3oQAuodm+HvPw+ YvtqBGUAU1uT5uNDqHi87fJdlO9ZVgQB+ui5zm6Sc0sXscogXQTkUtAhlJFMWsSyG3yA JzFGrxkbyrEKE2zoWx9rgnM1msKxQktFEvfT+BZkmohiiDzpOqf51phZox8yuII1Aa0X 9ZD+vCWO/idJ/e+8QrFhVBWUhlzEgLp6ZmPRQHcOcNnbewvaWS7tn8BeH8W7DVtynn/4 NFWw== X-Gm-Message-State: AOAM531e1ZNsnl42x4bvuMPKO7OfDAp8CO2B34cTryHNQCHqRA7Djn6x 0xW5LrwW7ChWNwcyGpTfQ+uMRhBNilXWEg== X-Google-Smtp-Source: ABdhPJzCePdhAq1B6ZXIJwS43hpXcypBgKMHHrzakU9kd7yxE38rvzgBku2wMcr9AUarGqBbhU720A== X-Received: by 2002:a05:6638:a83:: with SMTP id 3mr27946443jas.37.1593021114054; Wed, 24 Jun 2020 10:51:54 -0700 (PDT) Received: from GuixPotato ([208.89.170.24]) by smtp.gmail.com with ESMTPSA id f5sm11784799iog.49.2020.06.24.10.51.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jun 2020 10:51:53 -0700 (PDT) From: Caleb Ristvedt In-Reply-To: <87366lxwcd.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Wed, 24 Jun 2020 00:15:30 +0200") References: <20200623163649.32444-1-mail@cbaines.net> <87366lxwcd.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) Date: Wed, 24 Jun 2020 12:51:48 -0500 Message-ID: <87v9jg4aiz.fsf@cune.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-Spam-Score: -0.0 (/) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-Spam-Score: -1.0 (-) X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=fail (rsa verify failed) header.d=cune-org.20150623.gappssmtp.com header.s=20150623 header.b=ash13EVO; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Spam-Score: -2.11 X-TUID: A1fs7ckcc3re --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ludovic Court=C3=A8s writes: >> + (call-with-retrying-transaction db >> + (lambda () > ^^ > Too much indentation (maybe we miss a rule in .dir-locals.el?). My bad, my understanding of our .dir-locals.el was more-or-less cargo culting until I read the lisp-indent-function documentation just now. The 'scheme-indent-function should be 1 for both call-with-transaction and call-with-retrying-transaction. Patch attached. > Two questions: > > 1. Can another process come and fiddle with TO-REGISTER while we=E2=80= =99re > still in =E2=80=98reset-timestamps=E2=80=99? Or can GC happen while= we=E2=80=99re in > =E2=80=98reset-timestamps=E2=80=99 and delete TO-REGISTER and remove= it from the > database? > > I think none of these scenarios can happen, as long as we=E2=80=99ve take= n the > .lock file for TO-REGISTER before, like =E2=80=98finalize-store-file=E2= =80=99 does. The lock file has no bearing on liveness of the path it locks, though the liveness of the path it locks *does* count as liveness for the lock file and for the .chroot file; see tryToDelete() in nix/libstore/gc.cc. (Well, semi-liveness; .lock and .chroot files won't show up in a list of live paths, but they will still be protected from deletion if their associated store item is a temp root) So general "fiddling" can't happen, but GC can. The responsibility for both locking and registering as temporary roots falls on the caller, currently, as I believe it should. We may want to document this responsibility in the docstring for register-items, though. So currently finalize-store-file is safe from clobbering by another process attempting to finalize to the same path, but not safe from garbage collection between when the temporary store file is renamed and when it is registered. It needs an add-temp-root prior to renaming. > > 2. After the transaction, TO-REGISTER is considered valid. But are > the effects of the on-going deduplication observable, due to > non-atomicity of some operation? Subdirectories of store items need to be made writable prior to renaming the temp link, so there will necessarily be a window of time during which various subdirectories will appear writable. I don't think this should cause problems; we already assume that the .lock file is held, so we should be the only process attempting to deduplicate it. On a related note, we should probably use dynamic-wind for setting and restoring the permissions in replace-with-link. Also, replace-with-link doesn't check whether the "containing directory" is the store like optimisePath_() does, so in theory if another process tried to make a permanent change to the store's permissions it could be clobbered when replace-with-link restores the permissions. I don't know of any instance where this could happen currently, but it's something to keep in mind. And, of course, there's the inherent visible change of deduplication - possible modification of inode number, but I don't see how that could cause problems with any reasonable programs. > I think the =E2=80=98replace-with-link=E2=80=99 dance is atomic, so we sh= ould be fine. > > Thoughts? replace-with-link is atomic, but it can fail if the "canonical" link in .links is deleted by the garbage collector between when its existence is checked in 'deduplicate' and when temp link creation in replace-with-link happens. Then ENOENT would be returned, and 'deduplicate' wouldn't catch that exception, nor would optimisePath_() in nix/libstore/optimise-store.cc. The proper behavior there, in my opinion, would be to retry the deduplication. Attached is a patch that makes 'deduplicate' do that. Also, while I'm perusing optimisePath_(), there's a minor bug in which EMLINK generated by renaming the temp link may still be treated as a fatal error. This is because errno may be clobbered by unlink() prior to being checked (according to the errno man page it can still be modified even if the call succeeds). In summary, there are potential issues, but nothing that should be affected by reducing the transaction scope AFAIK. =2D reepca --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-deduplication-retry-on-ENOENT.patch Content-Transfer-Encoding: quoted-printable From=20461064da9e169df3dd939b734bb2860598d113f4 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Wed, 24 Jun 2020 00:56:50 -0500 Subject: [PATCH 1/2] deduplication: retry on ENOENT. It's possible for the garbage collector to remove the "canonical" link after it's been detected as existing by 'deduplicate'. This would cause an ENOENT error when replace-with-link attempts to create the temporary link. This changes it so that it will properly handle that by retrying. * guix/store/deduplication.scm (deduplicate): retry on ENOENT. =2D-- guix/store/deduplication.scm | 64 +++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/guix/store/deduplication.scm b/guix/store/deduplication.scm index 6784ee0b92..b6d94e49c2 100644 =2D-- a/guix/store/deduplication.scm +++ b/guix/store/deduplication.scm @@ -161,26 +161,44 @@ under STORE." (scandir* path)) (let ((link-file (string-append links-directory "/" (bytevector->nix-base32-string has= h)))) =2D (if (file-exists? link-file) =2D (replace-with-link link-file path =2D #:swap-directory links-directory) =2D (catch 'system-error =2D (lambda () =2D (link path link-file)) =2D (lambda args =2D (let ((errno (system-error-errno args))) =2D (cond ((=3D errno EEXIST) =2D ;; Someone else put an entry for PATH in =2D ;; LINKS-DIRECTORY before we could. Let's us= e it. =2D (replace-with-link path link-file =2D #:swap-directory links-dir= ectory)) =2D ((=3D errno ENOSPC) =2D ;; There's not enough room in the directory i= ndex for =2D ;; more entries in .links, but that's fine: w= e can =2D ;; just stop. =2D #f) =2D ((=3D errno EMLINK) =2D ;; PATH has reached the maximum number of lin= ks, but =2D ;; that's OK: we just can't deduplicate it mo= re. =2D #f) =2D (else (apply throw args))))))))))) + (let retry () + (if (file-exists? link-file) + (catch 'system-error + (lambda () + (replace-with-link link-file path + #:swap-directory links-directory)) + (lambda args + (if (and (=3D (system-error-errno args) ENOENT) + ;; ENOENT can be produced because: + ;; - LINK-FILE has missing directory componen= ts + ;; - LINKS-DIRECTORY has missing directory + ;; components + ;; - PATH has missing directory components + ;; + ;; the last two are errors, the first just + ;; requires a retry. + (file-exists? (dirname path)) + (file-exists? links-directory)) + (retry) + (apply throw args)))) + (catch 'system-error + (lambda () + (link path link-file)) + (lambda args + (let ((errno (system-error-errno args))) + (cond ((=3D errno EEXIST) + ;; Someone else put an entry for PATH in + ;; LINKS-DIRECTORY before we could. Let's use + ;; it. + (retry)) + ((=3D errno ENOSPC) + ;; There's not enough room in the directory i= ndex + ;; for more entries in .links, but that's fin= e: + ;; we can just stop. + #f) + ((=3D errno EMLINK) + ;; PATH has reached the maximum number of lin= ks, + ;; but that's OK: we just can't deduplicate it + ;; more. + #f) + (else (apply throw args)))))))))))) =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-.dir-locals.el-fix-call-with-retrying-transaction-in.patch Content-Transfer-Encoding: quoted-printable From=20a14ff0a9dab4d73680befaf9d244d6cce0a73ab3 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Wed, 24 Jun 2020 01:00:40 -0500 Subject: [PATCH 2/2] .dir-locals.el: fix call-with-{retrying-}transaction indenting. * .dir-locals.el (call-with-transaction, call-with-retrying-transaction): change scheme-indent-function property from 2 to 1. =2D-- .dir-locals.el | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index 92979fc5ed..84e2738bca 100644 =2D-- a/.dir-locals.el +++ b/.dir-locals.el @@ -88,9 +88,9 @@ (eval . (put 'let-system 'scheme-indent-function 1)) =20 (eval . (put 'with-database 'scheme-indent-function 2)) =2D (eval . (put 'call-with-transaction 'scheme-indent-function 2)) + (eval . (put 'call-with-transaction 'scheme-indent-function 1)) (eval . (put 'with-statement 'scheme-indent-function 3)) =2D (eval . (put 'call-with-retrying-transaction 'scheme-indent-function = 2)) + (eval . (put 'call-with-retrying-transaction 'scheme-indent-function 1)) (eval . (put 'call-with-savepoint 'scheme-indent-function 1)) (eval . (put 'call-with-retrying-savepoint 'scheme-indent-function 1)) =20 =2D-=20 2.26.2 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl7zkrQACgkQwWaqSV9/ GJxIRgf/QT0sGFVpqv3WM9fzF7pNxR48ahSkyE2Y5bTnQBCzqVpwSu2Tf8l6j5GR Fyw25tTWvkid7cuXwsfXBP5/Qe1eh+J2N1XO73idmEoXXubXlz6/eqYLi0Q0KJ3+ 2tDQ0TKiugPvyv9r7UhMNxnGXJgaoJqcmSXLi816Vv2NgV7E7broeulY6e7Txvm4 uXAYbMdV5GiXxJ7XYSgIPiEuP/H3mGjAFScZDlt1kkdEdik523pXHKTaYDB2QBG8 eXd+Uxv3SxS7LDl8u/UxqPdOZGPSBDFKYtR+0SS004F/EC7QmVKc2GiEUobHgu+5 kbKZ0wDcUSNnzzS3bNM2ovQO/9CF+g== =Ueya -----END PGP SIGNATURE----- --==-=-=--