|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/6] tools/migration: introduce PAGE_DATA_LZ4 stream record type
On Tue, 4 Aug 2026 at 10:27, Frediano Ziglio <freddy77@xxxxxxxxx> wrote: > On Tue, 4 Aug 2026 at 10:15, Teddy Astie <teddy.astie@xxxxxxxxxx> wrote: >> >> Le 03/08/2026 à 19:23, Marcus Granado a écrit : >>> simplify to reusing PAGE_DATA and spending one octet of its reserved word >>> as follows: >>> >>> >>> 0 1 2 3 4 5 6 7 octet >>> +-----------------------+-----+-------------------+ >>> | count (C) | comp| (reserved) | >>> +-----------------------+-----+-------------------+ >>> | pfn[0] | >>> +-------------------------------------------------+ >>> ... >>> +-------------------------------------------------+ >>> | pfn[C-1] | >>> +-------------------------------------------------+ >>> | page_data[0..N-1] if comp == 0 | >>> | or page_cdata if comp != 0 | >>> +-------------------------------------------------+ >>> >>> with two new entries in the field table: >>> >>> comp Compression algorithm applied to the page contents. 0 >>> means none, and the record is exactly as it is today. 1 >>> means LZ4 block format. Other values are reserved for >>> other future formats like ZSTD etc. A comp != 0 can only >>> be emitted if 0 < len(page_cdata) < N * page_size. An >>> unknown comp must cause the receiver to fail with a >>> "Compression algorithm <value> not handled" error. >>> >>> page_cdata Present instead of page_data when comp is non-zero. A >>> single compressed object holding the concatenation of the >>> N page_data entries. The receiver must verify that >>> 0 < count <= MAX_BATCH_SIZE. >>> >>> >>> Teddy, I hope this format covers your points: it's no longer LZ4 specific, >>> the inline clen inconsistency in libxenguest record disappears, it's one >>> block over an immutable copy of the batch instead of one per page, the >>> decompressed size is known up front, and the allocation derived from count >>> is bounded on the receiver. >>> >> >> Looks good to me. >> >>> On Wed, 22 Jul 2026 at 20:42, Frediano Ziglio <freddy77@xxxxxxxxx> wrote: >>>> Don't we need to bump the version number while we add a new mandatory >>>> record? >>> >>> I believe we may avoid having to do a version bump if we adopt the property >>> that 0 < len(page_cdata) < N * page_size when comp != 0, as in this case >>> the compressed data sent to an old receiver would fail in handle_page_data() >>> with "PAGE_DATA record wrong size". Bumping the version would make >>> uncompressed migrations fail if they are sent to old receivers that also >>> understand uncompressed migration, so avoiding if possible would be good. >>> The spec says migration tools "shall always save images using version V", >>> so it doesn't seem like we could bump the version only when compression is >>> on. Thanks. I will take the discussion so far as the record format above being agreed, so the only open point is how it is carried. >> The specification says >> >> > Padding and reserved fields are set to zero on save and must be >> ignored during restore. >> >> Which is not really going to help if we add a new field that matters on >> how the content is organized. Agreed that the sentence as it stands does not cover it, but I think that is a documentation problem rather than a format problem: once the octet is given a meaning by the specification it is no longer a reserved field, so the rule stops applying to it. The v2 spec patch would define comp in the PAGE_DATA field table alongside count and pfn, and leave the remaining three octets reserved and required to be zero. To make the compatibility argument rest on the specification rather than on one implementation, I would also add these rules to it in the same patch: * a saver may only emit comp != 0 when 0 < len(page_cdata) < N * page_size * a restoring side must reject a PAGE_DATA record whose length is not exactly sizeof(hdr) + C * 8 + N * page_size when comp == 0 * a restoring side must reject a record with count > MAX_BATCH_SIZE, and must fail on a comp value it does not implement The second is what handle_page_data() already does today, so writing it down in the specification is easy and turns the PAGE_DATA argument below into a guarantee rather than an observation about the current code. > Yes, there's no check for "_res1" being 0, however there's this check: > if ( rec->length != (sizeof(*pages) + ... > so, as long as we require that the compressed data is less than the > uncompressed one (we should) it's fine. > Not strong but if I could vote I would just use part of "_res1" as > proposed. I would vote the same way. To be explicit about it: an old receiver will reject a compressed record through the length check rather than through the mandatory record rule, so the diagnostic is "PAGE_DATA record wrong size" rather than one naming the unsupported compression. I think that is an acceptable price for not adding a record type, given the strict inequality above makes the rejection guaranteed rather than incidental. Andrew, Roger, the one open point is whether to carry this in a PAGE_DATA reserved octet or in a separate PAGE_DATA_COMPRESSED mandatory record. Unless a maintainer prefers the latter, I will implement v2 with the comp octet in PAGE_DATA and the three rules above. Marcus
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |