Re: [PATCH v1 1/6] tools/migration: introduce PAGE_DATA_LZ4 stream record type

Teddy Astie <[email protected]> Tue, 4 Aug 2026 11:15:02 +0200
Newsgroups	org.xenproject.lists.xen-devel
Message-ID	<1785834903.8631fc262581453bbf619ec5b2062170.19fcc0e18f8000e099@vates.tech>
Le 03/08/2026 à 19:23, Marcus Granado a écrit :
> On Wed, 29 Jul 2026 at 10:14, Frediano Ziglio <[email protected]> wrote:
>> About compressing all together and considering also the issue of
>> memory changing while sending I would vote to copy the memory in a
>> temporary buffer to avoid this. One advantage is that it simplified
>> the format. The current LZ4 implementation seems to cope with data
>> changes but nothing guarantees it in the future, the buffer you are
>> passing is not supposed to change while you compress it.
> 
> 
> Agreed. Frediano and I discussed this further, including simplifications on
> the compression record for v2 so that the page data compression:
> * compresses the whole batch together
> * and uses a staged copy for it as you and Teddy suggested (so that we do
> not need to worry about mutating buffers affecting LZ4 or other algorithms).
> 
> Once there's exactly one compressed blob per record, there's no need for
> extra fields to describe the compressed data, as the uncompressed batch
> size is known (and bounded by MAX_BATCH_SIZE * page_size), and its
> compressed size can be inferred from the size of the emitted compressed
> record without a need for an explicit field for clen or a field with the
> number of raw compressed blobs. This means the separate PAGE_DATA_LZ4
> record type is not needed, and the proposal for v2 compression record could
> 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.

> 
> This simplification is also forward-compatible in two different ways: your
> 64KB chunking for cache locality doesn't depend on the format, as the
> staging copy can be done in chunks while still making a single compress
> call over the whole batch. And if we find benefits in splitting the payload
> into several compressed units, that can be implemented as a new comp value
> using a self-delimiting format like zstd or lz4f frames, which report the
> consumed bytes without a need to specify clen or extra framing fields at the
> record level.
> 
> 
> On Wed, 22 Jul 2026 at 20:42, Frediano Ziglio <[email protected]> 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.
> 
> 
>> Is there no kind of dialog about the supported version?
> 
> There is no in-stream negotiation, and this proposal does not add one.
> Compression is opt-in at the sender via xl migrate --compress, so an operator
> who enables it against an old receiver gets a clean failure rather than
> corruption.
> 
>> Why not extending the generalization compressing all payload of
>> uncompressed packets (type+body),
> 
> A composable wrapper would be a clean generic mechanism, but I think there
> are two reasons not to go that way in v2: PAGE_DATA is effectively all of the
> stream bytes, so compressing the other record types would not show up in a
> measurement. And the no-extra-fields property above depends on PAGE_DATA
> specifically: a generic wrapper has no pfn array from where we can derive
> the uncompressed size, so it would need explicit algorithm, compressed
> size and uncompressed size fields on every record, which re-adds the fields
> that this proposal tries to avoid.
> 
> 
> Please let me know if the ideas above capture what you had in mind in terms of
> suggestions to improve v1.
> 
> In particular, I wonder what the maintainers think of the use of one of the
> octets of the PAGE_DATA reserved field for the purpose of indicating the data
> is compressed, or is it preferable to use a new PAGE_DATA_COMPRESSED (0x13)
> type record as a mandatory record so that an old receiver fails more cleanly
> when it doesn't understand compression "Mandatory record <name> not handled"
> (caused by the specification of mandatory records) instead of with a generic
> error "PAGE_DATA record wrong size" (caused by the current safety
> implementation that rejects unexpected record sizes)?
> 

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.

To avoid confusion, it may be desirable to add a new type 
(PAGE_DATA_COMPRESSED), but I'm not fully sold on it.

> 
> Marcus
> 
> 

Teddy
OpenPGP_0x660FA9D102CBCFD0.asc (application/pgp-keys, 2.4 KB)
OpenPGP_signature.asc (application/pgp-signature, 665 B)