Repository navigation
Conversation
obbardc
force-pushed
the
wip/obbardc/nbd-bits
branch
from
August 25, 2026 19:17
93b5291 to
3e67fd7
Compare
obbardc
commented
Aug 25, 2026
obbardc
force-pushed
the
wip/obbardc/nbd-bits
branch
3 times, most recently
from
August 25, 2026 19:47
931eff3 to
ff2a9b3
Compare
igoropaniuk
requested changes
Nov 10, 2026
igoropaniuk
left a comment
igoropaniuk
left a comment
Contributor
There was a problem hiding this comment.
This is a really nice cleanup, thanks!
Just some nits: 0d30cc4 subject says "LIN sector size" (should be LUN), c3b47b1 body has "nbdkigt"
Could you also rebase onto the latest master? When rebasing: master now has a .load callback that sets the progname and forces ux_set_color_mode(UX_COLOR_NEVER). Please keep both .load and .after_fork. With colour forced off, redirecting stdout here won't leak ANSI escapes into the nbdkit log, which is good IMO.
The nbdkit plugin is an optional build target: meson only configures it when the nbdkit pkg-config file is present: on the CI runner it was silently skipped and the plugin sources were never compiled. Install nbdkit-plugin-dev on the Linux job so the plugin is build tested and kept from bit-rotting alongside the rest of the tree. Also configure with -Dnbdkit=enabled so the job fails loudly if the plugin is ever skipped again, e.g. because the dependency goes missing. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The plugin currently defaults to LUN 1 when no LUN is configured. On typical Qualcomm UFS partition layouts, LUN 1 contains bootloader partitions while LUN 0 contains the userspace/HLOS/Linux partitions that users are more likely to wish to target using the plugin. Devices with eMMC storage may expose only LUN 0 so defaulting to LUN 1 can select a non-existent LUN. The plugin documentation already uses LUN 0 in its examples which makes the current default inconsistent with the documented usage. Switch the default to LUN 0, which is typically the userarea on UFS and the only LUN available on most eMMC devices. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Currently the LUN parameter is parsed through atoi() which has no
reliable error reporting. For example, "lun=abc" is silently parsed as
LUN 0 instead of being rejected.
There is no range check for the parsed LUN either; values outside of
the valid LUN range (0-7 inclusive) are passed to firehose and fails
much later: once the device has connected, booted into firehose and the
LUN metadata is queried.
Instead, use nbdkit_parse_int() to reject malformed values and integers
outside the range representable by an int. Separately check that the
requested LUN is within the maximum of eight physical LUNs supported by
UFS devices.
Firehose still detects when a LUN within that range is not provisioned
on the device. For example, requesting LUN 6 on a device provisioned
with only five LUNs fails cleanly in firehose with:
error: failed to query size of LUN 6
Malformed and out-of-range values now fail during plugin configuration
at startup, while device-specific LUN availability checks are handled
by firehose.
Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Currently the debug parameter accepts only "true" and "1" as true and silently treats all other values as false. As a result, values such as "debug=yes" and "debug=info" are accepted but just end up being treated as false without any user feedback. Instead, use nbdkit_parse_bool() to parse the value. This accepts the boolean spellings documented by nbdkit, such as "1", "true", "t", "yes", "y", "on" (case-insensitive), along with their false equivalents and reports invalid values as parsing errors. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
Enabling the debug parameter correctly enables qdl_debug and therefore debug logging in the qdl library, but those messages are dropped when qdl is used through the nbd plugin. qdl emits debug messages through ux_*() which write to stdout. The exception is ux_err() which writes to stderr. As a result qdl debug messages disappear while qdl error messages emitted through ux_err() remain visible. nbdkit redirects stdout to /dev/null once configuration is complete, both in foreground mode and when running as a daemon. This prevents nbdkit plugins from interfering with modes where nbdkit use stdin/stdout to communicate with clients. When debugging is enabled, restore stdout from stderr in .after_fork; the first callback invoked after this redirection. This makes qdl's ux_*() messages go to the same destination as nbdkit's (and the plugin's) own debug output. This still requires "nbdkit -f" since a daemonised server also redirects stderr to /dev/null. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
When the first client connects to the daemon, the nbdkit plugin callback invokes qdl_open(), which calls usb_open(). usb_open() loops until a device appears. If no device is ever connected, it leaves nbdkit stuck indefinitely and eventually causes the daemon to crash. This is undesirable for a server: a client connecting before the device is attached can block indefinitely, the usleep() between probes cannot be interrupted during shutdown and the wait is only reported through ux_info(), which may not be visible through nbdkit. Instead, build a custom wait and timeout loop around usb_open_once(), the single scan-and-open operation used by usb_open(). This moves the wait policy into the plugin where it can expose a deadline through a new timeout parameter, sleep with nbdkit_nanosleep() so an nbdkit shutdown can interrupt the wait and report failures through nbdkit_error(). Report when a device is present but cannot be opened separately since when nbdkit runs as a service user this can indicate permission problems or another process locking the device rather than an absent device. The timeout defaults to 60 seconds, while timeout=0 preserves the previous wait-forever behaviour. A timeout only fails the current client connection: the next client which opens retries accessing the device, so nbdkit can remain running until the device is attached. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The plugin currently does not report which LUN it is serving or the geometry reported by firehose. If the exported size looks wrong, diagnosing the problem requires checking the export from the client side and comparing it with the expected device layout. Log the LUN, sector size and sector count after connecting to the device. This makes it clear which storage geometry the plugin is exporting and also provides the sector size needed when comparing or configuring block sizes on NBD clients. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The plugin currently does not validate the sector size reported by firehose before using it to serve the export. Firehose only probes 512-byte and 4096-byte sectors; any other value means the geometry was not established correctly. Continuing with such a value could leave the export unusable: .pread and .pwrite align every request against the reported sector size, causing all I/O to be rejected as unaligned. A sector size of 0 is worse, since the alignment checks would divide by zero. Reject unsupported sector sizes during setup and report the offending value. This causes the connection to fail immediately with a clear nbdkit error instead of surfacing later as unexplained I/O failures. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The plugin currently reports only the total export size, so nbdkit
advertises its default minimum block size of 512 bytes. On a UFS LUN
with 4096-byte sectors, a client may issue a legal request aligned to
512 bytes, even though firehose can only access whole device sectors.
Such requests are rejected by .pread and .pwrite with EINVAL so the
client sees an I/O error for a request which the export advertises as
valid.
Implement .block_size and advertise the device sector size as both the
minimum and preferred block size, allowing clients which honour these
properties to align requests correctly before issuing I/O. nbdkit
requires these values to be powers of two, which is guaranteed by the
sector size validation performed during setup.
Unfortunately, not all NBD clients obey the advertised block size. For
example, nbd-client relies on the user to specify the block size
explicitly and defaults to 512 byte sectors when unset. The values are
nevertheless exposed by the NBD protocol and can be queried with
nbdinfo, for example:
block_size_minimum: 4096 (4K)
block_size_preferred: 4096 (4K)
block_size_maximum: 4294967295
This gives users a way to determine the device sector size before
configuring clients which require it to be supplied manually.
The .block_size callback was only added in nbdkit 1.30, so register it
only when building against 1.30 or later. Older versions, such as the
nbdkit 1.24 shipped in Ubuntu 22.04, still build the plugin but keep
advertising nbdkit's default block size.
Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The nbd "how-to" guide is quite hard to follow for a first-time reader (e.g. I wrote this change based on my notes from following the documentation for the first time knowing little about NBD and nbdkit). Rework it into a numbered walkthrough that covers the whole session, from installing packages to detaching the client. The most important addition is the sector size. nbd-client always assumes 512-byte sectors and does not honour the block size the server advertises; attaching a UFS LUN without "-b 4096" produces confusing read issues on the client without any clear error message (e.g. device partition table not read properly). Document how to read the sector size from either the nbdkit debug log or nbdinfo and use "-b 4096" when calling nbd-client in the example, along with details on what the value should be and how to read it from the device. Also correct the build dependency; only the nbdkit plugin development files are needed to build the plugin (nbdkit-plugin-dev on Debian), not the nbdkit runtime and list the client-side packages needed to follow the example. Describe more precisely when the device is contacted: on the first client connection, staying in firehose mode until nbdkit exits and then resets the device. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
obbardc
force-pushed
the
wip/obbardc/nbd-bits
branch
from
November 13, 2026 21:40
ff2a9b3 to
bd8b542
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When updating the Debian packaging to ship the nbdkit-qdl-plugin, I tried the built plugin with an RB3gen2 and the documentation under
doc/nbd.md.I found the documentation quite hard to follow, so I reworked it and while I was there I added some small quality-of-life changes to the plugin implementation along the way. The main one is the cleaner timeout when connecting to the device.
There are also some minimal future changes I'd like to add in later follow-ups, if you are interested, including:
qdl firehose-reset <mode>command toqdl; I will open that PR shortly.Let me know if you think any of these are not worth implementing.
The Linux CI job now builds the plugin with
-Dnbdkit=enabled. The.block_sizecallback needs nbdkit 1.30 or later, so it is only registered when building against a new enough nbdkit (Ubuntu 22.04 ships 1.24); on older versions the plugin still builds but keeps nbdkit's default block size advertisement.Changes since v1:
.loadcallback and.after_fork.github:prefix and configure with-Dnbdkit=enabledon the Linux job..block_sizebehindNBDKIT_VERSION >= 1.30to fix the Ubuntu 22.04 build.textfence and note that theblock_size_*lines need nbdkit 1.30+.