On 2/22/24 21:22, Vincent Vanlaer wrote:
> 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@xxxxxxxxxxx>
>>> ---
>>> 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)
Perfect! This was the piece I was missing. Thanks.
Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
and merged. Congratulations on your first libvirt contribution!
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
