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

On Tue, 4 Aug 2026 at 10:27, Frediano Ziglio <[email protected]> wrote:=0A=
> On Tue, 4 Aug 2026 at 10:15, Teddy Astie <[email protected]> wrote:=
=0A=
>>=0A=
>> Le 03/08/2026 =E0 19:23, Marcus Granado a =E9crit :=0A=
>>> simplify to reusing PAGE_DATA and spending one octet of its reserved wo=
rd=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 specif=
ic,=0A=
>>> the inline clen inconsistency in libxenguest record disappears, it's on=
e=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 co=
unt=0A=
>>> is bounded on the receiver.=0A=
>>>=0A=
>>=0A=
>> Looks good to me.=0A=
>>=0A=
>>> On Wed, 22 Jul 2026 at 20:42, Frediano Ziglio <[email protected]> wrot=
e:=0A=
>>>> Don't we need to bump the version number while we add a new mandatory=
=0A=
>>>> record?=0A=
>>>=0A=
>>> I believe we may avoid having to do a version bump if we adopt the prop=
erty=0A=
>>> that 0 < len(page_cdata) < N * page_size when comp !=3D 0, as in this c=
ase=0A=
>>> the compressed data sent to an old receiver would fail in handle_page_d=
ata()=0A=
>>> with "PAGE_DATA record wrong size". Bumping the version would make=0A=
>>> uncompressed migrations fail if they are sent to old receivers that als=
o=0A=
>>> understand uncompressed migration, so avoiding if possible would be goo=
d.=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=0A=
>>> on.=0A=
=0A=
=0A=
Thanks. I will take the discussion so far as the record format above being=
=0A=
agreed, so the only open point is how it is carried.=0A=
=0A=
>> The specification says=0A=
>>=0A=
>>  > Padding and reserved fields are set to zero on save and must be=0A=
>> ignored during restore.=0A=
>>=0A=
>> Which is not really going to help if we add a new field that matters on=
=0A=
>> how the content is organized.=0A=
=0A=
Agreed that the sentence as it stands does not cover it, but I think that=
=0A=
is a documentation problem rather than a format problem: once the octet is=
=0A=
given a meaning by the specification it is no longer a reserved field, so=
=0A=
the rule stops applying to it. The v2 spec patch would define comp in the=
=0A=
PAGE_DATA field table alongside count and pfn, and leave the remaining=0A=
three octets reserved and required to be zero.=0A=
=0A=
To make the compatibility argument rest on the specification rather than on=
=0A=
one implementation, I would also add these rules to it in the same patch:=
=0A=
=0A=
* a saver may only emit comp !=3D 0 when 0 < len(page_cdata) < N * page_siz=
e=0A=
* a restoring side must reject a PAGE_DATA record whose length is not=0A=
  exactly sizeof(hdr) + C * 8 + N * page_size when comp =3D=3D 0=0A=
* a restoring side must reject a record with count > MAX_BATCH_SIZE, and=0A=
  must fail on a comp value it does not implement=0A=
=0A=
The second is what handle_page_data() already does today, so writing it=0A=
down in the specification is easy and turns the PAGE_DATA argument below=0A=
into a guarantee rather than an observation about the current code.=0A=
=0A=
> Yes, there's no check for "_res1" being 0, however there's this check:=0A=
>    if ( rec->length !=3D (sizeof(*pages) +  ...=0A=
> so, as long as we require that the compressed data is less than the=0A=
> uncompressed one (we should) it's fine.=0A=
=0A=
> Not strong but if I could vote I would just use part of "_res1" as=0A=
> proposed.=0A=
=0A=
I would vote the same way. To be explicit about it: an old receiver=0A=
will reject a compressed record through the length check rather than=0A=
through the mandatory record rule, so the diagnostic is "PAGE_DATA record=
=0A=
wrong size" rather than one naming the unsupported compression. I think=0A=
that is an acceptable price for not adding a record type, given the strict=
=0A=
inequality above makes the rejection guaranteed rather than incidental.=0A=
=0A=
Andrew, Roger, the one open point is whether to carry this in a PAGE_DATA=
=0A=
reserved octet or in a separate PAGE_DATA_COMPRESSED mandatory record.=0A=
Unless a maintainer prefers the latter, I will implement v2 with the comp=
=0A=
octet in PAGE_DATA and the three rules above.=0A=
=0A=
Marcus=0A=
=0A=