search

sahlberg / libnfs

NFS client library
Other
510 stars 200 forks source link

Added (more) TCP and RPC resiliency. #458

Closed linuxsmiths closed 4 months ago

linuxsmiths commented 4 months ago

This change adds the following resiliency to libnfs clients.

  1. Set TCP keepalive to recognize servers that silently go away or stop responding to TCP.
  2. Add RPC timeout and retransmit support. Added following two mount options to control RPC retransmit behaviour:
    • timeo=< int >
    • retrans=< int > As we can see these are deliberately named similar to the Linux NFS mount options for ease of use/understanding.

Testing done: Tested by blocking TCP usign iptables to induce retransmit and make sure data xfer is valid. Various stress runs while constantly killing/blocking connections.

sahlberg commented 4 months ago

Causal first review looks mostly good. I will review properly tonight or tomorrow.

One small thing that failed to spot earlier is that we shouldn't assume that stderr is available. Can you change the LOG* functions to instead log to a file descriptor that the application sppecifies through rpc|nfs_set_log_fd() or something. Default to -1 to indicate "no logging".

linuxsmiths commented 4 months ago

Causal first review looks mostly good. I will review properly tonight or tomorrow.

One small thing that failed to spot earlier is that we shouldn't assume that stderr is available. Can you change the LOG* functions to instead log to a file descriptor that the application sppecifies through rpc|nfs_set_log_fd() or something. Default to -1 to indicate "no logging".

Since RPC_LOG() was logging to stderr I made LOG() (and earlier TLS_LOG) also log to that. In general I think logging needs more thought. How about we tackle that in a separate change. Since we are a library, it's best to use the logger provided by the user, f.e., user may want to add timestamp and/or some tag to the logs. Something like how gnutls does it - it asks for the logging functions and then calls them. f.e., gnutls_global_set_log_function(), gnutls_global_set_audit_log_function()

sahlberg commented 4 months ago

On Mon, 29 Apr 2024 at 16:48, linuxsmiths @.***> wrote:

Causal first review looks mostly good. I will review properly tonight or tomorrow.

One small thing that failed to spot earlier is that we shouldn't assume that stderr is available. Can you change the LOG* functions to instead log to a file descriptor that the application sppecifies through rpc|nfs_set_log_fd() or something. Default to -1 to indicate "no logging".

Since RPC_LOG() was logging to stderr I made LOG() (and earlier TLS_LOG) also log to that. In general I think logging needs more thought. How about we tackle that in a separate change. Since we are a library, it's best to use the logger provided by the user, f.e., user may want to add timestamp and/or some tag to the logs. Something like how gnutls does it - it asks for the logging functions and then calls them. f.e., gnutls_global_set_log_function(), gnutls_global_set_audit_log_function()

Agree. Lets handle logging as a separate issue.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

sahlberg commented 4 months ago

Merged, thanks