Call echo service immediately after socket reconnect
diff mbox

Message ID 1476969267-22325-1-git-send-email-sprabhu@redhat.com
State New
Headers show

Commit Message

Sachin Prabhu Oct. 20, 2016, 1:14 p.m. UTC
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>
---
 fs/cifs/connect.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

Pavel Shilovsky Oct. 20, 2016, 8:30 p.m. UTC | #1
2016-10-20 6:14 GMT-07:00 Sachin Prabhu <sprabhu@redhat.com>:
> 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.

Looks like a right thing to do. Comments are inlined below:

>
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> ---
>  fs/cifs/connect.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index aab5227..43fd838 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,17 +424,25 @@ 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 = server->echo_interval;
> +       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;

Suppose we are setting echo_interval to 0 above

> +       else
> +               echo_interval = server->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 + echo_interval - HZ))
>                 goto requeue_echo;

then we are sending an echo message and re-queue the echo work with
echo_interval==0:

rc = server->ops->echo ? server->ops->echo(server) : -ENOSYS;
....
queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);

Shouldn't we queue the echo work with server->echo_interval (not 0) here?
Sachin Prabhu Oct. 20, 2016, 11:55 p.m. UTC | #2
On Thu, 2016-10-20 at 13:30 -0700, Pavel Shilovsky wrote:
> 2016-10-20 6:14 GMT-07:00 Sachin Prabhu <sprabhu@redhat.com>:
> > 
> > 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.
> 
> Looks like a right thing to do. Comments are inlined below:
> 
> > 
> > ..
> > +       if (server->tcpStatus == CifsNeedNegotiate)
> > +               echo_interval = 0;
> 
> Suppose we are setting echo_interval to 0 above
> 
> > 
> > 
..
> then we are sending an echo message and re-queue the echo work with
> echo_interval==0:
> 
> rc = server->ops->echo ? server->ops->echo(server) : -ENOSYS;
> ....
> queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);
> 
> Shouldn't we queue the echo work with server->echo_interval (not 0)
> here?
> 

Hello Pavel,

Yes, you are right. I had overlooked the second echo_interval in the
function. I've sent a version 2 of the patch.

Sachin Prabhu
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index aab5227..43fd838 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,17 +424,25 @@  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 = server->echo_interval;
+	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 = server->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 + echo_interval - HZ))
 		goto requeue_echo;