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&gt; --- 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) {

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&gt; >> --- >>   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)