Re: [PATCH] daemon: fix wrong request count for sparse stream

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)

Cheers,
Vincent
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux