Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NVMeoF / TCP boot support #2184

Merged
merged 3 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions man/dracut.cmdline.7.asc
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,15 @@ NVMf
**rd.nonvmf**::
Disable NVMf

**rd.nvmf.nonbft**::
Disable connecting to targets from the NVMe Boot Firmware Table. Without
this parameter, NBFT connections will take precedence over _rd.nvmf.discover_.

**rd.nvmf.nostatic**::
Disable connecting to targets that have been statically configured when
the initramfs was built. Targets specified with rd.nvmf.discover on the
kernel command line will still be tried.

**rd.nvmf.hostnqn=**__<hostNQN>__::
NVMe host NQN to use

Expand Down
28 changes: 20 additions & 8 deletions modules.d/95nvmf/module-setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# called by dracut
check() {
require_binaries nvme || return 1
require_binaries nvme jq || return 1
[ -f /etc/nvme/hostnqn ] || return 255
[ -f /etc/nvme/hostid ] || return 255

Expand All @@ -25,17 +25,27 @@ check() {
[[ $trtype == "fc" ]] || [[ $trtype == "tcp" ]] || [[ $trtype == "rdma" ]]
}

has_nbft() {
local f found=
for f in /sys/firmware/acpi/tables/NBFT*; do
[ -f "$f" ] || continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks only for regular files. Is that intentional?

You could check for -e (existence) instead.

Copy link
Contributor Author

@mwilck mwilck Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

Actually, no. Technically, this check serves to avoid a case where no NBFT* file exists, in which case $f would equal /sys/firmware/acpi/tables/NBFT* (as we don't use nullglob). But in the (extremely unlikely) case that a directory entry NBFT* existed in sysfs, and was not a regular file, we'd definitely want to skip it, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, folders, I didn't think of that - but It might be symlink or other file-like type as well f.e.; but if only regular files are expected/accepted, I'm fine with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only regular files should be accepted.

Note that [ (aka test) dereferences symlinks for all flags except -h and -L. Thus if /sys/firmware/acpi/tables/NBFT* was a symlink to some existing regular file, this test wouldn't skip it, which looks wrong. But this case would indicate a severely broken kernel. I believe we can ignore it. In the worst case, we'd include the NBFT code in the initramfs on a broken system.

found=1
break
done
[[ $found ]]
}

[[ $hostonly ]] || [[ $mount_needs ]] && {
pushd . > /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not part of the PR, but does anyone know why pushd . is here? I think this is basically a no-op.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just checking that for_each_host_dev_and_slaves does not run popd? I'd love to know the reasoning behind this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed not part of this PR, therefore I'd like to close this conversation.

AFAICS, the pushd/popd code (like lots of the initial nvmf code) has been copied from 95iscsi/module-setup.sh, where it had been added by @haraldh in commit b093aa2 ("beautified shell code") 10y ago, without further explanation. Perhaps at that time for_each_host_dev_and_slaves could change the working directory? I don't know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good to know the origin of it. Thanks for the response!

for_each_host_dev_and_slaves is_nvmf
local _is_nvmf=$?
popd > /dev/null || exit
[[ $_is_nvmf == 0 ]] || return 255
if [ ! -f /sys/class/fc/fc_udev_device/nvme_discovery ]; then
if [ ! -f /etc/nvme/discovery.conf ]; then
echo "No discovery arguments present"
return 255
fi
if [ ! -f /sys/class/fc/fc_udev_device/nvme_discovery ] \
&& [ ! -f /etc/nvme/discovery.conf ] \
&& [ ! -f /etc/nvme/config.json ] && ! has_nbft; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: same usage of -f vs -e as stated above.

Copy link
Contributor Author

@mwilck mwilck Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-f is correct here. We wouldn't want to account for non-regular files by the respective names.

echo "No discovery arguments present"
return 255
fi
}
return 0
Expand All @@ -50,7 +60,7 @@ depends() {
# called by dracut
installkernel() {
instmods nvme_fc lpfc qla2xxx
hostonly="" instmods nvme_tcp nvme_fabrics
hostonly="" instmods nvme_tcp nvme_fabrics 8021q
}

# called by dracut
Expand Down Expand Up @@ -126,10 +136,12 @@ install() {
inst_multiple ip sed

inst_script "${moddir}/nvmf-autoconnect.sh" /sbin/nvmf-autoconnect.sh
inst_script "${moddir}/nbftroot.sh" /sbin/nbftroot

inst_multiple nvme
inst_multiple nvme jq
inst_hook cmdline 92 "$moddir/parse-nvmf-boot-connections.sh"
inst_simple "/etc/nvme/discovery.conf"
inst_simple "/etc/nvme/config.json"
inst_rules /usr/lib/udev/rules.d/71-nvmf-iopolicy-netapp.rules
inst_rules "$moddir/95-nvmf-initqueue.rules"
dracut_need_initqueue
Expand Down
5 changes: 5 additions & 0 deletions modules.d/95nvmf/nbftroot.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#! /bin/sh
# This script is called from /sbin/netroot

/sbin/nvmf-autoconnect.sh online
exit 0
55 changes: 52 additions & 3 deletions modules.d/95nvmf/nvmf-autoconnect.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,54 @@
#!/bin/bash
#!/bin/sh
# Argument $1 is "settled", "online", or "timeout", indicating
# the queue from which the script is called.
# In the "timeout" case, try everything.
# Otherwise, try options according to the priorities below.

[ -f /sys/class/fc/fc_udev_device/nvme_discovery ] || exit 1
echo add > /sys/class/fc/fc_udev_device/nvme_discovery
[ "$RD_DEBUG" != yes ] || set -x

if [ "$1" = timeout ]; then
[ ! -f /sys/class/fc/fc_udev_device/nvme_discovery ] \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: same usage of -f vs -e as stated above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, -f is correct here. If this existed and was anything else but a regular file, we'd have to ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks!

|| echo add > /sys/class/fc/fc_udev_device/nvme_discovery
/usr/sbin/nvme connect-all
exit 0
fi

NVMF_HOSTNQN_OK=
[ ! -f "/etc/nvme/hostnqn" ] || [ ! -f "/etc/nvme/hostid" ] || NVMF_HOSTNQN_OK=1

# Only nvme-cli 2.5 or newer supports the options --nbft and --no-nbft
# for the connect-all command.
# Make sure we don't use unsupported options with earlier versions.
NBFT_SUPPORTED=
# shellcheck disable=SC2016
/usr/sbin/nvme connect-all --help 2>&1 | sed -n '/[[:space:]]--nbft[[:space:]]/q1;$q0' \
|| NBFT_SUPPORTED=1

if [ -e /tmp/nvmf-fc-auto ] && [ "$NVMF_HOSTNQN_OK" ] \
&& [ -f /sys/class/fc/fc_udev_device/nvme_discovery ]; then
# prio 1: cmdline override "rd.nvmf.discovery=fc,auto"
echo add > /sys/class/fc/fc_udev_device/nvme_discovery
exit 0
fi
if [ "$NBFT_SUPPORTED" ] && [ -e /tmp/valid_nbft_entry_found ]; then
# prio 2: NBFT
/usr/sbin/nvme connect-all --nbft
exit 0
fi
if [ -f /etc/nvme/discovery.conf ] || [ -f /etc/nvme/config.json ] \
&& [ "$NVMF_HOSTNQN_OK" ]; then
# prio 3: configuration from initrd and/or kernel command line
# We can get here even if "rd.nvmf.nonbft" was given, thus use --no-nbft
if [ "$NBFT_SUPPORTED" ]; then
/usr/sbin/nvme connect-all --no-nbft
else
/usr/sbin/nvme connect-all
fi
exit 0
fi
if [ "$NVMF_HOSTNQN_OK" ] \
&& [ -f /sys/class/fc/fc_udev_device/nvme_discovery ]; then
# prio 4: no discovery entries, try NVMeoFC autoconnect
echo add > /sys/class/fc/fc_udev_device/nvme_discovery
fi
exit 0
Loading