zfsonlinux / pkg-zfs

Native ZFS packaging for Debian and Ubuntu
https://launchpad.net/~zfs-native/+archive/daily
308 stars 55 forks source link

Ubuntu 17.04, initramfs zfs script does not honor datasets with canmount property set to none #221

Closed samvde closed 6 years ago

samvde commented 7 years ago

I hope this is the right place for this.

The zfs script in the Ubuntu 17.04 initramfs defines and uses a "mount_fs" function that doesn't honor the canmount property, mounting all datasets (even those with the canmount property set to "off") using "mount -o zfsutil -t zfs". This can cause the boot process to fail when processing subsequent datasets.

Update: the mountpoint gets overruled in some cases as well. Datasets with canmount set to off and mountpoint set to none, still get mounted.

My expectation would be that, other than the rootfs, mount_fs does not proces datasets that do not have mountable options set.

samvde commented 7 years ago

Relevant code in https://github.com/zfsonlinux/zfs/blob/master/contrib/initramfs/scripts/zfs:

mount_fs() { local fs="$1" local mountpoint

# Check that the filesystem exists
"${ZFS}" list -oname -tfilesystem -H "${fs}" > /dev/null 2>&1
[ "$?" -ne 0 ] && return 1

# Need the _original_ datasets mountpoint!
mountpoint=$(get_fs_value "$fs" mountpoint)
if [ "$mountpoint" = "legacy" -o "$mountpoint" = "none" ]; then
    # Can't use the mountpoint property. Might be one of our
    # clones. Check the 'org.zol:mountpoint' property set in
    # clone_snap() if that's usable.
    mountpoint=$(get_fs_value "$fs" org.zol:mountpoint)
    if [ "$mountpoint" = "legacy" -o \
        "$mountpoint" = "none" -o \
        "$mountpoint" = "-" ]
    then
        if [ "$fs" != "${ZFS_BOOTFS}" ]; then
            # We don't have a proper mountpoint, this
            # isn't the root fs. So extract the root fs
            # value from the filesystem, and we should
            # (hopefully!) have a mountpoint we can use.
            mountpoint="${fs##$ZFS_BOOTFS}"
        else
            # Last hail-mary: Hope 'rootmnt' is set!
            mountpoint=""
        fi
    fi

    if [ "$mountpoint" = "legacy" ]; then
        ZFS_CMD="mount -t zfs"
    else
        # If it's not a legacy filesystem, it can only be a
        # native one...
        ZFS_CMD="mount -o zfsutil -t zfs"
    fi
else
    ZFS_CMD="mount -o zfsutil -t zfs"
fi

# Possibly decrypt a filesystem using native encryption.
decrypt_fs "$fs"

[ "$quiet" != "y" ] && \
    zfs_log_begin_msg "Mounting '${fs}' on '${rootmnt}/${mountpoint}'"
[ -n "${ZFS_DEBUG}" ] && \
    zfs_log_begin_msg "CMD: '$ZFS_CMD ${fs} ${rootmnt}/${mountpoint}'"

ZFS_STDERR=$(${ZFS_CMD} "${fs}" "${rootmnt}/${mountpoint}" 2>&1)
ZFS_ERROR=$?
if [ "${ZFS_ERROR}" != 0 ]
then
    [ "$quiet" != "y" ] && zfs_log_failure_msg "${ZFS_ERROR}"

    disable_plymouth
    echo ""
    echo "Command: ${ZFS_CMD} ${fs} ${rootmnt}/${mountpoint}"
    echo "Message: $ZFS_STDERR"
    echo "Error: $ZFS_ERROR"
    echo ""
    echo "Failed to mount ${fs} on ${rootmnt}/${mountpoint}."
    echo "Manually mount the filesystem and exit."
    /bin/sh
else
    [ "$quiet" != "y" ] && zfs_log_end_msg
fi

return 0

}

rlaager commented 7 years ago

I'm not sure this pkg-zfs repo is the correct place to file these bugs. If the bug exists in upstream ZFS (and your URL suggests it does), that's probably the right issue tracker.

rlaager commented 6 years ago

Can you provide your zfs list output? The script seems designed to only import filesystems below the filesystem that is the root filesystem. In the typical case, the root filesystem is something like rpool/ROOT/ubuntu. There typically shouldn't be children of rpool/ROOT/ubuntu.

samvde commented 6 years ago

I used to have datasets underneath the root filesystem, but the topology of my pools changed in the meantime (I have multiple root filesystems in the pool now) so that is no longer the case.

Said this: while we can agree on eventual best practices about it, there is absolutely nothing wrong with having children on the root filesystem dataset. There is no reason for this script to break that, it overrides settings I explicitly configured to avoid running into this.

rlaager commented 6 years ago

This was fixed in https://github.com/zfsonlinux/zfs/pull/6897 right?

samvde commented 6 years ago

Yep, that was my use case.