ZFS Features Support: Dataset property inheritance

[TheRealGramdalf]

I'm getting an error when trying to run disko with the following config:

{
  disko.devices = {
    disk."zdisk" = {
      device = "/dev/sda";
      type = "disk";
      content = {
        type = "gpt";
        partitions = {
          # This allows a GPT partition table to be used with legacy BIOS systems. See https://www.gnu.org/software/grub/manual/grub/html_node/BIOS-installation.html
          "mbr" = {
            label = "mbr";
            size = "1M";
            type = "EF02";
          };
          "zroot" = {
            label = "zroot";
            size = "100%";
            content = {
              type = "zfs";
              pool = "zroot";
          };};
          "ZSYS" = {
            label = "ZSYS";
            end = "-512M"; # Negative size to place it at the end of the partition
            type = "EF00"; # ^^ Mostly desirable with resizeable partitions like BTRFS/EXT4, used here for compatibility
            content = {
              type = "filesystem";
              format = "vfat";
              mountpoint = "/boot";
          };};
        };
      };
    };
    zpool."zroot" = {
      type = "zpool";
      options.ashift = "12";
      rootFsOptions = {
        # These are inherited to all child datasets as the default value
        mountpoint = "none";
        compression = "zstd";
        xattr = "sa";
        acltype = "posix";
      };

      datasets = {
        "ephemeral" = {
          type = "zfs_fs";
          options = {
            canmount = "noauto";
            mountpoint = "legacy";
        };};
        "ephemeral/nix" = {
          type = "zfs_fs";
          mountpoint = "/nix";
          options = {
            atime = "off";
            canmount = "noauto";
        };};
        "safe" = {
          type = "zfs_fs";
          options = {
            canmount = "noauto";
            mountpoint = "legacy";
        };};
        "safe/persist" = {
          type = "zfs_fs";
          mountpoint = "/persist";
          options = {
            canmount = "noauto";
        };};
        "safe/home" = {
          type = "zfs_fs";
          mountpoint = "/home";
          options = {
            canmount = "noauto";
        };};
        "system-state" = {
          type = "zfs_fs";
          mountpoint = "/";
          options = {
            canmount = "noauto";
            mountpoint = "legacy";
        };};
      };
      #preMountHook = "zfs snapshot -r zroot@blank";
      # Needs fixing, runs multiple times
    };
  };
}

When mounting, it attempts to run mount -t zfs zroot/safe/home /mnt/home -o X-mount.mkdir -o defaults -o zfsutil, which fails saying

filesystem 'zroot/safe/home' cannot be mounted using 'zfs mount'.
Use 'zfs set mountpoint=/mnt/home' or 'mount -t zfs zroot/safe/home /mnt/home'.
See zfs(8) for more information.

I tracked it down to this line, which to my understanding adds -o zfsutil if the disko option datasets.<datasetname>.options.mountpoint = legacy is not set - this is an issue in my case because the option technically is set by natural zfs inheritance, but since it isn't declared within disko, it still adds -o zfsutil, which fails since the zfsprop mountpoint is set to legacy, not /path/to/mountpoint as expected by zfs mount.

For now I'll just add mountpoint = legacy in the disko config, but handling this more gracefully would be ideal. I'm really not sure how it would be best implemented, since you could either add custom glue to represent inheritance within disko (keeping in mind that not all values are inherited, i.e. canmount), or just run checks at runtime instead of checking against the disko config provided.

Originally posted by @TheRealGramdalf in https://github.com/nix-community/disko/issues/298#issuecomment-1949322912

[Lassulus]

we could move the logic for adding zfsutil to the mountLine into an extraOption you could disable of the inherited datasets? maybe that settings can be done on zpool leven so we don't need to set it for every dataset manually?

[TheRealGramdalf]

Apologies for the long wait, I've been busy.

I think it boils down to what disko is meant to be. In my mind, disko is supposed to be a translation layer between disk utilities (sgdisk, zfs/zpool create, etc) rather than trying to manage everything itself. While some level of basic type checking is warranted, I think it makes sense to follow ZFS as closely as possible.

[Enzime]

I think there are two ways to handle this, either reimplement the inheritance logic for datasets (however this may be a bit messy as we'll have to either match prefixes as we currently have datasets at a flat level or change the disko configuration to be nested), or require the option mountpoint to be explicitly set. Ideally, the first one would be good as it would be inherited for all other ZFS dataset options.

[iFreilicht]

Hmm, if we require mountpoint to be set explicitly, we would also need to support null as users might not want to set a mountpoint in the config (though I'm not sure how often that's the case). That would also be a breaking change.

A third option would be to simply document that this is a limitation in disko, and that you need to set mountpoint=legacy for all nested zfs datasets manually.

Regarding your comment in the duplicated issue:

I don't think this will be easy to change at runtime because mount options get set in the NixOS config under fileSystems and get baked into /etc/fstab

That's a good point, I only looked at _mount, not _config.