[PATCH-SERIES cluster 00/12] addressing large ipc message responses

[PATCH-SERIES cluster 00/12] addressing large ipc message responses

[Daniel Kral]

Currently, the IPC server can only send MAX_MSG_SIZE (currently, 1 MiB)
to IPC clients, i.e. users of PVE::IPCC::ipcc_send_recv(). Otherwise,
the IPC clients will fail with:

    ipcc_send_rec[13] failed: Transport endpoint is not connected

and the IPC server will log:

    [ipcs] crit: qb_ipcs_response_send: Resource temporarily unavailable
    [libqb] error: error receiving from setup sock (/dev/shm/qb-[...]/qb): Bad file descriptor (9)


I've reached the MAX_MSG_SIZE using CFS_IPC_GET_GUEST_CONFIG_PROPERTIES
while working on a patch series, which intends the pve-ha-crm to be able
to infer certain node affinity constraints from the guest configs, e.g.
guests being limited to certain nodes by their storages and devices.

While testing it by only gathering the 'efidisk0' property from 10,000
VMs with identical efidisk property strings (except the 5-digit vmid):

    efidisk0: local:10000/vm-10000-disk-0.qcow2,efitype=4m,format=qcow2,ms-cert=2023k,pre-enrolled-keys=1,size=1M

the MAX_MSG_SIZE was already exceeded (1250253 bytes > 1048576 bytes).

As noted in the patch message in patch #5, some of the current responses
are already quite large and could be exceeded in large clusters:

    For example, in a test cluster with 10,000 stopped guests, the
    response of CFS_IPC_GET_RRD_DUMP does already have 919 KiB, where
    many of the runtime values are undefined (U) and therefore the
    response size would be much longer if they were all running. Similar
    response sizes can be reached with CFS_IPC_GET_GUEST_LIST and
    CFS_IPC_GET_GUEST_CONFIG_PROPERTIES.


The first few non-RFC patches of this series do some cleanup and report
such occurrences of large IPC response messages:

PATCH 1-2   make root `make tidy` run pmxcfs' tidy target
PATCH 3-4   some prepatory cleanup
PATCH 5     report IPC responses exceeding MAX_MSG_SIZE
PATCH 6-10  some after cleanup (prepatory for #11-#12)
PATCH 11    preparation for RFC patch #12


The RFC patch #12 implements a simple mechanism to split these large IPC
messages in chunks, send them to the client and make the client
reconstruct these message chunks to the original IPC response.

This solution uses an additional request header and an additional
response header to communicate whether the client can handle multiple
message chunk responses and when to stop the client to receive messages.
See more information about the design in patch #12.



Daniel Kral (12):
  buildsys: include pmxcfs tidy target in top-level tidy target
  run make tidy
  pmxcfs: server: simplify non-negative ipc message result responses
  pmxcfs: server: simplify ipc message response logic
  pmxcfs: server: report responses exceeding the maximum message size
  pmxcfs: server: remove unnecessary local variable alignment attributes
  pmxcfs: server: move message handling to separate function
  pmxcfs: server: move message response sending to separate function
  pmxcfs: server: simplify message response size calculation
  ipcc: move the data pointer initialization up
  pmxcfs: server: do not forward qb_ipc_request_header to
    s1_msg_handle_request
  allow large ipc responses by sending the response message in chunks

 Makefile                    |   1 +
 src/Makefile                |   3 +
 src/PVE/IPCC.xs             |  89 +++++++++++++-----
 src/PVE/Makefile            |   1 +
 src/pmxcfs/cfs-ipc-common.h |  56 ++++++++++++
 src/pmxcfs/server.c         | 175 +++++++++++++++++++++++++-----------
 src/pmxcfs/status.c         |   2 +-
 7 files changed, 254 insertions(+), 73 deletions(-)
 create mode 100644 src/pmxcfs/cfs-ipc-common.h

-- 
2.47.3

[PATCH cluster 01/12] buildsys: include pmxcfs tidy target in top-level tidy target

[Daniel Kral]

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 Makefile     | 1 +
 src/Makefile | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 5fa96ab..5228a5b 100644
--- a/Makefile
+++ b/Makefile
@@ -19,6 +19,7 @@ all: $(DEB) $(DBG_DEB)
 .PHONY: tidy
 tidy:
 	git ls-files ':*.p[ml]'| xargs -n4 -P0 proxmox-perltidy
+	$(MAKE) -C src tidy
 
 $(BUILDDIR):
 	rm -rf $@ $@.tmp
diff --git a/src/Makefile b/src/Makefile
index 50dd6aa..7359660 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -10,6 +10,9 @@ all:
 install:
 	set -e; for i in $(SUBDIRS); do $(MAKE) -C $$i $@; done
 
+.PHONY: tidy
+tidy:
+	$(MAKE) -C pmxcfs tidy
 
 .PHONY: check
 check:
-- 
2.47.3

[PATCH cluster 02/12] run make tidy

[Daniel Kral]

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index 12a6c46..98ea616 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -1231,7 +1231,7 @@ static inline const char *rrd_skip_data(const char *data, int count, char separa
     return data;
 }
 
-static inline void checked_mkdir(char* path, int mode) {
+static inline void checked_mkdir(char *path, int mode) {
     if (!mkdir(path, mode) && errno != EEXIST) {
         cfs_message("could not create directory %s: %s", path, strerror(errno));
     };
-- 
2.47.3

[PATCH cluster 03/12] pmxcfs: server: simplify non-negative ipc message result responses

[Daniel Kral]

The result variable is used as the error code in the QB response header,
where libqb and PVE::IPCC expects it to be 0 for successful responses.

The request handler for CFS_IPC_GET_CONFIG is the last remaining handler
to set result to a non-negative value: it does set result to the
returned config file size in case of a successful memdb_read() request.

Therefore, set result to 0 in case of a successful read operation and
remove the now unnecessary catch-all zero assignment when handling
successful responses.

No functional changes intended.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 278e2c9..22e812c 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -251,6 +251,7 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
                 if (result > 0) {
                     g_string_append_len(outbuf, tmp, result);
                     g_free(tmp);
+                    result = 0;
                 }
             }
         }
@@ -452,7 +453,6 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
 
     if (result >= 0) {
         resp = outbuf->str;
-        result = 0;
     }
 
     int iov_len = 2;
-- 
2.47.3

[PATCH cluster 04/12] pmxcfs: server: simplify ipc message response logic

[Daniel Kral]

The response should always be empty for negative results. Simplify the
logic here to truncate the response string to remove the unnecessary
`resp` and `resp_data_len` variables.

This is done in preparation of the following patch, which reports and
discards the response data if the response data exceeds the maximum
message size allowed by the client's buffer.

No functional changes intended.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 22e812c..abe1bee 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -171,8 +171,6 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
     int32_t request_size __attribute__((aligned(8))) = req_pt->size;
     cfs_debug("process msg:%d, size:%d", request_id, request_size);
 
-    char *resp = NULL;
-
     g_string_truncate(outbuf, 0);
 
     int32_t result = -ECHRNG;
@@ -451,24 +449,23 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
 
     cfs_debug("process result %d", result);
 
-    if (result >= 0) {
-        resp = outbuf->str;
+    /* Do not send the response body in case of an error */
+    if (result < 0) {
+        g_string_truncate(outbuf, 0);
     }
 
     int iov_len = 2;
     struct iovec iov[iov_len];
     struct qb_ipc_response_header res_header;
 
-    int resp_data_len = resp ? outbuf->len : 0;
-
     res_header.id = request_id;
-    res_header.size = sizeof(res_header) + resp_data_len;
+    res_header.size = sizeof(res_header) + outbuf->len;
     res_header.error = result;
 
     iov[0].iov_base = (char *)&res_header;
     iov[0].iov_len = sizeof(res_header);
-    iov[1].iov_base = resp;
-    iov[1].iov_len = resp_data_len;
+    iov[1].iov_base = outbuf->str;
+    iov[1].iov_len = outbuf->len;
 
     ssize_t res = qb_ipcs_response_sendv(c, iov, iov_len);
     if (res < 0) {
-- 
2.47.3

[PATCH cluster 05/12] pmxcfs: server: report responses exceeding the maximum message size

[Daniel Kral]

The shared memory ring buffer has a fixed maximum buffer size of
MAX_MSG_SIZE (currently 1 MiB), which is set by the pmxcfs' libqb-based
IPC client. If the maximium buffer size is exceeded, the QB ringbuffer
will fail to allocate the chunk for the response.

qb_ipcs_response_sendv() will fail with -EAGAIN, which will be logged by
pmxcfs as a rather confusing user error:

    [ipcs] crit: qb_ipcs_response_send: Resource temporarily unavailable
    [libqb] error: error receiving from setup sock (/dev/shm/qb-[...]/qb): Bad file descriptor (9)

and makes users of $PVE::Cluster::ipcc_send_rec{,_json}() fail with:

    ipcc_send_rec[13] failed: Transport endpoint is not connected

In huge cluster setups with many nodes, storages, and many thousands of
guests, the current MAX_MSG_SIZE of 1 MiB could be exceeded.

For example, in a test cluster with 10,000 stopped guests, the response
of CFS_IPC_GET_RRD_DUMP does already have 919 KiB, where many of the
runtime values are undefined (U) and therefore the response size would
be much longer if they were all running. Similar response sizes can be
reached with CFS_IPC_GET_GUEST_LIST and
CFS_IPC_GET_GUEST_CONFIG_PROPERTIES.

Report response messages that exceed the maximum message size in the
client and server to make the issue clearer to discern from other
possible errors, e.g. an unresponsive IPC server.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index abe1bee..54cd3aa 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -449,15 +449,23 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
 
     cfs_debug("process result %d", result);
 
+    int iov_len = 2;
+    struct iovec iov[iov_len];
+    struct qb_ipc_response_header res_header;
+    int32_t max_msg_size = qb_ipcs_connection_get_buffer_size(c);
+
+    if (sizeof(res_header) + outbuf->len > max_msg_size) {
+        cfs_critical(
+            "response for id %d is too large: %ld > %d", request_id, outbuf->len, max_msg_size
+        );
+        result = -EMSGSIZE;
+    }
+
     /* Do not send the response body in case of an error */
     if (result < 0) {
         g_string_truncate(outbuf, 0);
     }
 
-    int iov_len = 2;
-    struct iovec iov[iov_len];
-    struct qb_ipc_response_header res_header;
-
     res_header.id = request_id;
     res_header.size = sizeof(res_header) + outbuf->len;
     res_header.error = result;
-- 
2.47.3

[PATCH cluster 06/12] pmxcfs: server: remove unnecessary local variable alignment attributes

[Daniel Kral]

Even though the id and size fields are aligned to an 8-byte boundary in
the struct qb_ipc_request_header, these do not have to be explicitly
aligned here in the function's stack itself.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 54cd3aa..58689fd 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -167,8 +167,8 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
         return 0;
     }
 
-    int32_t request_id __attribute__((aligned(8))) = req_pt->id;
-    int32_t request_size __attribute__((aligned(8))) = req_pt->size;
+    int32_t request_id = req_pt->id;
+    int32_t request_size = req_pt->size;
     cfs_debug("process msg:%d, size:%d", request_id, request_size);
 
     g_string_truncate(outbuf, 0);
-- 
2.47.3

[PATCH cluster 07/12] pmxcfs: server: move message handling to separate function

[Daniel Kral]

Move the handling of the different IPC request operations to a separate
function to make it more apparent, which input/output variables the
message handling part reads/writes (besides the global outbuf).

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 58689fd..c9fabb4 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -156,21 +156,9 @@ static int32_t s1_connection_closed_fn(qb_ipcs_connection_t *c) {
     return 0;
 }
 
-static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t size) {
-    struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *)data;
-
-    struct s1_context *ctx = (struct s1_context *)qb_ipcs_context_get(c);
-
-    if (!ctx) {
-        cfs_critical("qb_ipcs_context_get failed");
-        qb_ipcs_disconnect(c);
-        return 0;
-    }
-
-    int32_t request_id = req_pt->id;
-    int32_t request_size = req_pt->size;
-    cfs_debug("process msg:%d, size:%d", request_id, request_size);
-
+static int32_t s1_msg_handle_request(
+    struct s1_context *ctx, void *data, int32_t request_id, int32_t request_size
+) {
     g_string_truncate(outbuf, 0);
 
     int32_t result = -ECHRNG;
@@ -447,6 +435,25 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
         }
     }
 
+    return result;
+}
+
+static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t size) {
+    struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *)data;
+
+    struct s1_context *ctx = (struct s1_context *)qb_ipcs_context_get(c);
+
+    if (!ctx) {
+        cfs_critical("qb_ipcs_context_get failed");
+        qb_ipcs_disconnect(c);
+        return 0;
+    }
+
+    int32_t request_id = req_pt->id;
+    int32_t request_size = req_pt->size;
+    cfs_debug("process msg:%d, size:%d", request_id, request_size);
+
+    int32_t result = s1_msg_handle_request(ctx, data, request_id, request_size);
     cfs_debug("process result %d", result);
 
     int iov_len = 2;
-- 
2.47.3

[PATCH cluster 08/12] pmxcfs: server: move message response sending to separate function

[Daniel Kral]

Move the sending of IPC request responses to a separate function in
preparation of an upcoming patch, which allows sending large responses
as multiple chunks and therefore increases the complexity slightly.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index c9fabb4..0af6745 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -438,24 +438,7 @@ static int32_t s1_msg_handle_request(
     return result;
 }
 
-static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t size) {
-    struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *)data;
-
-    struct s1_context *ctx = (struct s1_context *)qb_ipcs_context_get(c);
-
-    if (!ctx) {
-        cfs_critical("qb_ipcs_context_get failed");
-        qb_ipcs_disconnect(c);
-        return 0;
-    }
-
-    int32_t request_id = req_pt->id;
-    int32_t request_size = req_pt->size;
-    cfs_debug("process msg:%d, size:%d", request_id, request_size);
-
-    int32_t result = s1_msg_handle_request(ctx, data, request_id, request_size);
-    cfs_debug("process result %d", result);
-
+static void s1_msg_send_response(qb_ipcs_connection_t *c, int32_t request_id, int32_t result) {
     int iov_len = 2;
     struct iovec iov[iov_len];
     struct qb_ipc_response_header res_header;
@@ -487,6 +470,27 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
         cfs_critical("qb_ipcs_response_send: %s", strerror(errno));
         qb_ipcs_disconnect(c);
     }
+}
+
+static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t size) {
+    struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *)data;
+
+    struct s1_context *ctx = (struct s1_context *)qb_ipcs_context_get(c);
+
+    if (!ctx) {
+        cfs_critical("qb_ipcs_context_get failed");
+        qb_ipcs_disconnect(c);
+        return 0;
+    }
+
+    int32_t request_id = req_pt->id;
+    int32_t request_size = req_pt->size;
+    cfs_debug("process msg:%d, size:%d", request_id, request_size);
+
+    int32_t result = s1_msg_handle_request(ctx, data, request_id, request_size);
+    cfs_debug("process result %d", result);
+
+    s1_msg_send_response(c, request_id, result);
 
     return 0;
 }
-- 
2.47.3

[PATCH cluster 09/12] pmxcfs: server: simplify message response size calculation

[Daniel Kral]

Calculate the full message size by the actual length of the iovecs,
which are copied over memory to remove some small code duplication.

No functional changes intended.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 0af6745..c53d4e0 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -456,15 +456,15 @@ static void s1_msg_send_response(qb_ipcs_connection_t *c, int32_t request_id, in
         g_string_truncate(outbuf, 0);
     }
 
-    res_header.id = request_id;
-    res_header.size = sizeof(res_header) + outbuf->len;
-    res_header.error = result;
-
     iov[0].iov_base = (char *)&res_header;
     iov[0].iov_len = sizeof(res_header);
     iov[1].iov_base = outbuf->str;
     iov[1].iov_len = outbuf->len;
 
+    res_header.id = request_id;
+    res_header.size = iov[0].iov_len + iov[1].iov_len;
+    res_header.error = result;
+
     ssize_t res = qb_ipcs_response_sendv(c, iov, iov_len);
     if (res < 0) {
         cfs_critical("qb_ipcs_response_send: %s", strerror(errno));
-- 
2.47.3

[PATCH cluster 10/12] ipcc: move the data pointer initialization up

[Daniel Kral]

The data pointer does not have to be reassigned in case of a recached
connection.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/IPCC.xs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/IPCC.xs b/src/PVE/IPCC.xs
index 4661df0..626971c 100644
--- a/src/PVE/IPCC.xs
+++ b/src/PVE/IPCC.xs
@@ -97,6 +97,11 @@ SV * data;
 PROTOTYPE: $;$
 CODE:
 {
+	size_t len = 0;
+	char *dataptr = NULL;
+	if (data && SvPOK(data))
+		dataptr = SvPV(data, len);
+
 	uint8_t retried_cache_connection = 0;
 	pid_t cpid = getpid();
 
@@ -117,11 +122,6 @@ recache_connection:
 		conn_pid = cpid;
 	}
 
-	size_t len = 0;
-	char *dataptr = NULL;
-	if (data && SvPOK(data))
-		dataptr = SvPV(data, len);
-
 	int iov_len = 2;
 	struct iovec iov[iov_len];
 
-- 
2.47.3

[PATCH cluster 11/12] pmxcfs: server: do not forward qb_ipc_request_header to s1_msg_handle_request

[Daniel Kral]

request_id and request_size is already passed to
s1_msg_handle_request(), which therefore do not need the information
from struct qb_ipc_request_header passed on to them.

This is done in preparation of an upcoming patch, where the IPC server
has to handle two different types of request headers to negotiate what
responses the client can handle. The request headers do have different
sizes and therefore an union cannot be used here.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/pmxcfs/server.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index c53d4e0..46ad381 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -56,18 +56,15 @@ static GCond server_stopped_cond;
 static GMutex server_started_mutex;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     char name[256];
 } cfs_status_update_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     char name[256];
     char nodename[256];
 } cfs_status_get_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     uint8_t priority;
     uint8_t ident_len;
     uint8_t tag_len;
@@ -75,7 +72,6 @@ typedef struct {
 } cfs_log_msg_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     uint32_t max_entries;
     uint32_t res1;
     uint32_t res2;
@@ -83,21 +79,18 @@ typedef struct {
 } cfs_log_get_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     uint32_t vmid;
     char property[];
 } cfs_guest_config_propery_get_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
     uint32_t vmid;
     uint8_t num_props;
     char props[]; /* list of \0 terminated properties */
 } cfs_guest_config_properties_get_request_header_t;
 
 typedef struct {
-    struct qb_ipc_request_header req_header;
-    char token[];
+    char *token;
 } cfs_verify_token_request_header_t;
 
 struct s1_context {
@@ -164,7 +157,7 @@ static int32_t s1_msg_handle_request(
     int32_t result = -ECHRNG;
     if (request_id == CFS_IPC_GET_FS_VERSION) {
 
-        if (request_size != sizeof(struct qb_ipc_request_header)) {
+        if (request_size != 0) {
             result = -EINVAL;
         } else {
             result = cfs_create_version_msg(outbuf);
@@ -172,7 +165,7 @@ static int32_t s1_msg_handle_request(
 
     } else if (request_id == CFS_IPC_GET_CLUSTER_INFO) {
 
-        if (request_size != sizeof(struct qb_ipc_request_header)) {
+        if (request_size != 0) {
             result = -EINVAL;
         } else {
             result = cfs_create_memberlist_msg(outbuf);
@@ -180,7 +173,7 @@ static int32_t s1_msg_handle_request(
 
     } else if (request_id == CFS_IPC_GET_GUEST_LIST) {
 
-        if (request_size != sizeof(struct qb_ipc_request_header)) {
+        if (request_size != 0) {
             result = -EINVAL;
         } else {
             result = cfs_create_vmlist_msg(outbuf);
@@ -220,14 +213,14 @@ static int32_t s1_msg_handle_request(
         }
     } else if (request_id == CFS_IPC_GET_CONFIG) {
 
-        int pathlen = request_size - sizeof(struct qb_ipc_request_header);
+        int pathlen = request_size;
 
         if (pathlen <= 0) {
             result = -EINVAL;
         } else {
             /* make sure path is 0 terminated */
             ((char *)data)[request_size - 1] = 0;
-            char *path = (char *)data + sizeof(struct qb_ipc_request_header);
+            char *path = (char *)data;
 
             if (ctx->read_only && path_is_private(path)) {
                 result = -EPERM;
@@ -294,7 +287,7 @@ static int32_t s1_msg_handle_request(
         }
     } else if (request_id == CFS_IPC_GET_RRD_DUMP) {
 
-        if (request_size != sizeof(struct qb_ipc_request_header)) {
+        if (request_size != 0) {
             result = -EINVAL;
         } else {
             cfs_rrd_dump(outbuf);
@@ -484,7 +477,8 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
     }
 
     int32_t request_id = req_pt->id;
-    int32_t request_size = req_pt->size;
+    int32_t request_size = req_pt->size - sizeof(struct qb_ipc_request_header);
+    data = (uint8_t *)data + sizeof(struct qb_ipc_request_header);
     cfs_debug("process msg:%d, size:%d", request_id, request_size);
 
     int32_t result = s1_msg_handle_request(ctx, data, request_id, request_size);
-- 
2.47.3

[RFC PATCH cluster 12/12] allow large ipc responses by sending the response message in chunks

[Daniel Kral]

If the response to a pmxcfs IPC request is larger than the client's
buffer size of MAX_MSG_SIZE (currently, 1 MiB), the IPC server responds
with an -EMSGSIZE. This can happen for some IPC requests in huge cluster
setups, such as CFS_IPC_GET_GUEST_LIST, CFS_IPC_GET_RRD_DUMP, and
CFS_IPC_GET_GUEST_CONFIG_PROPERTIES.

Allow the IPC client to receive and the IPC server to respond with
larger messages by sending the response body as several chunks to the
IPC client, which reconstructs them to the original response message.

This introduces a new request header, which makes the IPC server aware
that the IPC client can handle receiving multiple message chunks, and a
new response header, which tells the IPC client the response state.
Currently, the response state tells the IPC client when to expect more
message chunks (CFS_IPC_RES_FLAG_CHUNK) and the last message chunk
(CFS_IPC_RES_FLAG_CHUNK_LAST).

The new request header is introduced as a form to indicate which
response types the IPC client can handle from the IPC server. As an
upgrade of the pve-cluster package will not restart all PVE daemons,
which still use the old PVE::IPCC and therefore will neither send the
new request header nor handle the new response header, the IPC server
must respond without the new response header and send a maximum of one
message.

The ms_timeout variable has been replaced by the default timeout value
that corresponds to -1 with qb_ipcc_sendv_recv(), which is not handled
the same with qb_ipcc_recv().

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
I've found that if only the pve-cluster package is upgraded, which
contains the `pmxcfs` binary (server) and `PVE::IPCC` module (client),
then this doesn't (always?) trigger e.g. pve-firewall's and
pve-ha-manager's postinst, which reloads/restarts the PVE daemon.
However, it seems like the other *pve-cluster* packages trigger that.

For safety, I have implemented the request header to handle these kind
of scenarios, so we ensure that the IPC server will never respond with
something the IPC client cannot handle. This can be reused if there's
any other changes to the pmxcfs IPC message protocol, e.g. compressed
messages, etc.

However, feedback is always appreciated about such designs.


 src/PVE/IPCC.xs             |  79 +++++++++++++++++++++-----
 src/PVE/Makefile            |   1 +
 src/pmxcfs/cfs-ipc-common.h |  56 ++++++++++++++++++
 src/pmxcfs/server.c         | 109 ++++++++++++++++++++++++++++--------
 4 files changed, 206 insertions(+), 39 deletions(-)
 create mode 100644 src/pmxcfs/cfs-ipc-common.h

diff --git a/src/PVE/IPCC.xs b/src/PVE/IPCC.xs
index 626971c..b9da1b6 100644
--- a/src/PVE/IPCC.xs
+++ b/src/PVE/IPCC.xs
@@ -29,6 +29,8 @@
 #include <qb/qblog.h>
 #include <qb/qbipcc.h>
 
+#include "cfs-ipc-common.h"
+
 #define RESTART_FLAG_FILE "/run/pve-cluster/cfs-restart-flag"
 #define RESTART_GRACE_PERIOD 10
 
@@ -37,6 +39,8 @@
 #define PCS_SERVICE1 1
 #define MAX_MSG_SIZE (8192*128)
 
+#define MAX_MSG_RECV_WAIT_MS 2000
+
 static qb_ipcc_connection_t *conn;
 static pid_t conn_pid;
 
@@ -125,18 +129,19 @@ recache_connection:
 	int iov_len = 2;
 	struct iovec iov[iov_len];
 
-	struct qb_ipc_request_header req_header;
+	struct cfs_ipc_request_header_t req_header;
 
-	req_header.id = msgid;
-	req_header.size = sizeof(req_header) + len;
+	req_header.qb_header.id = msgid;
+	req_header.qb_header.size = sizeof(req_header) + len;
+	req_header.magic = CFS_IPC_MAGIC;
+	req_header.version = CFS_IPC_CURRENT_VERSION;
 
 	iov[0].iov_base = (char *)&req_header;
 	iov[0].iov_len = sizeof(req_header);
 	iov[1].iov_base = dataptr;
 	iov[1].iov_len = len;
 
-	int32_t ms_timeout = -1; // fixme:     
-	int res = qb_ipcc_sendv_recv(conn, iov, iov_len, ipcbuffer, sizeof(ipcbuffer), ms_timeout);
+	int res = qb_ipcc_sendv(conn, iov, iov_len);
 	if (res < 0) {
 		qb_ipcc_disconnect(conn);
 		conn = NULL;
@@ -146,25 +151,69 @@ recache_connection:
 			goto recache_connection;
 		}
 		errno = -res;
-		XSRETURN_UNDEF;
+		goto out_err;
 	}
 
-	struct qb_ipc_response_header *res_header;
+	SV *result = newSVpvs("");
+	struct cfs_ipc_response_header_t *res_header = (struct cfs_ipc_response_header_t *)ipcbuffer;
+	struct qb_ipc_response_header *qb_header = &res_header->qb_header;
 
-	res_header = (struct qb_ipc_response_header *)ipcbuffer;
-	int dsize = res_header->size - sizeof(struct qb_ipc_response_header);
+	int index = 0;
+	do {
+		res = qb_ipcc_recv(conn, ipcbuffer, sizeof(ipcbuffer), MAX_MSG_RECV_WAIT_MS);
+		if (res < 0) {
+			errno = -res;
+			goto out_drop_conn;
+		}
 
-	if (res_header->error < 0) {
-		errno = -res_header->error;
-		XSRETURN_UNDEF;
+		if (qb_header->id != msgid ||
+		    qb_header->size != res ||
+		    res_header->index != index) {
+			errno = -EBADMSG;
+			goto out_drop_conn;
+		}
+
+		int dsize = qb_header->size - sizeof(*res_header);
+
+		sv_catpvn(result, ipcbuffer + sizeof(*res_header), dsize);
+
+		if (res_header->flags & CFS_IPC_RES_FLAG_CHUNK_LAST) {
+			/* Either the last part of a multi-message response or
+			 * a standalone message response. There are no more
+			 * incoming message chunks to expect. */
+			break;
+		} else if (res_header->flags & CFS_IPC_RES_FLAG_CHUNK) {
+			/* An intermediate part of a multi-message response.
+			 * More incoming message chunks are expected. */
+			index++;
+			continue;
+		} else {
+			/* This should not happen */
+			errno = -EPROTO;
+			goto out_drop_conn;
+		}
+	} while (1);
+
+	if (qb_header->error < 0) {
+		errno = -qb_header->error;
+		goto out_drop_result;
 	} else {
 		errno = 0;
-		if (dsize > 0) {
-			RETVAL = newSVpv(ipcbuffer + sizeof(struct qb_ipc_response_header), dsize);
+		if (SvCUR(result) > 0) {
+			RETVAL = result;
+			goto done;
 		} else {
-			XSRETURN_UNDEF;
+			goto out_drop_result;
 		}
 	}
+out_drop_conn:
+	qb_ipcc_disconnect(conn);
+	conn = NULL;
+out_drop_result:
+	SvREFCNT_dec(result);
+out_err:
+	XSRETURN_UNDEF;
+done:
 }
 OUTPUT: RETVAL
 
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index 0ccd2f8..636f494 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -45,6 +45,7 @@ CC=gcc
 CFLAGS += -fPIC -Wl,-z,relro -Wall -Werror -Wno-strict-aliasing -g -O2 -shared
 CFLAGS += $(shell pkg-config --cflags libqb)
 CFLAGS += $(shell perl -MExtUtils::Embed -e perl_inc)
+CFLAGS += -I../pmxcfs
 LDFLAGS = $(shell pkg-config --libs libqb)
 
 .c.o:
diff --git a/src/pmxcfs/cfs-ipc-common.h b/src/pmxcfs/cfs-ipc-common.h
new file mode 100644
index 0000000..5c9c226
--- /dev/null
+++ b/src/pmxcfs/cfs-ipc-common.h
@@ -0,0 +1,56 @@
+/*
+  Copyright (C) 2026 Proxmox Server Solutions GmbH
+
+  This program is free software: you can redistribute it and/or modify
+  it under the terms of the GNU Affero General Public License as published by
+  the Free Software Foundation, either version 3 of the License, or
+  (at your option) any later version.
+
+  This program is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU Affero General Public License for more details.
+
+  You should have received a copy of the GNU Affero General Public License
+  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+  Author: Daniel Kral <d.kral@proxmox.com>
+
+*/
+
+#ifndef _PVE_IPC_COMMON_H_
+#define _PVE_IPC_COMMON_H_
+
+#include <qb/qbipc_common.h>
+
+#define CFS_IPC_MAGIC 0xBC24
+
+#define CFS_IPC_VERSION_NO_HEADER_SUPPORT 0
+
+#define CFS_IPC_MIN_VERSION_HEADER_SUPPORT 1
+#define CFS_IPC_MIN_VERSION_CHUNK_SUPPORT CFS_IPC_MIN_VERSION_HEADER_SUPPORT
+
+#define CFS_IPC_CURRENT_VERSION 1
+
+struct cfs_ipc_request_header_t {
+    struct qb_ipc_request_header qb_header;
+    uint32_t magic __attribute__((aligned(8)));
+    uint32_t version __attribute__((aligned(8)));
+} __attribute__((aligned(8)));
+
+enum cfs_ipc_response_flags {
+    /* An intermediate response message chunk. Receivers should expect to
+     * receive more messages afterwards. */
+    CFS_IPC_RES_FLAG_CHUNK = 1 << 1,
+    /* A standalone response message or the last response message chunk.
+     * Receivers can expect to not receive any message afterwards. */
+    CFS_IPC_RES_FLAG_CHUNK_LAST = 1 << 2,
+};
+
+struct cfs_ipc_response_header_t {
+    struct qb_ipc_response_header qb_header;
+    uint32_t flags __attribute__((aligned(8)));
+    uint32_t index __attribute__((aligned(8)));
+} __attribute__((aligned(8)));
+
+#endif /* _PVE_IPC_COMMON_H_ */
diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c
index 46ad381..839327c 100644
--- a/src/pmxcfs/server.c
+++ b/src/pmxcfs/server.c
@@ -37,6 +37,7 @@
 
 #include <glib.h>
 
+#include "cfs-ipc-common.h"
 #include "cfs-ipc-ops.h"
 #include "cfs-utils.h"
 #include "logger.h"
@@ -431,17 +432,31 @@ static int32_t s1_msg_handle_request(
     return result;
 }
 
-static void s1_msg_send_response(qb_ipcs_connection_t *c, int32_t request_id, int32_t result) {
+static void s1_msg_send_response(
+    qb_ipcs_connection_t *c, int32_t request_id, uint32_t request_version, int32_t result
+) {
     int iov_len = 2;
     struct iovec iov[iov_len];
-    struct qb_ipc_response_header res_header;
+    struct cfs_ipc_response_header_t res_header;
     int32_t max_msg_size = qb_ipcs_connection_get_buffer_size(c);
 
-    if (sizeof(res_header) + outbuf->len > max_msg_size) {
-        cfs_critical(
-            "response for id %d is too large: %ld > %d", request_id, outbuf->len, max_msg_size
-        );
-        result = -EMSGSIZE;
+    /* Only send the extended response header if the client supports it */
+    if (request_version >= CFS_IPC_MIN_VERSION_HEADER_SUPPORT) {
+        iov[0].iov_base = &res_header;
+        iov[0].iov_len = sizeof(res_header);
+    } else {
+        /* Otherwise send only the QB response header... */
+        iov[0].iov_base = &res_header.qb_header;
+        iov[0].iov_len = sizeof(res_header.qb_header);
+
+        /* ...and restrict response length to the maximum buffer size as the
+         * client doesn't support receiving message chunks. */
+        if (sizeof(res_header) + outbuf->len > max_msg_size) {
+            cfs_critical(
+                "response for id %d is too large: %ld > %d", request_id, outbuf->len, max_msg_size
+            );
+            result = -EMSGSIZE;
+        }
     }
 
     /* Do not send the response body in case of an error */
@@ -449,24 +464,58 @@ static void s1_msg_send_response(qb_ipcs_connection_t *c, int32_t request_id, in
         g_string_truncate(outbuf, 0);
     }
 
-    iov[0].iov_base = (char *)&res_header;
-    iov[0].iov_len = sizeof(res_header);
-    iov[1].iov_base = outbuf->str;
-    iov[1].iov_len = outbuf->len;
+    res_header.qb_header.id = request_id;
+    res_header.qb_header.error = result;
+    res_header.flags = 0;
 
-    res_header.id = request_id;
-    res_header.size = iov[0].iov_len + iov[1].iov_len;
-    res_header.error = result;
+    int index = 0;
+    size_t msg_offset, msg_size;
+    size_t max_chunk_size = max_msg_size - iov[0].iov_len;
+    do {
+        res_header.index = index;
 
-    ssize_t res = qb_ipcs_response_sendv(c, iov, iov_len);
-    if (res < 0) {
-        cfs_critical("qb_ipcs_response_send: %s", strerror(errno));
-        qb_ipcs_disconnect(c);
-    }
+        msg_offset = max_chunk_size * index;
+        msg_size = outbuf->len - msg_offset;
+
+        if (msg_size > max_chunk_size) {
+            msg_size = max_chunk_size;
+            res_header.flags |= CFS_IPC_RES_FLAG_CHUNK;
+        } else {
+            res_header.flags |= CFS_IPC_RES_FLAG_CHUNK_LAST;
+        }
+
+        iov[1].iov_base = outbuf->str + msg_offset;
+        iov[1].iov_len = msg_size;
+
+        res_header.qb_header.size = iov[0].iov_len + iov[1].iov_len;
+
+        ssize_t res = qb_ipcs_response_sendv(c, iov, iov_len);
+        if (res == -EAGAIN) {
+            /* The client is busy (e.g. the previous message chunk hasn't been
+             * read yet). Try to send the same message again after some small
+             * backoff time */
+            cfs_debug("msg %d chunk %d send response is -EAGAIN, try again", request_id, index);
+            usleep(1000);
+            continue;
+        } else if (res < 0) {
+            cfs_debug("msg %d chunk %d send response is %ld, abort", request_id, index, res);
+            cfs_critical("qb_ipcs_response_send: %s", strerror(errno));
+            qb_ipcs_disconnect(c);
+            break;
+        } else {
+            cfs_debug("msg %d chunk %d with %ld bytes sent successfully", request_id, index, res);
+
+            if (res_header.flags & CFS_IPC_RES_FLAG_CHUNK_LAST) {
+                break;
+            }
+
+            index++;
+        }
+    } while (1);
 }
 
 static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t size) {
-    struct qb_ipc_request_header *req_pt = (struct qb_ipc_request_header *)data;
+    struct cfs_ipc_request_header_t *req_pt = (struct cfs_ipc_request_header_t *)data;
 
     struct s1_context *ctx = (struct s1_context *)qb_ipcs_context_get(c);
 
@@ -476,15 +525,27 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz
         return 0;
     }
 
-    int32_t request_id = req_pt->id;
-    int32_t request_size = req_pt->size - sizeof(struct qb_ipc_request_header);
-    data = (uint8_t *)data + sizeof(struct qb_ipc_request_header);
+    int32_t request_id = req_pt->qb_header.id;
+    int32_t request_size = req_pt->qb_header.size;
+    uint32_t request_version = CFS_IPC_VERSION_NO_HEADER_SUPPORT;
     cfs_debug("process msg:%d, size:%d", request_id, request_size);
 
+    /* There might be older IPC clients, which do not send the new, versioned
+     * request header yet, so we need to handle the request data here
+     * differently since the request headers have different fields and sizes. */
+    if (request_size >= sizeof(struct cfs_ipc_request_header_t) && req_pt->magic == CFS_IPC_MAGIC) {
+        data = (uint8_t *)data + sizeof(struct cfs_ipc_request_header_t);
+        request_size -= sizeof(struct cfs_ipc_request_header_t);
+        request_version = req_pt->version;
+    } else {
+        data = (uint8_t *)data + sizeof(struct qb_ipc_request_header);
+        request_size -= sizeof(struct qb_ipc_request_header);
+    }
+
     int32_t result = s1_msg_handle_request(ctx, data, request_id, request_size);
     cfs_debug("process result %d", result);
 
-    s1_msg_send_response(c, request_id, result);
+    s1_msg_send_response(c, request_id, request_version, result);
 
     return 0;
 }
-- 
2.47.3