[PATCH 1/2] Add 'lvm-device-mapping'

DoneSubmitted by tsmish.
Details
3 participants
  • Lars-Dominik Braun
  • Ludovic Courtès
  • tsmish
Owner
unassigned
Severity
normal
T
T
tsmish wrote on 9 May 2020 03:12
[PATCH 1/2] mapped-devices: Allow target to be list of strings
(address . guix-patches@gnu.org)
CAMaATaPqFZTB-j9MyhS4+vrn8sjA25xzu2+DD71nE541yCiRwQ@mail.gmail.com
(let ...) stuff should be in function, but I don't know in which
module it should go.
Code is somewhat untested, proceed with caution.

---
gnu/services/base.scm | 5 ++++-
gnu/system.scm | 13 ++++++++-----
gnu/system/mapped-devices.scm | 2 +-
3 files changed, 13 insertions(+), 7 deletions(-)

Toggle diff (67 lines)
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 0c154d1c4e..3d09e8220c 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -408,7 +408,10 @@ FILE-SYSTEM."
 (define (mapped-device->shepherd-service-name md)
   "Return the symbol that denotes the shepherd service of MD, a
<mapped-device>."
   (symbol-append 'device-mapping-
-                 (string->symbol (mapped-device-target md))))
+                 (string->symbol (string-join
+                                  (let ((t (mapped-device-target md)))
+                                    (if (list? t) t (list t)))
+                                  "-"))))

 (define dependency->shepherd-service-name
   (match-lambda
diff --git a/gnu/system.scm b/gnu/system.scm
index 01baa248a2..75632c5e8a 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -390,9 +390,10 @@ marked as 'needed-for-boot'."
     (let ((device (file-system-device fs)))
       (if (string? device)                        ;title is 'device
           (filter (lambda (md)
-                    (string=? (string-append "/dev/mapper/"
-                                             (mapped-device-target md))
-                              device))
+                    (any (cut string=? device <>)
+                         (map (cut string-append "/dev/mapper" <>)
+                              (let ((t (mapped-device-target md)))
+                                (if (list? t) t (list t))))))
                   (operating-system-mapped-devices os))
           '())))

@@ -412,11 +413,13 @@ marked as 'needed-for-boot'."

 (define (mapped-device-users device file-systems)
   "Return the subset of FILE-SYSTEMS that use DEVICE."
-  (let ((target (string-append "/dev/mapper/" (mapped-device-target device))))
+  (let ((targets (map (cut string-append "/dev/mapper/" <>)
+                      (let ((t (mapped-device-target device)))
+                        (if (list? t) t (list t))))))
     (filter (lambda (fs)
               (or (member device (file-system-dependencies fs))
                   (and (string? (file-system-device fs))
-                       (string=? (file-system-device fs) target))))
+                       (any (cut string=? (file-system-device fs) <>)
targets))))
             file-systems)))

 (define (operating-system-user-mapped-devices os)
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 7c58f876a3..3339e509e0 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -72,7 +72,7 @@
   make-mapped-device
   mapped-device?
   (source    mapped-device-source)                ;string | list of strings
-  (target    mapped-device-target)                ;string
+  (target    mapped-device-target)                ;string | list of strings
   (type      mapped-device-type)                  ;<mapped-device-kind>
   (location  mapped-device-location
              (default (current-source-location)) (innate)))
-- 
2.26.2
T
T
tsmish wrote on 9 May 2020 03:22
[PATCH 2/2] mapped-devices: Add 'lvm-device-mapping'
(address . 41143@debbugs.gnu.org)
CAMaATaNkBUP-2sg-QX=AXQoP4z=2HcM_c9gpbL7PPLJ+sxDnrg@mail.gmail.com
"vgscan --mknodes" is a bit of a hack. Everyone else relies on udev to
create files in /dev/mapper, but since initrd doesn't have working
udevd, they need to be created this way.
Also, while this code is able to boot from root on LVM, grub in
current configuration can't find required files, This can be fixed by
placing (format port "insmod lvm") in grub configuration builder, but
this is somewhat hacky.

---
gnu/system/mapped-devices.scm | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)

Toggle diff (55 lines)
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 3339e509e0..03bc7c782d 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -34,7 +34,7 @@
   #:autoload   (gnu build linux-modules)
                  (missing-modules)
   #:autoload   (gnu packages cryptsetup) (cryptsetup-static)
-  #:autoload   (gnu packages linux) (mdadm-static)
+  #:autoload   (gnu packages linux) (mdadm-static lvm2-static)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -59,7 +59,8 @@
             check-device-initrd-modules           ;XXX: needs a better place

             luks-device-mapping
-            raid-device-mapping))
+            raid-device-mapping
+            lvm-device-mapping))

 ;;; Commentary:
 ;;;
@@ -269,4 +270,28 @@ TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
    (open open-raid-device)
    (close close-raid-device)))

+(define (open-lvm-device source target)
+  #~(begin
+       (use-modules
+        (srfi srfi-1)
+        (srfi srfi-26))
+       (and
+        (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                        "vgchange" "--activate" "y" #$source))
+        (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                         "vgscan" "--mknodes")) ; make /dev/mapper
nodes when in initrd
+        (every file-exists? (map (cut string-append "/dev/mapper/" <>)
+                                 (let ((t '#$target))
+                                   (if (list? t) t (list t))))))))
+
+
+(define (close-lvm-device source target)
+  #~(zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                    "vgchange" "--activate" "n" #$source)))
+
+(define lvm-device-mapping
+  (mapped-device-kind
+   (open open-lvm-device)
+   (close close-lvm-device)))
+
 ;;; mapped-devices.scm ends here
-- 
2.26.2
M
M
Mikhail Tsykalov wrote on 15 May 2020 00:53
Some clarification
(address . 41143@debbugs.gnu.org)
7e96c284-c69c-965f-6bae-9546b1c711ad@gmail.com
Hi,

In LVM one volume group usually haves more than one logical volume, so
target need to be a list. Just setting target to list in config doesn't
work because that field is actually used for checking when (and whether
at all) the device needs to be mounted. Because we still need to support
using a string as target, we need to handle two situations: target is a
string and target is list of strings. Patch converts plain string target
to single element list and proceeds with assuming a list. grep by
mapped-device-target revealed two uses in gnu/system.scm and one in
gnu/services/base.scm. First two checked if any filesystems used a
device, so I replaced plain string comparison with comparison with every
element of list and returning true if any of them matched. Last one used
target for building service name for shepherd, so I replaced plain
target with concatenation of every target that mapped device has.

Moving on to second patch. Basically to mount LVM you need to activate
volume group(s) with command "vgchange -ay". It worked fine with
non-root mounts, but when it got moved in initrd it crashed. Quick look
around found that while /dev/dm-X devices were being created,
/dev/mapper/ nodes were not. Looking at man pages (and internet) "vgscan
--mknodes" looked like what I wanted (there is also dmsetup mknodes, but
basically first just calls it). After adding it, boot with root on LVM
started working, so I left it. Also I check that targets actually exist,
so you get crash in a bit more appropriate place. This check should
probably be moved to somewhere else, but I haven't found better solution.

Looking at
10-dm.rules looks like what creates /dev/mapper nodes in normal systems,
so probably more proper solution will be to add udevd to initrd, but I
don't know how to run daemons in initrd properly. dracut also creates
file /etc/lvm/lvm.conf, which doesn't seem to be very important, but I
probably should look closer to it. Also both dracut and LVM add
"--ignorelockingfailure" flag to vgchange, which I forgot and will
change in future patches.

There seems to be close to no resources about mounting LVM in initrd, so
I'm adding code from other initrd generators:

Dracut:

LVM:
M
M
Mikhail Tsykalov wrote on 15 May 2020 03:17
[PATCH] mapped-devices: Document lvm-mapping-device.
(address . 41143@debbugs.gnu.org)
b68a706e-a554-8dd8-13d1-963de34cb260@gmail.com
---
 doc/guix.texi | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

Toggle diff (76 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index d6fbd85fde..612a9b25e5 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -11738,7 +11738,6 @@ Guix extends this notion by considering any 
device or set of devices that
  are @dfn{transformed} in some way to create a new device; for instance,
  RAID devices are obtained by @dfn{assembling} several other devices, such
  as hard disks or partitions, into a new one that behaves as one partition.
-Other examples, not yet implemented, are LVM logical volumes.

  Mapped devices are declared using the @code{mapped-device} form,
  defined as follows; for examples, see below.
@@ -11751,15 +11750,20 @@ the system boots up.
  @item source
  This is either a string specifying the name of the block device to be 
mapped,
  such as @code{"/dev/sda3"}, or a list of such strings when several devices
-need to be assembled for creating a new one.
+need to be assembled for creating a new one. In case of LVM this is a
+string specifying name of the volume group to be mapped.

  @item target
-This string specifies the name of the resulting mapped device. For
-kernel mappers such as encrypted devices of type 
@code{luks-device-mapping},
+This is either a string specifying the name of the resulting mapped
+device, or a list of such strings in case there are several, which is
+common while using LVM.
+For kernel mappers such as encrypted devices of type 
@code{luks-device-mapping},
  specifying @code{"my-partition"} leads to the creation of
  the @code{"/dev/mapper/my-partition"} device.
  For RAID devices of type @code{raid-device-mapping}, the full device name
  such as @code{"/dev/md0"} needs to be given.
+LVM logical volumes of type @code{lvm-device-mapping} need to
+be specified as @code{"VGNAME-LVNAME"}.

  @item type
  This must be a @code{mapped-device-kind} object, which specifies how
@@ -11780,6 +11784,11 @@ module for the appropriate RAID level to be 
loaded, such as @code{raid456}
  for RAID-4, RAID-5 or RAID-6, or @code{raid10} for RAID-10.
  @end defvr

+@defvr {Scheme Variable} lvm-device-mapping
+This defines LVM logical volume(s). Volume group is activated by
+@command{vgchange} command from the package @code{lvm2}.
+@end defvr
+
  @cindex disk encryption
  @cindex LUKS
  The following example specifies a mapping from @file{/dev/sda3} to
@@ -11837,6 +11846,19 @@ Note that the RAID level need not be given; it 
is chosen during the
  initial creation and formatting of the RAID device and is determined
  automatically later.

+LVM logical volumes ``alpha'' and ``beta'' from volume group ``vg0'' can
+be declared as follows:
+
+@lisp
+(mapped-device
+  (source "vg0")
+  (target (list "vg0-alpha" "vg0-beta"))
+  (type lvm-device-mapping))
+@end lisp
+
+Devices @file{/dev/mapper/vg0-alpha} and @file{/dev/mapper/vg0-beta} can
+then be used as the @code{device} of a @code{file-system} declaration
+(@pxref{File Systems}).

  @node User Accounts
  @section User Accounts
-- 
2.26.2
L
L
Ludovic Courtès wrote on 16 May 2020 19:38
control message for bug #41143
(address . control@debbugs.gnu.org)
87pnb3pypo.fsf@gnu.org
retitle 41143 [PATCH 1/2] Add 'lvm-device-mapping'
quit
L
L
Lars-Dominik Braun wrote on 6 Jun 2020 15:40
Re: [PATCH 1/2] mapped-devices: Allow target to be list of strings
(name . tsmish)(address . tsymsh@gmail.com)(address . guix-patches@gnu.org)
20200606134015.GA204960@noor.fritz.box
Hi,

I’ve tried the patches, but `guix system reconfigure` fails(?) with

building /gnu/store/cy6d2a4b8bcqcjiaz8bm367j7vbdrslc-upgrade-shepherd-services.scm.drv...
guix system: error: exception caught while executing 'start' on service 'device-mapping-disk3-data':
error: <>: unbound variable

Relevant part of my config.scm:

(mapped-devices
(list (mapped-device
(source "disk3")
(target "disk3-data")
(type lvm-device-mapping))))
(file-systems (append
(list …
(file-system
(device "/dev/mapper/disk3-data")
(mount-point "/storage/disk3")
(type "ext4")
(dependencies mapped-devices))
)
…))

I’m also curious why sources is a list of VG’s as opposed to a list of device
nodes (i.e. /dev/sdX), like the other mappings. Does `pvscan --cache` not work
here?

Lars
M
M
Mikhail Tsykalov wrote on 6 Jun 2020 22:16
(name . Lars-Dominik Braun)(address . lars@6xq.net)(address . 41143@debbugs.gnu.org)
8c852805-38e6-92b2-15cf-43d47818782e@gmail.com
Hi,

Thanks for the report, guess that's what I get by assuming instead of
testing.

Toggle quote (6 lines)
> I’ve tried the patches, but `guix system reconfigure` fails(?) with
>
> building /gnu/store/cy6d2a4b8bcqcjiaz8bm367j7vbdrslc-upgrade-shepherd-services.scm.drv...
> guix system: error: exception caught while executing 'start' on service 'device-mapping-disk3-data':
> error: <>: unbound variable

Can you test with the attached patch, it seems like `cut` macros is
undefined while shepherd service is expanded(?). I fixed it by replacing
it with a plain lambda. I managed to mount a flash disk with LVM using
this patch, so I think it should be working now.

Toggle quote (4 lines)
> I’m also curious why sources is a list of VG’s as opposed to a list of device
> nodes (i.e. /dev/sdX), like the other mappings. Does `pvscan --cache` not work
> here?

I always thought that it wasn't possible to mount LVM by PV's, but
looking at man, `pvscan --cache -aay` seems to do the thing. Still, I
think that using VG's instead of PV's is better, since you must `pvscan`
all PV's in a VG in order to activate it, so you'll still need to think
in terms of VG's. This also can be a source of errors, for example
adding a device to a VG will result in entire VG not being activated
until the configuration is changed and the system is reconfigured. Also
I think that LVM doesn't put that much importance on individual PV's as
it does on VG's as a place where LV's live.

All of this goes out of the window if you use LVM on only one PV, so it
may be possible to have a heuristic that will check if source is a
device node as opposed to a VG name and perform appropriate actions, but
I'm not sure if the resulting complexity is worth it.

Mikhail
From e9a2b441190fe18ebc4f5a8c73707edab3459d58 Mon Sep 17 00:00:00 2001
From: Mikhail Tsykalov <tsymsh@gmail.com>
Date: Sat, 9 May 2020 03:27:13 +0300
Subject: [PATCH 2/2] mapped-devices: Add 'lvm-device-mapping'

---
gnu/system/mapped-devices.scm | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)

Toggle diff (52 lines)
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 3339e509e0..641cddc146 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -34,7 +34,7 @@
   #:autoload   (gnu build linux-modules)
                  (missing-modules)
   #:autoload   (gnu packages cryptsetup) (cryptsetup-static)
-  #:autoload   (gnu packages linux) (mdadm-static)
+  #:autoload   (gnu packages linux) (mdadm-static lvm2-static)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -59,7 +59,8 @@
             check-device-initrd-modules           ;XXX: needs a better place
 
             luks-device-mapping
-            raid-device-mapping))
+            raid-device-mapping
+            lvm-device-mapping))
 
 ;;; Commentary:
 ;;;
@@ -269,4 +270,26 @@ TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
    (open open-raid-device)
    (close close-raid-device)))
 
+(define (open-lvm-device source target)
+  #~(begin
+       (use-modules (srfi srfi-1))
+       (and
+        (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                        "vgchange" "--activate" "ay" #$source))
+        (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                         "vgscan" "--mknodes")) ; make /dev/mapper nodes when in initrd
+        (every file-exists? (map (lambda (file) (string-append "/dev/mapper/" file))
+                                 (let ((t '#$target))
+                                   (if (list? t) t (list t))))))))
+
+
+(define (close-lvm-device source target)
+  #~(zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                    "vgchange" "--activate" "n" #$source)))
+
+(define lvm-device-mapping
+  (mapped-device-kind
+   (open open-lvm-device)
+   (close close-lvm-device)))
+
 ;;; mapped-devices.scm ends here
-- 
2.26.2
L
L
Lars-Dominik Braun wrote on 7 Jun 2020 08:48
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
20200607064852.GA1408@noor
Hi,

Toggle quote (4 lines)
> Can you test with the attached patch, it seems like `cut` macros is
> undefined while shepherd service is expanded(?). I fixed it by replacing
> it with a plain lambda. I managed to mount a flash disk with LVM using
> this patch, so I think it should be working now.
yes, both `guix system reconfigure` and a reboot are working properly
now. (This is a non-boot disk though.) Thanks!

Toggle quote (3 lines)
> Still, I think that using VG's instead of PV's is better, since you
> must `pvscan` all PV's in a VG in order to activate it, so you'll
> still need to think in terms of VG's.
I agree, using device nodes would probably break guix’ rollback if you
decide to change the VG.

Lars
L
L
Ludovic Courtès wrote on 9 Sep 2020 22:38
Re: [bug#41143] [PATCH 1/2] mapped-devices: Allow target to be list of strings
(name . tsmish)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
874ko6n0s5.fsf@gnu.org
Hi Mikhail,

Sorry for the very late reply! Vacations came by, and by now this entry
is at the bottom of the patch tracker. :-)

People repeatedly ask for LVM support, so I guess you’ll make them all
happy! Great you got it into shape.

tsmish <tsymsh@gmail.com> skribis:

Toggle quote (10 lines)
> (let ...) stuff should be in function, but I don't know in which
> module it should go.
> Code is somewhat untested, proceed with caution.
>
> ---
> gnu/services/base.scm | 5 ++++-
> gnu/system.scm | 13 ++++++++-----
> gnu/system/mapped-devices.scm | 2 +-
> 3 files changed, 13 insertions(+), 7 deletions(-)

Side note: We’ll a commit log that follows our conventions¹ but that’s
something I can help with.


Toggle quote (15 lines)
> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
> index 0c154d1c4e..3d09e8220c 100644
> --- a/gnu/services/base.scm
> +++ b/gnu/services/base.scm
> @@ -408,7 +408,10 @@ FILE-SYSTEM."
> (define (mapped-device->shepherd-service-name md)
> "Return the symbol that denotes the shepherd service of MD, a
> <mapped-device>."
> (symbol-append 'device-mapping-
> - (string->symbol (mapped-device-target md))))
> + (string->symbol (string-join
> + (let ((t (mapped-device-target md)))
> + (if (list? t) t (list t)))
> + "-"))))

To avoid duplicating the (if (list? t) …) everywhere, I propose instead
the following approach:

1. Rename ‘target’ to ‘targets’ (plural) and likewise for the
accessor, and agree that it always contains a list;

2. Rename ‘mapped-device’ to ‘%mapped-device’ and add a
‘mapped-device’ backward-compatibility macro that allows for a
‘target’ (singular) field and automatically turns its value into a
list. See the ‘origin’ macro in (guix packages) for an example of
how to do that (that macro allows users to specify ‘sha256’ instead
of ‘hash’).

3. Add a deprecated ‘mapped-device-target’ (singular) that returns the
first element returned by ‘mapped-device-targets’.

We’ll need to adjust doc/guix.texi accordingly.

How does that sound?

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 9 Sep 2020 22:48
Re: [bug#41143] [PATCH 2/2] mapped-devices: Add 'lvm-device-mapping'
(name . tsmish)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
87sgbqllrd.fsf@gnu.org
tsmish <tsymsh@gmail.com> skribis:

Toggle quote (4 lines)
> "vgscan --mknodes" is a bit of a hack. Everyone else relies on udev to
> create files in /dev/mapper, but since initrd doesn't have working
> udevd, they need to be created this way.

Oh, I guess that’s fine. Could you place this as a comment above the
“vgscan” invocation?

Toggle quote (5 lines)
> Also, while this code is able to boot from root on LVM, grub in
> current configuration can't find required files, This can be fixed by
> placing (format port "insmod lvm") in grub configuration builder, but
> this is somewhat hacky.

Uh, future work. :-)

Toggle quote (6 lines)
> +(define (open-lvm-device source target)
> + #~(begin
> + (use-modules
> + (srfi srfi-1)
> + (srfi srfi-26))

Since this gets spliced into the initrd expression (not at the top
level), we cannot have ‘use-modules’ here (well, it may not work as
expected, as you found out with srfi-26 macros).

I’d suggest adding (srfi srfi-1) to the ‘use-modules’ form in
‘raw-initrd’, in (gnu system linux-initrd), so you can rely on it
(srfi-26 is already there).

It would be great to have a system test for LVM support. We have tests
for Btrfs, RAID with mdadm, etc., but these are system installation
tests in (gnu tests install). Do you think we could have either an
installation test or maybe a less expensive test for LVM?

Thanks for your work!

Ludo’.
M
M
Mikhail Tsykalov wrote on 24 Sep 2020 18:09
Re: [bug#41143] [PATCH 1/2] mapped-devices: Allow target to be list of strings
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 41143@debbugs.gnu.org)
9ac560cc-8660-f034-c0d8-536b68d20f68@gmail.com
Hi Ludovic,

On 09.09.2020 23:38, Ludovic Courtès wrote:
Toggle quote (30 lines)
>> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
>> index 0c154d1c4e..3d09e8220c 100644
>> --- a/gnu/services/base.scm
>> +++ b/gnu/services/base.scm
>> @@ -408,7 +408,10 @@ FILE-SYSTEM."
>> (define (mapped-device->shepherd-service-name md)
>> "Return the symbol that denotes the shepherd service of MD, a
>> <mapped-device>."
>> (symbol-append 'device-mapping-
>> - (string->symbol (mapped-device-target md))))
>> + (string->symbol (string-join
>> + (let ((t (mapped-device-target md)))
>> + (if (list? t) t (list t)))
>> + "-"))))
> To avoid duplicating the (if (list? t) …) everywhere, I propose instead
> the following approach:
>
> 1. Rename ‘target’ to ‘targets’ (plural) and likewise for the
> accessor, and agree that it always contains a list;
>
> 2. Rename ‘mapped-device’ to ‘%mapped-device’ and add a
> ‘mapped-device’ backward-compatibility macro that allows for a
> ‘target’ (singular) field and automatically turns its value into a
> list. See the ‘origin’ macro in (guix packages) for an example of
> how to do that (that macro allows users to specify ‘sha256’ instead
> of ‘hash’).
>
> 3. Add a deprecated ‘mapped-device-target’ (singular) that returns the
> first element returned by ‘mapped-device-targets’.

While this looks like a good idea, doesn't this break code that
implements mapped-device and assumes that target is a string. Suddenly
passing a string to a mapped-device constructor results in a list passed
to open/close. Also, what functions should do if they expect a string
but get a list of them? Ignore everything but the first item? Implement
mandatory check function? Doesn't this change push complexity out of
mapped-device to implementations of it.

Basically, this change will break all current implementations of
mapped-devices, so I wanted to hear your thoughts on it before continuing.

Mikhail
L
L
Ludovic Courtès wrote on 25 Sep 2020 11:34
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
87r1qq2o92.fsf@gnu.org
Hi Mikhail,

Mikhail Tsykalov <tsymsh@gmail.com> skribis:

Toggle quote (39 lines)
> On 09.09.2020 23:38, Ludovic Courtès wrote:
>>> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
>>> index 0c154d1c4e..3d09e8220c 100644
>>> --- a/gnu/services/base.scm
>>> +++ b/gnu/services/base.scm
>>> @@ -408,7 +408,10 @@ FILE-SYSTEM."
>>> (define (mapped-device->shepherd-service-name md)
>>> "Return the symbol that denotes the shepherd service of MD, a
>>> <mapped-device>."
>>> (symbol-append 'device-mapping-
>>> - (string->symbol (mapped-device-target md))))
>>> + (string->symbol (string-join
>>> + (let ((t (mapped-device-target md)))
>>> + (if (list? t) t (list t)))
>>> + "-"))))
>> To avoid duplicating the (if (list? t) …) everywhere, I propose instead
>> the following approach:
>>
>> 1. Rename ‘target’ to ‘targets’ (plural) and likewise for the
>> accessor, and agree that it always contains a list;
>>
>> 2. Rename ‘mapped-device’ to ‘%mapped-device’ and add a
>> ‘mapped-device’ backward-compatibility macro that allows for a
>> ‘target’ (singular) field and automatically turns its value into a
>> list. See the ‘origin’ macro in (guix packages) for an example of
>> how to do that (that macro allows users to specify ‘sha256’ instead
>> of ‘hash’).
>>
>> 3. Add a deprecated ‘mapped-device-target’ (singular) that returns the
>> first element returned by ‘mapped-device-targets’.
>
> While this looks like a good idea, doesn't this break code that
> implements mapped-device and assumes that target is a string. Suddenly
> passing a string to a mapped-device constructor results in a list
> passed to open/close. Also, what functions should do if they expect a
> string but get a list of them? Ignore everything but the first item?
> Implement mandatory check function? Doesn't this change push
> complexity out of mapped-device to implementations of it.

The intent of what I propose above is (1) to not break existing code,
and (2) to avoid duplicating checks and conversions at every call site.

#1 is achieved by providing a deprecated ‘mapped-device-target’
(singular) procedure, for example.

Does that make sense?

Thanks,
Ludo’.
M
M
Mikhail Tsykalov wrote on 25 Sep 2020 15:36
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 41143@debbugs.gnu.org)
cc872b95-370a-6247-cf44-64989e76bac6@gmail.com
Hi, Ludovic

On 25.09.2020 12:34, Ludovic Courtès wrote:
Toggle quote (49 lines)
> Hi Mikhail,
>
> Mikhail Tsykalov <tsymsh@gmail.com> skribis:
>
>> On 09.09.2020 23:38, Ludovic Courtès wrote:
>>>> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
>>>> index 0c154d1c4e..3d09e8220c 100644
>>>> --- a/gnu/services/base.scm
>>>> +++ b/gnu/services/base.scm
>>>> @@ -408,7 +408,10 @@ FILE-SYSTEM."
>>>> (define (mapped-device->shepherd-service-name md)
>>>> "Return the symbol that denotes the shepherd service of MD, a
>>>> <mapped-device>."
>>>> (symbol-append 'device-mapping-
>>>> - (string->symbol (mapped-device-target md))))
>>>> + (string->symbol (string-join
>>>> + (let ((t (mapped-device-target md)))
>>>> + (if (list? t) t (list t)))
>>>> + "-"))))
>>> To avoid duplicating the (if (list? t) …) everywhere, I propose instead
>>> the following approach:
>>>
>>> 1. Rename ‘target’ to ‘targets’ (plural) and likewise for the
>>> accessor, and agree that it always contains a list;
>>>
>>> 2. Rename ‘mapped-device’ to ‘%mapped-device’ and add a
>>> ‘mapped-device’ backward-compatibility macro that allows for a
>>> ‘target’ (singular) field and automatically turns its value into a
>>> list. See the ‘origin’ macro in (guix packages) for an example of
>>> how to do that (that macro allows users to specify ‘sha256’ instead
>>> of ‘hash’).
>>>
>>> 3. Add a deprecated ‘mapped-device-target’ (singular) that returns the
>>> first element returned by ‘mapped-device-targets’.
>> While this looks like a good idea, doesn't this break code that
>> implements mapped-device and assumes that target is a string. Suddenly
>> passing a string to a mapped-device constructor results in a list
>> passed to open/close. Also, what functions should do if they expect a
>> string but get a list of them? Ignore everything but the first item?
>> Implement mandatory check function? Doesn't this change push
>> complexity out of mapped-device to implementations of it.
> The intent of what I propose above is (1) to not break existing code,
> and (2) to avoid duplicating checks and conversions at every call site.
>
> #1 is achieved by providing a deprecated ‘mapped-device-target’
> (singular) procedure, for example.
>
> Does that make sense?

I'm sorry if I didn't make myself clear, but it doesn't seem like
open/close functions even use any mapped-device-* procedures, they just
get passed source and target field directly. What I meant was this
change will require changes to luks-device-mapping, raid-device-mapping
and all other device mappings that users may have implemented in their
local trees/config.

To be fair, after thinking about it for a bit, I think that this issue
can be solved by renaming mapped-device-kind and providing compatibility
macros similar to %mapped-device. Still question remains about what
should we do if a list gets passed to a kind that doesn't expect it, but
I think we can just raise an error in macro if that's the case. Does
this sound fine to you?

Thanks,
Mikhail
L
L
Ludovic Courtès wrote on 25 Sep 2020 18:20
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
87pn69j09o.fsf@gnu.org
Hi,

Mikhail Tsykalov <tsymsh@gmail.com> skribis:

[...]

Toggle quote (22 lines)
>>> While this looks like a good idea, doesn't this break code that
>>> implements mapped-device and assumes that target is a string. Suddenly
>>> passing a string to a mapped-device constructor results in a list
>>> passed to open/close. Also, what functions should do if they expect a
>>> string but get a list of them? Ignore everything but the first item?
>>> Implement mandatory check function? Doesn't this change push
>>> complexity out of mapped-device to implementations of it.
>> The intent of what I propose above is (1) to not break existing code,
>> and (2) to avoid duplicating checks and conversions at every call site.
>>
>> #1 is achieved by providing a deprecated ‘mapped-device-target’
>> (singular) procedure, for example.
>>
>> Does that make sense?
>
> I'm sorry if I didn't make myself clear, but it doesn't seem like
> open/close functions even use any mapped-device-* procedures, they
> just get passed source and target field directly. What I meant was
> this change will require changes to luks-device-mapping,
> raid-device-mapping and all other device mappings that users may have
> implemented in their local trees/config.

Ah yes, got it.

I tend to think it’s OK though, if we assume all the implementations are
in-tree, which would be consistent with the fact that the internals (how
to implement a mapped device type) are undocumented.

WDYT?

IOW, I think we must provide compatibility for users (people writing
their OS config, including perhaps service definitions) while leaving us
the ability to change internal interfaces.

Thanks,
Ludo’.
M
M
Mikhail Tsykalov wrote on 2 Oct 2020 00:48
[PATCH v2 1/2] mapped-devices: Allow target to be list of strings.
(address . ludo@gnu.org)
20201001224900.28989-1-tsymsh@gmail.com
* gnu/system/mapped-devices.scm (<mapped-device>): Rename constructor to
%mapped-device.
[target]: Remove field.
[targets]: New field. Adjust users.
(mapped-device-compatibility-helper, mapped-device): New macros.
(mapped-device-target): New deprecated procedure.
---
gnu/services/base.scm | 3 ++-
gnu/system.scm | 11 +++++-----
gnu/system/linux-initrd.scm | 2 +-
gnu/system/mapped-devices.scm | 40 ++++++++++++++++++++++++++++-------
4 files changed, 41 insertions(+), 15 deletions(-)

Toggle diff (165 lines)
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 04bc991356..4aa14ebf99 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -291,7 +291,8 @@ FILE-SYSTEM."
 (define (mapped-device->shepherd-service-name md)
   "Return the symbol that denotes the shepherd service of MD, a <mapped-device>."
   (symbol-append 'device-mapping-
-                 (string->symbol (mapped-device-target md))))
+                 (string->symbol (string-join
+                                  (mapped-device-targets md) "-"))))
 
 (define dependency->shepherd-service-name
   (match-lambda
diff --git a/gnu/system.scm b/gnu/system.scm
index bdb696fe2e..1bb812256f 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -444,9 +444,9 @@ marked as 'needed-for-boot'."
     (let ((device (file-system-device fs)))
       (if (string? device)                        ;title is 'device
           (filter (lambda (md)
-                    (string=? (string-append "/dev/mapper/"
-                                             (mapped-device-target md))
-                              device))
+                    (any (cut string=? device <>)
+                         (map (cut string-append "/dev/mapper" <>)
+                              (mapped-device-targets md))))
                   (operating-system-mapped-devices os))
           '())))
 
@@ -466,11 +466,12 @@ marked as 'needed-for-boot'."
 
 (define (mapped-device-users device file-systems)
   "Return the subset of FILE-SYSTEMS that use DEVICE."
-  (let ((target (string-append "/dev/mapper/" (mapped-device-target device))))
+  (let ((targets (map (cut string-append "/dev/mapper/" <>)
+                      (mapped-device-targets device))))
     (filter (lambda (fs)
               (or (member device (file-system-dependencies fs))
                   (and (string? (file-system-device fs))
-                       (string=? (file-system-device fs) target))))
+                       (any (cut string=? (file-system-device fs) <>) targets))))
             file-systems)))
 
 (define (operating-system-user-mapped-devices os)
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index b8a30c0abc..db02059a26 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -196,7 +196,7 @@ upon error."
     ;; List of gexps to open the mapped devices.
     (map (lambda (md)
            (let* ((source (mapped-device-source md))
-                  (target (mapped-device-target md))
+                  (target (mapped-device-targets md))
                   (type   (mapped-device-type md))
                   (open   (mapped-device-kind-open type)))
              (open source target)))
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 31c50c4e40..8622418fcf 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -28,6 +28,7 @@
                           formatted-message
                           &fix-hint
                           &error-location))
+  #:use-module (guix deprecation)
   #:use-module (gnu services)
   #:use-module (gnu services shepherd)
   #:use-module (gnu system uuid)
@@ -42,10 +43,12 @@
   #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
-  #:export (mapped-device
+  #:export (%mapped-device
+            mapped-device
             mapped-device?
             mapped-device-source
             mapped-device-target
+            mapped-device-targets
             mapped-device-type
             mapped-device-location
 
@@ -70,15 +73,36 @@
 ;;;
 ;;; Code:
 
-(define-record-type* <mapped-device> mapped-device
+(define-record-type* <mapped-device> %mapped-device
   make-mapped-device
   mapped-device?
   (source    mapped-device-source)                ;string | list of strings
-  (target    mapped-device-target)                ;string
+  (targets   mapped-device-targets)               ;list of strings
   (type      mapped-device-type)                  ;<mapped-device-kind>
   (location  mapped-device-location
              (default (current-source-location)) (innate)))
 
+(define-syntax mapped-device-compatibility-helper
+  (syntax-rules (target)
+    ((_ () (fields ...))
+     (%mapped-device fields ...))
+    ((_ ((target exp) rest ...) (others ...))
+     (%mapped-device others ...
+                      (targets (list exp))
+                      rest ...))
+    ((_ (field rest ...) (others ...))
+     (mapped-device-compatibility-helper (rest ...)
+                                         (others ... field)))))
+
+(define-syntax-rule (mapped-device fields ...)
+  "Build an <mapped-device> record, automatically converting 'target' field
+specifications to 'targets'."
+  (mapped-device-compatibility-helper (fields ...) ()))
+
+(define-deprecated (mapped-device-target md)
+  mapped-device-targets
+  (car (mapped-device-targets md)))
+
 (define-record-type* <mapped-device-type> mapped-device-kind
   make-mapped-device-kind
   mapped-device-kind?
@@ -100,7 +124,7 @@
      (($ <mapped-device> source target
                          ($ <mapped-device-type> open close))
       (shepherd-service
-       (provision (list (symbol-append 'device-mapping- (string->symbol target))))
+       (provision (list (symbol-append 'device-mapping- (string->symbol (string-join target "-")))))
        (requirement '(udev))
        (documentation "Map a device node using Linux's device mapper.")
        (start #~(lambda () #$(open source target)))
@@ -198,12 +222,12 @@ option of @command{guix system}.\n")
                                 (error "LUKS partition not found" source))
                             source)
 
-                        #$target)))))
+                        #$(car target))))))
 
 (define (close-luks-device source target)
   "Return a gexp that closes TARGET, a LUKS device."
   #~(zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
-                    "close" #$target)))
+                    "close" #$(car target))))
 
 (define* (check-luks-device md #:key
                             needed-for-boot?
@@ -259,12 +283,12 @@ TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
       ;; Use 'mdadm-static' rather than 'mdadm' to avoid pulling its whole
       ;; closure (80 MiB) in the initrd when a RAID device is needed for boot.
       (zero? (apply system* #$(file-append mdadm-static "/sbin/mdadm")
-                    "--assemble" #$target sources))))
+                    "--assemble" #$(car target) sources))))
 
 (define (close-raid-device sources target)
   "Return a gexp that stops the RAID device TARGET."
   #~(zero? (system* #$(file-append mdadm-static "/sbin/mdadm")
-                    "--stop" #$target)))
+                    "--stop" #$(car target))))
 
 (define raid-device-mapping
   ;; The type of RAID mapped devices.
-- 
2.28.0
M
M
Mikhail Tsykalov wrote on 2 Oct 2020 00:49
[PATCH v2 2/2] mapped-devices: Add 'lvm-device-mapping'.
(address . ludo@gnu.org)
20201001224900.28989-2-tsymsh@gmail.com
* gnu/system/mapped-devices.scm (lvm-device-mapping, open-lvm-device,
close-lvm-device): New variables.

* gnu/tests/install.scm (%lvm-separate-home-os,
%lvm-separate-home-os-source, %lvm-separate-home-installation-script,
%test-lvm-separate-home-os): New variables.

* gnu/system/linux-initrd.scm (raw-initrd): Add (srfi srfi-1) to initrd expression.
---
gnu/system/linux-initrd.scm | 1 +
gnu/system/mapped-devices.scm | 25 +++++++++-
gnu/tests/install.scm | 87 +++++++++++++++++++++++++++++++++++
3 files changed, 111 insertions(+), 2 deletions(-)

Toggle diff (167 lines)
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index db02059a26..41cbe28c3c 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -217,6 +217,7 @@ upon error."
                       (gnu system file-systems)
                       ((guix build utils) #:hide (delete))
                       (guix build bournish)   ;add the 'bournish' meta-command
+                      (srfi srfi-1)
                       (srfi srfi-26)
 
                       ;; FIXME: The following modules are for
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 8622418fcf..8088bd99a3 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -36,7 +36,7 @@
   #:autoload   (gnu build linux-modules)
                  (missing-modules)
   #:autoload   (gnu packages cryptsetup) (cryptsetup-static)
-  #:autoload   (gnu packages linux) (mdadm-static)
+  #:autoload   (gnu packages linux) (mdadm-static lvm2-static)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -64,7 +64,8 @@
             check-device-initrd-modules           ;XXX: needs a better place
 
             luks-device-mapping
-            raid-device-mapping))
+            raid-device-mapping
+            lvm-device-mapping))
 
 ;;; Commentary:
 ;;;
@@ -296,4 +297,24 @@ TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
    (open open-raid-device)
    (close close-raid-device)))
 
+(define (open-lvm-device source targets)
+  #~(and
+     (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                     "vgchange" "--activate" "ay" #$source))
+     ; /dev/mapper nodes are usually created by udev, but udev may be unavailable at the time we run this. So we create them here.
+     (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                     "vgscan" "--mknodes"))
+     (every file-exists? (map (lambda (file) (string-append "/dev/mapper/" file))
+                              '#$targets))))
+
+
+(define (close-lvm-device source targets)
+  #~(zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                    "vgchange" "--activate" "n" #$source)))
+
+(define lvm-device-mapping
+  (mapped-device-kind
+   (open open-lvm-device)
+   (close close-lvm-device)))
+
 ;;; mapped-devices.scm ends here
diff --git a/gnu/tests/install.scm b/gnu/tests/install.scm
index dee2b870e8..84fe8bda78 100644
--- a/gnu/tests/install.scm
+++ b/gnu/tests/install.scm
@@ -65,6 +65,7 @@
             %test-btrfs-root-on-subvolume-os
             %test-jfs-root-os
             %test-f2fs-root-os
+            %test-lvm-separate-home-os
 
             %test-gui-installed-os
             %test-gui-installed-os-encrypted
@@ -794,6 +795,92 @@ build (current-guix) and then store a couple of full system images.")
       (run-basic-test %encrypted-root-os command "encrypted-root-os"
                       #:initialization enter-luks-passphrase)))))
 
+
+;;;
+;;; Separate /home on LVM
+;;;
+
+; Since LVM support in guix currently doesn't allow root-on-LVM we use /home on LVM
+(define-os-with-source (%lvm-separate-home-os %lvm-separate-home-os-source)
+  (use-modules (gnu) (gnu tests))
+
+  (operating-system
+    (host-name "separate-home-on-lvm")
+    (timezone "Europe/Paris")
+    (locale "en_US.utf8")
+
+    (bootloader (bootloader-configuration
+                 (bootloader grub-bootloader)
+                 (target "/dev/vdb")))
+    (kernel-arguments '("console=ttyS0"))
+
+    (mapped-devices (list (mapped-device
+                           (source "vg0")
+                           (target "vg0-home")
+                           (type lvm-device-mapping))))
+    (file-systems (cons* (file-system
+                           (device (file-system-label "root-fs"))
+                           (mount-point "/")
+                           (type "ext4"))
+                         (file-system
+                           (device "/dev/mapper/vg0-home")
+                           (mount-point "/home")
+                           (type "ext4")
+                           (dependencies mapped-devices))
+                        %base-file-systems))
+    (users %base-user-accounts)
+    (services (cons (service marionette-service-type
+                             (marionette-configuration
+                              (imported-modules '((gnu services herd)
+                                                  (guix combinators)))))
+                    %base-services))))
+
+(define %lvm-separate-home-installation-script
+  "\
+. /etc/profile
+set -e -x
+guix --version
+
+export GUIX_BUILD_OPTIONS=--no-grafts
+parted --script /dev/vdb mklabel gpt \\
+  mkpart primary ext2 1M 3M \\
+  mkpart primary ext2 3M 1.6G \\
+  mkpart primary 1.6G 3.2G \\
+  set 1 boot on \\
+  set 1 bios_grub on
+pvcreate /dev/vdb3
+vgcreate vg0 /dev/vdb3
+lvcreate -L 1.6G -n home vg0
+vgchange -ay
+mkfs.ext4 -L root-fs /dev/vdb2
+mkfs.ext4 /dev/mapper/vg0-home
+mount /dev/vdb2 /mnt
+mkdir /mnt/home
+mount /dev/mapper/vg0-home /mnt/home
+df -h /mnt /mnt/home
+herd start cow-store /mnt
+mkdir /mnt/etc
+cp /etc/target-config.scm /mnt/etc/config.scm
+guix system init /mnt/etc/config.scm /mnt --no-substitutes
+sync
+reboot\n")
+
+(define %test-lvm-separate-home-os
+  (system-test
+   (name "lvm-separate-home-os")
+   (description
+    "Test functionality of an OS installed with a LVM /home partition")
+   (value
+    (mlet* %store-monad ((image   (run-install %lvm-separate-home-os
+                                               %lvm-separate-home-os-source
+                                               #:script
+                                               %lvm-separate-home-installation-script
+                                               #:packages (list lvm2-static)
+                                               #:target-size (* 3200 MiB)))
+                         (command (qemu-command/writable-image image)))
+      (run-basic-test %lvm-separate-home-os
+                      `(,@command) "lvm-separate-home-os")))))
+
 
 ;;;
 ;;; Btrfs root file system.
-- 
2.28.0
M
M
Mikhail Tsykalov wrote on 2 Oct 2020 01:15
Re: [bug#41143] [PATCH 1/2] mapped-devices: Allow target to be list of strings
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 41143@debbugs.gnu.org)
02538eb3-f3cf-2783-b99a-15d77e2be546@gmail.com
Hi,

Hoping I didn't mess up too much with patches. System tests seem to be
somewhat unstable and really long, but I think raid-root-os and
encrypted-root-os pass. Somewhat unsure about adding "(srfi srfi-1)" to
raw-initrd since current implementation shouldn't ever end up in initrd,
but this would be helpful if we actually implement booting from LVM and
I think I saw request to add it there in open-raid-device.

I also tend not to answer to things I agree with and was a bit busy with
work recently, so I hope you understand me taking so long to answer.

Thanks,
Mikhail
L
L
Ludovic Courtès wrote on 4 Oct 2020 12:28
Re: [PATCH v2 1/2] mapped-devices: Allow target to be list of strings.
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
874knaffov.fsf@gnu.org
Hi Mikhail,

Mikhail Tsykalov <tsymsh@gmail.com> skribis:

Toggle quote (7 lines)
> * gnu/system/mapped-devices.scm (<mapped-device>): Rename constructor to
> %mapped-device.
> [target]: Remove field.
> [targets]: New field. Adjust users.
> (mapped-device-compatibility-helper, mapped-device): New macros.
> (mapped-device-target): New deprecated procedure.

Thanks for following up. I think we’re almost done, some comments
below:

Toggle quote (9 lines)
> --- a/gnu/system/linux-initrd.scm
> +++ b/gnu/system/linux-initrd.scm
> @@ -196,7 +196,7 @@ upon error."
> ;; List of gexps to open the mapped devices.
> (map (lambda (md)
> (let* ((source (mapped-device-source md))
> - (target (mapped-device-target md))
> + (target (mapped-device-targets md))

I think we should write ‘targets’ (plural) everywhere. That can help
avoid confusion IMO.

Toggle quote (9 lines)
> - #$target)))))
> + #$(car target))))))
>
> (define (close-luks-device source target)
> "Return a gexp that closes TARGET, a LUKS device."
> #~(zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
> - "close" #$target)))
> + "close" #$(car target))))

As per our coding convention (info "(guix) Data Types and Pattern
Matching"), I’d recommend using ‘match’

(define (close-luks-device source targets)
(match targets
((target)
#~(zero? (system* … #$target)))))

That has the added benefit that it errors out if TARGETS is not exactly
a one-element list.

Toggle quote (6 lines)
> (define (close-raid-device sources target)
> "Return a gexp that stops the RAID device TARGET."
> #~(zero? (system* #$(file-append mdadm-static "/sbin/mdadm")
> - "--stop" #$target)))
> + "--stop" #$(car target))))

Same here.

Could you also update “Mapped Devices” in doc/guix.texi to mention the
new ‘targets’ field?

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 4 Oct 2020 12:34
Re: [PATCH v2 2/2] mapped-devices: Add 'lvm-device-mapping'.
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
87y2kme0u0.fsf@gnu.org
Mikhail Tsykalov <tsymsh@gmail.com> skribis:

Toggle quote (9 lines)
> * gnu/system/mapped-devices.scm (lvm-device-mapping, open-lvm-device,
> close-lvm-device): New variables.
>
> * gnu/tests/install.scm (%lvm-separate-home-os,
> %lvm-separate-home-os-source, %lvm-separate-home-installation-script,
> %test-lvm-separate-home-os): New variables.
>
> * gnu/system/linux-initrd.scm (raw-initrd): Add (srfi srfi-1) to initrd expression.

Nice!

Toggle quote (7 lines)
> +++ b/gnu/system/linux-initrd.scm
> @@ -217,6 +217,7 @@ upon error."
> (gnu system file-systems)
> ((guix build utils) #:hide (delete))
> (guix build bournish) ;add the 'bournish' meta-command
> + (srfi srfi-1)

Maybe add a comment saying this is for ‘lvm-device-mapping’.

Toggle quote (6 lines)
> +(define %test-lvm-separate-home-os
> + (system-test
> + (name "lvm-separate-home-os")
> + (description
> + "Test functionality of an OS installed with a LVM /home partition")

Great.

Do you know what it would take to have a LVM on root? (As future work,
of course.)

Could you document ‘lvm-device-mapping’ under “Mapped Devices” in
doc/guix.texi? Essentially adding a @defvr for ‘lvm-device-mapping’ and
a paragraph with an example.

There’s also a sentence somewhere in guix.texi that says LVM is not
supported, we can probably remove it. :-)

Last: you can add an entry in etc/news.scm to mention LVM support since
that’s a long awaited feature!

Thanks!

Ludo’.
L
L
Ludovic Courtès wrote on 5 Nov 2020 10:48
Re: [bug#41143] [PATCH v2 1/2] mapped-devices: Allow target to be list of strings.
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143@debbugs.gnu.org)
87o8kc5e2w.fsf@gnu.org
Hi Mikhail,

Did you have a chance to look into the proposed changes?


Would be nice to have LVM support integrated!

TIA,
Ludo’.

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

Toggle quote (59 lines)
> Hi Mikhail,
>
> Mikhail Tsykalov <tsymsh@gmail.com> skribis:
>
>> * gnu/system/mapped-devices.scm (<mapped-device>): Rename constructor to
>> %mapped-device.
>> [target]: Remove field.
>> [targets]: New field. Adjust users.
>> (mapped-device-compatibility-helper, mapped-device): New macros.
>> (mapped-device-target): New deprecated procedure.
>
> Thanks for following up. I think we’re almost done, some comments
> below:
>
>> --- a/gnu/system/linux-initrd.scm
>> +++ b/gnu/system/linux-initrd.scm
>> @@ -196,7 +196,7 @@ upon error."
>> ;; List of gexps to open the mapped devices.
>> (map (lambda (md)
>> (let* ((source (mapped-device-source md))
>> - (target (mapped-device-target md))
>> + (target (mapped-device-targets md))
>
> I think we should write ‘targets’ (plural) everywhere. That can help
> avoid confusion IMO.
>
>> - #$target)))))
>> + #$(car target))))))
>>
>> (define (close-luks-device source target)
>> "Return a gexp that closes TARGET, a LUKS device."
>> #~(zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> - "close" #$target)))
>> + "close" #$(car target))))
>
> As per our coding convention (info "(guix) Data Types and Pattern
> Matching"), I’d recommend using ‘match’
>
> (define (close-luks-device source targets)
> (match targets
> ((target)
> #~(zero? (system* … #$target)))))
>
> That has the added benefit that it errors out if TARGETS is not exactly
> a one-element list.
>
>> (define (close-raid-device sources target)
>> "Return a gexp that stops the RAID device TARGET."
>> #~(zero? (system* #$(file-append mdadm-static "/sbin/mdadm")
>> - "--stop" #$target)))
>> + "--stop" #$(car target))))
>
> Same here.
>
> Could you also update “Mapped Devices” in doc/guix.texi to mention the
> new ‘targets’ field?
>
> Thanks,
> Ludo’.
M
M
Mikhail Tsykalov wrote on 6 Nov 2020 10:47
[PATCH v3 2/2] mapped-devices: Add 'lvm-device-mapping'.
(address . ludo@gnu.org)
20201106094738.132011-2-tsymsh@gmail.com
* gnu/system/mapped-devices.scm (lvm-device-mapping, open-lvm-device,
close-lvm-device): New variables.

* gnu/tests/install.scm (%lvm-separate-home-os,
%lvm-separate-home-os-source, %lvm-separate-home-installation-script,
%test-lvm-separate-home-os): New variables.

* gnu/system/linux-initrd.scm (raw-initrd): Add (srfi srfi-1) to initrd expression.
---
doc/guix.texi | 25 +++++++++-
gnu/system/linux-initrd.scm | 1 +
gnu/system/mapped-devices.scm | 25 +++++++++-
gnu/tests/install.scm | 87 +++++++++++++++++++++++++++++++++++
4 files changed, 134 insertions(+), 4 deletions(-)

Toggle diff (231 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 02b92a9b69..acb6453165 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -12713,7 +12713,6 @@ Guix extends this notion by considering any device or set of devices that
 are @dfn{transformed} in some way to create a new device; for instance,
 RAID devices are obtained by @dfn{assembling} several other devices, such
 as hard disks or partitions, into a new one that behaves as one partition.
-Other examples, not yet implemented, are LVM logical volumes.
 
 Mapped devices are declared using the @code{mapped-device} form,
 defined as follows; for examples, see below.
@@ -12726,7 +12725,8 @@ the system boots up.
 @item source
 This is either a string specifying the name of the block device to be mapped,
 such as @code{"/dev/sda3"}, or a list of such strings when several devices
-need to be assembled for creating a new one.
+need to be assembled for creating a new one. In case of LVM this is a
+string specifying name of the volume group to be mapped.
 
 @item target
 This string specifies the name of the resulting mapped device.  For
@@ -12735,6 +12735,9 @@ specifying @code{"my-partition"} leads to the creation of
 the @code{"/dev/mapper/my-partition"} device.
 For RAID devices of type @code{raid-device-mapping}, the full device name
 such as @code{"/dev/md0"} needs to be given.
+LVM logical volumes of type @code{lvm-device-mapping} need to
+be specified as @code{"VGNAME-LVNAME"}.
+
 @item targets
 This list of strings specifies names of the resulting mapped devices in case
 there are several. The format is identical to @var{target}.
@@ -12758,6 +12761,11 @@ module for the appropriate RAID level to be loaded, such as @code{raid456}
 for RAID-4, RAID-5 or RAID-6, or @code{raid10} for RAID-10.
 @end defvr
 
+@defvr {Scheme Variable} lvm-device-mapping
+This defines LVM logical volume(s). Volume group is activated by
+@command{vgchange} command from the package @code{lvm2}.
+@end defvr
+
 @cindex disk encryption
 @cindex LUKS
 The following example specifies a mapping from @file{/dev/sda3} to
@@ -12815,6 +12823,19 @@ Note that the RAID level need not be given; it is chosen during the
 initial creation and formatting of the RAID device and is determined
 automatically later.
 
+LVM logical volumes ``alpha'' and ``beta'' from volume group ``vg0'' can
+be declared as follows:
+
+@lisp
+(mapped-device
+  (source "vg0")
+  (target (list "vg0-alpha" "vg0-beta"))
+  (type lvm-device-mapping))
+@end lisp
+
+Devices @file{/dev/mapper/vg0-alpha} and @file{/dev/mapper/vg0-beta} can
+then be used as the @code{device} of a @code{file-system} declaration
+(@pxref{File Systems}).
 
 @node User Accounts
 @section User Accounts
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 3e2f1282cc..85e493fecb 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -217,6 +217,7 @@ upon error."
                       (gnu system file-systems)
                       ((guix build utils) #:hide (delete))
                       (guix build bournish)   ;add the 'bournish' meta-command
+                      (srfi srfi-1)           ;for lvm-device-mapping
                       (srfi srfi-26)
 
                       ;; FIXME: The following modules are for
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 8b5aec983d..559c27bb28 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -36,7 +36,7 @@
   #:autoload   (gnu build linux-modules)
                  (missing-modules)
   #:autoload   (gnu packages cryptsetup) (cryptsetup-static)
-  #:autoload   (gnu packages linux) (mdadm-static)
+  #:autoload   (gnu packages linux) (mdadm-static lvm2-static)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -64,7 +64,8 @@
             check-device-initrd-modules           ;XXX: needs a better place
 
             luks-device-mapping
-            raid-device-mapping))
+            raid-device-mapping
+            lvm-device-mapping))
 
 ;;; Commentary:
 ;;;
@@ -304,4 +305,24 @@ TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
    (open open-raid-device)
    (close close-raid-device)))
 
+(define (open-lvm-device source targets)
+  #~(and
+     (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                     "vgchange" "--activate" "ay" #$source))
+     ; /dev/mapper nodes are usually created by udev, but udev may be unavailable at the time we run this. So we create them here.
+     (zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                     "vgscan" "--mknodes"))
+     (every file-exists? (map (lambda (file) (string-append "/dev/mapper/" file))
+                              '#$targets))))
+
+
+(define (close-lvm-device source targets)
+  #~(zero? (system* #$(file-append lvm2-static "/sbin/lvm")
+                    "vgchange" "--activate" "n" #$source)))
+
+(define lvm-device-mapping
+  (mapped-device-kind
+   (open open-lvm-device)
+   (close close-lvm-device)))
+
 ;;; mapped-devices.scm ends here
diff --git a/gnu/tests/install.scm b/gnu/tests/install.scm
index dee2b870e8..2d99638ecc 100644
--- a/gnu/tests/install.scm
+++ b/gnu/tests/install.scm
@@ -65,6 +65,7 @@
             %test-btrfs-root-on-subvolume-os
             %test-jfs-root-os
             %test-f2fs-root-os
+            %test-lvm-separate-home-os
 
             %test-gui-installed-os
             %test-gui-installed-os-encrypted
@@ -795,6 +796,92 @@ build (current-guix) and then store a couple of full system images.")
                       #:initialization enter-luks-passphrase)))))
 
 
+;;;
+;;; Separate /home on LVM
+;;;
+
+;; Since LVM support in guix currently doesn't allow root-on-LVM we use /home on LVM
+(define-os-with-source (%lvm-separate-home-os %lvm-separate-home-os-source)
+  (use-modules (gnu) (gnu tests))
+
+  (operating-system
+    (host-name "separate-home-on-lvm")
+    (timezone "Europe/Paris")
+    (locale "en_US.utf8")
+
+    (bootloader (bootloader-configuration
+                 (bootloader grub-bootloader)
+                 (target "/dev/vdb")))
+    (kernel-arguments '("console=ttyS0"))
+
+    (mapped-devices (list (mapped-device
+                           (source "vg0")
+                           (target "vg0-home")
+                           (type lvm-device-mapping))))
+    (file-systems (cons* (file-system
+                           (device (file-system-label "root-fs"))
+                           (mount-point "/")
+                           (type "ext4"))
+                         (file-system
+                           (device "/dev/mapper/vg0-home")
+                           (mount-point "/home")
+                           (type "ext4")
+                           (dependencies mapped-devices))
+                        %base-file-systems))
+    (users %base-user-accounts)
+    (services (cons (service marionette-service-type
+                             (marionette-configuration
+                              (imported-modules '((gnu services herd)
+                                                  (guix combinators)))))
+                    %base-services))))
+
+(define %lvm-separate-home-installation-script
+  "\
+. /etc/profile
+set -e -x
+guix --version
+
+export GUIX_BUILD_OPTIONS=--no-grafts
+parted --script /dev/vdb mklabel gpt \\
+  mkpart primary ext2 1M 3M \\
+  mkpart primary ext2 3M 1.6G \\
+  mkpart primary 1.6G 3.2G \\
+  set 1 boot on \\
+  set 1 bios_grub on
+pvcreate /dev/vdb3
+vgcreate vg0 /dev/vdb3
+lvcreate -L 1.6G -n home vg0
+vgchange -ay
+mkfs.ext4 -L root-fs /dev/vdb2
+mkfs.ext4 /dev/mapper/vg0-home
+mount /dev/vdb2 /mnt
+mkdir /mnt/home
+mount /dev/mapper/vg0-home /mnt/home
+df -h /mnt /mnt/home
+herd start cow-store /mnt
+mkdir /mnt/etc
+cp /etc/target-config.scm /mnt/etc/config.scm
+guix system init /mnt/etc/config.scm /mnt --no-substitutes
+sync
+reboot\n")
+
+(define %test-lvm-separate-home-os
+  (system-test
+   (name "lvm-separate-home-os")
+   (description
+    "Test functionality of an OS installed with a LVM /home partition")
+   (value
+    (mlet* %store-monad ((image   (run-install %lvm-separate-home-os
+                                               %lvm-separate-home-os-source
+                                               #:script
+                                               %lvm-separate-home-installation-script
+                                               #:packages (list lvm2-static)
+                                               #:target-size (* 3200 MiB)))
+                         (command (qemu-command/writable-image image)))
+      (run-basic-test %lvm-separate-home-os
+                      `(,@command) "lvm-separate-home-os")))))
+
+
 ;;;
 ;;; Btrfs root file system.
 ;;;
-- 
2.20.1
M
M
Mikhail Tsykalov wrote on 6 Nov 2020 10:47
[PATCH v3 1/2] mapped-devices: Allow target to be list of strings.
(address . ludo@gnu.org)
20201106094738.132011-1-tsymsh@gmail.com
* gnu/system/mapped-devices.scm (<mapped-device>): Rename constructor to
%mapped-device.
[target]: Remove field.
[targets]: New field. Adjust users.
(mapped-device-compatibility-helper, mapped-device): New macros.
(mapped-device-target): New deprecated procedure.
---
doc/guix.texi | 3 +
gnu/services/base.scm | 3 +-
gnu/system.scm | 11 ++-
gnu/system/linux-initrd.scm | 10 +-
gnu/system/mapped-devices.scm | 174 ++++++++++++++++++++--------------
5 files changed, 119 insertions(+), 82 deletions(-)

Toggle diff (325 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 79c79b6a96..02b92a9b69 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -12735,6 +12735,9 @@ specifying @code{"my-partition"} leads to the creation of
 the @code{"/dev/mapper/my-partition"} device.
 For RAID devices of type @code{raid-device-mapping}, the full device name
 such as @code{"/dev/md0"} needs to be given.
+@item targets
+This list of strings specifies names of the resulting mapped devices in case
+there are several. The format is identical to @var{target}.
 
 @item type
 This must be a @code{mapped-device-kind} object, which specifies how
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 04bc991356..4aa14ebf99 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -291,7 +291,8 @@ FILE-SYSTEM."
 (define (mapped-device->shepherd-service-name md)
   "Return the symbol that denotes the shepherd service of MD, a <mapped-device>."
   (symbol-append 'device-mapping-
-                 (string->symbol (mapped-device-target md))))
+                 (string->symbol (string-join
+                                  (mapped-device-targets md) "-"))))
 
 (define dependency->shepherd-service-name
   (match-lambda
diff --git a/gnu/system.scm b/gnu/system.scm
index bdb696fe2e..1bb812256f 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -444,9 +444,9 @@ marked as 'needed-for-boot'."
     (let ((device (file-system-device fs)))
       (if (string? device)                        ;title is 'device
           (filter (lambda (md)
-                    (string=? (string-append "/dev/mapper/"
-                                             (mapped-device-target md))
-                              device))
+                    (any (cut string=? device <>)
+                         (map (cut string-append "/dev/mapper" <>)
+                              (mapped-device-targets md))))
                   (operating-system-mapped-devices os))
           '())))
 
@@ -466,11 +466,12 @@ marked as 'needed-for-boot'."
 
 (define (mapped-device-users device file-systems)
   "Return the subset of FILE-SYSTEMS that use DEVICE."
-  (let ((target (string-append "/dev/mapper/" (mapped-device-target device))))
+  (let ((targets (map (cut string-append "/dev/mapper/" <>)
+                      (mapped-device-targets device))))
     (filter (lambda (fs)
               (or (member device (file-system-dependencies fs))
                   (and (string? (file-system-device fs))
-                       (string=? (file-system-device fs) target))))
+                       (any (cut string=? (file-system-device fs) <>) targets))))
             file-systems)))
 
 (define (operating-system-user-mapped-devices os)
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index b8a30c0abc..3e2f1282cc 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -195,11 +195,11 @@ upon error."
   (define device-mapping-commands
     ;; List of gexps to open the mapped devices.
     (map (lambda (md)
-           (let* ((source (mapped-device-source md))
-                  (target (mapped-device-target md))
-                  (type   (mapped-device-type md))
-                  (open   (mapped-device-kind-open type)))
-             (open source target)))
+           (let* ((source  (mapped-device-source md))
+                  (targets (mapped-device-targets md))
+                  (type    (mapped-device-type md))
+                  (open    (mapped-device-kind-open type)))
+             (open source targets)))
          mapped-devices))
 
   (define kodir
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 31c50c4e40..8b5aec983d 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -28,6 +28,7 @@
                           formatted-message
                           &fix-hint
                           &error-location))
+  #:use-module (guix deprecation)
   #:use-module (gnu services)
   #:use-module (gnu services shepherd)
   #:use-module (gnu system uuid)
@@ -42,10 +43,12 @@
   #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
-  #:export (mapped-device
+  #:export (%mapped-device
+            mapped-device
             mapped-device?
             mapped-device-source
             mapped-device-target
+            mapped-device-targets
             mapped-device-type
             mapped-device-location
 
@@ -70,15 +73,36 @@
 ;;;
 ;;; Code:
 
-(define-record-type* <mapped-device> mapped-device
+(define-record-type* <mapped-device> %mapped-device
   make-mapped-device
   mapped-device?
   (source    mapped-device-source)                ;string | list of strings
-  (target    mapped-device-target)                ;string
+  (targets   mapped-device-targets)               ;list of strings
   (type      mapped-device-type)                  ;<mapped-device-kind>
   (location  mapped-device-location
              (default (current-source-location)) (innate)))
 
+(define-syntax mapped-device-compatibility-helper
+  (syntax-rules (target)
+    ((_ () (fields ...))
+     (%mapped-device fields ...))
+    ((_ ((target exp) rest ...) (others ...))
+     (%mapped-device others ...
+                      (targets (list exp))
+                      rest ...))
+    ((_ (field rest ...) (others ...))
+     (mapped-device-compatibility-helper (rest ...)
+                                         (others ... field)))))
+
+(define-syntax-rule (mapped-device fields ...)
+  "Build an <mapped-device> record, automatically converting 'target' field
+specifications to 'targets'."
+  (mapped-device-compatibility-helper (fields ...) ()))
+
+(define-deprecated (mapped-device-target md)
+  mapped-device-targets
+  (car (mapped-device-targets md)))
+
 (define-record-type* <mapped-device-type> mapped-device-kind
   make-mapped-device-kind
   mapped-device-kind?
@@ -97,14 +121,14 @@
   (shepherd-service-type
    'device-mapping
    (match-lambda
-     (($ <mapped-device> source target
+     (($ <mapped-device> source targets
                          ($ <mapped-device-type> open close))
       (shepherd-service
-       (provision (list (symbol-append 'device-mapping- (string->symbol target))))
+       (provision (list (symbol-append 'device-mapping- (string->symbol (string-join targets "-")))))
        (requirement '(udev))
        (documentation "Map a device node using Linux's device mapper.")
-       (start #~(lambda () #$(open source target)))
-       (stop #~(lambda _ (not #$(close source target))))
+       (start #~(lambda () #$(open source targets)))
+       (stop #~(lambda _ (not #$(close source targets))))
        (respawn? #f))))))
 
 (define (device-mapping-service mapped-device)
@@ -162,48 +186,52 @@ option of @command{guix system}.\n")
 ;;; Common device mappings.
 ;;;
 
-(define (open-luks-device source target)
+(define (open-luks-device source targets)
   "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
 'cryptsetup'."
   (with-imported-modules (source-module-closure
                           '((gnu build file-systems)))
-    #~(let ((source #$(if (uuid? source)
-                          (uuid-bytevector source)
-                          source)))
-        ;; XXX: 'use-modules' should be at the top level.
-        (use-modules (rnrs bytevectors)           ;bytevector?
-                     ((gnu build file-systems)
-                      #:select (find-partition-by-luks-uuid)))
-
-        ;; Use 'cryptsetup-static', not 'cryptsetup', to avoid pulling the
-        ;; whole world inside the initrd (for when we're in an initrd).
-        (zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
-                        "open" "--type" "luks"
-
-                        ;; Note: We cannot use the "UUID=source" syntax here
-                        ;; because 'cryptsetup' implements it by searching the
-                        ;; udev-populated /dev/disk/by-id directory but udev may
-                        ;; be unavailable at the time we run this.
-                        (if (bytevector? source)
-                            (or (let loop ((tries-left 10))
-                                  (and (positive? tries-left)
-                                       (or (find-partition-by-luks-uuid source)
-                                           ;; If the underlying partition is
-                                           ;; not found, try again after
-                                           ;; waiting a second, up to ten
-                                           ;; times.  FIXME: This should be
-                                           ;; dealt with in a more robust way.
-                                           (begin (sleep 1)
-                                                  (loop (- tries-left 1))))))
-                                (error "LUKS partition not found" source))
-                            source)
-
-                        #$target)))))
-
-(define (close-luks-device source target)
+    (match targets
+      ((target)
+       #~(let ((source #$(if (uuid? source)
+                             (uuid-bytevector source)
+                             source)))
+           ;; XXX: 'use-modules' should be at the top level.
+           (use-modules (rnrs bytevectors) ;bytevector?
+                        ((gnu build file-systems)
+                         #:select (find-partition-by-luks-uuid)))
+
+           ;; Use 'cryptsetup-static', not 'cryptsetup', to avoid pulling the
+           ;; whole world inside the initrd (for when we're in an initrd).
+           (zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
+                           "open" "--type" "luks"
+
+                           ;; Note: We cannot use the "UUID=source" syntax here
+                           ;; because 'cryptsetup' implements it by searching the
+                           ;; udev-populated /dev/disk/by-id directory but udev may
+                           ;; be unavailable at the time we run this.
+                           (if (bytevector? source)
+                               (or (let loop ((tries-left 10))
+                                     (and (positive? tries-left)
+                                          (or (find-partition-by-luks-uuid source)
+                                              ;; If the underlying partition is
+                                              ;; not found, try again after
+                                              ;; waiting a second, up to ten
+                                              ;; times.  FIXME: This should be
+                                              ;; dealt with in a more robust way.
+                                              (begin (sleep 1)
+                                                     (loop (- tries-left 1))))))
+                                   (error "LUKS partition not found" source))
+                               source)
+
+                           #$target)))))))
+
+(define (close-luks-device source targets)
   "Return a gexp that closes TARGET, a LUKS device."
-  #~(zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
-                    "close" #$target)))
+  (match targets
+    ((target)
+     #~(zero? (system* #$(file-append cryptsetup-static "/sbin/cryptsetup")
+                       "close" #$target)))))
 
 (define* (check-luks-device md #:key
                             needed-for-boot?
@@ -235,36 +263,40 @@ option of @command{guix system}.\n")
    (close close-luks-device)
    (check check-luks-device)))
 
-(define (open-raid-device sources target)
+(define (open-raid-device sources targets)
   "Return a gexp that assembles SOURCES (a list of devices) to the RAID device
 TARGET (e.g., \"/dev/md0\"), using 'mdadm'."
-  #~(let ((sources '#$sources)
-
-          ;; XXX: We're not at the top level here.  We could use a
-          ;; non-top-level 'use-modules' form but that doesn't work when the
-          ;; code is eval'd, like the Shepherd does.
-          (every   (@ (srfi srfi-1) every))
-          (format  (@ (ice-9 format) format)))
-      (let loop ((attempts 0))
-        (unless (every file-exists? sources)
-          (when (> attempts 20)
-            (error "RAID devices did not show up; bailing out"
-                   sources))
-
-          (format #t "waiting for RAID source devices~{ ~a~}...~%"
-                  sources)
-          (sleep 1)
-          (loop (+ 1 attempts))))
-
-      ;; Use 'mdadm-static' rather than 'mdadm' to avoid pulling its whole
-      ;; closure (80 MiB) in the initrd when a RAID device is needed for boot.
-      (zero? (apply system* #$(file-append mdadm-static "/sbin/mdadm")
-                    "--assemble" #$target sources))))
-
-(define (close-raid-device sources target)
+  (match targets
+    ((target)
+     #~(let ((sources '#$sources)
+
+             ;; XXX: We're not at the top level here.  We could use a
+             ;; non-top-level 'use-modules' form but that doesn't work when the
+             ;; code is eval'd, like the Shepherd does.
+             (every   (@ (srfi srfi-1) every))
+             (format  (@ (ice-9 format) format)))
+         (let loop ((attempts 0))
+           (unless (every file-exists? sources)
+             (when (> attempts 20)
+               (error "RAID devices did not show up; bailing out"
+                      sources))
+
+             (format #t "waiting for RAID source devices~{ ~a~}...~%"
+                     sources)
+             (sleep 1)
+             (loop (+ 1 attempts))))
+
+         ;; Use 'mdadm-static' rather than 'mdadm' to avoid pulling its whole
+         ;; closure (80 MiB) in the initrd when a RAID device is needed for boot.
+         (zero? (apply system* #$(file-append mdadm-static "/sbin/mdadm")
+                       "--assemble" #$target sources))))))
+
+(define (close-raid-device sources targets)
   "Return a gexp that stops the RAID device TARGET."
-  #~(zero? (system* #$(file-append mdadm-static "/sbin/mdadm")
-                    "--stop" #$target)))
+  (match targets
+    ((target)
+     #~(zero? (system* #$(file-append mdadm-static "/sbin/mdadm")
+                       "--stop" #$target)))))
 
 (define raid-device-mapping
   ;; The type of RAID mapped devices.
-- 
2.20.1
L
L
Ludovic Courtès wrote on 26 Nov 2020 00:09
(name . Mikhail Tsykalov)(address . tsymsh@gmail.com)(address . 41143-done@debbugs.gnu.org)
87o8jlc9ub.fsf@gnu.org
Hi Mikhail,

Mikhail Tsykalov <tsymsh@gmail.com> skribis:

Toggle quote (7 lines)
> * gnu/system/mapped-devices.scm (<mapped-device>): Rename constructor to
> %mapped-device.
> [target]: Remove field.
> [targets]: New field. Adjust users.
> (mapped-device-compatibility-helper, mapped-device): New macros.
> (mapped-device-target): New deprecated procedure.

[...]

Toggle quote (9 lines)
> * gnu/system/mapped-devices.scm (lvm-device-mapping, open-lvm-device,
> close-lvm-device): New variables.
>
> * gnu/tests/install.scm (%lvm-separate-home-os,
> %lvm-separate-home-os-source, %lvm-separate-home-installation-script,
> %test-lvm-separate-home-os): New variables.
>
> * gnu/system/linux-initrd.scm (raw-initrd): Add (srfi srfi-1) to initrd expression.

Pushed as a9a2fdaabcc78e7a54d9a6bcfa4ee3de308e9a90!

Sorry for the delay, the release process took longer than one could hope
for. :-)

I added a news entry so users will learn about it upon ‘guix pull’.

Thank you!

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 41143@debbugs.gnu.org