[bug#42023] [PATCH] database: register-items: reduce transaction scope.

[Caleb Ristvedt <caleb.ristvedt@cune.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4153 bytes --]

Ludovic Courtès <ludo@gnu.org> 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’re
>      still in ‘reset-timestamps’?  Or can GC happen while we’re in
>      ‘reset-timestamps’ and delete TO-REGISTER and remove it from the
>      database?

>
> I think none of these scenarios can happen, as long as we’ve taken the
> .lock file for TO-REGISTER before, like ‘finalize-store-file’ 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 ‘replace-with-link’ dance is atomic, so we should 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.

- reepca


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-deduplication-retry-on-ENOENT.patch --]
[-- Type: text/x-patch, Size: 4619 bytes --]

From 461064da9e169df3dd939b734bb2860598d113f4 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
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.
---
 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
--- 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 hash))))
-          (if (file-exists? link-file)
-              (replace-with-link link-file path
-                                 #:swap-directory links-directory)
-              (catch 'system-error
-                (lambda ()
-                  (link path link-file))
-                (lambda args
-                  (let ((errno (system-error-errno args)))
-                    (cond ((= errno EEXIST)
-                           ;; Someone else put an entry for PATH in
-                           ;; LINKS-DIRECTORY before we could.  Let's use it.
-                           (replace-with-link path link-file
-                                              #:swap-directory links-directory))
-                          ((= errno ENOSPC)
-                           ;; There's not enough room in the directory index for
-                           ;; more entries in .links, but that's fine: we can
-                           ;; just stop.
-                           #f)
-                          ((= errno EMLINK)
-                           ;; PATH has reached the maximum number of links, but
-                           ;; that's OK: we just can't deduplicate it more.
-                           #f)
-                          (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 (= (system-error-errno args) ENOENT)
+                             ;; ENOENT can be produced because:
+                             ;; - LINK-FILE has missing directory components
+                             ;; - 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 ((= errno EEXIST)
+                             ;; Someone else put an entry for PATH in
+                             ;; LINKS-DIRECTORY before we could.  Let's use
+                             ;; it.
+                             (retry))
+                            ((= errno ENOSPC)
+                             ;; There's not enough room in the directory index
+                             ;; for more entries in .links, but that's fine:
+                             ;; we can just stop.
+                             #f)
+                            ((= errno EMLINK)
+                             ;; PATH has reached the maximum number of links,
+                             ;; but that's OK: we just can't deduplicate it
+                             ;; more.
+                             #f)
+                            (else (apply throw args))))))))))))
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-.dir-locals.el-fix-call-with-retrying-transaction-in.patch --]
[-- Type: text/x-patch, Size: 1247 bytes --]

From a14ff0a9dab4d73680befaf9d244d6cce0a73ab3 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
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.
---
 .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
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -88,9 +88,9 @@
    (eval . (put 'let-system 'scheme-indent-function 1))
 
    (eval . (put 'with-database 'scheme-indent-function 2))
-   (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))
-   (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))
 
-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]