Re: [Libguestfs] [libnbd PATCH v3 02/22] internal: Refactor layout of replies in sbuf

On Fri, May 26, 2023 at 05:53:38PM +0200, Laszlo Ersek wrote: Indeed. 'union sbuf' is mostly used as a dumping grounds for "give me enough scratch storage that I can have a place to read in all places of interest across multiple recv() calls; I don't need the resulting storage to match wire format, but any time multiple pieces of information are needed at once, they must reside in non-overlapping portions of sbuf, via a sub-struct. But where we know we will not be accessing disjoint regions of sbuf, we can use further nested unions to compress the overall memory taken by sbuf". While we will never have a simple header used with a reply payload (because the only reply with payload for a simple header is NBD_CMD_READ, and we read that directly into the user's buffer instead of into sbuf), it doesn't hurt that union sbuf has reserved that much space (because we do need that much space for handilng a structured reply, and soon to handle an extended reply). Can do. Looks like I'll be spinning a v4, but I'll let you get further along in the review process to minimize list churn before I start posting updates. Correct, and since option replies are distinct from server replies, I agree with your argument of splitting removal of the attribute((packed)) into a separate patch. Also correct. While it happens to more directly touch the rest of the layout update in this patch, it is still a better fit logically to remove the attribute to both sub-structs at the same time, independently from reflowing the layout of sbuf.simple_reply and sbuf.sr. 'union sbuf' is already a union. If we didn't need or.option_reply and or.payload at the same time, then it would need make sense to just have: union sbuf { ... struct nbd_fixed_new_option_reply option_reply; struct nbd_fixed_ne struct nbd_fixed_new_option_reply_server server; char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ } __attribute__ ((packed)) or_payload_server; struct nbd_fixed_new_option_reply_info_export or_payload_export; ... }; But because we DO need access to both or.option_reply and or.payload at the same time (for example, see generator/states-newstyle.c:handle_reply_error() called after both pieces of sbuf.or have been populated), we instead packaged the entire option reply into a nested struct, while still recognizing that the distinct payload types can overlap via a nested union: union sbuf { ... struct { struct nbd_fixed_new_option_reply option_reply; union { struct nbd_fixed_ne struct nbd_fixed_new_option_reply_server server; char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ } __attribute__ ((packed)) server; struct nbd_fixed_new_option_reply_info_export export; ... } payload; } or; ... }; And referring back to your earlier insight, or.payload.server is a nested struct that DOES need __attribute__((packed)), because that is a case where we DO use a single recv() to populate two distinct C types into guaranteed-adjacent memory, and don't want the compiler throwing in any padding that might mess with us. Bingo. On the wire, the two portions of the reply message happen to be adjacent, and we need both portions of the reply in memory to decipher the reply. But there is no reason why our in-memory representation has to be packed to match the wire layout. And allowing the compiler to insert padding can sometimes improve performance. And convinced me of it likewise. A half hour well spent. This line is wrong. Pre-patch, it is 'struct sr', not 'union sr'. I should indeed try harder to follow your useful example of generating specific patches with more than the default 3 lines of context, when it would make review easier. Alas, 'git format-patch' doesn't seem to have an easy way to pick a different context size on a per-patch basis, so this basically implies writing (or finding and reusing existing) wrapper tooling to automate that. It happens to work, but I agree with you that we are probably stretching aliasing rules about memory effective types that it is shaky ground to begin with. Even adding a STATIC_ASSERT that offsetof(struct simple_reply, handle) == offsetof(struct structured_reply, handle) would be helpful to show that we depend on that. While both structs share only a common prefix of uint32_t magic, they intentionally DO share uint64_t handle at the SAME offset of 8 bytes in. The commit message tries to show that visually both pre- and post-patch. (Oh, that reminds me; I need to do a patch to s/handle/cookie/ to match my recent patch to the NBD spec.) Hmm. As long as this resides only in sbuf, it makes sense for libnbd. But if we also touch the structs in lib/nbd-protocol.h, then it is code that is shared with nbdkit and has an even wider ripple effect. My initial problem with THIS proposal is that it REQUIRES the use of multiple __attribute__((packed)) if we intend to read in multiple fields in one recv: for example the struct for nbd_reply_header.simple starts with uint32_t followed by uint64_t, so many ABIs want to align it as: offsets 0-3 error offsets 4-7 padding offsets 8-15 handle and when thrown in the larger nbd_reply_header, you now have a natural alignment that prefers: offsets 0-3 magic offsets 4-7 padding offsets 8-11 error offsets 12-15 padding offsets 16-23 handle Note that in lib/nbd-protocol.h, 'struct nbd_simple_reply' is already marked packed; but even if it weren't, the sizings of the member fields HAPPEN to be naturally aligned, for both simple and structured replies (I can't speak to the design of the simple reply since that pre-dates my involvement in Linux, but as I was the one that added structured replies to the NBD spec, it was intentional on my part to pick sizes that filled in all space assuming alignment matches sizing, and where magic and handle both overlap). But a STATIC_ASSERT wouldn't hurt, showing that we want: offsets 0-3 magic offsets 4-7 error OR offsets 4-5 flags, offsets 6-7 type offsets 8-15 handle (Hmm - another thing I should consider: why do we use NBD_ATTRIBUTE_PACKED in nbd-protocol.h but open-code __attribute__((packed)) in internal.h? Consistent spelling would be nice.) A secondary problem with this proposal is what happens later in the series when adding extended headers. When we added structured replies, we were forced to do (at least) two partial reads (which type of reply packet is on the wire, followed by reading the rest of the packet). But with extended headers, we know a priori that all expected responses will have a 32-byte header, and can do a single recv() for all 32 bytes. And for a server that does not support structured replies, always reading 16 bytes happens to work for simple replies (it is only when we have structured replies that the difference of 16 vs. 20 bytes of recv() is dependent on magic). Thus, having the state machine try to favor as few recv() as possible is still a worthwhile goal. But having said all that, maybe I could live with: struct nbd_reply_header { uint32_t magic; union { uint32_t simple_error; struct { uint16_t flags; uint16_t type; } structured; } middle; uint64_t handle; uint32_t length; /* only used for structured */ }; although once extended headers are in play, it would end up looking more like struct nbd_reply_header { uint32_t magic; union { uint32_t simple_error; struct { uint16_t flags; uint16_t type; } structured; /* including extended */ } middle; uint64_t handle; union { /* unused for simple */ uint32_t structured_length; struct { uint64_t offset; uint64_t length; } extended; } tail; }; At any rate, as written there, natural alignment still works for the struct prefix (no 64-bit entities in struct middle.structured, so the overall union middle has only 4-byte alignment and 4-byte size). Okay, natural alignment says the overall struct nbd_reply_header would be 24 bytes, even though structured replies only take 20 bytes (because of the 8-byte handle, the compiler inserts four bytes of padding after length), so we may still want __attribute__((packed)) or at a bare minimum STATIC_ASSERT to show that we insist on handle being at offset 8. If we weren't guaranteed that 'handle' overlaps between the two types, I might agree. But my argument is that the wire protocol DOES intend for handle to be at the same offset, regardless of the interpretation of the middle bytes, and forcing multiple recv() is just more syscalls for something that we know we'll eventually need to interpret according to magic. Even if we just read into a char[] and then reinterpret into simple or structured, rather than reading a prefix into simple and then reading more bytes into a tail of structured, is at least fewer syscalls, and also less likely to run foul of C's effective type rules (I've recently had to refamiliarize myself with C17 section 6.5 paragraph 7 as part of working on how POSIX should specify intentional aliasing occuring in struct sockaddr and friends, https://austingroupbugs.net/view.php?id=1641). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org