Re: [PATCH] daemon: fix wrong request count for sparse stream
On 22/02/2024 16:22, Michal Prívozník wrote:
On 2/19/24 23:24, Vincent Vanlaer wrote:
> Similar to when actual data is being written to the stream, it is
> necessary to acknowledge handling of the client request when a hole is
> encountered. This is done later in daemonStreamHandleWrite by sending a
> fake zero-length reply if the status variable is set to
> VIR_STREAM_CONTINUE. It seems that setting status from the message
> header was missed for holes in the introduction of the sparse stream
> feature.
>
> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa(a)volkihar.be>
> ---
> src/remote/remote_daemon_stream.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
> index 1a89ff822c..453728a66b 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -747,6 +747,7 @@ daemonStreamHandleWrite(virNetServerClient *client,
> * Otherwise just carry on with processing stream
> * data. */
> ret = daemonStreamHandleHole(client, stream, msg);
> + status = msg->header.status;
> } else if (msg->header.type == VIR_NET_STREAM) {
> status = msg->header.status;
> switch (status) {
I'm wondering why is this needed. I mean - is there a bug and what are
the steps to reproduce? It's been a while since I touched this part of
the codebase.
Michal
What turns this into a visible bug is that libvirt tracks how many
requests from a certain connection are being executed, to prevent one
client from occupying all threads of the libvirt daemon (this is
configurable by the max_client_requests option). From what I can gather,
libvirt considers a request handled when a reply is sent to the client.
In the case of streams, no replies need to be sent to the client.
Instead, an fake reply is sent just for bookkeeping later in
daemonStreamHandleWrite:
/* 'CONTINUE' messages don't send a reply (unless
error
* occurred), so to release the 'msg' object we need to
* send a fake zero-length reply. Nothing actually gets
* onto the wire, but this causes the client to reset
* its active request count / throttling
*/
if (status == VIR_NET_CONTINUE) {
virNetMessageClear(msg);
msg->header.type = VIR_NET_REPLY;
if (virNetServerClientSendMessage(client, msg) < 0) {
virNetMessageFree(msg);
virNetServerClientImmediateClose(client);
return -1;
}
}
At the start, "status" is set to "VIR_NET_OK", which in the current
version of the code means that if a hole is received, no fake reply is
sent back. If this happens enough for a certain connection (the default
limit of max_client_requests is 5), you get the following warning:
virNetServerClientDispatchRead:1266 : Client hit max requests limit
50. This may result in keep-alive timeouts. Consider tuning the max_client_requests server
parameter
At this point your connection is effectively dead, as libvirt will no
longer process requests, even those unrelated to the stream.
This patch fixes that by actually updating "status" in the case a hole
is received.
The steps to reproduce this bug would be to try to call
virStreamSendHole in a loop, and notice that at some point the
connection goes stale. Here is some C code that should reproduce the bug
(provided you have pool with the name images, and no volume test in it):
#include <stdio.h>
#include <libvirt/libvirt.h>
void main() {
virConnectPtr conn = virConnectOpen("qemu:///system");
virStoragePoolPtr pool = virStoragePoolLookupByName(conn, "images");
const char *vol_xml =
"<volume><name>test</name><capacity>1048576</capacity><target><format
type='qcow2'/></target></volume>";
virStorageVolPtr vol = virStorageVolCreateXML(pool, vol_xml, 0);
virStreamPtr stream = virStreamNew(conn, 0);
virStorageVolUpload(vol, stream, 0, 1024 * 1024,
VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM);
int i = 0;
while (1) {
printf("Sending hole %d\n", i);
i++;
virStreamSendHole(stream, 1, 0);
}
}
With the bug present, this will hang relatively quickly (after 120
iterations on my system with max_client_requests set to 50, the
difference is presumably due to the socket buffers)
Cheers,
Vincent