Re: [Libguestfs] [libnbd PATCH 3/4] api: Add new nbd_stats_*() statistics counters

On 09/03/22 18:30, Richard W.M. Jones wrote: > On Fri, Sep 02, 2022 at 05:14:24PM -0500, Eric Blake wrote: >> I was trying to write a test that determined whether a failure was due >> to a short-circuit test on the client side or an actual failure >> returned by the server. Such a test is a lot easier if we can watch >> status counters increase according to how much traffic goes over the >> wire. We may add more stats later (for example, how many bytes were >> written by nbd_[aio_]pread, or even how many times we had to grab the >> lock as part of an nbd_* command); or even to reset the counters for >> determining the protocol overhead around a particular set of I/O >> options, but this is a fun start. >> >> In this patch, update an example to serve as a compile time test of >> the new API; the next patch will add a runtime test of some simple >> uses while also checking the language bindings. >> --- >> docs/libnbd.pod | 20 ++++++++ >> generator/API.ml | 78 +++++++++++++++++++++++++++++- >> lib/handle.c | 28 +++++++++++ >> examples/strict-structured-reads.c | 13 ++++- >> 4 files changed, 135 insertions(+), 4 deletions(-) >> >> diff --git a/docs/libnbd.pod b/docs/libnbd.pod >> index 7a01179a..d256cd65 100644 >> --- a/docs/libnbd.pod >> +++ b/docs/libnbd.pod >> @@ -940,6 +940,26 @@ should return C<1> on all control paths, even when handling errors >> (note that with automatic retirement, assigning into C<error> is >> pointless as there is no later API to see that value). >> >> +=head1 STATISTICS COUNTERS >> + >> +Libnbd tracks several statistics counters, useful for tracking how >> +much traffic was sent over the connection. The counters track the >> +number of bytes sent and received, as well as the number of protocol >> +chunks (a group of bytes delineated by a magic number). >> + >> + printf ("bytes: sent=%" PRIu64 " received=%" PRIu64, >> + nbd_stats_bytes_sent (nbd), nbd_stats_bytes_received (nbd)); >> + printf ("chunks: sent=%" PRIu64 " received=%" PRIu64, >> + nbd_stats_chunks_sent (nbd), nbd_stats_chunks_received (nbd)); > > You probably also want to note that if TLS is enabled these don't > relate closely to either bytes or packets sent. > >> +Note that if a command fails early at the client without needing to >> +consult the server, the counters will not be incremented; conversely, >> +the chunk counts can increase by more than one for a single API call >> +(for example, L<nbd_opt_go(3)> defaults to sending a two-option >> +sequence of C<NBD_OPT_SET_META_CONTEXT> and C<NBD_OPT_GO>, and the >> +server in turn replies with multiple C<NBD_REP_INFO> before concluding >> +with C<NBD_REP_ACK>). > > The trouble is that by writing this down, we need to keep supporting > this. If you don't write it down, but it's implicit in our tests then > we can change the API later if it turns out to be hard to support. So > my preference would be to simply delete this paragraph and similar > below ... We ought to be able to document the (originally) intended purpose of a feature such that it does not come with "support strings attached". Commit messages are good, code comments are good too, yes, but what about explicit documentation? E.g. "p2v-hacking.pod" is a great thing. Anyway I don't insist; I just thought (without realizing the "support strings attached") that exactly those paragraphs were the nicest touch ;)

So either way: Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt; Laszlo > >> =head1 SIGNALS >> >> Libnbd does not install signal handlers. It attempts to disable >> diff --git a/generator/API.ml b/generator/API.ml >> index e4e2ea0a..dbfde284 100644 >> --- a/generator/API.ml >> +++ b/generator/API.ml >> @@ -307,6 +307,70 @@ "clear_debug_callback", { >> callback was associated this does nothing."; >> }; >> >> + "stats_bytes_sent", { >> + default_call with >> + args = []; ret = RUInt64; >> + may_set_error = false; >> + shortdesc = "statistics of bytes sent over socket so far"; >> + longdesc = "\ >> +Return the number of bytes that the client has sent to the server."; >> + see_also = [Link "stats_chunks_sent"; >> + Link "stats_bytes_received"; Link "stats_chunks_received"]; >> + }; >> + >> + "stats_chunks_sent", { >> + default_call with >> + args = []; ret = RUInt64; >> + may_set_error = false; >> + shortdesc = "statistics of chunks sent over socket so far"; >> + longdesc = "\ >> +Return the number of chunks that the client has sent to the >> +server, where a chunk is a group of bytes delineated by a magic >> +number that cannot be further subdivided without breaking the >> +protocol. >> + >> +This number increments when a request is sent to the server; >> +it is unchanged for an API that fails during initial client-side >> +sanity checking (see L<nbd_set_strict_mode(3)>), and can >> +increment by more than one for APIs that wrap multiple >> +underlying requests (such as L<nbd_opt_go(3)>)."; > > ^ This one. > >> + see_also = [Link "stats_bytes_sent"; >> + Link "stats_bytes_received"; Link "stats_chunks_received"; >> + Link "set_strict_mode"]; >> + }; >> + >> + "stats_bytes_received", { >> + default_call with >> + args = []; ret = RUInt64; >> + may_set_error = false; >> + shortdesc = "statistics of bytes received over socket so far"; >> + longdesc = "\ >> +Return the number of bytes that the client has received from the server."; >> + see_also = [Link "stats_chunks_received"; >> + Link "stats_bytes_sent"; Link "stats_chunks_sent"]; >> + }; >> + >> + "stats_chunks_received", { >> + default_call with >> + args = []; ret = RUInt64; >> + may_set_error = false; >> + shortdesc = "statistics of chunks received over socket so far"; >> + longdesc = "\ >> +Return the number of chunks that the client has received from the >> +server, where a chunk is a group of bytes delineated by a magic >> +number that cannot be further subdivided without breaking the >> +protocol. >> + >> +This number increments when a reply is received from the server; >> +it is unchanged for an API that fails during initial client-side >> +sanity checking (see L<nbd_set_strict_mode(3)>), and can >> +increment by more than one when the server utilizes structured >> +replies (see L<nbd_get_structured_replies_negotiated(3)>)."; > > ^ This one. > >> + see_also = [Link "stats_bytes_received"; >> + Link "stats_bytes_sent"; Link "stats_chunks_sent"; >> + Link "get_structured_replies_negotiated"]; >> + }; >> + >> "set_handle_name", { >> default_call with >> args = [ String "handle_name" ]; ret = RErr; >> @@ -945,8 +1009,14 @@ "set_strict_mode", { >> such, when attempting to relax only one specific bit while keeping >> remaining checks at the client side, it is wiser to first call >> L<nbd_get_strict_mode(3)> and modify that value, rather than >> -blindly setting a constant value."; >> - see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; >> +blindly setting a constant value. >> + >> +When commands are not being attempted in parallel, it is possible >> +to observe whether a given command was filtered at the client >> +or triggered a server response by whether the statistics >> +counters increase (such as L<nbd_stats_bytes_sent(3)>)."; > > ^ This one. > >> + see_also = [Link "get_strict_mode"; Link "set_handshake_flags"; >> + Link "stats_bytes_sent"; Link "stats_bytes_received"]; >> }; >> >> "get_strict_mode", { >> @@ -3274,6 +3344,10 @@ let first_version = >> (* Added in 1.15.x development cycle, will be stable and supported in 1.16. *) >> "poll2", (1, 16); >> "supports_vsock", (1, 16); >> + "stats_bytes_sent", (1, 16); >> + "stats_chunks_sent", (1, 16); >> + "stats_bytes_received", (1, 16); >> + "stats_chunks_received", (1, 16); >> >> (* These calls are proposed for a future version of libnbd, but >> * have not been added to any released version so far. >> diff --git a/lib/handle.c b/lib/handle.c >> index 7fcdc8cd..d4426260 100644 >> --- a/lib/handle.c >> +++ b/lib/handle.c >> @@ -548,3 +548,31 @@ nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow) >> h->uri_allow_local_file = allow; >> return 0; >> } >> + >> +/* NB: may_set_error = false. */ >> +uint64_t >> +nbd_unlocked_stats_bytes_sent (struct nbd_handle *h) >> +{ >> + return h->bytes_sent; >> +} >> + >> +/* NB: may_set_error = false. */ >> +uint64_t >> +nbd_unlocked_stats_chunks_sent (struct nbd_handle *h) >> +{ >> + return h->chunks_sent; >> +} >> + >> +/* NB: may_set_error = false. */ >> +uint64_t >> +nbd_unlocked_stats_bytes_received (struct nbd_handle *h) >> +{ >> + return h->bytes_received; >> +} >> + >> +/* NB: may_set_error = false. */ >> +uint64_t >> +nbd_unlocked_stats_chunks_received (struct nbd_handle *h) >> +{ >> + return h->chunks_received; >> +} >> diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c >> index 6ea17006..b56a4825 100644 >> --- a/examples/strict-structured-reads.c >> +++ b/examples/strict-structured-reads.c >> @@ -4,6 +4,10 @@ >> * qemu-nbd -f $format -k $sock -r image >> * ./strict-structured-reads $sock >> * >> + * With nbdkit (but less useful until nbdkit learns to send holes): >> + * >> + * nbdkit -U- sparse-random 1G --run './strict-structured-reads "$unixsocket"' >> + * > > Is this hunk meant to be included? > >> * This will perform read randomly over the image and check that all >> * structured replies comply with the NBD spec (chunks may be out of >> * order or interleaved, but no read succeeds unless chunks cover the >> @@ -257,8 +261,11 @@ main (int argc, char *argv[]) >> exit (EXIT_FAILURE); >> } >> >> - nbd_close (nbd); >> - >> + printf ("traffic:\n"); >> + printf (" bytes sent: %10" PRIu64 "\n", nbd_stats_bytes_sent (nbd)); >> + printf (" bytes received: %10" PRIu64 "\n", nbd_stats_bytes_received (nbd)); >> + printf (" chunks sent: %10" PRIu64 "\n", nbd_stats_chunks_sent (nbd)); >> + printf (" chunks received: %10" PRIu64 "\n", nbd_stats_chunks_received (nbd)); >> printf ("totals:\n"); >> printf (" data chunks: %10d\n", total_data_chunks); >> printf (" data bytes: %10" PRId64 "\n", total_data_bytes); >> @@ -270,5 +277,7 @@ main (int argc, char *argv[]) >> printf (" bytes read: %10" PRId64 "\n", total_bytes); >> printf (" compliant: %10d\n", total_success); >> >> + nbd_close (nbd); >> + >> exit (EXIT_SUCCESS); >> } >> -- > > Rich. >