Tirpc and PORTMAP for communitarian with rpc.statd
Hello All, I have two questions related to NLM and rpc.statd. Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any 1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default? 2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now. Is this correct way to handle this? The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++-- 1 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true; + LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp"); if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void) void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false; if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name); PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) } if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1 Thanks & Regards, Suhrud
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
That's odd that a flag named PORTMAP would change from using tcp to udp...
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Hmm, I need to look into what the implications of rpc.statd restarting are. If it at all looks like a server reboot then we should do the appropriate thing inside Ganesha. If it's transparent, then yes, recreating the connection is good. Frank
Is this correct way to handle this? The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Discussion inline. On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working? Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Dan, nsm_disconnect() is disconnecting every time as opposed to only when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism. Regards, Malahal. PS: I just looked at this nsm code for a customer issue I posted on IRC yesterday! Thank you Suhrud. On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though. Frank, any ideas? Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com> wrote:
Dan, nsm_disconnect() is disconnecting every time as opposed to only when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism.
Regards, Malahal. PS: I just looked at this nsm code for a customer issue I posted on IRC yesterday! Thank you Suhrud.
On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though.
Frank, any ideas?
Hmm, that should work. How expensive is it to do the connect/disconnect for each nsm_monitor call? Man, if we didn't have rpc.statd as a separate process... Ganesha would just write the records and receive the SM_NOTIFY from clients. I don't know if we have to worry about other consumers of rpc.statd at all, if so, we just need to receive SM_MON and SM_UNMON from localhost and make the callback RPC call if a monitored client sends SM_NOTIFY. Meanwhile our monitoring would all be handled by function calls and no reliance on RPC. Frank
Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com> wrote:
Dan, nsm_disconnect() is disconnecting every time as opposed to only when
nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism.
Regards, Malahal. PS: I just looked at this nsm code for a customer issue I posted on IRC
yesterday! Thank you Suhrud.
On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com>
wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection + "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
nsm_monitor and nsm_unmonitor use a single lock to serialize everything. I am not sure what problem would it cause. On Wed, Oct 24, 2018 at 7:25 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though.
Frank, any ideas?
Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com> wrote:
Dan, nsm_disconnect() is disconnecting every time as opposed to only
when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism.
Regards, Malahal. PS: I just looked at this nsm code for a customer issue I posted on IRC
yesterday! Thank you Suhrud.
On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com>
wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS,
"tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s",
host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
It depends on how the statd is configured. If statd send notifications on restart and Ganesha is not restarted, then there will be inconsistency. Ganesha will think the client is monitored but statd will not have the client monitored as sm-notify will clear it. If statd is configured not to send notifications on its own, then statd restart is NOOP, we just need to reconnect. On Thu, Oct 25, 2018 at 9:31 AM Malahal Naineni <malahal@gmail.com> wrote:
nsm_monitor and nsm_unmonitor use a single lock to serialize everything. I am not sure what problem would it cause.
On Wed, Oct 24, 2018 at 7:25 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though.
Frank, any ideas?
Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com> wrote:
Dan, nsm_disconnect() is disconnecting every time as opposed to only when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism.
Regards, Malahal. PS: I just looked at this nsm code for a customer issue I posted on IRC yesterday! Thank you Suhrud.
On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
You're right, I'd missed that. So the only issues left are keeping nsm_count correct, and making sure that statd behaves correctly when we disconnect and reconnect. I think I'd prefer the version with a force argument for the error cases. That way, under normal circumstances, things stay the same. Daniel On 10/25/2018 12:00 AM, Malahal Naineni wrote:
nsm_monitor and nsm_unmonitor use a single lock to serialize everything. I am not sure what problem would it cause.
On Wed, Oct 24, 2018 at 7:25 PM Daniel Gryniewicz <dang@redhat.com <mailto:dang@redhat.com>> wrote:
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though.
Frank, any ideas?
Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com <mailto:malahal@gmail.com>> wrote: > > Dan, nsm_disconnect() is disconnecting every time as opposed to only when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism. > > Regards, Malahal. > PS: I just looked at this nsm code for a customer issue I posted on IRC yesterday! Thank you Suhrud. > > On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com <mailto:dang@redhat.com>> wrote: >> >> Discussion inline. >> >> On 10/23/2018 07:58 AM, Suhrud Patankar wrote: >> > Hello All, >> > >> > I have two questions related to NLM and rpc.statd. >> > >> > Setup: >> > Ganesha version: V2.5.0.4 >> > CentOS Linux release 7.5.1804 >> > FSAL – Any >> > >> > 1. By default libntirpc has -DPORTMAP build flag enabled. As I >> > understand this will use UDP to communicate with rpc.statd. With this >> > Ganesha fails to monitor the NLM clients and hence lock requests fail. >> > The call to clnt_tp_ncreate_timed() fails in >> > clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as >> > sizeof(cu->cu_raddr) is 128. >> > If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to >> > connect to rpc.statd and everything works as expected. >> > Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is >> > this flag enabled by default? >> >> This isn't related to UDP vs. TCP. The issue is that one address buffer >> passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit >> into an IPv4 buffer. It's definitely a bug, but I don't see exactly how >> this is happening. Can you give a more detailed explanation, with >> callpaths to get there? >> >> > >> > 2. Case: rpc.statd service restarts. Ganesha is not restarted. >> > nsm_connect() is done only once. On failure to connect to rpc.statd, >> > we don’t retry. This causes the NLM lock request to fail with “No >> > lock” error. >> > Is it expected that Ganesha will restart when rpc.statd restarts? I >> > added a patch to re-call nsm_connect() on failure and it works for me >> > now. >> > >> > Is this correct way to handle this? >> >> We should probably re-connect when statd fails, but I'm not sure how >> this patch accomplishes that. All it does is retry if the initial >> connect fails, it doesn't seem to retry later on. Can you explain more >> how this is working? >> >> Daniel >> >> > The patch is as below. >> > ------------------------------ >> > From: Suhrud Patankar <suhrudpatankar@gmail.com <mailto:suhrudpatankar@gmail.com>> >> > Date: Thu, 11 Oct 2018 00:35:45 -0700 >> > Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure >> > >> > Ganesha creates the statd connection context only once. >> > In case statd service restarts, we fail to monitor new clients. >> > On failure, destroy the STATD connection and recreate new one. >> > --- >> > src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++-- >> > >> > 1 files changed, 19 insertions(+), 2 deletions(-) >> > >> > >> > diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c >> > >> > index 09909d9..8bb41d7 100644 >> > --- a/src/Protocols/NLM/nsm.c >> > +++ b/src/Protocols/NLM/nsm.c >> > @@ -47,6 +47,8 @@ bool nsm_connect(void) >> > if (nsm_clnt != NULL) >> > return true; >> > >> > + LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); >> > + >> > nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp"); >> > >> > if (CLNT_FAILURE(nsm_clnt)) { >> > @@ -66,7 +68,7 @@ bool nsm_connect(void) >> > >> > void nsm_disconnect(void) >> > { >> > - if (nsm_count == 0 && nsm_clnt != NULL) { >> > + if (nsm_clnt != NULL) { >> > CLNT_DESTROY(nsm_clnt); >> > nsm_clnt = NULL; >> > AUTH_DESTROY(nsm_auth); >> > @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) >> > enum clnt_stat ret; >> > char client_ip[SOCK_NAME_MAX]; >> > char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ >> > + bool retry = false; >> > >> > if (host == NULL) >> > return true; >> > @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) >> > LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name); >> > >> > PTHREAD_MUTEX_lock(&nsm_mutex); >> > - >> > +again: >> > /* create a connection to nsm on the localhost */ >> > if (!nsm_connect()) { >> > LogCrit(COMPONENT_NLM, >> > @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) >> > } >> > >> > if (ret != RPC_SUCCESS) { >> > + if (! retry) { >> > + /* Handle case when statd restarts. >> > + * Need to reconnect with statd >> > + */ >> > + LogFullDebug(COMPONENT_NLM, >> > + "Try to reconnect with STATD"); >> > + >> > + retry = true; >> > + nsm_mon.mon_id.my_id.my_name = NULL; >> > + clnt_req_release(cc); >> > + nsm_disconnect(); >> > + goto again; >> > + } >> > + >> > t = rpc_sperror(&cc->cc_error, "failed"); >> > LogCrit(COMPONENT_NLM, >> > "Monitor %s SM_MON %s", >> > -- >> > 1.7.1 >> > >> > Thanks & Regards, >> > Suhrud >> > _______________________________________________ >> > Devel mailing list -- devel@lists.nfs-ganesha.org <mailto:devel@lists.nfs-ganesha.org> >> > To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org <mailto:devel-leave@lists.nfs-ganesha.org> >> > >> _______________________________________________ >> Devel mailing list -- devel@lists.nfs-ganesha.org <mailto:devel@lists.nfs-ganesha.org> >> To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org <mailto:devel-leave@lists.nfs-ganesha.org>
I put this in our IBM repo for now. Happy to take whatever form goes to upstream later. Appreciate for any review comments: TOP commit here: https://github.com/malahal/nfs-ganesha/commits/ibm2.5 On Thu, Oct 25, 2018 at 6:53 PM Daniel Gryniewicz <dang@redhat.com> wrote:
You're right, I'd missed that. So the only issues left are keeping nsm_count correct, and making sure that statd behaves correctly when we disconnect and reconnect.
I think I'd prefer the version with a force argument for the error cases. That way, under normal circumstances, things stay the same.
Daniel
On 10/25/2018 12:00 AM, Malahal Naineni wrote:
nsm_monitor and nsm_unmonitor use a single lock to serialize everything. I am not sure what problem would it cause.
On Wed, Oct 24, 2018 at 7:25 PM Daniel Gryniewicz <dang@redhat.com <mailto:dang@redhat.com>> wrote:
Ah, okay, I see now. The problem is, I'm not sure how force-disconnect interacts with nsm_count. I don't think we can just disregard it. In fact, looking at it, we can't force delete nsm_clnt, because another nsm_monitor() call might be using it. Maybe we need a separate client for each nsm_monitor() call? I don't know if that works with the lock manager, though.
Frank, any ideas?
Daniel On Wed, Oct 24, 2018 at 4:22 AM Malahal Naineni <malahal@gmail.com <mailto:malahal@gmail.com>> wrote: > > Dan, nsm_disconnect() is disconnecting every time as opposed to only when nsm_count==0. I think this works because it is connecting and disconnecting everything, not just on error. We could pass a flag to nsm_disconnect(force=true) on error and nsm_disconnect(force=false) with normal case. We need this as a way to recovery mechanism. > > Regards, Malahal. > PS: I just looked at this nsm code for a customer issue I posted on IRC yesterday! Thank you Suhrud. > > On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com <mailto:dang@redhat.com>> wrote: >> >> Discussion inline. >> >> On 10/23/2018 07:58 AM, Suhrud Patankar wrote: >> > Hello All, >> > >> > I have two questions related to NLM and rpc.statd. >> > >> > Setup: >> > Ganesha version: V2.5.0.4 >> > CentOS Linux release 7.5.1804 >> > FSAL – Any >> > >> > 1. By default libntirpc has -DPORTMAP build flag enabled. As I >> > understand this will use UDP to communicate with rpc.statd. With this >> > Ganesha fails to monitor the NLM clients and hence lock requests fail. >> > The call to clnt_tp_ncreate_timed() fails in >> > clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as >> > sizeof(cu->cu_raddr) is 128. >> > If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to >> > connect to rpc.statd and everything works as expected. >> > Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is >> > this flag enabled by default? >> >> This isn't related to UDP vs. TCP. The issue is that one address buffer >> passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit >> into an IPv4 buffer. It's definitely a bug, but I don't see exactly how >> this is happening. Can you give a more detailed explanation, with >> callpaths to get there? >> >> > >> > 2. Case: rpc.statd service restarts. Ganesha is not restarted. >> > nsm_connect() is done only once. On failure to connect to rpc.statd, >> > we don’t retry. This causes the NLM lock request to fail with “No >> > lock” error. >> > Is it expected that Ganesha will restart when rpc.statd restarts? I >> > added a patch to re-call nsm_connect() on failure and it works for me >> > now. >> > >> > Is this correct way to handle this? >> >> We should probably re-connect when statd fails, but I'm not sure how >> this patch accomplishes that. All it does is retry if the initial >> connect fails, it doesn't seem to retry later on. Can you explain more >> how this is working? >> >> Daniel >> >> > The patch is as below. >> > ------------------------------ >> > From: Suhrud Patankar <suhrudpatankar@gmail.com <mailto:suhrudpatankar@gmail.com>> >> > Date: Thu, 11 Oct 2018 00:35:45 -0700 >> > Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure >> > >> > Ganesha creates the statd connection context only once. >> > In case statd service restarts, we fail to monitor new clients. >> > On failure, destroy the STATD connection and recreate new one. >> > --- >> > src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++-- >> > >> > 1 files changed, 19 insertions(+), 2 deletions(-) >> > >> > >> > diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c >> > >> > index 09909d9..8bb41d7 100644 >> > --- a/src/Protocols/NLM/nsm.c >> > +++ b/src/Protocols/NLM/nsm.c >> > @@ -47,6 +47,8 @@ bool nsm_connect(void) >> > if (nsm_clnt != NULL) >> > return true; >> > >> > + LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); >> > + >> > nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp"); >> > >> > if (CLNT_FAILURE(nsm_clnt)) { >> > @@ -66,7 +68,7 @@ bool nsm_connect(void) >> > >> > void nsm_disconnect(void) >> > { >> > - if (nsm_count == 0 && nsm_clnt != NULL) { >> > + if (nsm_clnt != NULL) { >> > CLNT_DESTROY(nsm_clnt); >> > nsm_clnt = NULL; >> > AUTH_DESTROY(nsm_auth); >> > @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) >> > enum clnt_stat ret; >> > char client_ip[SOCK_NAME_MAX]; >> > char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ >> > + bool retry = false; >> > >> > if (host == NULL) >> > return true; >> > @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) >> > LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name); >> > >> > PTHREAD_MUTEX_lock(&nsm_mutex); >> > - >> > +again: >> > /* create a connection to nsm on the localhost */ >> > if (!nsm_connect()) { >> > LogCrit(COMPONENT_NLM, >> > @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) >> > } >> > >> > if (ret != RPC_SUCCESS) { >> > + if (! retry) { >> > + /* Handle case when statd restarts. >> > + * Need to reconnect with statd >> > + */ >> > + LogFullDebug(COMPONENT_NLM, >> > + "Try to reconnect with STATD"); >> > + >> > + retry = true; >> > + nsm_mon.mon_id.my_id.my_name = NULL; >> > + clnt_req_release(cc); >> > + nsm_disconnect(); >> > + goto again; >> > + } >> > + >> > t = rpc_sperror(&cc->cc_error, "failed"); >> > LogCrit(COMPONENT_NLM, >> > "Monitor %s SM_MON %s", >> > -- >> > 1.7.1 >> > >> > Thanks & Regards, >> > Suhrud >> > _______________________________________________ >> > Devel mailing list -- devel@lists.nfs-ganesha.org <mailto:devel@lists.nfs-ganesha.org> >> > To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org <mailto:devel-leave@lists.nfs-ganesha.org> >> > >> _______________________________________________ >> Devel mailing list -- devel@lists.nfs-ganesha.org <mailto:devel@lists.nfs-ganesha.org> >> To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org <mailto:devel-leave@lists.nfs-ganesha.org>
On Tue, Oct 23, 2018 at 7:55 PM Daniel Gryniewicz <dang@redhat.com> wrote:
Discussion inline.
On 10/23/2018 07:58 AM, Suhrud Patankar wrote:
Hello All,
I have two questions related to NLM and rpc.statd.
Setup: Ganesha version: V2.5.0.4 CentOS Linux release 7.5.1804 FSAL – Any
1. By default libntirpc has -DPORTMAP build flag enabled. As I understand this will use UDP to communicate with rpc.statd. With this Ganesha fails to monitor the NLM clients and hence lock requests fail. The call to clnt_tp_ncreate_timed() fails in clnt_dg_control(CLSET_SVC_ADDR) as addr->len is 16 where as sizeof(cu->cu_raddr) is 128. If I disable -DPORTMAP build flag in tirpc, then Ganesha uses “tcp” to connect to rpc.statd and everything works as expected. Are we supposed to use “tcp” and disable the -DPORTMAP flag? Why is this flag enabled by default?
This isn't related to UDP vs. TCP. The issue is that one address buffer passed in is IPv4, and the other is IPv6, and an IPv6 address cannot fit into an IPv4 buffer. It's definitely a bug, but I don't see exactly how this is happening. Can you give a more detailed explanation, with callpaths to get there?
nsm_connect() calls clnt_ncreate(“localhost”, SM_PROG, SM_VERS, “tcp”) which calls clnt_ncreate_timed() and then clnt_tp_ncreate_timed() which calls __rpcb_findaddr_timed() to get the addr __rpcb_findaddr_timed() if PORTMAP is defined then changes the “tcp” to “udp” It has comments as “Try UDP only - there are some portmappers out …“. clnt_tp_ncreate_timed() uses CLNT_CONTROL and tries to set the CLSET_SVC_ADDR as given by __rpcb_findaddr_timed() This is where we hit the problem. CLNT_CONTROL is clnt_dg_control() Which has the check for the addr len. if (addr->len < sizeof(cu->cu_raddr)) The addr is IPv4 but cu->cu_raddr is IPv6. We want to copy the addr->buf to cu->cu_raddr but it is failing even if addr->len is less than sizeof(cu->cu_raddr). The if() above is buggy. If I disable PORTMAP then tirpc uses TCP to connect to statd and does not set the addr->len as per IPV4. So the code goes through correctly.
2. Case: rpc.statd service restarts. Ganesha is not restarted. nsm_connect() is done only once. On failure to connect to rpc.statd, we don’t retry. This causes the NLM lock request to fail with “No lock” error. Is it expected that Ganesha will restart when rpc.statd restarts? I added a patch to re-call nsm_connect() on failure and it works for me now.
Is this correct way to handle this?
We should probably re-connect when statd fails, but I'm not sure how this patch accomplishes that. All it does is retry if the initial connect fails, it doesn't seem to retry later on. Can you explain more how this is working?
Daniel
The patch is as below. ------------------------------ From: Suhrud Patankar <suhrudpatankar@gmail.com> Date: Thu, 11 Oct 2018 00:35:45 -0700 Subject: [PATCH] NFSv3: NLM - Reconnect with statd on failure
Ganesha creates the statd connection context only once. In case statd service restarts, we fail to monitor new clients. On failure, destroy the STATD connection and recreate new one. --- src/Protocols/NLM/nsm.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/Protocols/NLM/nsm.c b/src/Protocols/NLM/nsm.c
index 09909d9..8bb41d7 100644 --- a/src/Protocols/NLM/nsm.c +++ b/src/Protocols/NLM/nsm.c @@ -47,6 +47,8 @@ bool nsm_connect(void) if (nsm_clnt != NULL) return true;
+ LogFullDebug(COMPONENT_NLM, "Creating new nsm connection "); + nsm_clnt = clnt_ncreate("localhost", SM_PROG, SM_VERS, "tcp");
if (CLNT_FAILURE(nsm_clnt)) { @@ -66,7 +68,7 @@ bool nsm_connect(void)
void nsm_disconnect(void) { - if (nsm_count == 0 && nsm_clnt != NULL) { + if (nsm_clnt != NULL) { CLNT_DESTROY(nsm_clnt); nsm_clnt = NULL; AUTH_DESTROY(nsm_auth); @@ -83,6 +85,7 @@ bool nsm_monitor(state_nsm_client_t *host) enum clnt_stat ret; char client_ip[SOCK_NAME_MAX]; char server_ip[SOCK_NAME_MAX * 2]; /* space for proxy_ip */ + bool retry = false;
if (host == NULL) return true; @@ -120,7 +123,7 @@ bool nsm_monitor(state_nsm_client_t *host) LogDebug(COMPONENT_NLM, "Monitor %s", host->ssc_nlm_caller_name);
PTHREAD_MUTEX_lock(&nsm_mutex); - +again: /* create a connection to nsm on the localhost */ if (!nsm_connect()) { LogCrit(COMPONENT_NLM, @@ -144,6 +147,20 @@ bool nsm_monitor(state_nsm_client_t *host) }
if (ret != RPC_SUCCESS) { + if (! retry) { + /* Handle case when statd restarts. + * Need to reconnect with statd + */ + LogFullDebug(COMPONENT_NLM, + "Try to reconnect with STATD"); + + retry = true; + nsm_mon.mon_id.my_id.my_name = NULL; + clnt_req_release(cc); + nsm_disconnect(); + goto again; + } + t = rpc_sperror(&cc->cc_error, "failed"); LogCrit(COMPONENT_NLM, "Monitor %s SM_MON %s", -- 1.7.1
Thanks & Regards, Suhrud _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
_______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
On 10/25/18 12:17 AM, Suhrud Patankar wrote:
if (addr->len < sizeof(cu->cu_raddr)) The addr is IPv4 but cu->cu_raddr is IPv6. We want to copy the addr->buf to cu->cu_raddr but it is failing even if addr->len is less than sizeof(cu->cu_raddr). The if() above is buggy.
You are correct. That < should be >, to prevent copying too large an address in the memcpy(). That line hasn't been touched since the fork, and this bug is still in tirpc 1.0.2.
If I disable PORTMAP then tirpc uses TCP to connect to statd and does not set the addr->len as per IPV4. So the code goes through correctly.
This is the underlying problem. This version of the code is so old that it never gets tested, because nobody requires UDP instead of TCP. That's left over from 20 years ago. Better to just remove all the PORTMAP code entirely. IMnsHO.
On 10/27/2018 08:41 AM, William Allen Simpson wrote:
On 10/25/18 12:17 AM, Suhrud Patankar wrote:
if (addr->len < sizeof(cu->cu_raddr)) The addr is IPv4 but cu->cu_raddr is IPv6. We want to copy the addr->buf to cu->cu_raddr but it is failing even if addr->len is less than sizeof(cu->cu_raddr). The if() above is buggy.
You are correct. That < should be >, to prevent copying too large an address in the memcpy(). That line hasn't been touched since the fork, and this bug is still in tirpc 1.0.2.
If I disable PORTMAP then tirpc uses TCP to connect to statd and does not set the addr->len as per IPV4. So the code goes through correctly.
This is the underlying problem. This version of the code is so old that it never gets tested, because nobody requires UDP instead of TCP. That's left over from 20 years ago. Better to just remove all the PORTMAP code entirely. IMnsHO.
Good catch, I completely missed that. Suhrud, can you test with this: https://github.com/nfs-ganesha/ntirpc/pull/157 Daniel
On Mon, Oct 29, 2018 at 9:22 PM Daniel Gryniewicz <dang@redhat.com> wrote:
On 10/27/2018 08:41 AM, William Allen Simpson wrote:
On 10/25/18 12:17 AM, Suhrud Patankar wrote:
if (addr->len < sizeof(cu->cu_raddr)) The addr is IPv4 but cu->cu_raddr is IPv6. We want to copy the addr->buf to cu->cu_raddr but it is failing even if addr->len is less than sizeof(cu->cu_raddr). The if() above is buggy.
You are correct. That < should be >, to prevent copying too large an address in the memcpy(). That line hasn't been touched since the fork, and this bug is still in tirpc 1.0.2.
If I disable PORTMAP then tirpc uses TCP to connect to statd and does not set the addr->len as per IPV4. So the code goes through correctly.
This is the underlying problem. This version of the code is so old that it never gets tested, because nobody requires UDP instead of TCP. That's left over from 20 years ago. Better to just remove all the PORTMAP code entirely. IMnsHO.
Good catch, I completely missed that.
Suhrud, can you test with this:
The bug I was chasing is fixed in 1.7.1. It was related to clnt_dg_control(CLSET_VERS) . With 1.7.1, it is not required to fix the addr->len comparison. It works as expected. Thanks for looking into it!
Daniel
participants (5)
-
Daniel Gryniewicz -
Frank Filz -
Malahal Naineni -
Suhrud Patankar -
William Allen Simpson