7:12 a.m.
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>
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.