Merge pull request #1865 from dalto8/mount-mount

Changes to mount module for 3.3

src/modules/fstab/fstab.conf

@@ -5,57 +5,8 @@
 # Also creates mount points for all the filesystems.
 #
 # When creating fstab entries for a filesystem, this module
-# uses the options for the filesystem type to write to the
+# uses the options previously defined in the mount module
-# options field of the file.
 ---
-# Mount options to use for all filesystems. If a specific filesystem
-# is listed here, use those options, otherwise use the *default*
-# options from this mapping.
-#
-# With kernels 5.15 and newer be cautious of adding the option space_cache
-# to the btrfs mount options.  The default in 5.15 changed to space_cache=v2.
-# If space_cache or space_cache=v1 are specified, it may fail to remount.
-#
-# btrfs_swap options are used when a swapfile is chosen with a btrfs root
-# the options are applied to the subvolume which holds the swap partition
-#
-# The settings shown here apply only the btrfs defaults; these
-# are generally the right ones. Commented-out lines show other
-# options wich **might** be applicable for specific situations.
-mountOptions:
-    default: defaults,noatime
-    # btrfs: defaults,noatime,autodefrag,compress=zstd
-    btrfs: defaults
-    # btrfs_swap: defaults,noatime
-    btrfs_swap: defaults
-
-# Mount options to use for the EFI System Partition. If not defined, the
-# *mountOptions* for *vfat* are used, or if that is not set either,
-# *default* from *mountOptions*.
-efiMountOptions: umask=0077
-
-# If a filesystem is on an SSD, add the following options. If a specific
-# filesystem is listed here, use those options, otherwise no additional
-# options are set (i.e. there is no *default* like in *mountOptions*).
-#
-# This example configuration applies the *discard* option to most
-# common filesystems on an SSD. This may not be the right option
-# for your distribution. If you use a systemd timer to trim the
-# SSD, it may interfere with the *discard* option. Opinions vary
-# as to whether *discard* is worth the effort -- it depends on
-# the usage pattern of the disk as well.
-#
-# ssdExtraMountOptions:
-#     ext4: discard
-#     jfs: discard
-#     xfs: discard
-#     swap: discard
-#     btrfs: discard,compress=lzo
-#
-# The standard configuration applies asynchronous discard support and ssd optimizations to btrfs
-# and does nothing for other filesystems.
-ssdExtraMountOptions:
-    btrfs: discard=async,ssd
 
 # Additional options added to each line in /etc/crypttab
 crypttabOptions: luks

src/modules/fstab/fstab.schema.yaml

@@ -6,23 +6,4 @@ $id: https://calamares.io/schemas/fstab
 additionalProperties: false
 type: object
 properties:
-    mountOptions:
-        type: object
-        additionalProperties: true  # we don't know which FS exist
-        properties:
-            default: { type: string }
-            btrfs: { type: string }
-        required: [ default ]
-    ssdExtraMountOptions:
-        type: object
-        additionalProperties: true  # we don't know which FS exist
-        properties:
-            ext4: { type: string }
-            jfs: { type: string }
-            xfs: { type: string }
-            swap: { type: string }
-            btrfs: { type: string }
-            btrfs_swap: { type: string }
-    efiMountOptions: { type: string }
     crypttabOptions: { type: string }
-required: [ mountOptions ]

src/modules/fstab/main.py

@@ -103,15 +103,13 @@ class FstabGenerator(object):
 
     :param partitions:
     :param root_mount_point:
-    :param mount_options:
+    :param mount_options_list:
-    :param ssd_extra_mount_options:
     """
-    def __init__(self, partitions, root_mount_point, mount_options,
+    def __init__(self, partitions, root_mount_point, mount_options_list,
-                 ssd_extra_mount_options, crypttab_options):
+                 crypttab_options):
         self.partitions = partitions
         self.root_mount_point = root_mount_point
-        self.mount_options = mount_options
+        self.mount_options_list = mount_options_list
-        self.ssd_extra_mount_options = ssd_extra_mount_options
         self.crypttab_options = crypttab_options
         self.ssd_disks = set()
         self.root_is_ssd = False
@@ -236,17 +234,7 @@ class FstabGenerator(object):
             libcalamares.utils.debug("Ignoring foreign swap {!s} {!s}".format(disk_name, partition.get("uuid", None)))
             return None
 
-        # If this is btrfs subvol a dedicated to a swapfile, use different options than a normal btrfs subvol
+        options = self.get_mount_options(mount_point)
-        if filesystem == "btrfs" and partition.get("subvol", None) == "/@swap":
-            options = self.get_mount_options("btrfs_swap", mount_point)
-        else:
-            options = self.get_mount_options(filesystem, mount_point)
-
-        if is_ssd:
-            extra = self.ssd_extra_mount_options.get(filesystem)
-
-            if extra:
-                options += "," + extra
 
         if mount_point == "/" and filesystem != "btrfs":
             check = 1
@@ -258,10 +246,6 @@ class FstabGenerator(object):
         if mount_point == "/":
             self.root_is_ssd = is_ssd
 
-        # If there's a set-and-not-empty subvolume set, add it
-        if filesystem == "btrfs" and partition.get("subvol",None):
-            options = "subvol={},".format(partition["subvol"]) + options
-
         if has_luks:
             device = "/dev/mapper/" + partition["luksMapperName"]
         elif partition["uuid"] is not None:
@@ -292,15 +276,18 @@ class FstabGenerator(object):
             if partition["mountPoint"]:
                 mkdir_p(self.root_mount_point + partition["mountPoint"])
 
-    def get_mount_options(self, filesystem, mount_point):
+    def get_mount_options(self, mountpoint):
-        efiMountPoint = libcalamares.globalstorage.value("efiSystemPartition")
+        """
-        job_config = libcalamares.job.configuration
+        Returns the mount options for a given mountpoint
 
-        if (mount_point == efiMountPoint and "efiMountOptions" in job_config):
+        :param mountpoint: A string containing the mountpoint for the fstab entry
-            return job_config["efiMountOptions"]
+        :return: A string containing the mount options for the entry or "defaults" if nothing is found
-
+        """
-        return self.mount_options.get(filesystem,
+        mount_options_item = next((x for x in self.mount_options_list if x.get("mountpoint") == mountpoint), None)
-                                      self.mount_options["default"])
+        if mount_options_item:
+            return mount_options_item.get("option_string", "defaults")
+        else:
+            return "defaults"
 
 
 def create_swapfile(root_mount_point, root_btrfs):
@@ -383,21 +370,19 @@ def run():
             swap_choice = None
 
     libcalamares.job.setprogress(0.1)
-    mount_options = conf.get("mountOptions", {})
+    mount_options_list = global_storage.value("mountOptionsList")
-    ssd_extra_mount_options = conf.get("ssdExtraMountOptions", {})
     crypttab_options = conf.get("crypttabOptions", "luks")
 
     # We rely on mount_options having a default; if there wasn't one,
     # bail out with a meaningful error.
-    if not mount_options:
+    if not mount_options_list:
         return (_("Configuration Error"),
                 _("No <pre>{!s}</pre> configuration is given for <pre>{!s}</pre> to use.")
                 .format("mountOptions", "fstab"))
 
     generator = FstabGenerator(partitions,
                                root_mount_point,
-                               mount_options,
+                               mount_options_list,
-                               ssd_extra_mount_options,
                                crypttab_options)
 
     if swap_choice is not None:

src/modules/mount/main.py

@@ -16,6 +16,7 @@
 import tempfile
 import subprocess
 import os
+import re
 
 import libcalamares
 
@@ -42,6 +43,79 @@ def pretty_name():
     return _("Mounting partitions.")
 
 
+def disk_name_for_partition(partition):
+    """ Returns disk name for each found partition.
+
+    :param partition:
+    :return:
+    """
+    name = os.path.basename(partition["device"])
+
+    if name.startswith("/dev/mmcblk") or name.startswith("/dev/nvme"):
+        return re.sub("p[0-9]+$", "", name)
+
+    return re.sub("[0-9]+$", "", name)
+
+
+def is_ssd_disk(partition):
+    """ Checks if given partition is on an ssd disk.
+
+    :param partition: A dict containing the partition information
+    :return: True is the partition in on an ssd, False otherwise
+    """
+
+    try:
+        disk_name = disk_name_for_partition(partition)
+        filename = os.path.join("/sys/block", disk_name, "queue/rotational")
+
+        with open(filename) as sysfile:
+            return sysfile.read() == "0\n"
+    except:
+        return False
+
+
+def get_mount_options(filesystem, mount_options, partition):
+    """
+    Returns the mount options for the partition object and filesystem
+
+    :param filesystem: A string containing the filesystem
+    :param mount_options: A list of dicts that descripes the mount options for each mountpoint
+    :param partition: A dict containing information about the partition
+    :return: A comma seperated string containing the mount options suitable for passing to mount
+    """
+
+    # Extra mounts can optionally have "options" set, in this case, they override other all other settings
+    if "options" in partition:
+        return ",".join(partition["options"])
+
+    # If there are no mount options defined then we use the defaults
+    if mount_options is None:
+        return "defaults"
+
+    options = next((x for x in mount_options if x["filesystem"] == filesystem), None)
+
+    # If there is no match then check for default options
+    if options is None:
+        options = next((x for x in mount_options if x["filesystem"] == "default"), None)
+
+    # If it is still None, then fallback to returning defaults
+    if options is None:
+        return "defaults"
+
+    option_items = options.get("options", []).copy()
+
+    # Append the appropriate options for ssd or hdd if set
+    if is_ssd_disk(partition):
+        option_items.extend(options.get("ssdOptions", []))
+    else:
+        option_items.extend(options.get("hddOptions", []))
+
+    if option_items:
+        return ",".join(option_items)
+    else:
+        return "defaults"
+
+
 def get_btrfs_subvolumes(partitions):
     """
     Gets the job-configuration for btrfs subvolumes, or if there is
@@ -69,7 +143,9 @@ def get_btrfs_subvolumes(partitions):
     # If we have a swap **file**, give it a separate subvolume.
     swap_choice = libcalamares.globalstorage.value("partitionChoices")
     if swap_choice and swap_choice.get("swap", None) == "file":
-        btrfs_subvolumes.append({'mountPoint': '/swap', 'subvolume': '/@swap'})
+        swap_subvol = libcalamares.job.configuration.get("btrfsSwapSubvol", "/@swap")
+        btrfs_subvolumes.append({'mountPoint': '/swap', 'subvolume': swap_subvol})
+        libcalamares.globalstorage.insert("btrfsSwapSubvol", swap_subvol)
 
     return btrfs_subvolumes
 
@@ -138,9 +214,16 @@ def mount_zfs(root_mount_point, partition):
             raise ZfsException(_("Failed to set zfs mountpoint"))
 
 
-def mount_partition(root_mount_point, partition, partitions):
+def mount_partition(root_mount_point, partition, partitions, mount_options, mount_options_list):
     """
     Do a single mount of @p partition inside @p root_mount_point.
+
+    :param root_mount_point: A string containing the root of the install
+    :param partition: A dict containing information about the partition
+    :param partitions: The full list of partitions used to filter out btrfs subvols which have duplicate mountpoints
+    :param mount_options: The mount options from the config file
+    :param mount_options_list: A list of options for each mountpoint to be placed in global storage for future modules
+    :return:
     """
     # Create mount point with `+` rather than `os.path.join()` because
     # `partition["mountPoint"]` starts with a '/'.
@@ -176,11 +259,13 @@ def mount_partition(root_mount_point, partition, partitions):
     if fstype == "zfs":
         mount_zfs(root_mount_point, partition)
     else:  # fstype == "zfs"
+        mount_options_string = get_mount_options(fstype, mount_options, partition)
         if libcalamares.utils.mount(device,
                                     mount_point,
                                     fstype,
-                                    partition.get("options", "")) != 0:
+                                    mount_options_string) != 0:
             libcalamares.utils.warning("Cannot mount {}".format(device))
+        mount_options_list.append({"mountpoint": raw_mount_point, "option_string": mount_options_string})
 
     # Special handling for btrfs subvolumes. Create the subvolumes listed in mount.conf
     if fstype == "btrfs" and partition["mountPoint"] == '/':
@@ -207,13 +292,19 @@ def mount_partition(root_mount_point, partition, partitions):
             device = os.path.join("/dev/mapper", partition["luksMapperName"])
 
         # Mount the subvolumes
+        swap_subvol = libcalamares.job.configuration.get("btrfsSwapSubvol", "/@swap")
         for s in btrfs_subvolumes:
             mount_option = "subvol={}".format(s['subvolume'])
+            if s['subvolume'] == swap_subvol:
+                mount_option += "," + get_mount_options("btrfs_swap", mount_options, partition)
+            else:
+                mount_option += "," + get_mount_options(fstype, mount_options, partition)
             subvolume_mountpoint = mount_point[:-1] + s['mountPoint']
+            mount_options_list.append({"mountpoint": s['mountPoint'], "option_string": mount_option})
             if libcalamares.utils.mount(device,
                                         subvolume_mountpoint,
                                         fstype,
-                                        ",".join([mount_option, partition.get("options", "")])) != 0:
+                                        mount_option) != 0:
                 libcalamares.utils.warning("Cannot mount {}".format(device))
 
 
@@ -222,6 +313,7 @@ def run():
     Mount all the partitions from GlobalStorage and from the job configuration.
     Partitions are mounted in-lexical-order of their mountPoint.
     """
+
     partitions = libcalamares.globalstorage.value("partitions")
 
     if not partitions:
@@ -231,14 +323,18 @@ def run():
 
     root_mount_point = tempfile.mkdtemp(prefix="calamares-root-")
 
+    # Get the mountOptions, if this is None, that is OK and will be handled later
+    mount_options = libcalamares.job.configuration.get("mountOptions")
+
     # Guard against missing keys (generally a sign that the config file is bad)
     extra_mounts = libcalamares.job.configuration.get("extraMounts") or []
-    extra_mounts_efi = libcalamares.job.configuration.get("extraMountsEfi") or []
+    if not extra_mounts:
-    if not extra_mounts and not extra_mounts_efi:
         libcalamares.utils.warning("No extra mounts defined. Does mount.conf exist?")
 
-    if libcalamares.globalstorage.value("firmwareType") == "efi":
+    if libcalamares.globalstorage.value("firmwareType") != "efi":
-        extra_mounts.extend(extra_mounts_efi)
+        for mount in extra_mounts:
+            if mount.get("efi", None) is True:
+                extra_mounts.remove(mount)
 
     # Add extra mounts to the partitions list and sort by mount points.
     # This way, we ensure / is mounted before the rest, and every mount point
@@ -246,13 +342,17 @@ def run():
     # under /tmp, we make sure /tmp is mounted before the partition)
     mountable_partitions = [p for p in partitions + extra_mounts if "mountPoint" in p and p["mountPoint"]]
     mountable_partitions.sort(key=lambda x: x["mountPoint"])
+
+    # mount_options_list will be inserted into global storage for use in fstab later
+    mount_options_list = []
     try:
         for partition in mountable_partitions:
-            mount_partition(root_mount_point, partition, partitions)
+            mount_partition(root_mount_point, partition, partitions, mount_options, mount_options_list)
     except ZfsException as ze:
         return _("zfs mounting error"), ze.message
 
     libcalamares.globalstorage.insert("rootMountPoint", root_mount_point)
+    libcalamares.globalstorage.insert("mountOptionsList", mount_options_list)
 
     # Remember the extra mounts for the unpackfs module
     libcalamares.globalstorage.insert("extraMounts", extra_mounts)

src/modules/mount/mount.conf

@@ -5,16 +5,15 @@
 # target as a usable chroot / "live" system). Filesystems are
 # automatically mounted from the partitioning module. Filesystems
 # listed here are **extra**. The filesystems listed in *extraMounts*
-# are mounted in all target systems. The filesystems listed in
+# are mounted in all target systems.
-# *extraMountsEfi* are mounted in the target system **only** if
-# the host machine uses UEFI.
 ---
 # Extra filesystems to mount. The key's value is a list of entries; each
-# entry has four keys:
+# entry has five keys:
 #   - device    The device node to mount
 #   - fs (optional) The filesystem type to use
 #   - mountPoint Where to mount the filesystem
-#   - options (optional) Extra options to pass to mount(8)
+#   - options (optional) An array of options to pass to mount
+#   - efi (optional) A boolean that when true is only mounted for UEFI installs
 #
 # The device is not mounted if the mountPoint is unset or if the fs is
 # set to unformatted.
@@ -28,18 +27,17 @@ extraMounts:
       mountPoint: /sys
     - device: /dev
       mountPoint: /dev
-      options: bind
+      options: [ bind ]
     - device: tmpfs
       fs: tmpfs
       mountPoint: /run
     - device: /run/udev
       mountPoint: /run/udev
-      options: bind
+      options: [ bind ]
-
-extraMountsEfi:
     - device: efivarfs
       fs: efivarfs
       mountPoint: /sys/firmware/efi/efivars
+      efi: true
 
 # Btrfs subvolumes to create if root filesystem is on btrfs volume.
 # If *mountpoint* is mounted already to another partition, it is ignored.
@@ -47,10 +45,8 @@ extraMountsEfi:
 #
 # It is possible to prevent subvolume creation -- this is likely only relevant
 # for the root (/) subvolume -- by giving an empty string as a subvolume
-# name. In this case no subvolume will be created. When using snapper as
+# name. In this case no subvolume will be created.
-# a rollback mechanism, it is recommended to **not** create a subvolume
+#
-# for root.
-
 btrfsSubvolumes:
     - mountPoint: /
       subvolume: /@
@@ -63,3 +59,67 @@ btrfsSubvolumes:
       subvolume: /@cache
     - mountPoint: /var/log
       subvolume: /@log
+
+# The name of the btrfs subvolume holding the swapfile.  This only used when
+# a swapfile is selected and the root filesystem is btrfs
+#
+btrfsSwapSubvol: /@swap
+
+# The mount options used to mount each filesystem.
+#
+# filesystem contains the name of the filesystem or on of three special
+# values, "default", efi" and "btrfs_swap".  The logic is applied in this manner:
+#   - If the partition is the EFI partition, the "efi" entry will be used
+#   - If the fs is btrfs and the subvolume is for the swapfile,
+#     the "btrfs_swap" entry is used
+#   - If the  filesystem is an exact match for filesystem, that entry is used
+#   - If no match is found in the above, the default entry is used
+#   - If there is no match and no default entry, "defaults" is used
+#   - If the mountOptions key is not present, "defaults" is used
+#
+# Each filesystem entry contains 3 keys, all of which are optional
+#   options - An array of mount options that is used on all disk types
+#   ssdOptions - An array of mount options combined with options for ssds
+#   hddOptions - An array of mount options combined with options for hdds
+# If combining these options results in an empty array, "defaults" is used
+#
+# Example 1
+# In this example, there are specific options for ext4 and btrfs filesystems,
+# the EFI partition and the subvolume holding the btrfs swapfile.  All other
+# filesystems use the default entry.  For the btrfs filesystem, there are
+# additional options specific to hdds and ssds
+#
+# mountOptions:
+#    - filesystem: default
+#      options: [ defaults ]
+#    - filesystem: efi
+#      options: [ defaults, umask=0077 ]
+#    - filesystem: ext4
+#      options: [ defaults, noatime ]
+#    - filesystem: btrfs
+#      options: [ defaults, noatime, compress=zstd:1 ]
+#      ssdOptions: [ discard=async ]
+#      hddOptions: [ autodefrag ]
+#    - filesystem: btrfs_swap
+#      options: [ defaults, noatime ]
+#
+# Example 2
+# In this example there is a single default used by all filesystems
+#
+# mountOptions:
+#    - filesystem: default
+#      options: [ defaults, noatime ]
+#
+mountOptions:
+    - filesystem: default
+      options: [ defaults, noatime ]
+    - filesystem: efi
+      options: [ defaults, umask=0077 ]
+    - filesystem: btrfs
+      options: [ defaults, noatime, compress=lzo ]
+    - filesystem: btrfs_swap
+      options: [ defaults, noatime ]
+
+
+
+

src/modules/mount/mount.schema.yaml

@@ -6,7 +6,6 @@ $id: https://calamares.io/schemas/mount
 additionalProperties: false
 type: object
 properties:
-    # TODO: share the schema definition, since these are identical
     extraMounts:
         type: array
         items:
@@ -17,17 +16,7 @@ properties:
                 fs: { type: string }
                 mountPoint: { type: string }
                 options: { type: string }
-            required: [ device, mountPoint ]
+                efi: { type: boolean, default: false }
-    extraMountsEfi:
-        type: array
-        items:
-            type: object
-            additionalProperties: false
-            properties:
-                device: { type: string }
-                fs: { type: string }
-                mountPoint: { type: string }
-                options: { type: string }
             required: [ device, mountPoint ]
     btrfsSubvolumes:
         type: array
@@ -38,3 +27,17 @@ properties:
                 mountPoint: { type: string }
                 subvolume: { type: string }
             required: [ subvolume, mountPoint ]
+    btrfsSwapSubvol: { type: string }
+    mountOptions:
+        type: array
+        items:
+            type: object
+            additionalProperties: false
+            properties:
+                filesystem: { type: string }
+                options: { type: array }
+                ssdOptions: { type: array }
+                hddOptions: { type: array }
+            required: [ filesystem ]
+
+