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

On Tue, 4 Aug 2026 at 10:15, Teddy Astie <[email protected]> wrote:
>
> 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.
>

Yes, there's no check for "_res1" being 0, however there's this check:

    if ( rec->length != (sizeof(*pages) +
                         (sizeof(uint64_t) * pages->count) +
                         (PAGE_SIZE * pages_of_data)) )
    {
        ERROR("PAGE_DATA record wrong size: length %u, expected "
              "%zu + %zu + %lu", rec->length, sizeof(*pages),
              (sizeof(uint64_t) * pages->count), (PAGE_SIZE * pages_of_data));
        goto err;
    }

so, as long as we require that the compressed data is less than the
uncompressed one (we should) it's fine.


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

Not strong but if I could vote I would just use part of "_res1" as proposed.

> >
> > Marcus
> >
> >
>
> Teddy

Frediano