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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 42023 in the body.
You can then email your comments to 42023 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] database: register-items: reduce transaction scope.
Date: Tue, 23 Jun 2020 17:36:49 +0100

It was made transactional in a4678c6ba18d8dbd79d931f80426eebf61be7ebe, with
the reasoning to prevent broken intermediate states from being visible. I
think this means something like an entry being in ValidPaths, but the Refs not
being inserted.

Using a transaction for this makes sense, but I think using one single
transaction for the whole register-items call is unnecessary to avoid broken
states from being visible, and could block other writes to the store database
while register-items is running. Because the deduplication and resetting
timestamps happens within the transaction as well, even though these things
don't involve the database, writes to the database will still be blocked while
this is happening.

To reduce the potential for register-items to block other writers to the
database for extended periods, this commit moves the transaction to just wrap
the call to sqlite-register. This is the one place where writes occur, so that
should prevent the broken intermediate states issue above. The one difference
this will make is some of the registered items will be visible to other
connections while others may be still being added. I think this is OK, as it's
equivalent to just registering different items.

* guix/store/database.scm (register-items): Reduce transaction scope.
---
 guix/store/database.scm | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/guix/store/database.scm b/guix/store/database.scm
index a38e4d7e52..767335bc0f 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -441,24 +441,25 @@ in the database; #f means \"now\".  Write a progress report to LOG-PORT."
       (when reset-timestamps?
         (reset-timestamps real-file-name))
       (let-values (((hash nar-size) (nar-sha256 real-file-name)))
-        (sqlite-register db #:path to-register
-                         #:references (store-info-references item)
-                         #:deriver (store-info-deriver item)
-                         #:hash (string-append "sha256:"
-                                               (bytevector->base16-string hash))
-                         #:nar-size nar-size
-                         #:time registration-time)
+        (call-with-retrying-transaction db
+            (lambda ()
+              (sqlite-register db #:path to-register
+                               #:references (store-info-references item)
+                               #:deriver (store-info-deriver item)
+                               #:hash (string-append
+                                       "sha256:"
+                                       (bytevector->base16-string hash))
+                               #:nar-size nar-size
+                               #:time registration-time)))
         (when deduplicate?
           (deduplicate real-file-name hash #:store store-dir)))))
 
-  (call-with-retrying-transaction db
-      (lambda ()
-        (let* ((prefix   (format #f "registering ~a items" (length items)))
-               (progress (progress-reporter/bar (length items)
-                                                prefix log-port)))
-          (call-with-progress-reporter progress
-            (lambda (report)
-              (for-each (lambda (item)
-                          (register db item)
-                          (report))
-                        items)))))))
+  (let* ((prefix   (format #f "registering ~a items" (length items)))
+         (progress (progress-reporter/bar (length items)
+                                          prefix log-port)))
+    (call-with-progress-reporter progress
+      (lambda (report)
+        (for-each (lambda (item)
+                    (register db item)
+                    (report))
+                  items)))))
-- 
2.26.2

Message #8 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 42023 <at> debbugs.gnu.org, Caleb Ristvedt <caleb.ristvedt <at> cune.org>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce transaction
 scope.
Date: Wed, 24 Jun 2020 00:15:30 +0200

Hi,

(+Cc: reepca)

Christopher Baines <mail <at> cbaines.net> skribis:

> It was made transactional in a4678c6ba18d8dbd79d931f80426eebf61be7ebe, with
> the reasoning to prevent broken intermediate states from being visible. I
> think this means something like an entry being in ValidPaths, but the Refs not
> being inserted.
>
> Using a transaction for this makes sense, but I think using one single
> transaction for the whole register-items call is unnecessary to avoid broken
> states from being visible, and could block other writes to the store database
> while register-items is running. Because the deduplication and resetting
> timestamps happens within the transaction as well, even though these things
> don't involve the database, writes to the database will still be blocked while
> this is happening.
>
> To reduce the potential for register-items to block other writers to the
> database for extended periods, this commit moves the transaction to just wrap
> the call to sqlite-register. This is the one place where writes occur, so that
> should prevent the broken intermediate states issue above. The one difference
> this will make is some of the registered items will be visible to other
> connections while others may be still being added. I think this is OK, as it's
> equivalent to just registering different items.
>
> * guix/store/database.scm (register-items): Reduce transaction scope.


[...]

> +        (call-with-retrying-transaction db
> +            (lambda ()
             ^^
Too much indentation (maybe we miss a rule in .dir-locals.el?).

> +              (sqlite-register db #:path to-register
> +                               #:references (store-info-references item)
> +                               #:deriver (store-info-deriver item)
> +                               #:hash (string-append
> +                                       "sha256:"
> +                                       (bytevector->base16-string hash))
> +                               #:nar-size nar-size
> +                               #:time registration-time)))

I think it would be good to have a 2-line summary of the rationale right
above ‘call-with-retrying-transaction’.

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.

  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?

I think the ‘replace-with-link’ dance is atomic, so we should be fine.

Thoughts?

Ludo’.

Message #11 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 42023 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce transaction
 scope.
Date: Wed, 24 Jun 2020 12:51:48 -0500

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> 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

[0001-deduplication-retry-on-ENOENT.patch (text/x-patch, attachment)]

[0002-.dir-locals.el-fix-call-with-retrying-transaction-in.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
Cc: 42023 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce transaction
 scope.
Date: Thu, 25 Jun 2020 09:45:14 +0200

[Message part 1 (text/plain, inline)]

Hi,

Caleb Ristvedt <caleb.ristvedt <at> cune.org> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:

[...]

>> 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.

Yes, it would be good to add a sentence to document it.

> 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.

Ah, so we’d need to do that before applying the patch that reduces the
scope of the transaction, right?

>>   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.

Yes.  Here’s a patch for ‘dynamic-wind’:

[Message part 2 (text/x-patch, inline)]

diff --git a/.dir-locals.el b/.dir-locals.el
index 92979fc5ed..155255a770 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -37,6 +37,7 @@
    (eval . (put 'with-file-lock 'scheme-indent-function 1))
    (eval . (put 'with-file-lock/no-wait 'scheme-indent-function 1))
    (eval . (put 'with-profile-lock 'scheme-indent-function 1))
+   (eval . (put 'with-writable-file 'scheme-indent-function 1))
 
    (eval . (put 'package 'scheme-indent-function 0))
    (eval . (put 'origin 'scheme-indent-function 0))
diff --git a/guix/store/deduplication.scm b/guix/store/deduplication.scm
index 6784ee0b92..af52c03370 100644
--- a/guix/store/deduplication.scm
+++ b/guix/store/deduplication.scm
@@ -94,6 +94,20 @@ LINK-PREFIX."
             (try (tempname-in link-prefix))
             (apply throw args))))))
 
+(define (call-with-writable-file file thunk)
+  (let ((stat (lstat file)))
+    (dynamic-wind
+      (lambda ()
+        (make-file-writable file))
+      thunk
+      (lambda ()
+        (set-file-time file stat)
+        (chmod file (stat:mode stat))))))
+
+(define-syntax-rule (with-writable-file file exp ...)
+  "Make FILE writable for the dynamic extent of EXP..."
+  (call-with-writable-file file (lambda () exp ...)))
+
 ;; There are 3 main kinds of errors we can get from hardlinking: "Too many
 ;; things link to this" (EMLINK), "this link already exists" (EEXIST), and
 ;; "can't fit more stuff in this directory" (ENOSPC).
@@ -120,20 +134,14 @@ Note: TARGET, TO-REPLACE, and SWAP-DIRECTORY must be on the same file system."
   ;; If we couldn't create TEMP-LINK, that's OK: just don't do the
   ;; replacement, which means TO-REPLACE won't be deduplicated.
   (when temp-link
-    (let* ((parent (dirname to-replace))
-           (stat   (stat parent)))
-      (make-file-writable parent)
+    (with-writable-file (dirname to-replace)
       (catch 'system-error
         (lambda ()
           (rename-file temp-link to-replace))
         (lambda args
           (delete-file temp-link)
           (unless (= EMLINK (system-error-errno args))
-            (apply throw args))))
-
-      ;; Restore PARENT's mtime and permissions.
-      (set-file-time parent stat)
-      (chmod parent (stat:mode stat)))))
+            (apply throw args)))))))
 
 (define* (deduplicate path hash #:key (store %store-directory))
   "Check if a store item with sha256 hash HASH already exists.  If so,

[Message part 3 (text/plain, inline)]

> 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.

I guess we should also avoid changing permissions on /gnu/store, it
would be wiser.

> 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.

Yes, that’s fine.

>> 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).

Indeed, good catch!

> From 461064da9e169df3dd939b734bb2860598d113f4 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> 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))))

I feel like there are TOCTTOU issues here and redundant ‘stat’ calls,
plus the risk of catching a ‘system-error’ coming from “somewhere else.”

What about baking this logic directly in ‘replace-with-link’, and
replacing ‘file-exists?’ calls by 'system-error handling?

> From a14ff0a9dab4d73680befaf9d244d6cce0a73ab3 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> 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.

LGTM!

Thanks for your quick feedback and thorough analyses!

Ludo’.

Message #17 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
Cc: 42023 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce transaction
 scope.
Date: Sun, 26 Jul 2020 17:31:25 +0200

Hi reepca,

Did you have time to look into this (see below)?  I still see a lot of
contention on /var/guix/db/db.sqlite-shm on berlin and I feel like these
changes would be welcome.  :-)

Thanks,
Ludo’.

Ludovic Courtès <ludo <at> gnu.org> skribis:

> Hi,
>
> Caleb Ristvedt <caleb.ristvedt <at> cune.org> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>
> [...]
>
>>> 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.
>
> Yes, it would be good to add a sentence to document it.
>
>> 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.
>
> Ah, so we’d need to do that before applying the patch that reduces the
> scope of the transaction, right?
>
>>>   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.
>
> Yes.  Here’s a patch for ‘dynamic-wind’:
>
> diff --git a/.dir-locals.el b/.dir-locals.el
> index 92979fc5ed..155255a770 100644
> --- a/.dir-locals.el
> +++ b/.dir-locals.el
> @@ -37,6 +37,7 @@
>     (eval . (put 'with-file-lock 'scheme-indent-function 1))
>     (eval . (put 'with-file-lock/no-wait 'scheme-indent-function 1))
>     (eval . (put 'with-profile-lock 'scheme-indent-function 1))
> +   (eval . (put 'with-writable-file 'scheme-indent-function 1))
>  
>     (eval . (put 'package 'scheme-indent-function 0))
>     (eval . (put 'origin 'scheme-indent-function 0))
> diff --git a/guix/store/deduplication.scm b/guix/store/deduplication.scm
> index 6784ee0b92..af52c03370 100644
> --- a/guix/store/deduplication.scm
> +++ b/guix/store/deduplication.scm
> @@ -94,6 +94,20 @@ LINK-PREFIX."
>              (try (tempname-in link-prefix))
>              (apply throw args))))))
>  
> +(define (call-with-writable-file file thunk)
> +  (let ((stat (lstat file)))
> +    (dynamic-wind
> +      (lambda ()
> +        (make-file-writable file))
> +      thunk
> +      (lambda ()
> +        (set-file-time file stat)
> +        (chmod file (stat:mode stat))))))
> +
> +(define-syntax-rule (with-writable-file file exp ...)
> +  "Make FILE writable for the dynamic extent of EXP..."
> +  (call-with-writable-file file (lambda () exp ...)))
> +
>  ;; There are 3 main kinds of errors we can get from hardlinking: "Too many
>  ;; things link to this" (EMLINK), "this link already exists" (EEXIST), and
>  ;; "can't fit more stuff in this directory" (ENOSPC).
> @@ -120,20 +134,14 @@ Note: TARGET, TO-REPLACE, and SWAP-DIRECTORY must be on the same file system."
>    ;; If we couldn't create TEMP-LINK, that's OK: just don't do the
>    ;; replacement, which means TO-REPLACE won't be deduplicated.
>    (when temp-link
> -    (let* ((parent (dirname to-replace))
> -           (stat   (stat parent)))
> -      (make-file-writable parent)
> +    (with-writable-file (dirname to-replace)
>        (catch 'system-error
>          (lambda ()
>            (rename-file temp-link to-replace))
>          (lambda args
>            (delete-file temp-link)
>            (unless (= EMLINK (system-error-errno args))
> -            (apply throw args))))
> -
> -      ;; Restore PARENT's mtime and permissions.
> -      (set-file-time parent stat)
> -      (chmod parent (stat:mode stat)))))
> +            (apply throw args)))))))
>  
>  (define* (deduplicate path hash #:key (store %store-directory))
>    "Check if a store item with sha256 hash HASH already exists.  If so,
>
>
>> 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.
>
> I guess we should also avoid changing permissions on /gnu/store, it
> would be wiser.
>
>> 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.
>
> Yes, that’s fine.
>
>>> 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).
>
> Indeed, good catch!
>
>> From 461064da9e169df3dd939b734bb2860598d113f4 Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt <at> 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))))
>
> I feel like there are TOCTTOU issues here and redundant ‘stat’ calls,
> plus the risk of catching a ‘system-error’ coming from “somewhere else.”
>
> What about baking this logic directly in ‘replace-with-link’, and
> replacing ‘file-exists?’ calls by 'system-error handling?
>
>> From a14ff0a9dab4d73680befaf9d244d6cce0a73ab3 Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt <at> 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.
>
> LGTM!
>
> Thanks for your quick feedback and thorough analyses!
>
> Ludo’.

Message #20 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 42023 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce transaction
 scope.
Date: Sat, 08 Aug 2020 23:13:35 -0500

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi reepca,
>
> Did you have time to look into this (see below)?  I still see a lot of
> contention on /var/guix/db/db.sqlite-shm on berlin and I feel like these
> changes would be welcome.  :-)

Apologies for the long delay, I think I'm caught up on this issue
now. Patch series detailed below.

>
> Thanks,
> Ludo’.

>>> 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.
>>
>> Yes, it would be good to add a sentence to document it.

Patch attached.

>>
>>> 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.
>>
>> Ah, so we’d need to do that before applying the patch that reduces the
>> scope of the transaction, right?

AFAIK the only code path that actually uses finalize-store-file
currently is from the build hook / offload thingy, and it turns out that
the outputs of derivations should already be registered as temp roots by
the daemon process that launched the offload process (specifically
registered in haveDerivation() in nix/libstore/build.cc). So this is
technically not currently necessary before or after the scope-reducing
patch. But it makes sense to guarantee that the register-items
invocation will only happen on items that are protected from garbage
collection, so I've put a patch, right before the documenting of that
requirement, that modifies finalize-store-file to always add the file
being finalized as a temp root for the extent of the register-items
invocation.

>>> 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.
>>
>> I guess we should also avoid changing permissions on /gnu/store, it
>> would be wiser.

While rebasing my changes I noticed that the current implementation of
this uses (%store-directory) from (guix build utils), which may not
correspond to the #:store keyword argument of 'deduplicate'. So I added
a patch that propagates the store through to replace-with-link and from
there to with-writable-file.

>>> 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))))
>>
>> I feel like there are TOCTTOU issues here and redundant ‘stat’ calls,
>> plus the risk of catching a ‘system-error’ coming from “somewhere else.”
>>
>> What about baking this logic directly in ‘replace-with-link’, and
>> replacing ‘file-exists?’ calls by 'system-error handling?

Patch attached. I've renamed replace-with-link to canonicalize-with-link
since it may now have to create the target link. Unfortunately there are
places where 'file-exists?' error handling is necessary, simply because
of ambiguity in errnos. For example, link(oldpath, newpath) can return
ENOENT, but there's no way of knowing from that alone whether this is
because oldpath has missing directories or newpath has missing
directories or the directory components of oldpath are all present but
the file itself is missing (the case we need to be able to recognize and
retry in case of).

Also, I tried removing the first check for whether the canonical link
exists in favor of error handling like you suggested, but this actually
messes up the hard-coded number of link-invocations expected in
tests/store-deduplication.scm - it expects a single link invocation when
the canonical link already exists, but the error-handling approach uses
two - one to discover it exists, and another to create the temporary
link. I didn't know how to reconcile the testing strategy with this
behavior, so I kept the behavior of first using a 'file-exists?' call to
test for the existence of the canonical link.

All tests pass except for tests/packages.scm, which failed even without
the patches.

- reepca

[0001-.dir-locals.el-fix-call-with-retrying-transaction-in.patch (text/x-patch, attachment)]

[0002-deduplication-pass-store-directory-to-replace-with-l.patch (text/x-patch, attachment)]

[0003-deduplication-retry-on-ENOENT.patch (text/x-patch, attachment)]

[0004-nar-ensure-finalization-target-is-a-temp-root-during.patch (text/x-patch, attachment)]

[0005-database-document-extra-registration-requirements.patch (text/x-patch, attachment)]

[0006-database-register-items-reduce-transaction-scope.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #23 received at 42023 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
Cc: 42023 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#42023] [PATCH] database: register-items: reduce
 transaction scope.
Date: Mon, 14 Sep 2020 10:58:30 +0200

Hi Caleb,

And apologies for the delay.

I think we’ve drifted from the original patch and that’s become a tricky
7-patch series, which partly explains the delay—not that I’m looking for
an excuse.  ;-)

I decided to go ahead and apply some of these on your behalf.  Comments
below.

Caleb Ristvedt <caleb.ristvedt <at> cune.org> skribis:

> From 4c8f0cc50e2a1a33d9ce2f8e58cc426872676a7f Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Wed, 24 Jun 2020 01:00:40 -0500
> Subject: [PATCH 1/6] .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.

Applied.

> From 9717568f922e0921e5fdc320cbe6689768d29a29 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Sat, 8 Aug 2020 10:05:22 -0500
> Subject: [PATCH 2/6] deduplication: pass store directory to replace-with-link.
>
> This causes with-writable-file to take into consideration the actual store
> being used, as passed to 'deduplicate', rather than
> whatever (%store-directory) may return.
>
> * guix/store/deduplication.scm (replace-with-link): new keyword argument
>   'store'.  Pass to with-writable-file.
>   (with-writable-file, call-with-writable-file): new store argument.
>   (deduplicate): pass store to replace-with-link.

Applied.

> From b992a3aaac7e3b30222e0bf1df09093f18e25e6a Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Sat, 8 Aug 2020 11:25:57 -0500
> Subject: [PATCH 3/6] 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.

Would that ENOENT cause an error, or just a missed deduplication opportunity?

> * guix/store/deduplication.scm (replace-with-link): renamed to
>   canonicalize-with-link, now also handles the case where the target link
>   doesn't exist yet, and retries on ENOENT.
>   (deduplicate): modified to use canonicalize-with-link.

There’s an issue with this patch.  I gave it a spin (offloading a few
builds) and it got stuck in a infinite loop:

--8<---------------cut here---------------start------------->8---
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
--8<---------------cut here---------------end--------------->8---

I think we should work on reducing the complexity of that code (e.g.,
there are several layers of system-error handling).

I’ve omitted it and I propose discussing it in a separate issue if we
need to.

> From 6b7d011680c77642f24396be0eb0015c20413d1a Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Sat, 8 Aug 2020 08:31:38 -0500
> Subject: [PATCH 4/6] nar: ensure finalization target is a temp root during
>  registration.
>
> Note that this is currently unnecessary, as finalize-store-file is only used
> from the offload hook, and the daemon process that spawned the offload hook
> will have already registered the derivation outputs as temp roots prior to
> attempting to offload (see haveDerivation() in nix/libstore/build.cc).  But
> it's necessary to ensure that the register-items invocation works properly
> when finalize-store-file is used in other contexts.
>
> * guix/nar.scm (finalize-store-file): make target a temp root during extent of
>   register-items invocation.

[...]

> +          ;; TODO: don't use an RPC for this, add it to *this process's* temp
> +          ;; roots file.
> +          (with-store store
> +            (add-temp-root store target)

I agree that this is the right thing but as you note, it’s currently
unnecessary in the context of ‘guix offload’, and I’d rather avoid
opening more connections to the daemon from ‘guix offload’ and this
increases load, pressure on the store database, etc.

> From 55dd48e88d641bbc17b4d9484d6ee84acfb29766 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Wed, 8 Jul 2020 11:33:23 -0500
> Subject: [PATCH 5/6] database: document extra registration requirements.
>
> It's necessary that store items be locked and protected from garbage
> collection while they are being registered.  This documents that.
>
> * guix/store/database.scm (register-path, register-items): document GC
>   protection and locking requirements.

Applied.

> From 30afb453ce4eb161bb87645a0e6314e6af82a61f Mon Sep 17 00:00:00 2001
> From: Christopher Baines <mail <at> cbaines.net>
> Date: Tue, 23 Jun 2020 17:36:49 +0100
> Subject: [PATCH 6/6] database: register-items: reduce transaction scope.
>
> It was made transactional in a4678c6ba18d8dbd79d931f80426eebf61be7ebe, with
> the reasoning to prevent broken intermediate states from being visible. I
> think this means something like an entry being in ValidPaths, but the Refs not
> being inserted.
>
> Using a transaction for this makes sense, but I think using one single
> transaction for the whole register-items call is unnecessary to avoid broken
> states from being visible, and could block other writes to the store database
> while register-items is running. Because the deduplication and resetting
> timestamps happens within the transaction as well, even though these things
> don't involve the database, writes to the database will still be blocked while
> this is happening.
>
> To reduce the potential for register-items to block other writers to the
> database for extended periods, this commit moves the transaction to just wrap
> the call to sqlite-register. This is the one place where writes occur, so that
> should prevent the broken intermediate states issue above. The one difference
> this will make is some of the registered items will be visible to other
> connections while others may be still being added. I think this is OK, as it's
> equivalent to just registering different items.
>
> * guix/store/database.scm (register-items): Reduce transaction scope.

Applied.

Thanks Caleb & Chris!

Ludo’.

Message #30 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
To: bug-guix <at> gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH] Retry deduplication on ENOENT
Date: Tue, 15 Sep 2020 15:29:32 -0500

[Message part 1 (text/plain, inline)]

Continued where left off from 42023:

Ludovic Courtès <ludo <at> gnu.org> writes:

>> From b992a3aaac7e3b30222e0bf1df09093f18e25e6a Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
>> Date: Sat, 8 Aug 2020 11:25:57 -0500
>> Subject: [PATCH 3/6] 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.
>
> Would that ENOENT cause an error, or just a missed deduplication
> opportunity?

Depends on how we handle it. Previously the error would be uncaught
entirely; simply skipping deduplication of that file would work, though
it would be suboptimal.

>
>> * guix/store/deduplication.scm (replace-with-link): renamed to
>>   canonicalize-with-link, now also handles the case where the target link
>>   doesn't exist yet, and retries on ENOENT.
>>   (deduplicate): modified to use canonicalize-with-link.
>
> There’s an issue with this patch.  I gave it a spin (offloading a few
> builds) and it got stuck in a infinite loop:
>
> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
>

I believe I can explain this. In 'deduplicate' we currently treat
anything that isn't a directory as a hardlinkable thing. This includes
symlinks (although it's implementation-defined whether symlinks can be
hardlinked to - we use CAN_LINK_SYMLINK to test this in
nix/libstore/optimise-store.cc). This means that at present we
unconditionally attempt to deduplicate symlinks (which happens to work
on linux). However, 'file-exists?' uses stat, not lstat, to check for
file existence. Thus, if there is a dangling symlink, 'file-exists?'
will return #f when passed it, but of course attempting to call link()
to create it will fail with EEXIST. Attached is a modified patch that
tests for file existence with lstat instead. I expect that will fix the
problem.

We should probably still add a test in 'deduplicate' for whether
symlinks can be hardlinked to.

Tangent: I was curious why libwps-0.4.so would be a dangling symlink,
and it turns out that it's actually a relative symlink, so when
accessing it via /gnu/store/...-libwps-0.4.12/lib/libwps-0.4.so it isn't
dangling, but when accessing it via /gnu/store/.links/0k63r... it is.

> I think we should work on reducing the complexity of that code (e.g.,
> there are several layers of system-error handling).

I've since flattened it down into just one layer of catch'es. It adds a
bit of redundancy, but might make it clearer.

- reepca

[0001-deduplication-retry-on-ENOENT.patch (text/x-patch, attachment)]

Message #33 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
Cc: bug-guix <at> gnu.org
Subject: Re: [PATCH] Retry deduplication on ENOENT
Date: Wed, 16 Sep 2020 22:37:05 +0200

Hi!

Caleb Ristvedt <caleb.ristvedt <at> cune.org> skribis:

[...]

>> There’s an issue with this patch.  I gave it a spin (offloading a few
>> builds) and it got stuck in a infinite loop:
>>
>> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
>> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
>> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
>> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
>> stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
>> link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
>>
>
> I believe I can explain this. In 'deduplicate' we currently treat
> anything that isn't a directory as a hardlinkable thing. This includes
> symlinks (although it's implementation-defined whether symlinks can be
> hardlinked to - we use CAN_LINK_SYMLINK to test this in
> nix/libstore/optimise-store.cc). This means that at present we
> unconditionally attempt to deduplicate symlinks (which happens to work
> on linux). However, 'file-exists?' uses stat, not lstat, to check for
> file existence. Thus, if there is a dangling symlink, 'file-exists?'
> will return #f when passed it, but of course attempting to call link()
> to create it will fail with EEXIST. Attached is a modified patch that
> tests for file existence with lstat instead. I expect that will fix the
> problem.

Ah ha!

> We should probably still add a test in 'deduplicate' for whether
> symlinks can be hardlinked to.

If GNU/Linux and GNU/Hurd support it, it’s unnecessary.

> Tangent: I was curious why libwps-0.4.so would be a dangling symlink,
> and it turns out that it's actually a relative symlink, so when
> accessing it via /gnu/store/...-libwps-0.4.12/lib/libwps-0.4.so it isn't
> dangling, but when accessing it via /gnu/store/.links/0k63r... it is.

I see, good catch!

> From 12f5848e79b0ede95babebea240264b32e39812c Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt <at> cune.org>
> Date: Sat, 8 Aug 2020 11:25:57 -0500
> Subject: [PATCH] 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 (replace-with-link): renamed to
>   canonicalize-with-link, now also handles the case where the target link
>   doesn't exist yet, and retries on ENOENT.  Also modified to support
>   canonicalizing symbolic links, though it is the caller's responsibility to
>   ensure that the system supports hardlinking to a symbolic link (on Linux it
>   does).
>   (deduplicate): modified to use canonicalize-with-link.

[...]

> +            (lambda args
> +              (let ((errno (system-error-errno args)))
> +                (cond
> +                 ((= errno ENOENT)
> +                  ;; either SWAP-DIRECTORY has missing directory
> +                  ;; components or TARGET was deleted - this is a
> +                  ;; fundamental ambiguity to the errno produced by
> +                  ;; link()
> +                  (if (file-exists? swap-directory)
> +                      ;; we must assume link failed because target doesn't
> +                      ;; exist, so create it.

Nitpick: Please capitalize sentences, add a period at the end, and write
“'link'” instead of “link()” or “link” for clarity.

Otherwise LGTM.

I think we’ll have to stress-test it through offloading to catch any
remaining issues.

Thank you!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.