[PATCH] file-systems: Set default value of the check? field to #f for NFS

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Maxim Cournoyer
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
M
M
Maxim Cournoyer wrote on 20 Feb 2020 18:15
(name . guix-patches)(address . guix-patches@gnu.org)
87d0a9i3il.fsf@gmail.com
The attached patch makes the default value of the <file-system> record
check? filed smarter, and avoids boot failures when using an NFS system
and omitting to specify "check? #f".

There are no checkers for NFS and none are required.
From d6b5a47036b21ec4bf4109cd781ed76208713b71 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Thu, 20 Feb 2020 11:51:37 -0500
Subject: [PATCH] file-systems: Set default value of the check? field to #f for
NFS.

Network file systems do not need to be checked locally, so provide a better
default value.

* gnu/system/file-systems.scm (<file-system>): Make the check? field thunked,
and compute the default value based on its own type.
---
gnu/system/file-systems.scm | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

Toggle diff (27 lines)
diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
index 7b78731524..03b143fcac 100644
--- a/gnu/system/file-systems.scm
+++ b/gnu/system/file-systems.scm
@@ -101,6 +101,8 @@
(define-record-type* <file-system> %file-system
make-file-system
file-system?
+ this-file-system
+
(device file-system-device) ; string | <uuid> | <file-system-label>
(mount-point file-system-mount-point) ; string
(type file-system-type) ; string
@@ -113,7 +115,10 @@
(needed-for-boot? %file-system-needed-for-boot? ; Boolean
(default #f))
(check? file-system-check? ; Boolean
- (default #t))
+ (thunked)
+ (default (not (string-prefix-ci?
+ (file-system-type this-file-system)
+ "nfs"))))
(create-mount-point? file-system-create-mount-point? ; Boolean
(default #f))
(dependencies file-system-dependencies ; list of <file-system>
--
2.25.0
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEJ9WGpPiQCFQyn/CfEmDkZILmNWIFAl5OvrIACgkQEmDkZILm
NWJ9fBAAj+BIFeU+DYvglksrHnlT5znoramfzzBkTSVlCGKVhW9rTv3MjY+d597o
s4U0YdJVYxsCE/eo36e4goKWp8aM4HSVtU65LlCdN69YaqOMMWF/0Nf1vkvfuadT
kZOHBQvWH66Q/DaNwVAjg2AM7EBaZCyd6dFSzhcFct7Zk9aYCsh3cNwb2Skh6Fmw
4bvAtuXXaywl5Ap6R4QnyexvsKNrDmy7i/y6TRlmuZ2S2cY/eO3yu7KZshOj5nzG
YFPMh2cD3NyIQYLHQPEe11s44MWvo1/b0ZtVI7NWd1wuc8m8xMx0CvywWmoH9Foy
gDJeBF9U0l2rarahdl7ToXV9Nlsgzaw6UPH6SaprRpvAo3y1Tf9B/lVvw4eT1Gkq
5felyijOBV35PtuGsghhhC3eiW121cdW/jpB7LepL8emoBlK2waJBAS05HGBpSiS
4qLhT43s4f0tRQDGbcwQ2L9wiaiB551YvSwEluRSuFXebTOd8A3NGMaB7vikS9Js
ufszb3kat1M/bcDAGxVhlfQ9mGjCszhxl3dj16fDK9Uy3pL90geJItnzlM9L7UBN
E6YSVow1y0JyOpUFiGosAHwFpquzVDmx9bT5eNidAlBog0AAfKBOewpCn6RV/F0C
qPX729JKEn8Yf1QDvbetb1gB9X8x4x9NtYhsDLNX605JCIHKp0s=
=lOTZ
-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 20 Feb 2020 21:29
Re: bug#39698: Acknowledgement ([PATCH] file-systems: Set default value of the check? field to #f for NFS)
(address . 39698@debbugs.gnu.org)
87o8ttxas0.fsf@gmail.com
The patch sent earlier crashes 'guix system reconfigure config.scm'. It
occurs when attempting to lower the object returned by the fold-services
procedure, e.g.:

Toggle snippet (11 lines)
,m (guix scripts system)
scheme@(guix scripts system)> (define os (load* "/path/to/config.scm" %user-module))
scheme@(guix scripts system)> (lower-object os)
$2 = #<procedure 7fd849022e40 at guix/gexp.scm:231:5 (state)>
scheme@(guix scripts system)> ,import (guix monad-repl)
scheme@(guix scripts system)> ,run-in-store $2
While executing meta-command:
Throw to key `srfi-34' with args `(#<condition &gexp-input-error [input:
#<procedure 7fd84d36ac80 at ice-9/eval.scm:333:13 (a)>] 7fd84a760e00>)'.

I'm struggling to understand why. To be continued.
L
L
Ludovic Courtès wrote on 22 Feb 2020 12:17
Re: [bug#39698] [PATCH] file-systems: Set default value of the check? field to #f for NFS
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 39698@debbugs.gnu.org)
87zhdayipo.fsf@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (21 lines)
> --- a/gnu/system/file-systems.scm
> +++ b/gnu/system/file-systems.scm
> @@ -101,6 +101,8 @@
> (define-record-type* <file-system> %file-system
> make-file-system
> file-system?
> + this-file-system
> +
> (device file-system-device) ; string | <uuid> | <file-system-label>
> (mount-point file-system-mount-point) ; string
> (type file-system-type) ; string
> @@ -113,7 +115,10 @@
> (needed-for-boot? %file-system-needed-for-boot? ; Boolean
> (default #f))
> (check? file-system-check? ; Boolean
> - (default #t))
> + (thunked)
> + (default (not (string-prefix-ci?
> + (file-system-type this-file-system)
> + "nfs"))))

I’d rather avoid making ‘check?’ a thunked field (it adds a bit of
overhead and breaks ‘equal?’.)

Could we instead special-case NFS in (gnu build file-systems) such that
checking is a no-op?

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 24 Feb 2020 17:15
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 39698@debbugs.gnu.org)
87v9nwklm6.fsf@gmail.com
Hi Ludovic!

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

Toggle quote (34 lines)
> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> --- a/gnu/system/file-systems.scm
>> +++ b/gnu/system/file-systems.scm
>> @@ -101,6 +101,8 @@
>> (define-record-type* <file-system> %file-system
>> make-file-system
>> file-system?
>> + this-file-system
>> +
>> (device file-system-device) ; string | <uuid> | <file-system-label>
>> (mount-point file-system-mount-point) ; string
>> (type file-system-type) ; string
>> @@ -113,7 +115,10 @@
>> (needed-for-boot? %file-system-needed-for-boot? ; Boolean
>> (default #f))
>> (check? file-system-check? ; Boolean
>> - (default #t))
>> + (thunked)
>> + (default (not (string-prefix-ci?
>> + (file-system-type this-file-system)
>> + "nfs"))))
>
> I’d rather avoid making ‘check?’ a thunked field (it adds a bit of
> overhead and breaks ‘equal?’.)
>
> Could we instead special-case NFS in (gnu build file-systems) such that
> checking is a no-op?
>
> Thanks,
> Ludo’.

This actually was the first implementation I'd done, before attempting
to go fancy with the thunked default value ;-).

You'll find it attached.

Thanks!
From ca38de33a7a31c7b96f7e920038b2fb6352160a8 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Mon, 24 Feb 2020 11:08:42 -0500
Subject: [PATCH] build: file-systems: Skip check for NFS file systems

* gnu/build/file-systems.scm (mount-file-system): Do not call
`check-file-system' when the file system is of NFS type.
---
gnu/build/file-systems.scm | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

Toggle diff (46 lines)
diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index cfa3898f83..76c143654d 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -5,6 +5,7 @@
;;; Copyright © 2019 Guillaume Le Vaillant <glv@posteo.net>
;;; Copyright © 2019 Tobias Geerinckx-Rice <me@tobias.gr>
;;; Copyright © 2019 David C. Trudgian <dave@trudgian.net>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -663,6 +664,7 @@ corresponds to the symbols listed in FLAGS."
(string-append "," options)
"")))))
(let* ((type (file-system-type fs))
+ (nfs? (string-prefix? "nfs" type))
(fs-options (file-system-options fs))
(options (if (null? fs-options)
#f
@@ -671,7 +673,7 @@ corresponds to the symbols listed in FLAGS."
(mount-point (string-append root "/"
(file-system-mount-point fs)))
(flags (mount-flags->bit-mask (file-system-flags fs))))
- (when (file-system-check? fs)
+ (when (and (file-system-check? fs) (not nfs?))
(check-file-system source type))
;; Create the mount point. Most of the time this is a directory, but
@@ -683,11 +685,9 @@ corresponds to the symbols listed in FLAGS."
(call-with-output-file mount-point (const #t)))
(mkdir-p mount-point))
- (cond
- ((string-prefix? "nfs" type)
- (mount-nfs source mount-point type flags options))
- (else
- (mount source mount-point type flags options)))
+ (if nfs?
+ (mount-nfs source mount-point type flags options)
+ (mount source mount-point type flags options))
;; For read-only bind mounts, an extra remount is needed, as per
;; <http://lwn.net/Articles/281157/>, which still applies to Linux 4.0.
--
2.25.0
Maxim
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEJ9WGpPiQCFQyn/CfEmDkZILmNWIFAl5T9pEACgkQEmDkZILm
NWLwRA//dEhiPaYF6OHsIEim7PW6lwx3P5YtbC2jyDEncnppPikcMSx2oPn8FK/Q
EwzisIOtRUQMeqS/6E56jaN9Ww2VBdBx8OeNX3fuwj01w88Wddxc0FGmSi6T1O8U
D1SbO/PFFwSPwou2Yh6yiRKUmNQxuyVLSjkoXQkoniHOlIc8nOkpULAcxhqxVYjY
wGIMxuu1hy9epCJiAKiqTMAcnE5GH+u2tM95cZMIagSDhbJH7d2KYRchnksegsEV
V5EJ26OUtFktlifSZJC0KuQiT1cBcL7U/GKjanSXh7MZzfidRv0vTP7C21gIIH/V
hxVXGo/Nq5VhKA3+cxzwswI1DPoCf+4bl2NHrNV5Sb9XGrU4Jwak3JR5JeHohIUd
Duqa+f1qWzSFgyIGn9UWj6t05Q62i+fuH/l0Gs+w1mz80LauQcIQrDrldQ5sZCIY
XT2kWwxp4V/VgFq/OSOorZEq3KjpP0r1mN24eydWRb5s06AcDuESnySnNhhbM56p
OsvZIIpt/iAP7jiP2IVPNJbkJ0XpVMAcAJ63YftrPme57uEZE3+IYiK39wzfZzzB
1GkiOS2Mc66v+pJui7392RyCbrxarxIgMS3rslYRdg+gxX5iJ4uaLpnwJIgp3wqw
PERLO3C1N90DtrjzN3e51syCYYc40y+rTA9mYbgItwjsD1L46KI=
=JZwX
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 26 Feb 2020 21:41
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 39698@debbugs.gnu.org)
878skpcc8k.fsf@gnu.org
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (39 lines)
> From ca38de33a7a31c7b96f7e920038b2fb6352160a8 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Mon, 24 Feb 2020 11:08:42 -0500
> Subject: [PATCH] build: file-systems: Skip check for NFS file systems
>
> * gnu/build/file-systems.scm (mount-file-system): Do not call
> `check-file-system' when the file system is of NFS type.
> ---
> gnu/build/file-systems.scm | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
> index cfa3898f83..76c143654d 100644
> --- a/gnu/build/file-systems.scm
> +++ b/gnu/build/file-systems.scm
> @@ -5,6 +5,7 @@
> ;;; Copyright © 2019 Guillaume Le Vaillant <glv@posteo.net>
> ;;; Copyright © 2019 Tobias Geerinckx-Rice <me@tobias.gr>
> ;;; Copyright © 2019 David C. Trudgian <dave@trudgian.net>
> +;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
> ;;;
> ;;; This file is part of GNU Guix.
> ;;;
> @@ -663,6 +664,7 @@ corresponds to the symbols listed in FLAGS."
> (string-append "," options)
> "")))))
> (let* ((type (file-system-type fs))
> + (nfs? (string-prefix? "nfs" type))
> (fs-options (file-system-options fs))
> (options (if (null? fs-options)
> #f
> @@ -671,7 +673,7 @@ corresponds to the symbols listed in FLAGS."
> (mount-point (string-append root "/"
> (file-system-mount-point fs)))
> (flags (mount-flags->bit-mask (file-system-flags fs))))
> - (when (file-system-check? fs)
> + (when (and (file-system-check? fs) (not nfs?))
> (check-file-system source type))

Looking more closely, I see this:

Toggle snippet (17 lines)
(define (check-file-system device type)
"Run a file system check of TYPE on DEVICE."
(define check-procedure
(cond
((string-prefix? "ext" type) check-ext2-file-system)
((string-prefix? "btrfs" type) check-btrfs-file-system)
((string-suffix? "fat" type) check-fat-file-system)
((string-prefix? "jfs" type) check-jfs-file-system)
(else #f)))

(if check-procedure
…
(format (current-error-port)
"No file system check procedure for ~a; skipping~%"
device)))

Isn’t it already taking care of not attempting to check NFS?

Or, to get rid of the warning, what about:
Toggle diff (12 lines)
diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index ee6375515f..faf64ce304 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -581,6 +581,7 @@ were found."
((string-prefix? "btrfs" type) check-btrfs-file-system)
((string-suffix? "fat" type) check-fat-file-system)
((string-prefix? "jfs" type) check-jfs-file-system)
+ ((string-prefix? "nfs" type) (const 'pass))
(else #f)))
(if check-procedure
?

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 3 Mar 2020 06:05
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 39698-done@debbugs.gnu.org)
87tv36jaee.fsf@gmail.com
Hi again,

Toggle quote (13 lines)
> Or, to get rid of the warning, what about:
>
> diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
> index ee6375515f..faf64ce304 100644
> --- a/gnu/build/file-systems.scm
> +++ b/gnu/build/file-systems.scm
> @@ -581,6 +581,7 @@ were found."
> ((string-prefix? "btrfs" type) check-btrfs-file-system)
> ((string-suffix? "fat" type) check-fat-file-system)
> ((string-prefix? "jfs" type) check-jfs-file-system)
> + ((string-prefix? "nfs" type) (const 'pass))
> (else #f)))

I've now pushed the above to master. Closing!

Maxim
Closed
?