sahlberg / libnfs

NFS client library
Other
510 stars 200 forks source link

Implementing mountstats like telemetry support in libnfs #490

Closed linuxsmiths closed 3 weeks ago

linuxsmiths commented 4 weeks ago

Hi @sahlberg, I'd like to add mountstats like stats to my application using libnfs.

READ: 2153647 ops (99%) avg bytes sent per op: 160 avg bytes received per op: 111522 backlog wait: 0.334419 RTT: 202.121215 total execute time: 202.464851 (milliseconds) WRITE: 11966 ops (0%) avg bytes sent per op: 1048681 avg bytes received per op: 160 backlog wait: 47487.649173 RTT: 175.793498 total execute time: 47663.462393 (milliseconds)

If you see it prints "avg bytes sent/received per op". Kernel NFS client adds this to /proc/self/mountstats from where mountstatss reads it.

What do you think is a good way to achieve this?

For the req direction, it's easy since rpcnfs3*_task() returns the pdu and this information can be obtained from pdu->zdr->pos. We just have to export some function to do that.

For the resp direction we have to somehow pass it in the callback. For that I'm thinking it'll be better to pass the rpc_pdu itself as a cb parameter.

typedef void (rpc_cb)(struct rpc_context rpc, int status, void data, __struct rpc_pdu pdu,__ void *private_data);

and then the user can use the same function to find the response size (of course for reads he has to add the data length, like he has to do it for writes for requests).

Passing pdu to the cb allows us flexibility to pass more PDU specific information to the user, in future.

What do you think?

linuxsmiths commented 4 weeks ago

I just checked and I see that the zdr used for response is a temp variable in rpc_process_pdu() and not the pdu->zdr. Yet to check it, but maybe we can add zdr_out to the PDU? Or I see that we do copy the local zdr to pdu->zdr but that's being done only for zero copy read, but I guess we can do it for all cases. But one downside I can see is that if we modify the same pdu->zdr in the rpc_process_pdu() for reply then we can modify it while user may be reading it, so we need a separate zdr_out.

linuxsmiths commented 4 weeks ago

Looking further, we don't even need to pass pdu to the cb (else it requires changes in many places). rpc_context has the pdu and caller can get that inside the callback.

linuxsmiths commented 4 weeks ago

Created following PR, pls check

https://github.com/sahlberg/libnfs/pull/491

sahlberg commented 3 weeks ago

Please see the stats-cb callback I added to master. This will allow you to collect this type of data in your application.