[Xenial,SRU] Call echo service immediately after socket reconnect

Submitted by Tim Gardner on March 6, 2017, 2:03 p.m.

Details

Message ID 1488809000-2809-1-git-send-email-tim.gardner@canonical.com
State New
Headers show

Commit Message

Tim Gardner March 6, 2017, 2:03 p.m.
From: Sachin Prabhu <sprabhu@redhat.com>

BugLink: http://bugs.launchpad.net/bugs/1669941

Commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
long after socket reconnect") changes the behaviour of the SMB2 echo
service and causes it to renegotiate after a socket reconnect. However
under default settings, the echo service could take up to 120 seconds to
be scheduled.

The patch forces the echo service to be called immediately resulting a
negotiate call being made immediately on reconnect.

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <smfrench@gmail.com>
(back ported from commit b8c600120fc87d53642476f48c8055b38d6e14c7)
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Conflicts:
	fs/cifs/connect.c
---
 fs/cifs/connect.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Comments

Seth Forshee March 8, 2017, 9:07 p.m.
Backport looks reasonable.
Thadeu Lima de Souza Cascardo March 15, 2017, 4:02 p.m.
On Mon, Mar 06, 2017 at 07:03:20AM -0700, Tim Gardner wrote:
> From: Sachin Prabhu <sprabhu@redhat.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1669941
> 
> Commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
> long after socket reconnect") changes the behaviour of the SMB2 echo
> service and causes it to renegotiate after a socket reconnect. However
> under default settings, the echo service could take up to 120 seconds to
> be scheduled.
> 
> The patch forces the echo service to be called immediately resulting a
> negotiate call being made immediately on reconnect.
> 
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> Signed-off-by: Steve French <smfrench@gmail.com>
> (back ported from commit b8c600120fc87d53642476f48c8055b38d6e14c7)
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> 
> Conflicts:
> 	fs/cifs/connect.c
> ---
>  fs/cifs/connect.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 3521512..61ae54f 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -412,6 +412,9 @@ cifs_reconnect(struct TCP_Server_Info *server)
>  		}
>  	} while (server->tcpStatus == CifsNeedReconnect);
>  
> +	if (server->tcpStatus == CifsNeedNegotiate)
> +		mod_delayed_work(cifsiod_wq, &server->echo, 0);
> +
>  	return rc;
>  }
>  
> @@ -421,18 +424,27 @@ cifs_echo_request(struct work_struct *work)
>  	int rc;
>  	struct TCP_Server_Info *server = container_of(work,
>  					struct TCP_Server_Info, echo.work);
> +	unsigned long echo_interval;
> +
> +	/*
> +	 * If we need to renegotiate, set echo interval to zero to
> +	 * immediately call echo service where we can renegotiate.
> +	 */
> +	if (server->tcpStatus == CifsNeedNegotiate)
> +		echo_interval = 0;
> +	else
> +		echo_interval = SMB_ECHO_INTERVAL;
>  
>  	/*
> -	 * We cannot send an echo if it is disabled or until the
> -	 * NEGOTIATE_PROTOCOL request is done, which is indicated by
> -	 * server->ops->need_neg() == true. Also, no need to ping if
> -	 * we got a response recently.
> +	 * We cannot send an echo if it is disabled.
> +	 * Also, no need to ping if we got a response recently.
>  	 */
>  
>  	if (server->tcpStatus == CifsNeedReconnect ||
> -	    server->tcpStatus == CifsExiting || server->tcpStatus == CifsNew ||
> +	    server->tcpStatus == CifsExiting ||
> +	    server->tcpStatus == CifsNew ||
>  	    (server->ops->can_echo && !server->ops->can_echo(server)) ||
> -	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
> +	    time_before(jiffies, server->lstrp + echo_interval - HZ))
>  		goto requeue_echo;
>  
>  	rc = server->ops->echo ? server->ops->echo(server) : -ENOSYS;

[...]

> @@ -441,7 +453,7 @@ cifs_echo_request(struct work_struct *work)
>  			 server->hostname);
>  
>  requeue_echo:
> -	queue_delayed_work(cifsiod_wq, &server->echo, SMB_ECHO_INTERVAL);
> +	queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);

Upstream commit adfeb3e00e8e1b9fb4ad19eb7367e7c272d16003 introduced the tunable
echo_interval, and used the local variable here because it was always assigned
the server tunable. Then, the backported commit
b8c600120fc87d53642476f48c8055b38d6e14c7 changed it to server->echo_interval
because now the local variable could be assigned 0. So, the right thing here is
to keep the original line.

Cascardo.

>  }
>  
>  static bool
> -- 
> 2.7.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch hide | download patch | download mbox

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 3521512..61ae54f 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -412,6 +412,9 @@  cifs_reconnect(struct TCP_Server_Info *server)
 		}
 	} while (server->tcpStatus == CifsNeedReconnect);
 
+	if (server->tcpStatus == CifsNeedNegotiate)
+		mod_delayed_work(cifsiod_wq, &server->echo, 0);
+
 	return rc;
 }
 
@@ -421,18 +424,27 @@  cifs_echo_request(struct work_struct *work)
 	int rc;
 	struct TCP_Server_Info *server = container_of(work,
 					struct TCP_Server_Info, echo.work);
+	unsigned long echo_interval;
+
+	/*
+	 * If we need to renegotiate, set echo interval to zero to
+	 * immediately call echo service where we can renegotiate.
+	 */
+	if (server->tcpStatus == CifsNeedNegotiate)
+		echo_interval = 0;
+	else
+		echo_interval = SMB_ECHO_INTERVAL;
 
 	/*
-	 * We cannot send an echo if it is disabled or until the
-	 * NEGOTIATE_PROTOCOL request is done, which is indicated by
-	 * server->ops->need_neg() == true. Also, no need to ping if
-	 * we got a response recently.
+	 * We cannot send an echo if it is disabled.
+	 * Also, no need to ping if we got a response recently.
 	 */
 
 	if (server->tcpStatus == CifsNeedReconnect ||
-	    server->tcpStatus == CifsExiting || server->tcpStatus == CifsNew ||
+	    server->tcpStatus == CifsExiting ||
+	    server->tcpStatus == CifsNew ||
 	    (server->ops->can_echo && !server->ops->can_echo(server)) ||
-	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
+	    time_before(jiffies, server->lstrp + echo_interval - HZ))
 		goto requeue_echo;
 
 	rc = server->ops->echo ? server->ops->echo(server) : -ENOSYS;
@@ -441,7 +453,7 @@  cifs_echo_request(struct work_struct *work)
 			 server->hostname);
 
 requeue_echo:
-	queue_delayed_work(cifsiod_wq, &server->echo, SMB_ECHO_INTERVAL);
+	queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);
 }
 
 static bool