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

On Wed, 29 Jul 2026 at 10:14, Frediano Ziglio <[email protected]> wrote:=
=0A=
> About compressing all together and considering also the issue of=0A=
> memory changing while sending I would vote to copy the memory in a=0A=
> temporary buffer to avoid this. One advantage is that it simplified=0A=
> the format. The current LZ4 implementation seems to cope with data=0A=
> changes but nothing guarantees it in the future, the buffer you are=0A=
> passing is not supposed to change while you compress it.=0A=
=0A=
=0A=
Agreed. Frediano and I discussed this further, including simplifications on=
=0A=
the compression record for v2 so that the page data compression:=0A=
* compresses the whole batch together=0A=
* and uses a staged copy for it as you and Teddy suggested (so that we do=
=0A=
not need to worry about mutating buffers affecting LZ4 or other algorithms)=
.=0A=
=0A=
Once there's exactly one compressed blob per record, there's no need for=0A=
extra fields to describe the compressed data, as the uncompressed batch=0A=
size is known (and bounded by MAX_BATCH_SIZE * page_size), and its=0A=
compressed size can be inferred from the size of the emitted compressed=0A=
record without a need for an explicit field for clen or a field with the=0A=
number of raw compressed blobs. This means the separate PAGE_DATA_LZ4=0A=
record type is not needed, and the proposal for v2 compression record could=
=0A=
simplify to reusing PAGE_DATA and spending one octet of its reserved word=
=0A=
as follows:=0A=
=0A=
=0A=
     0     1     2     3     4     5     6     7 octet=0A=
    +-----------------------+-----+-------------------+=0A=
    | count (C)             | comp| (reserved)        |=0A=
    +-----------------------+-----+-------------------+=0A=
    | pfn[0]                                          |=0A=
    +-------------------------------------------------+=0A=
    ...=0A=
    +-------------------------------------------------+=0A=
    | pfn[C-1]                                        |=0A=
    +-------------------------------------------------+=0A=
    | page_data[0..N-1] if comp =3D=3D 0                  |=0A=
    | or page_cdata     if comp !=3D 0                  |=0A=
    +-------------------------------------------------+=0A=
=0A=
with two new entries in the field table:=0A=
=0A=
comp        Compression algorithm applied to the page contents. 0=0A=
            means none, and the record is exactly as it is today. 1=0A=
            means LZ4 block format. Other values are reserved for=0A=
            other future formats like ZSTD etc. A comp !=3D 0 can only=0A=
            be emitted if 0 < len(page_cdata) < N * page_size. An=0A=
            unknown comp must cause the receiver to fail with a=0A=
            "Compression algorithm <value> not handled" error.=0A=
=0A=
page_cdata  Present instead of page_data when comp is non-zero. A=0A=
            single compressed object holding the concatenation of the=0A=
            N page_data entries. The receiver must verify that=0A=
            0 < count <=3D MAX_BATCH_SIZE.=0A=
=0A=
=0A=
Teddy, I hope this format covers your points: it's no longer LZ4 specific,=
=0A=
the inline clen inconsistency in libxenguest record disappears, it's one=0A=
block over an immutable copy of the batch instead of one per page, the=0A=
decompressed size is known up front, and the allocation derived from count=
=0A=
is bounded on the receiver.=0A=
=0A=
=0A=
This simplification is also forward-compatible in two different ways: your=
=0A=
64KB chunking for cache locality doesn't depend on the format, as the=0A=
staging copy can be done in chunks while still making a single compress=0A=
call over the whole batch. And if we find benefits in splitting the payload=
=0A=
into several compressed units, that can be implemented as a new comp value=
=0A=
using a self-delimiting format like zstd or lz4f frames, which report the=
=0A=
consumed bytes without a need to specify clen or extra framing fields at th=
e=0A=
record level.=0A=
=0A=
=0A=
On Wed, 22 Jul 2026 at 20:42, Frediano Ziglio <[email protected]> wrote:=
=0A=
> Don't we need to bump the version number while we add a new mandatory rec=
ord?=0A=
=0A=
I believe we may avoid having to do a version bump if we adopt the property=
=0A=
that 0 < len(page_cdata) < N * page_size when comp !=3D 0, as in this case=
=0A=
the compressed data sent to an old receiver would fail in handle_page_data(=
)=0A=
with "PAGE_DATA record wrong size". Bumping the version would make=0A=
uncompressed migrations fail if they are sent to old receivers that also=0A=
understand uncompressed migration, so avoiding if possible would be good.=
=0A=
The spec says migration tools "shall always save images using version V",=
=0A=
so it doesn't seem like we could bump the version only when compression is =
on.=0A=
=0A=
=0A=
> Is there no kind of dialog about the supported version?=0A=
=0A=
There is no in-stream negotiation, and this proposal does not add one.=0A=
Compression is opt-in at the sender via xl migrate --compress, so an operat=
or=0A=
who enables it against an old receiver gets a clean failure rather than=0A=
corruption.=0A=
=0A=
> Why not extending the generalization compressing all payload of=0A=
> uncompressed packets (type+body),=0A=
=0A=
A composable wrapper would be a clean generic mechanism, but I think there=
=0A=
are two reasons not to go that way in v2: PAGE_DATA is effectively all of t=
he=0A=
stream bytes, so compressing the other record types would not show up in a=
=0A=
measurement. And the no-extra-fields property above depends on PAGE_DATA=0A=
specifically: a generic wrapper has no pfn array from where we can derive=
=0A=
the uncompressed size, so it would need explicit algorithm, compressed=0A=
size and uncompressed size fields on every record, which re-adds the fields=
=0A=
that this proposal tries to avoid.=0A=
=0A=
=0A=
Please let me know if the ideas above capture what you had in mind in terms=
 of=0A=
suggestions to improve v1.=0A=
=0A=
In particular, I wonder what the maintainers think of the use of one of the=
=0A=
octets of the PAGE_DATA reserved field for the purpose of indicating the da=
ta=0A=
is compressed, or is it preferable to use a new PAGE_DATA_COMPRESSED (0x13)=
=0A=
type record as a mandatory record so that an old receiver fails more cleanl=
y=0A=
when it doesn't understand compression "Mandatory record <name> not handled=
"=0A=
(caused by the specification of mandatory records) instead of with a generi=
c=0A=
error "PAGE_DATA record wrong size" (caused by the current safety=0A=
implementation that rejects unexpected record sizes)?=0A=
=0A=
=0A=
Marcus=0A=
=0A=