Re: [Libguestfs] [libnbd PATCH v3 08/22] block_status: Track 64-bit extents internally

On Fri, Jun 09, 2023 at 11:39:27AM +0200, Laszlo Ersek wrote: The disclaimer is much appreciated. And I likewise hope my reply is not seen as combative, although I readily admit it is long-winded. Maybe I can improve my wording in that part of the commit message. But first, let's step back and see the big picture. For the sake of argument, let's suppose there are two NBD servers in play; server A that does not know extended headers but supports "base:allocation" (a good set of servers today), and server B that has implemented extended headers and has added a new metacontext (let's call it "x-new:big") that returns 64-bit status flags, but is willing to also accept older clients. (I hope that someday I can get nbdkit to be such a server, but doing that requires adding a v3 protocol for nbdkit plugins; the current v2 plugin protocol can ONLY provide "base:allocation", but being able to let a plugin provide its own metacontexts will add some nice expressive power to nbdkit). Clients compiled against libnbd 1.16 do NOT have access to nbd_block_status_64() (the API did not exist yet); they expect that nbd_block_status() (which returns responses constrained to 32-bit lengths, by its API contract) will work. When run with the .so for libnbd 1.16 (currently named libnbd.so.0.0.0, because we have never broken ABI compatibility across version updates - everything has been additions-only), the client does not request extended headers (that was not part of the code base), so we can reasonably state that if such a client requests "x-new:big" during negotiation with server B, a compliant server will reply with NBD_REP_ERR_EXT_HEADER_REQD; libnbd 1.16 doesn't recognize that constant, but does recognize it as an error reply, and will gracefully proceed by telling the client it cannot access "x-new:big", so there are never any 64-bit flag values to worry about. You are also right that if that same app requests "base:allocation", the server will accept the negotiation, and reply with NBD_REPLY_TYPE_BLOCK_STATUS where lengths are inherently capped at 32 bits for a status flag value that never exceeds 32 bits. But now what happens when you upgrade your system to libnbd 1.18? I see at least three plausible designs: 1. We keep the .so version unchanged and state that libnbd blindly defaults to older behavior, where extended headers require explicit opt-in (for example, where you used to use 'h = nbd_create(); nbd_connect_uri(h, uri);', you now have to edit your code to add 'nbd_set_request_extended_headers(h, true);' in between). In this scenario, backwards-compatibility is easy: the app compiled against 1.16 maintains the same behavior, and newer apps can write conditional code that guards the call to nbd_set_request_extended_headers() by #ifdef LIBNBD_HAVE_NBD_SET_REQUEST_EXTENDED_HEADERS to compile against old and new libnbd. However, over time, as more and more servers support extended headers, we've actively made it harder for newer applications to quickly connect to the full power of such servers: the boilerplate for connection with maximal features has to be repeated in every client, which does not scale. 2. We keep the .so version unchanged (or maybe bump the micro to denote new APIs were added - but we haven't been doing that - I'm not sure if we are correctly following libtool-recommended practice on how to number our .so), and state that libnbd blindly defaults to requesting extended headers but you can now explicitly opt-out. The existence of nbd_set_request_extended_headers(h, false) is the escape valve that new code can use to guarantee behavior that 1.16 provided, and #ifdef LIBNBD_HAVE_NBD_SET_REQUEST_EXTENDED_HEADERS lets you write code compatible to either set of headers. The common case of connecting to a server with as many features as possible remains as short as possible when writing a client. 3. We take a hybrid approach, bump the .so version, and figure out some way to make the behavior determined by what version of the headers you compiled/linked against. For example, maybe have some linker magic where you export both nbd_create@@1_16 which starts a handle with request_eh=false, and nbd_create@@1_18 which starts a handle with request_eh=true; apps compiled against 1.16 will have nbd_create() resolve to nbd_create@@1_16 and never see a change in behavior, while apps compiled against 1.18 will have nbd_create() resolve to nbd_create@@1_18. This adds a lot of complexity to our build scripts (glibc does this sort of backwards-compatibility, but it can be very tricky to get right, and very few other libraries do it). This series went with option 2: default to new behavior, while bending over backwards to avoid scenarios in older clients that already worked. Now, what are the implications of that change? Back to the example I started earlier: If I write a new client against libnbd 1.18, and request "x-new:big" from server B, I don't have to do anything special for the negotiation to work: libnbd negotiated extended headers automatically, so the server allows my request. But when I want to get block status, I have two interfaces exposed: nbd_block_status() and nbd_block_status_64(). Presumably, because I just wrote the client against libnbd 1.18, the documentation hints are good enough that I should use nbd_block_status_64(), and all is well in the world (the server can send 64-bit lengths and 64-bit flags, and I can see them, with no munging required). What's more, even when I ask for "base:allocation", even though I never need 64-bit flags, I automatically benefit from 64-bit lengths when I use nbd_block_status_64() (patches 12-17 in this series show what is required to make an app use the new interfaces - it's boilerplate that has to be copied to every client written against older libnbd, but it is not too bad, and scaling is less of a concern because there are only a finite number of older apps). If that same client now talks to server A, the request for extended headers will fail, and the request for "x-new:big" will fail (the server doesn't support it); but the request for "base:allocation" will succeed. But we've already written the new client to call nbd_block_status_64() which reports 64-bit lengths; this works because under the hood, even though server A is sending us NBD_REPLY_TYPE_BLOCK_STATUS with 32-bit lengths, it is easy to widen that into our 64-bit internal representation, which we then hand to the client app. The app could also choose to explicitly check nbd_get_extended_headers_negotiated() in order to make a runtime decision of whether to call nbd_block_status() or nbd_block_status_64() based on what the server supports, but that's a lot of boilerplate unlikely to be used in most new clients. Thus, I prefer my decision to normalize to a 64-bit internal representation as soon as possible (at the risk of having to convert it back to 32-bits if the client made a conscious choice to use the old interface), rather than bending over backwards to store the server's response as-is as long as possible (and only convert it to the other width if the client called the API opposite from what the server negotiated). But back to my old client compiled against 1.16 and now dynamically loading the 1.18 .so. When talking to server A, the new .so defaults result in the old client now requesting (but not getting) get extended headers, and then calls to nbd_block_status() (the only API available at the time the app was compiled) will see server A's NBD_REPLY_TYPE_BLOCK_STATUS with 32-bit answers, temporarly widened into our 64-bit internal representation, and then the shim narrows it back to 32 bits for presentation to the client application, with no loss of information. Because of the double conversion, this may be slightly slower than it was with the 1.16 .so, but it is not an obvious behavior change. But the final combination is where the fun enters in. The old client talking to server B using the 1.18 .so defaults to requesting extended headers, and the server honors that request. However, the old client does not have access to the nbd_block_status_64() API (it was not around at the time of compilation), so it WILL be using the 32-bit interface nbd_block_status(). Whereas with the 1.16 .so an attempt to request "x-new:big" was rejected during negotiation, that request is now honored, and so we really do risk a flags value that we have to represent to the client as an EOVERFLOW error (an early failure is now a late failure; fortunately, this behavior change is not common in practice - other than clients like nbdsh that really do allow you to decide at runtime which metacontext(s) you want to request, most applications know in advance what they want from the server, and won't be asking for "x-new:big"). But more importantly, our request for "base:allocation" is STILL granted, and the server is now replying with NBD_REPLY_TYPE_BLOCK_STATUS_EXT. Even though the flags values will never exceed 32 bits, the server _can_ now give us lengths larger than 4G for an extent. We do _not_ want to fail the request with EOVERFLOW just because the server was now permitted to answer us with a larger extent than before, but we can take advantage of the NBD spec, which says that as long as the reply to NBD_CMD_BLOCK_STATUS makes progress, it does NOT have to go as far as possible - even though the server gave us a length larger than 4G over the wire, we are still compliant to the spec from the client's point of view if we truncate that response to being less than 4G - it still made progress, and just as with server A, the client can then ask for block status at the next byte after where the current answer ended. The tl;dr summary of the above discourse: There are two orthogonal communications going on: libnbd <-> server choice of NBD_REPLY_TYPE_BLOCK_STATUS{,_EXT} app <-> libnbd choice of nbd_block_status() or nbd_block_status_64() and all four combinations are easy to encounter when loading the .so for libnbd 1.18: 32 x 32 (app compiled against libnbd 1.16 to server A) 64 x 32 (app compiled against libnbd 1.16 to server B) 32 x 64 (app compiled against libnbd 1.18 to server A) 64 x 64 (app compiled against libnbd 1.18 to server B) and we want all four combinations to work insofar as possible. 32x32 and 64x64 obviously work, as does 32x64 (widening the server's responses never fails); for 32x64 (using the 32-bit nbd_block_status() API to access a server's response through 64-bit NBD_REPLY_TYPE_BLOCK_STATUS), accessing a metacontext with large flags changes from fail early to fail late; and accessing a metacontext with 32-bit flags but now potential 64-bit lengths obeys overall NBD semantics that a block status response can be truncated as long as it makes progress (the app shouldn't care whether it was the server or libnbd that did the truncation). I think I see where you missed something. The decision of whether a metacontext can have 64-bit flag content is dependent on the metacontext, and therefore something that can be fully decided at the time of NBD_OPT_SET_META_CONTEXT and/or NBD_OPT_GO (it is not a decision that needs to wait until NBD_CMD_BLOCK_STATUS). Either the metacontext has the potential for 64-bit flags (and the server will have failed with NBD_REP_ERR_EXT_HEADER_REQD if the client did not request extended headers), or it does not. But the decision of whether a metacontext can have 64-bit lengths is NOT dependent on the metacontext, but rather on the image size and our ability to make large queries. If we have a 5G image that is known to be all data, we HAVE to truncate our reply to less than 4G for a client that did not request extended headers, and then the client asks for a second block status at the point where it left off. But once extended headers are negotiated, we can give a single reply stating that the entire image is a single all-data extent. The server does not know at the time of NBD_CMD_BLOCK_STATUS whether the client is using 32-bit nbd_block_status or 64-bit nbd_block_status_64(). It is this distinction between length and flags that explains why I want nbd_block_status_64() to return a struct, rather than an array of uint64_t. Even though the wire representation of the extent is a pair of uint64_t values, the interpretation of those values is vastly different: length should never exceed 63 bits (because of off_t or other limitations) but can easily exceed 4G; while flags could be a full 64 bits, or in the case of "base:allocation" will never be more than 2 bits. The definition of flags is context-dependent, but the definition of length is not. We can, but that breaks backwards compatibility. We do not want an app compiled against libnbd 1.16 to stop working merely because it is now loading the 1.18 .so. It is indeed a bug if a server replies with NBD_REPLY_TYPE_BLOCK_STATUS_EXT for a client that did not request extended headers. But it is also a bug if a server replise with NBD_REPLY_TYPE_BLOCK_STATUS for a client that did request extended headers, even if the reply does not need more than 32 bits for either the length or the flags. The fact that we cap all incoming server replies to at most 64M (see pre-series REPLY.STRUCTURED_REPLY.CHECK which rejects payloads larger than MAX_REQUEST_SIZE). Even though the NBD spec only recommends (rather than requires) that a server should not send more than 2M extents in a single block status reply, we will have entered the DEAD state if the server sent more extents than fit in MAX_REQUEST_SIZE. Yes, I can add 'assert (nr_entries < SIZE_MAX / 2 / sizeof *array);' or similar, along with a comment pointing to the MAX_REQUEST_SIZE check. Yes, the code is set up to gracefully ignore server replies sent with the wrong reply type (the code does try hard to keep the connection alive when possible, by using payload lengths to skip to the next viable reply header; but moves to the DEAD state when it was not viable - either way, nbd_block_status() cannot succeed if the server uses the wrong reply type). Yes, backwards compatibility is always something that requires careful design. Okay, I see what you are getting at. We do one malloc() in response to whether the server sent NBD_REPLY_TYPE_BLOCK_STATUS or NBD_REPLY_TYPE_BLOCK_STATUS_EXT, and then another malloc() if the client called nbd_block_status(). You are worried that having two malloc()s doubles the chance of hitting a failure at an odd point, making it harder to give correct responses to the user's callbacks. Here's a strawman idea for a different internal representation: what if we store BOTH 32- and 64-bit data in a single malloc()? That is, in highly-abbreviated code sketch (doing it without abusing type aliases will obviously take more lines of code): REPLY.STRUCTURED_REPLY.CHECK: ... case NBD_REPLY_TYPE_BLOCK_STATUS: compute nr_entries; // number of uint32_t pairs char *bs_entries = malloc (6 * nr_entries); // relies on malloc being 8-byte aligned rbuf = bs_entries + nr_entries * 4 * sizeof(uint32_t); break; case NBD_REPLY_TYPE_BLOCK_STATUS_EXT: compute nr_entries; // number of uint64_t pairs char *bs_entries = malloc (nr_entries * 3); rbuf = bs_entries; break; ... REPLY.STRUCTURED_REPLY.BS_ENTRIES: ... if (type == NBD_REPLY_TYPE_BLOCK_STATUS) { // widen 32-bit entries in last third into 64-bit entries in first two-thirds for (i = 0; i < nr_entries * 2; i++) *(uint64_t*)bs_entries[i*sizeof(uint64_t)] = *(uint32_t*)bs_entries[(i+nr_entries*4)*sizeof(uint32_t)] } else /* type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT) { // narrow 64-bit entries in first two-thirds into 32-bit entries in last third for (i = 0; i < nr_entries; i++) { // check for overflow or truncation as needed } } if (/* user called nbd_block_status */) CALL_CALLBACK (cmd->cb.fn.extent32 (..., nr_entries*2, (uint32_t*)&bs_entries[nr_entries * 4 * sizeof(uint32_t)] ...) else /* user called nbd_block_status_64 */ CALL_CALLBACK (cmd->cb.fn.extent64 (..., nr_entries, (nbd_extent*)&bs_entries[0] ...) In this approach, no shims are needed (both 32- and 64-bit nbd_block_status* API calls natively call into the applications callback with the correct typing), and we have fewer malloc() calls, but need more memory reserved per the duration of a callback, and waste time on conversion for every API call. Or add even more complicated code to only do the conversion when we compare the user's API call to the server's response type. Just because it is a possible approach doesn't mean it is more appealing. Then again, you are right that having a shim makes life harder on verifying that we are doing correct memory management on all code paths, and coding things so that no shim is required might make the code easier to reason about despite its complexity in managing more memory. Vladimir also didn't like my attempts to do in-place conversions in the qemu counterpart code (see https://listman.redhat.com/archives/libguestfs/2023-May/031685.html), in part because you MUST be careful about index directions (incrementing when narrowing, decrementing when widening). So yet another approach is to read the server's memory as-is, then copy into the form passed to the user (rather than doing in-place conversions), regardless of whether that copy widens or narrows. That's back to two malloc's per reply (one to get the server's bytes, another for what to hand to the callback), but may be easier to reason about. Yes, I'm still working to rebase on your earlier comments (the helper function bs_reply_length_ok() idea made sense), and naming the actual variable rather than having to check that the correct type is doable (even though it makes for longer lines and possibly more line breaks). Aha - now that I've read this far, you are steering towards the same solution I started reaching above. Use two malloc()s, but be more specific about which types are represented. Your idea of releasing 'storage' and reassigning with 'wide' or 'narrow' is clever, but definitely nicer type-wise than in-place reinterpretation of a single malloc() across different types where (during the conversion) there is no single type that properly describes the overall storage. We always know what width the server send, and what width the caller requested. But since the two are orthonal, and we must support all four combinations, two of those pairing need a conversion. You are right that we can do conversions only when needed, and even set it up so that we do not need a shim. I'll play with ideas mentioned in this review (probably leaning towards the double-buffering idea: we have to read in the server's bytes, and have to do byte swapping - but how we do the byte-swapping can either be in-place when no widening or narrowing is needed, or double-buffered if we also have to adjust to match the caller's size choice). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org