Re: [Libguestfs] [PATCH] spec: Add NBD_OPT_EXTENDED_HEADERS

On Mon, Dec 06, 2021 at 02:40:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: Easy enough to do (the payload length will be 0 except for NBD_CMD_READ). The two happened to be orthogonal enough in my implementation. It was easy to demonstrate either one without the other, and it IS easier to write a client using non-structured replies (structured reads ARE tougher than simple reads, even if it is less efficient when it comes to reading zeros). But you are also right that we could require structured reads prior to allowing 64-bit operations, and then have only one supported reply type on the wire when negotiated. Wouter, which way do you prefer? Indeed, both qemu and libnbd implementations explicitly kill the connection to any server that replies with more than the max buffer used for NBD_CMD_READ/WRITE (32M for qemu, 64M for libnbd). And if the spec is not already clear on the topic, I should add an independent patch to NBD_CMD_BLOCK_STATUS to make it obvious that a server cannot reply with too many extents because of such clients. So none of my proof-of-concept code ever used the full 64-bits of the reply header length. On the other hand, there is indeed the symmetry argument - if someone writes a server willing to accept a 4G NBD_CMD_WRITE, then it should also support a 4G NBD_CMD_READ, even if no known existing server or client allows buffers that large.. Consider: struct header[100]; if sizeof(header[0]) is a power of 2 <= the cache line size (and the compiler prefers to start arrays aligned to the cache line) then we are guaranteed that all array members each reside in a single cache line. But if it is not a power of 2, some of the array members straddle two cache lines. Will there be code that wants to create an array of headers? Perhaps so, because that is a logical way (along with scatter/gather to combine the header with variable-sized payloads) of tracking the headers for multiple commands issued in parallel. Do I have actual performance numbers? No. But there's plenty of google hits for why sizing structs to a power of 2 is a good idea. Right now, the handle is in the same offset for both simple and structured replies, and for both normal and extended headers. My proof-of-concept for qemu always reads just the magic number and handle, then decides how many more bytes to read (if any) (1 syscall for simple compact headers, 2 syscalls for compact structured and for both extended styles); while my proof-of-concept for libnbd actually decides up front to only do a 32-byte read if extended headers are in use for fewer syscalls. I don't know if one way is better than the other, but the differences in styles fell out naturally from the rest of those code bases, and certainly anything that can be done with fewer syscalls per transaction is going to show a modest improvement. But you are right that repositioning the handle to live at some other offset (including forcing it to live in the payload with a 16-byte header, instead of having a 32-byte header) would be more invasive. Doable? Maybe. That's why this is an RFC. But unless there is a compelling reason to try, I'd rather not go to that effort. I'm not dropping the requirement; what was listed here is redundant with what appears elsewhere under NBD_REPLY_TYPE_BLOCK_STATUS, where the addition of NBD_REPLY_TYPE_BLOCK_STATUS_EXT made it too wordy to keep the redundancy here. But yes, I can try and separate the patch into minor cleanups separate from new additions. ... Previously, only NBD_REPLY_TYPE_BLOCK_STATUS counts as a status chunk, now we have two reply types with that qualification. But I can indeed split up the terminology addition from the addition of the second type of status chunk. _64 would be fine with me. As this is an RFC, the naming is not locked in stone. I like that. The change is that it is no longer to have exactly one of these per reply (you can have a BLOCK_STATUS_EXT instead). True, not much of a change, but it is because of the new type. Again, adding the notion of exactly one status chunk per metadata id (even with only one possible status chunk) in one patch, then adding the second status chunk with extended headers, may be easier to review, so I'll try that for v2. Yes. That is supposed to still be in effect. If I deleted that restriction, it was unintentional. That is, the cumulative length (and thus each individual extent length, since no extent can be larger than the cumulative length) is not allowed to exceed the client's length request except in the case of the last extent, and even then only when REQ_ONE was not in use. I envision the following as valid: OPT_SET_META_CONTEXT("base:allocation", "my:extension") => id 1: "base:allocation", id 2: "my:extension" OPT_GO ... CMD_BLOCK_STATUS(offset=0, length=3G) => REPLY_TYPE_BLOCK_STATUS(id=1, extent[2] { length=2G flags=0, length=2G flags=1 }) => REPLY_TYPE_BLOCK_STATUS_EXT(id=2, extent[1] { length=3G flags=0 }) CMD_BLOCK_STATUS(offset=3G, length=6G) => REPLY_TYPE_BLOCK_STATUS_EXT(id 1, extent[1] { length=5G flags=0 }) => REPLY_TYPE_BLOCK_STATUS(id 2, extent[2] { length=3.5G flags=0, length=3.5G flags=1 }) Note that the first id=1 responded with a cumulative length larger than the client's request, and the cumulative length is > 4G, but the response itself gets away with only 32-bit extents. The first id=2 response is < 3G, but the server chose to use a 64-bit extent anyway. The second id=1 response has to use the 64-bit response (because even though 5G is shorter than the client's request for 6G, it is larger than the 4G maximum of a 32-bit response). The second id=2 is similar to the first id=1 in that it uses a 32-bit response even though the cumulative length is >4G. There is no requirement that the cumulative lengths of the two ids be identical. And since REQ_ONE is not in effect, the last extent of a given extent array can cause the cumulative value to exceed the client's request. What is invalid: CMD_BLOCK_STATUS(offset=0, length=3G) => REPLY_TYPE_BLOCK_STATUS(id=1, extent[2] { length=2G flags=0, length=2G flags=1 }) => REPLY_TYPE_BLOCK_STATUS_EXT(id=1, extent[1] { length=3G flags=0 }) because it used two status chunks both for context id=1. Maybe I need to add the phrase "within a given NBD_CMD_BLOCK_STATUS response", to make it clear that exactly one status chunk per id is chosen, but whether the server chooses a 32- or 64-bit status chunk is dependent solely on the server's whims, and the client must be prepared for either, regardless of the length the client used originally. ... Glad to hear it! Yes, I'll do that for v2. That's not new behavior. The client's length has always been a mere hint to the server, where the only constraints are that the server must make progress on success, and that if REQ_ONE is in use, the server may not report more than then length the client asked about. Or are you proposing that we relax REQ_ONE, and allow a server to report additional length in 64-bit mode even when REQ_ONE is in use? The 32-bit limitation on not sending back too much length with compact structured replies was because qemu as client at one point would abort if the cumulative length was too long (and qemu has always used REQ_ONE). But if extended headers are in use, qemu no longer aborts on oversize answers, and no other client is new enough to have extended headers. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org