[PATCH 0/2] Fancier version handling for migration

newer
[PATCH 0/2] Trivial cleanups to...

older
[net,v3] tcp: correct handling of...

David Gibson

31 Jan 2025 31 Jan '25

5:52 a.m.

This series introduces more flexible version handling for our device state migration stream. This is based on my previous assorted migration improvements series. David Gibson (2): migrate: Merge protocol version information into one table migrate: More sophisticated versioning migrate.c | 213 +++++++++++++++++++++++++++++++++++------------------- migrate.h | 73 +++++++++++-------- 2 files changed, 179 insertions(+), 107 deletions(-) -- 2.48.1

Show replies by date

David Gibson

31 Jan 31 Jan

5:52 a.m.

New subject: [PATCH 1/2] migrate: Merge protocol version information into one table

Currently information on protocol version specific things is split between struct migrate_data, which lists the memory blocks to save and restore and struct migrate_target_handlers which gives the per-version (target) pre and post handlers to run. In addition, despite having already looked up the version in the migrate_data table at the top-level, we look it up again in migrate_target_state() and do a similar lookup in target_handlers in migrate_target_{pre,post}(). Combine the per-version information into a single struct migrate_version, look it up once at the top level, then pass that through to the other functions within migrate_meta. Signed-off-by: David Gibson --- migrate.c | 68 ++++++++++++++++++++----------------------------------- migrate.h | 55 +++++++++++++++++++++----------------------- 2 files changed, 51 insertions(+), 72 deletions(-) diff --git a/migrate.c b/migrate.c index 4279d1f8..f9e967cf 100644 --- a/migrate.c +++ b/migrate.c @@ -42,19 +42,6 @@ static union migrate_header header = { .voidp_size = htonl_constant(sizeof(void *)), }; -/* Data sections for version 1 */ -static struct iovec sections_v1[] = { - { &header, sizeof(header) }, -}; - -/* Set of data versions */ -static struct migrate_data data_versions[] = { - { - 1, sections_v1, - }, - { 0 }, -}; - /* Handlers to call in source before sending data */ struct migrate_handler handlers_source_pre[] = { { flow_migrate_source_pre }, @@ -76,12 +63,18 @@ struct migrate_handler handlers_target_post_v1[] = { { 0 }, }; -/* Versioned sets of migration handlers */ -struct migrate_target_handlers target_handlers[] = { +/* Data sections for version 1 */ +static struct iovec sections_v1[] = { + { &header, sizeof(header) }, +}; + +/* Supported protocol versions */ +static const struct migrate_version versions[] = { { - 1, - handlers_target_pre_v1, - handlers_target_post_v1, + .v = 1, + .pre = handlers_target_pre_v1, + .sections = sections_v1, + .post = handlers_target_post_v1, }, { 0 }, }; @@ -116,17 +109,12 @@ static int migrate_source_pre(struct ctx *c, struct migrate_meta *m) */ static int migrate_source_state(int fd, const struct migrate_meta *m) { - static struct migrate_data *d; int count, rc; - (void)m; - - for (d = data_versions; d->v != MIGRATE_VERSION; d++); - - for (count = 0; d->sections[count].iov_len; count++); + for (count = 0; m->v->sections[count].iov_len; count++); debug("Writing %u migration sections", count - 1 /* minus header */); - rc = write_remainder(fd, d->sections, count, 0); + rc = write_remainder(fd, m->v->sections, count, 0); if (rc < 0) return errno; @@ -160,6 +148,9 @@ static int migrate_source(struct ctx *c, int fd) struct migrate_meta m; int rc; + m.version = MIGRATE_VERSION; + for (m.v = versions; m.v->v != m.version; m.v++); + if ((rc = migrate_source_pre(c, &m))) { err("Source pre-migration failed: %s, abort", strerror_(rc)); return rc; @@ -185,19 +176,19 @@ static int migrate_source(struct ctx *c, int fd) */ static int migrate_target_read_header(int fd, struct migrate_meta *m) { - static struct migrate_data *d; union migrate_header h; if (read_all_buf(fd, &h, sizeof(h))) return errno; + m->version = ntohl(h.version); + debug("Source magic: 0x%016" PRIx64 ", sizeof(void *): %u, version: %u", - h.magic, ntohl(h.voidp_size), ntohl(h.version)); + h.magic, ntohl(h.voidp_size), m->version); - for (d = data_versions; d->v != ntohl(h.version) && d->v; d++); - if (!d->v) + for (m->v = versions; m->v->v != m->version; m->v++); + if (!m->v->v) return ENOTSUP; - m->v = d->v; if (h.magic == MIGRATE_MAGIC) m->bswap = false; @@ -235,12 +226,9 @@ static int migrate_target_read_header(int fd, struct migrate_meta *m) */ static int migrate_target_pre(struct ctx *c, struct migrate_meta *m) { - struct migrate_target_handlers *th; struct migrate_handler *h; - for (th = target_handlers; th->v != m->v && th->v; th++); - - for (h = th->pre; h->fn; h++) { + for (h = m->v->pre; h->fn; h++) { int rc; if ((rc = h->fn(c, m))) @@ -261,16 +249,13 @@ static int migrate_target_pre(struct ctx *c, struct migrate_meta *m) */ static int migrate_target_state(int fd, const struct migrate_meta *m) { - static struct migrate_data *d; unsigned cnt; int rc; - for (d = data_versions; d->v != m->v && d->v; d++); - - for (cnt = 0; d->sections[cnt + 1 /* skip header */].iov_len; cnt++); + for (cnt = 0; m->v->sections[cnt + 1 /* skip header */].iov_len; cnt++); debug("Reading %u migration sections", cnt); - rc = read_remainder(fd, d->sections + 1, cnt, 0); + rc = read_remainder(fd, m->v->sections + 1, cnt, 0); if (rc < 0) return errno; @@ -284,12 +269,9 @@ static int migrate_target_state(int fd, const struct migrate_meta *m) */ static void migrate_target_post(struct ctx *c, struct migrate_meta *m) { - struct migrate_target_handlers *th; struct migrate_handler *h; - for (th = target_handlers; th->v != m->v && th->v; th++); - - for (h = th->post; h->fn; h++) + for (h = m->v->post; h->fn; h++) h->fn(c, m); } diff --git a/migrate.h b/migrate.h index 3432940d..b532bde0 100644 --- a/migrate.h +++ b/migrate.h @@ -6,24 +6,6 @@ #ifndef MIGRATE_H #define MIGRATE_H -/** - * struct migrate_meta - Migration metadata - * @v: Chosen migration data version, host order - * @bswap: Source has opposite endianness - * @peer_64b: Source uses 64-bit void * - * @time_64b: Source uses 64-bit time_t - * @flow_size: Size of union flow in source - * @flow_sidx_size: Size of struct flow_sidx in source - */ -struct migrate_meta { - uint32_t v; - bool bswap; - bool source_64b; - bool time_64b; - size_t flow_size; - size_t flow_sidx_size; -}; - /** * union migrate_header - Migration header from source * @magic: 0xB1BB1D1B0BB1D1B0, host order @@ -46,15 +28,7 @@ union migrate_header { uint8_t unused[65536]; }; -/** - * struct migrate_data - Data sections for given source version - * @v: Source version this applies to, host order - * @sections: Array of data sections, NULL-terminated - */ -struct migrate_data { - uint32_t v; - struct iovec *sections; -}; +struct migrate_meta; /** * struct migrate_handler - Function to handle a specific data section @@ -65,17 +39,40 @@ struct migrate_handler { }; /** - * struct migrate_target_handlers - Versioned sets of migration target handlers + * struct migrate_version - Handlers and data sections for a protocol version * @v: Source version this applies to, host order * @pre: Set of functions to execute in target before data copy + * @sections: Array of data sections, NULL-terminated * @post: Set of functions to execute in target after data copy */ -struct migrate_target_handlers { +struct migrate_version { uint32_t v; struct migrate_handler *pre; + struct iovec *sections; struct migrate_handler *post; }; +/** + * struct migrate_meta - Migration metadata + * @version: Version number, host order + * @v: Version information + * @bswap: Source has opposite endianness + * @peer_64b: Source uses 64-bit void * + * @time_64b: Source uses 64-bit time_t + * @flow_size: Size of union flow in source + * @flow_sidx_size: Size of struct flow_sidx in source + */ +struct migrate_meta { + uint32_t version; + const struct migrate_version *v; + bool bswap; + bool source_64b; + bool time_64b; + size_t flow_size; + size_t flow_sidx_size; +}; + + void migrate_init(struct ctx *c); void migrate_close(struct ctx *c); void migrate_request(struct ctx *c, int fd, bool target); -- 2.48.1

Stefano Brivio

6:53 a.m.

New subject: [PATCH 1/2] migrate: Merge protocol version information into one table

On Fri, 31 Jan 2025 15:52:14 +1100 David Gibson wrote:

...

+/** + * struct migrate_meta - Migration metadata + * @version: Version number, host order + * @v: Version information + * @bswap: Source has opposite endianness + * @peer_64b: Source uses 64-bit void * + * @time_64b: Source uses 64-bit time_t + * @flow_size: Size of union flow in source + * @flow_sidx_size: Size of struct flow_sidx in source + */ +struct migrate_meta { + uint32_t version; + const struct migrate_version *v; + bool bswap; + bool source_64b; + bool time_64b; + size_t flow_size; + size_t flow_sidx_size; +};

What's the purpose of this, if this information should go away quite soon (even before merge, perhaps)? -- Stefano

David Gibson

7:17 a.m.

New subject: [PATCH 1/2] migrate: Merge protocol version information into one table

On Fri, Jan 31, 2025 at 06:53:44AM +0100, Stefano Brivio wrote:

...

On Fri, 31 Jan 2025 15:52:14 +1100 David Gibson wrote:

...
+/** + * struct migrate_meta - Migration metadata + * @version: Version number, host order + * @v: Version information + * @bswap: Source has opposite endianness + * @peer_64b: Source uses 64-bit void * + * @time_64b: Source uses 64-bit time_t + * @flow_size: Size of union flow in source + * @flow_sidx_size: Size of struct flow_sidx in source + */ +struct migrate_meta { + uint32_t version; + const struct migrate_version *v; + bool bswap; + bool source_64b; + bool time_64b; + size_t flow_size; + size_t flow_sidx_size; +};

What's the purpose of this, if this information should go away quite soon (even before merge, perhaps)?

Soon, but not yet. I don't want to break the stuff you're working on until I have something else to replace it. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

David Gibson

5:52 a.m.

New subject: [PATCH 2/2] migrate: More sophisticated versioning

At present the header for our device state migration stream is sent as a buffer in the sections array, like anything else. It contains both version information, and details on the source ABI which are specific to v1 of the migration protocol. Alter this for greater flexibility: * We separate out a minimal fixed header, which we will need to keep for every future version, from a version specific header, containing (for v1) the ABI data * Handle both the headers separately from the data sections making for better symmetry between the source and target sides * Add a "compat_version" field. This will allow us to make future protocol extensions which are backwards compatible with older targets, while retaining the ability to also make breaking protocol extensions. This establishes a minimal header with fixed representation to maintain for all future versions. Signed-off-by: David Gibson --- migrate.c | 157 ++++++++++++++++++++++++++++++++++++++++-------------- migrate.h | 26 ++++++--- 2 files changed, 138 insertions(+), 45 deletions(-) diff --git a/migrate.c b/migrate.c index f9e967cf..3cad9892 100644 --- a/migrate.c +++ b/migrate.c @@ -12,6 +12,7 @@ * Author: Stefano Brivio */ +#include #include #include @@ -28,14 +29,20 @@ /* Current version of migration data */ #define MIGRATE_VERSION 1 +#define MAGIC_INIT { '{', 'p', 'a', 's', 's', 't', 'y', '}' } + +static const struct migrate_header header = { + .magic = MAGIC_INIT, + .version = htonl_constant(MIGRATE_VERSION), + .compat_version = htonl_constant(MIGRATE_VERSION), +}; + /* Magic as we see it and as seen with reverse endianness */ -#define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 -#define MIGRATE_MAGIC_SWAPPED 0xB0D1B1B01B1DBBB1 +#define MIGRATE_ENDIAN_TAG 0x12345678 /* Migration header to send from source */ -static union migrate_header header = { - .magic = MIGRATE_MAGIC, - .version = htonl_constant(MIGRATE_VERSION), +static union migrate_header_v1 header_v1 = { + .endian = MIGRATE_ENDIAN_TAG, .time_t_size = htonl_constant(sizeof(time_t)), .flow_size = htonl_constant(sizeof(union flow)), .flow_sidx_size = htonl_constant(sizeof(struct flow_sidx)), @@ -65,13 +72,69 @@ struct migrate_handler handlers_target_post_v1[] = { /* Data sections for version 1 */ static struct iovec sections_v1[] = { - { &header, sizeof(header) }, + { NULL, 0 }, }; +/** + * migrate_write_header_v1() - Send v1 ABI information from source + * @fd: Descriptor for state transfer + * + * Return: 0 on success, error code on failure + */ +static int migrate_write_header_v1(int fd) +{ + if (write_all_buf(fd, &header_v1, sizeof(header_v1))) + return errno; + + return 0; +} + +/** + * migrate_read_header_v1() - Read and check ABI information sent by source + * @fd: Descriptor for state transfer + * @m: Migration metadata, updated on return + * + * Return: 0 on success, error code on failure + */ +static int migrate_read_header_v1(int fd, struct migrate_meta *m) +{ + union migrate_header_v1 h; + + if (read_all_buf(fd, &h, sizeof(h))) + return errno; + + if (h.endian == MIGRATE_ENDIAN_TAG) + m->bswap = false; + else if (bswap_32(h.endian) == MIGRATE_ENDIAN_TAG) + m->bswap = true; + else + return ENOTSUP; + + if (ntohl(h.voidp_size) == 4) + m->source_64b = false; + else if (ntohl(h.voidp_size) == 8) + m->source_64b = true; + else + return ENOTSUP; + + if (ntohl(h.time_t_size) == 4) + m->time_64b = false; + else if (ntohl(h.time_t_size) == 8) + m->time_64b = true; + else + return ENOTSUP; + + m->flow_size = ntohl(h.flow_size); + m->flow_sidx_size = ntohl(h.flow_sidx_size); + + return 0; +} + /* Supported protocol versions */ static const struct migrate_version versions[] = { { .v = 1, + .read_header_v = migrate_read_header_v1, .pre = handlers_target_pre_v1, .sections = sections_v1, .post = handlers_target_post_v1, @@ -79,6 +142,20 @@ static const struct migrate_version versions[] = { { 0 }, }; +/** + * migrate_write_header() - Send basic version information from source + * @fd: Descriptor for state transfer + * + * Return: 0 on success, error code on failure + */ +static int migrate_write_header(int fd) +{ + if (write_all_buf(fd, &header, sizeof(header))) + return errno; + + return 0; +} + /** * migrate_source_pre() - Pre-migration tasks as source * @c: Execution context @@ -151,6 +228,18 @@ static int migrate_source(struct ctx *c, int fd) m.version = MIGRATE_VERSION; for (m.v = versions; m.v->v != m.version; m.v++); + rc = migrate_write_header(fd); + if (rc) { + err("Source migration header failed: %s", strerror_(rc)); + return rc; + } + + rc = migrate_write_header_v1(fd); + if (rc) { + err("Source migration header v1 failed: %s", strerror_(rc)); + return rc; + } + if ((rc = migrate_source_pre(c, &m))) { err("Source pre-migration failed: %s, abort", strerror_(rc)); return rc; @@ -168,52 +257,35 @@ static int migrate_source(struct ctx *c, int fd) } /** - * migrate_target_read_header() - Set metadata in target from source header + * migrate_read_header() - Read header and check versions from source * @fd: Descriptor for state transfer * @m: Migration metadata, filled on return * * Return: 0 on success, error code on failure */ -static int migrate_target_read_header(int fd, struct migrate_meta *m) +static int migrate_read_header(int fd, struct migrate_meta *m) { - union migrate_header h; + struct migrate_header h; + uint32_t compat_version; if (read_all_buf(fd, &h, sizeof(h))) return errno; m->version = ntohl(h.version); + compat_version = ntohl(h.compat_version); - debug("Source magic: 0x%016" PRIx64 ", sizeof(void *): %u, version: %u", - h.magic, ntohl(h.voidp_size), m->version); + debug("Source magic: '%c%c%c%c%c%c%c%c version: %u compat_version: %u", + h.magic[0], h.magic[1], h.magic[2], h.magic[3], + h.magic[4], h.magic[5], h.magic[6], h.magic[7], + m->version, compat_version); - for (m->v = versions; m->v->v != m->version; m->v++); + for (m->v = versions; + m->v->v > m->version && m->v->v < compat_version; + m->v++) + ; if (!m->v->v) return ENOTSUP; - if (h.magic == MIGRATE_MAGIC) - m->bswap = false; - else if (h.magic == MIGRATE_MAGIC_SWAPPED) - m->bswap = true; - else - return ENOTSUP; - - if (ntohl(h.voidp_size) == 4) - m->source_64b = false; - else if (ntohl(h.voidp_size) == 8) - m->source_64b = true; - else - return ENOTSUP; - - if (ntohl(h.time_t_size) == 4) - m->time_64b = false; - else if (ntohl(h.time_t_size) == 8) - m->time_64b = true; - else - return ENOTSUP; - - m->flow_size = ntohl(h.flow_size); - m->flow_sidx_size = ntohl(h.flow_sidx_size); - return 0; } @@ -252,10 +324,10 @@ static int migrate_target_state(int fd, const struct migrate_meta *m) unsigned cnt; int rc; - for (cnt = 0; m->v->sections[cnt + 1 /* skip header */].iov_len; cnt++); + for (cnt = 0; m->v->sections[cnt].iov_len; cnt++); debug("Reading %u migration sections", cnt); - rc = read_remainder(fd, m->v->sections + 1, cnt, 0); + rc = read_remainder(fd, m->v->sections, cnt, 0); if (rc < 0) return errno; @@ -287,12 +359,19 @@ static int migrate_target(struct ctx *c, int fd) struct migrate_meta m; int rc; - rc = migrate_target_read_header(fd, &m); + rc = migrate_read_header(fd, &m); if (rc) { err("Migration header check failed: %s, abort", strerror_(rc)); return rc; } + rc = m.v->read_header_v(fd, &m); + if (rc) { + err("Migration header v%d check failed: %s, abort", + m.v->v, strerror_(rc)); + return rc; + } + if ((rc = migrate_target_pre(c, &m))) { err("Target pre-migration failed: %s, abort", strerror_(rc)); return rc; diff --git a/migrate.h b/migrate.h index b532bde0..343448c8 100644 --- a/migrate.h +++ b/migrate.h @@ -6,20 +6,32 @@ #ifndef MIGRATE_H #define MIGRATE_H +typedef char migrate_magic_t[8]; + +/** + * struct migrate_header - Initial header used for all protocol versions + * @magic: '{passty}' as a byte array + * @version: Version of the protocol used by sender, network order + * @compat_version: Lowest compatible protocol version, network order + */ +struct migrate_header { + migrate_magic_t magic; + uint32_t version; + uint32_t compat_version; +}; + /** - * union migrate_header - Migration header from source - * @magic: 0xB1BB1D1B0BB1D1B0, host order - * @version: Source sends highest known, target aborts if unsupported + * union migrate_header_v1 - Migration header from source + * @endian: 0x12345678, source host order * @voidp_size: sizeof(void *), network order * @time_t_size: sizeof(time_t), network order * @flow_size: sizeof(union flow), network order * @flow_sidx_size: sizeof(struct flow_sidx_t), network order * @unused: Go figure */ -union migrate_header { +union migrate_header_v1 { struct { - uint64_t magic; - uint32_t version; + uint32_t endian; uint32_t voidp_size; uint32_t time_t_size; uint32_t flow_size; @@ -41,12 +53,14 @@ struct migrate_handler { /** * struct migrate_version - Handlers and data sections for a protocol version * @v: Source version this applies to, host order + * @read_header_v: Read version specific header(s) * @pre: Set of functions to execute in target before data copy * @sections: Array of data sections, NULL-terminated * @post: Set of functions to execute in target after data copy */ struct migrate_version { uint32_t v; + int (*read_header_v)(int fd, struct migrate_meta *m); struct migrate_handler *pre; struct iovec *sections; struct migrate_handler *post; -- 2.48.1

Stefano Brivio

6:53 a.m.

New subject: [PATCH 2/2] migrate: More sophisticated versioning

On Fri, 31 Jan 2025 15:52:15 +1100 David Gibson wrote:

...

At present the header for our device state migration stream is sent as a buffer in the sections array, like anything else. It contains both version information, and details on the source ABI which are specific to v1 of the migration protocol. Alter this for greater flexibility:

* We separate out a minimal fixed header, which we will need to keep for every future version, from a version specific header, containing (for v1) the ABI data

What's the purpose of this if there should be no ABI data? I have this for the moment: /* Immutable part of header structure: keep these two sections at the * beginning, because they are enough to identify a version regardless * of metadata. */ .magic = MIGRATE_MAGIC, .version = htonl_constant(MIGRATE_VERSION), /* End of immutable part of header structure */ ...which to be honest already felt like wasted effort, despite being just two comments.

...

* Handle both the headers separately from the data sections making for better symmetry between the source and target sides * Add a "compat_version" field. This will allow us to make future protocol extensions which are backwards compatible with older targets, while retaining the ability to also make breaking protocol extensions.

This establishes a minimal header with fixed representation to maintain for all future versions.

Signed-off-by: David Gibson --- migrate.c | 157 ++++++++++++++++++++++++++++++++++++++++-------------- migrate.h | 26 ++++++--- 2 files changed, 138 insertions(+), 45 deletions(-) ^^^

...is this really a good idea if we want to drop this right away? I might be missing something but I think it would more sense to focus on the "targeted" data transfer instead. -- Stefano

David Gibson

7:21 a.m.

New subject: [PATCH 2/2] migrate: More sophisticated versioning

On Fri, Jan 31, 2025 at 06:53:48AM +0100, Stefano Brivio wrote:

...

On Fri, 31 Jan 2025 15:52:15 +1100 David Gibson wrote:

...
At present the header for our device state migration stream is sent as a buffer in the sections array, like anything else. It contains both version information, and details on the source ABI which are specific to v1 of the migration protocol. Alter this for greater flexibility:

* We separate out a minimal fixed header, which we will need to keep for every future version, from a version specific header, containing (for v1) the ABI data

What's the purpose of this if there should be no ABI data?

We might get rid of the ABI data, but it seems very likely to me that future protocol versions will want some sort of header / metadata information. More specifically, it seems likely to me that they'll want some sort of meta information that needs to be checked / processed, but not read into a long term data structure as-is. The read_header_v() hook allows us to do that easily.

...

I have this for the moment:

/* Immutable part of header structure: keep these two sections at the * beginning, because they are enough to identify a version regardless * of metadata. */ .magic = MIGRATE_MAGIC, .version = htonl_constant(MIGRATE_VERSION), /* End of immutable part of header structure */

...which to be honest already felt like wasted effort, despite being just two comments.

...
* Handle both the headers separately from the data sections making for better symmetry between the source and target sides * Add a "compat_version" field. This will allow us to make future protocol extensions which are backwards compatible with older targets, while retaining the ability to also make breaking protocol extensions.

This establishes a minimal header with fixed representation to maintain for all future versions.

Signed-off-by: David Gibson --- migrate.c | 157 ++++++++++++++++++++++++++++++++++++++++-------------- migrate.h | 26 ++++++--- 2 files changed, 138 insertions(+), 45 deletions(-) ^^^

...is this really a good idea if we want to drop this right away? I might be missing something but I think it would more sense to focus on the "targeted" data transfer instead.

I'm seeing this as preliminary steps towards doing the targeted data transfer. For one thing I want to be able to experiment with that (let's call it v2) without breaking your v1 stuff. Even if we drop v1 before merge, I think that's useful during dev. But even without that, I think I'm going to want some sort of "header" information for v2, and this provides an easy way to hook that in. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

102

Age (days ago)

102

Last active (days ago)

List overview

Download

6 comments

2 participants

participants (2)

David Gibson
Stefano Brivio