[Cluster-devel] DLM: Do not count redundant connection attempts against retries

[Cluster-devel] DLM: Do not count redundant connection attempts against retries

[Bob Peterson]

Hi,

Before this patch, multiple GFS2 mounts would result in multiple
connection attempts. They were all ignored, and rightly so, but
they were being counted against the connection attempt retries.
This patch moves the retry check later those redundant attempts
are simply ignored, as they should be.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 7d398d3..42c8457 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -1123,13 +1123,13 @@ static void tcp_connect_to_sock(struct connection *con)
 	}
 
 	mutex_lock(&con->sock_mutex);
-	if (con->retries++ > MAX_CONNECT_RETRIES)
-		goto out;
-
 	/* Some odd races can cause double-connects, ignore them */
 	if (con->sock)
 		goto out;
 
+	if (con->retries++ > MAX_CONNECT_RETRIES)
+		goto out;
+
 	/* Create a socket to communicate with */
 	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
 				  SOCK_STREAM, IPPROTO_TCP, &sock);

[Cluster-devel] DLM: Do not count redundant connection attempts against retries

[David Teigland]

On Thu, Apr 20, 2017 at 04:02:20PM -0400, Bob Peterson wrote:
> Hi,
> 
> Before this patch, multiple GFS2 mounts would result in multiple
> connection attempts. They were all ignored, and rightly so, but
> they were being counted against the connection attempt retries.
> This patch moves the retry check later those redundant attempts
> are simply ignored, as they should be.

Each mount/dlm_new_lockspace() calls dlm_lowcomm_connect_node() to
establish connections to other nodes in the lockspace.  If a connection to
a node already exists, nothing is needed, but it seems strange this isn't
checked earlier, e.g. in dlm_lowcomms_connect_node() or
lowcomms_connect_sock().  Maybe concurrent closes or connects require
checking things in tcp_connect_to_sock()?

Dave

> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 7d398d3..42c8457 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -1123,13 +1123,13 @@ static void tcp_connect_to_sock(struct connection *con)
>  	}
>  
>  	mutex_lock(&con->sock_mutex);
> -	if (con->retries++ > MAX_CONNECT_RETRIES)
> -		goto out;
> -
>  	/* Some odd races can cause double-connects, ignore them */
>  	if (con->sock)
>  		goto out;
>  
> +	if (con->retries++ > MAX_CONNECT_RETRIES)
> +		goto out;
> +
>  	/* Create a socket to communicate with */
>  	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
>  				  SOCK_STREAM, IPPROTO_TCP, &sock);

[Cluster-devel] DLM: Do not count redundant connection attempts against retries

[Bob Peterson]

----- Original Message -----
| On Thu, Apr 20, 2017 at 04:02:20PM -0400, Bob Peterson wrote:
| > Hi,
| > 
| > Before this patch, multiple GFS2 mounts would result in multiple
| > connection attempts. They were all ignored, and rightly so, but
| > they were being counted against the connection attempt retries.
| > This patch moves the retry check later those redundant attempts
| > are simply ignored, as they should be.
| 
| Each mount/dlm_new_lockspace() calls dlm_lowcomm_connect_node() to
| establish connections to other nodes in the lockspace.  If a connection to
| a node already exists, nothing is needed, but it seems strange this isn't
| checked earlier, e.g. in dlm_lowcomms_connect_node() or
| lowcomms_connect_sock().  Maybe concurrent closes or connects require
| checking things in tcp_connect_to_sock()?
| 
| Dave
| 
| > 
| > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
| > ---
| > diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
| > index 7d398d3..42c8457 100644
| > --- a/fs/dlm/lowcomms.c
| > +++ b/fs/dlm/lowcomms.c
| > @@ -1123,13 +1123,13 @@ static void tcp_connect_to_sock(struct connection
| > *con)
| >  	}
| >  
| >  	mutex_lock(&con->sock_mutex);
| > -	if (con->retries++ > MAX_CONNECT_RETRIES)
| > -		goto out;
| > -
| >  	/* Some odd races can cause double-connects, ignore them */
| >  	if (con->sock)
| >  		goto out;
| >  
| > +	if (con->retries++ > MAX_CONNECT_RETRIES)
| > +		goto out;
| > +
| >  	/* Create a socket to communicate with */
| >  	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
| >  				  SOCK_STREAM, IPPROTO_TCP, &sock);
| 

Hi,

Perhaps we should ask Chrissie, since afaik, she authored lowcomms.c,
and patch 391fbdc5d5 in particular, which added function dlm_lowcomms_connect_node().
She might have more insight than me, having known the original intent of the code.
Adding her to the cc.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] DLM: Do not count redundant connection attempts against retries

[Christine Caulfield]

On 24/04/17 13:37, Bob Peterson wrote:
> ----- Original Message -----
> | On Thu, Apr 20, 2017 at 04:02:20PM -0400, Bob Peterson wrote:
> | > Hi,
> | > 
> | > Before this patch, multiple GFS2 mounts would result in multiple
> | > connection attempts. They were all ignored, and rightly so, but
> | > they were being counted against the connection attempt retries.
> | > This patch moves the retry check later those redundant attempts
> | > are simply ignored, as they should be.
> | 
> | Each mount/dlm_new_lockspace() calls dlm_lowcomm_connect_node() to
> | establish connections to other nodes in the lockspace.  If a connection to
> | a node already exists, nothing is needed, but it seems strange this isn't
> | checked earlier, e.g. in dlm_lowcomms_connect_node() or
> | lowcomms_connect_sock().  Maybe concurrent closes or connects require
> | checking things in tcp_connect_to_sock()?
> | 

Dave is right, it would probably be better, or at least more efficient,
to check it earlier rather than let it get into the workqueue before
rejecting a duplicate. The only downside I can see in that is that you
would need to hold the sock_mutex to do the check which is an extra
locking operation in the connect path - and I haven't checked it for
ordering.

If you go with Bob's patch (which looks sane enough in itself), the
equivalent SCTP routine probably also needs doing. For consistency if
nothing else.

Chrissie


> | Dave
> | 
> | > 
> | > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> | > ---
> | > diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> | > index 7d398d3..42c8457 100644
> | > --- a/fs/dlm/lowcomms.c
> | > +++ b/fs/dlm/lowcomms.c
> | > @@ -1123,13 +1123,13 @@ static void tcp_connect_to_sock(struct connection
> | > *con)
> | >  	}
> | >  
> | >  	mutex_lock(&con->sock_mutex);
> | > -	if (con->retries++ > MAX_CONNECT_RETRIES)
> | > -		goto out;
> | > -
> | >  	/* Some odd races can cause double-connects, ignore them */
> | >  	if (con->sock)
> | >  		goto out;
> | >  
> | > +	if (con->retries++ > MAX_CONNECT_RETRIES)
> | > +		goto out;
> | > +
> | >  	/* Create a socket to communicate with */
> | >  	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> | >  				  SOCK_STREAM, IPPROTO_TCP, &sock);
> | 
> 
> Hi,
> 
> Perhaps we should ask Chrissie, since afaik, she authored lowcomms.c,
> and patch 391fbdc5d5 in particular, which added function dlm_lowcomms_connect_node().
> She might have more insight than me, having known the original intent of the code.
> Adding her to the cc.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
>