Message ID | 20161130183635.6513-2-david@craven.ch |
---|---|
State | New |
Headers | show |
David Craven <david@craven.ch> skribis: > * gnu/system/linux-initrd.scm (linux-modules, helper-packages): Add > btrfs modules when a btrfs file-system is used. > * gnu/build/file-systems.scm (check-file-system-irrecoverable-error, > check-file-system-ext): New variables. > (check-file-system): Support non ext file systems gracefully. [...] > -(define (check-file-system device type) > - "Run a file system check of TYPE on DEVICE." > - (define fsck > - (string-append "fsck." type)) > - > - (let ((status (system* fsck "-v" "-p" "-C" "0" device))) > +(define (check-file-system-irrecoverable-error prog code device) > + (format (current-error-port) > + "'~a' exited with code ~a on ~a; spawning Bourne-like REPL~%" > + prog code device) > + (start-repl %bournish-language)) This is confusing because this procedure doesn’t check anything, despite the name. :-) > +(define (check-file-system-ext device type) > + (let* ((fsck (string-append "fsck." type)) > + (status (system* fsck "-v" "-p" "-C" "0" device))) > (match (status:exit-val status) > (0 > #t) I think I made a different suggestion to Marius, so I may well be contradicting myself now, but what comes to mind now is to define a list of “checkers” corresponding to each file system type: (define-record-type <file-system-checker> (file-system-checker predicate checker) file-system-checker? (predicate file-system-checker-predicate) (checker file-system-checker-procedure)) Then we can have an alist: (define %ext2-checker (file-system-checker (cut string-prefix? "ext" <>) (lambda (device type) …))) (define %file-system-checkers (list %ext2-checker %vfat-checker %btrfs-checker)) Each checker procedure would return an enum (either using (rnrs enums) or returning a symbol like 'pass, 'errors-corrected, or 'reboot-needed). The code to start the REPL would be outside the checkers themselves, in the generic ‘check-file-system’. WDYT? > --- a/gnu/system/linux-initrd.scm > +++ b/gnu/system/linux-initrd.scm > @@ -193,6 +193,9 @@ loaded at boot time in the order in which they appear." > ,@(if (find (file-system-type-predicate "9p") file-systems) > virtio-9p-modules > '()) > + ,@(if (find (file-system-type-predicate "btrfs") file-systems) > + '("btrfs") > + '()) > ,@(if volatile-root? > '("fuse") > '()) > @@ -200,11 +203,12 @@ loaded at boot time in the order in which they appear." > > (define helper-packages > ;; Packages to be copied on the initrd. > - `(,@(if (find (lambda (fs) > - (string-prefix? "ext" (file-system-type fs))) > - file-systems) > + `(,@(if (find (file-system-type-predicate "ext4") file-systems) > (list e2fsck/static) > '()) > + ,@(if (find (file-system-type-predicate "btrfs") file-systems) > + (list btrfs-progs/static) > + '()) > ,@(if volatile-root? > (list unionfs-fuse/static) > '()))) This part LGTM. Thanks! Ludo’.
The nice thing about having the check procedure be part of the <file-system> is that it can be overridden. I'm not sure what use cases there are yet. One I can think of is that btrfs device scan is only required when using a multi-device configuration like raid. So I don't know if we want to run it by default as a %btrfs-checker or do nothing by default, and let the user add a custom file system pre-mount hook (file-system-checker) when needed. A version of Marius'es patch that works would be here: http://lists.gnu.org/archive/html/guix-devel/2016-12/msg00087.html
David Craven <david@craven.ch> skribis: > The nice thing about having the check procedure be part of the > <file-system> is that it can be overridden. I'm not sure what use > cases there are yet. One I can think of is that btrfs device scan is > only required when using a multi-device configuration like raid. So I > don't know if we want to run it by default as a %btrfs-checker or do > nothing by default, and let the user add a custom file system > pre-mount hook (file-system-checker) when needed. I don’t know enough about btrfs. Is it really useful in practice to have a check procedure that is specific to the instance at hand? For the other FS types, the check method really belongs in the FS type, not in the FS instance. That is, all ext4 file systems are checked in the same way. Thus, it doesn’t seem natural to store the fsck method in the <file-system> object itself. There’s also a practical issue with that: users would have to specify the fsck method for every single <file-system> object: (list (file-system ;; … (type "ext4") (fsck %ext4-fsck)) (file-system ;; … (type "ext4") (fsck %ext4-fsck)) ;again (file-system ;; … (type "ext4") (fsck %ext4-fsck))) ;and again! :-) > A version of Marius'es patch that works would be here: > http://lists.gnu.org/archive/html/guix-devel/2016-12/msg00087.html OK, this patch doesn’t have the problem above, but it’s kinda cheating. :-) I’ll reply separately! Ludo’.
diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm index 431b287..9f57ee5 100644 --- a/gnu/build/file-systems.scm +++ b/gnu/build/file-systems.scm @@ -410,12 +410,15 @@ the following: (else (error "unknown device title" title)))) -(define (check-file-system device type) - "Run a file system check of TYPE on DEVICE." - (define fsck - (string-append "fsck." type)) - - (let ((status (system* fsck "-v" "-p" "-C" "0" device))) +(define (check-file-system-irrecoverable-error prog code device) + (format (current-error-port) + "'~a' exited with code ~a on ~a; spawning Bourne-like REPL~%" + prog code device) + (start-repl %bournish-language)) + +(define (check-file-system-ext device type) + (let* ((fsck (string-append "fsck." type)) + (status (system* fsck "-v" "-p" "-C" "0" device))) (match (status:exit-val status) (0 #t) @@ -428,10 +431,17 @@ the following: (sleep 3) (reboot)) (code - (format (current-error-port) "'~a' exited with code ~a on ~a; \ -spawning Bourne-like REPL~%" - fsck code device) - (start-repl %bournish-language))))) + (check-file-system-irrecoverable-error fsck code device))))) + +(define (check-file-system device type) + "Run a file system check of TYPE on DEVICE." + (cond + ((string-prefix? "ext" type) + (check-file-system-ext device type)) + ((string-prefix? "btrfs" type) + (zero? (system* "btrfs" "device" "scan"))) + (#t (format (current-error-port) + "Don't know how to check '~a' file systems; skipping~%" type)))) (define (mount-flags->bit-mask flags) "Return the number suitable for the 'flags' argument of 'mount' that diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm index 174239a..de8b785 100644 --- a/gnu/system/linux-initrd.scm +++ b/gnu/system/linux-initrd.scm @@ -193,6 +193,9 @@ loaded at boot time in the order in which they appear." ,@(if (find (file-system-type-predicate "9p") file-systems) virtio-9p-modules '()) + ,@(if (find (file-system-type-predicate "btrfs") file-systems) + '("btrfs") + '()) ,@(if volatile-root? '("fuse") '()) @@ -200,11 +203,12 @@ loaded at boot time in the order in which they appear." (define helper-packages ;; Packages to be copied on the initrd. - `(,@(if (find (lambda (fs) - (string-prefix? "ext" (file-system-type fs))) - file-systems) + `(,@(if (find (file-system-type-predicate "ext4") file-systems) (list e2fsck/static) '()) + ,@(if (find (file-system-type-predicate "btrfs") file-systems) + (list btrfs-progs/static) + '()) ,@(if volatile-root? (list unionfs-fuse/static) '())))