[2/2] system: Add btrfs file system support.

* 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.
---
 gnu/build/file-systems.scm  | 30 ++++++++++++++++++++----------
 gnu/system/linux-initrd.scm | 10 +++++++---
 2 files changed, 27 insertions(+), 13 deletions(-)

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