what size should NBD_OPT_INFO report?
I noticed during integration testing that nbd-server blindly reports a size of 0 for all NBD_OPT_INFO requests, unless I pass a size argument on the command line to nbd-server. At first, I thought it was a side effect of me trying to use nbd-server on a block device (an LVM partition), as it is a common bug to rely on stat().st_size which only works for regular files (a block device has to use lseek(SEEK_END)); but then I noticed it happening when using nbd-server to serve regular files as well. I then turned to the source code, where I see that client->exportsize is set in just these places: commit_client() - exportsize = OFFT_MAX, then try setupexport() setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong) but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO. The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO. For comparison, both nbdkit and qemu as an NBD server always advertise the same size for both NBD_OPT_INFO and NBD_OPT_GO (but it should also be noted that neither of these has the complexity of multifile like nbd-server). Should we tweak the NBD standard to recommend that a server advertise a size of OFFT_MAX for NBD_OPT_INFO if it is prohibitive to determine an actual size, rather than 0? Furthermore, is it worth adding code to make NBD_OPT_INFO try harder to provide a sensible value when possible (if expected_size was set, or if we are not multifile or F_WAIT, then a stat() is enough to get the size for serving a regular file; and if stat() says we have a block device, we can still try the open/lseek/close)? Also, is it worth trying to specify that since no known NBD servers allow exports with sizes larger than OFFT_MAX, a server MUST NOT report a size equal or larger than 9223372036854775808 (0x80000000_00000000)? There are definitely a few places that I could simplify in libnbd if we have a protocol guarantee that a valid export size will never have the most significant bit set, and therefore we don't have to worry about whether size is represented as a signed or unsigned value (which also implies that libnbd's function nbd_get_size() returning ssize_t instead of size_t is acceptable). But if we do that, then maybe 0x80000000_00000000 or even 0xffffffff_ffffffff would serve as a better recommended sentinel than 0x7fffffff_ffffffff for representing an indeterminate size. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
On Wed, Feb 21, 2024 at 12:19:49PM -0600, Eric Blake wrote:
setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO. The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
Amending myself: reading size_autodetect(), it tries ioctl(fd, BLKGETSIZE64) first (even when fd is not a block device!), then falls back to fstat(fd) coupled with lseek(SEEK_END) if fstat() reported st_size of 0 and not a regular file; but if all of those fail, it reports UINT64_MAX, which is different than OFFT_MAX.
For comparison, both nbdkit and qemu as an NBD server always advertise the same size for both NBD_OPT_INFO and NBD_OPT_GO (but it should also be noted that neither of these has the complexity of multifile like nbd-server).
Should we tweak the NBD standard to recommend that a server advertise a size of OFFT_MAX for NBD_OPT_INFO if it is prohibitive to determine an actual size, rather than 0? Furthermore, is it worth adding code to make NBD_OPT_INFO try harder to provide a sensible value when possible (if expected_size was set, or if we are not multifile or F_WAIT, then a stat() is enough to get the size for serving a regular file; and if stat() says we have a block device, we can still try the open/lseek/close)?
Also, is it worth trying to specify that since no known NBD servers allow exports with sizes larger than OFFT_MAX, a server MUST NOT report a size equal or larger than 9223372036854775808 (0x80000000_00000000)? There are definitely a few places that I could simplify in libnbd if we have a protocol guarantee that a valid export size will never have the most significant bit set, and therefore we don't have to worry about whether size is represented as a signed or unsigned value (which also implies that libnbd's function nbd_get_size() returning ssize_t instead of size_t is acceptable). But if we do that, then maybe 0x80000000_00000000 or even 0xffffffff_ffffffff would serve as a better recommended sentinel than 0x7fffffff_ffffffff for representing an indeterminate size.
And the fact that nbd-server already has two different sentinel values (depending on whether it even attempted an open, vs whether the attempt to read size from an open fd failed) is another factor into what makes the most sense to do going forward. If we do tweak the NBD standard, at best it should only be an advisory on best server practice (with the caveat that clients have to be prepared for servers that return other odd values, like 0, even when a size is not known). -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
On Wed, Feb 21, 2024 at 12:26:08PM -0600, Eric Blake wrote:
On Wed, Feb 21, 2024 at 12:19:49PM -0600, Eric Blake wrote:
setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO. The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
Amending myself: reading size_autodetect(), it tries ioctl(fd, BLKGETSIZE64) first (even when fd is not a block device!), then falls back to fstat(fd) coupled with lseek(SEEK_END) if fstat() reported st_size of 0 and not a regular file; but if all of those fail, it reports UINT64_MAX, which is different than OFFT_MAX.
Another wrinkle: On BSD systems, lseek(fd_of_block, 0, SEEK_END) behaves identically to lseek(fd_of_block, 0, SEEK_START), because BSD has the idea that the size of a seekable file descriptor is whatever gets reported in stat().st_size which is 0 on block devices (Linux is the odd system out where lseek() and stat() disagree on this point - all other systems surveyed either reliably report the block capacity in st_size and SEEK_END, or use offset 0 in both places). See this POSIX bug for more details: https://austingroupbugs.net/view.php?id=1817 If we are going to report correct capacity of block devices, it requires a LOT more code; compare what nbdkit is adding: https://gitlab.com/nbdkit/nbdkit/-/merge_requests/54/diffs?commit_id=532b617... -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Hi Eric, On Tue, May 07, 2024 at 07:02:56AM -0500, Eric Blake wrote:
On Wed, Feb 21, 2024 at 12:26:08PM -0600, Eric Blake wrote:
On Wed, Feb 21, 2024 at 12:19:49PM -0600, Eric Blake wrote:
setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO. The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
Amending myself: reading size_autodetect(), it tries ioctl(fd, BLKGETSIZE64) first (even when fd is not a block device!), then falls back to fstat(fd) coupled with lseek(SEEK_END) if fstat() reported st_size of 0 and not a regular file; but if all of those fail, it reports UINT64_MAX, which is different than OFFT_MAX.
Another wrinkle:
On BSD systems, lseek(fd_of_block, 0, SEEK_END) behaves identically to lseek(fd_of_block, 0, SEEK_START), because BSD has the idea that the size of a seekable file descriptor is whatever gets reported in stat().st_size which is 0 on block devices (Linux is the odd system out where lseek() and stat() disagree on this point - all other systems surveyed either reliably report the block capacity in st_size and SEEK_END, or use offset 0 in both places). See this POSIX bug for more details:
Sigh :-/
If we are going to report correct capacity of block devices, it requires a LOT more code; compare what nbdkit is adding:
https://gitlab.com/nbdkit/nbdkit/-/merge_requests/54/diffs?commit_id=532b617...
Yeah. In this context, I've never done much on non-Linux systems. We do indeed try stat and lseek, but I didn't realize that was not portable. We also use the BLKGETSIZE64 ioctl to figure out the size of a block device, but that's *definitely* Linux-specific. I'm definitely OK with improving that situation if we can make it happen... -- w@uter.{be,co.za} wouter@{grep.be,fosdem.org,debian.org} I will have a Tin-Actinium-Potassium mixture, thanks.
Y'all might be interested in this code I recently copied from util-linux (originally "public domain") into nbdkit (BSD): https://gitlab.com/nbdkit/nbdkit/-/blob/master/common/utils/device-size.c?re... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Wed, Feb 21, 2024 at 12:19:47PM -0600, Eric Blake wrote:
I noticed during integration testing that nbd-server blindly reports a size of 0 for all NBD_OPT_INFO requests, unless I pass a size argument on the command line to nbd-server. At first, I thought it was a side effect of me trying to use nbd-server on a block device (an LVM partition), as it is a common bug to rely on stat().st_size which only works for regular files (a block device has to use lseek(SEEK_END)); but then I noticed it happening when using nbd-server to serve regular files as well.
I then turned to the source code, where I see that client->exportsize is set in just these places:
commit_client() - exportsize = OFFT_MAX, then try setupexport()
setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO. The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
For comparison, both nbdkit and qemu as an NBD server always advertise the same size for both NBD_OPT_INFO and NBD_OPT_GO (but it should also be noted that neither of these has the complexity of multifile like nbd-server).
Should we tweak the NBD standard to recommend that a server advertise a size of OFFT_MAX for NBD_OPT_INFO if it is prohibitive to determine an actual size, rather than 0?
No no no, this is a cop out! Just calculate the correct size. If nbdkit & qemu-nbd can do it, then so can nbd-server.
Furthermore, is it worth adding code to make NBD_OPT_INFO try harder to provide a sensible value when possible (if expected_size was set, or if we are not multifile or F_WAIT, then a stat() is enough to get the size for serving a regular file; and if stat() says we have a block device, we can still try the open/lseek/close)?
Also, is it worth trying to specify that since no known NBD servers allow exports with sizes larger than OFFT_MAX, a server MUST NOT report a size equal or larger than 9223372036854775808 (0x80000000_00000000)?
Yes, nbdkit supports up to and including 7fffffff_ffffffff and qemu up to and including 7fffffff_c0000000 (although that's a bug in qemu IMHO). The Linux kernel itself doesn't support anything larger than 7fffffff_ffffffff.
There are definitely a few places that I could simplify in libnbd if we have a protocol guarantee that a valid export size will never have the most significant bit set, and therefore we don't have to worry about whether size is represented as a signed or unsigned value (which also implies that libnbd's function nbd_get_size() returning ssize_t instead of size_t is acceptable). But if we do that, then maybe 0x80000000_00000000 or even 0xffffffff_ffffffff would serve as a better recommended sentinel than 0x7fffffff_ffffffff for representing an indeterminate size.
Yeah I don't know where 7fffffff_ffffffff meaning "unknown" comes from. nbdkit supports real, known sizes as large as this, and there's no such concept as "unknown size" in nbdkit. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On Wed, Feb 21, 2024 at 12:19:47PM -0600, Eric Blake wrote:
I noticed during integration testing that nbd-server blindly reports a size of 0 for all NBD_OPT_INFO requests, unless I pass a size argument on the command line to nbd-server. At first, I thought it was a side effect of me trying to use nbd-server on a block device (an LVM partition), as it is a common bug to rely on stat().st_size which only works for regular files (a block device has to use lseek(SEEK_END)); but then I noticed it happening when using nbd-server to serve regular files as well.
I then turned to the source code, where I see that client->exportsize is set in just these places:
commit_client() - exportsize = OFFT_MAX, then try setupexport()
setupexport() - default to client->server->expected_size (if one was provided), further validating that actual size is large enough when actual size can be computed - if neither treefile or F_WAIT is set, compute actual size by opening one or more files and using size_autodetect() (which does the right thing for block devices, so my earlier thought about over-reliance on stat() was wrong)
but these functions are only reached for NBD_OPT_EXPORT_NAME and NBD_OPT_GO, not NBD_OPT_INFO.
Yeah, that's a bug. Obviously we don't want to commit_client when we do OPT_INFO (it's meant to do "everything needed to make sure the export is ready to go"), but I apparently missed that it also makes sure it has certain bits of information that OPT_INFO needs.
The upshot is that for NBD_OPT_GO, there are some scenarios (treefile, F_WAIT) where nbd-server advertises a size of 9223372036854775807 (0x7fffffff_ffffffff) meaning unknown, but a size of 0 there is only possible if the file was successfully opened and really is zero bytes in length. Conversely, NBD_OPT_INFO is always advertising a size of 0, which means most of the time, the size changes between NBD_OPT_INFO and NBD_OPT_GO.
That should not happen :) I think the standard is clear enough that OPT_INFO and OPT_GO should return the same information. The fact that I messed up doesn't mean I think we should change the standard :-) -- w@uter.{be,co.za} wouter@{grep.be,fosdem.org,debian.org} I will have a Tin-Actinium-Potassium mixture, thanks.
participants (3)
-
Eric Blake -
Richard W.M. Jones -
Wouter Verhelst