[PATCH v2] Call echo service immediately after socket reconnect

[PATCH v2] Call echo service immediately after socket reconnect

[Sachin Prabhu]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index aab5227..4547aed 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;
@@ -442,7 +453,7 @@ cifs_echo_request(struct work_struct *work)
 			 server->hostname);
 
 requeue_echo:
-	queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);
+	queue_delayed_work(cifsiod_wq, &server->echo, server->echo_interval);
 }
 
 static bool
-- 
2.7.4

Re: [PATCH v2] Call echo service immediately after socket reconnect

[Pavel Shilovsky]

2016-10-20 16:52 GMT-07:00 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index aab5227..4547aed 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;
> @@ -442,7 +453,7 @@ cifs_echo_request(struct work_struct *work)
>                          server->hostname);
>
>  requeue_echo:
> -       queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);
> +       queue_delayed_work(cifsiod_wq, &server->echo, server->echo_interval);
>  }
>
>  static bool
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky

Re: [PATCH v2] Call echo service immediately after socket reconnect

[Steve French]

merged into cifs-2.6.git for-next

On Thu, Oct 20, 2016 at 8:33 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2016-10-20 16:52 GMT-07:00 Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/connect.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index aab5227..4547aed 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;
>> @@ -442,7 +453,7 @@ cifs_echo_request(struct work_struct *work)
>>                          server->hostname);
>>
>>  requeue_echo:
>> -       queue_delayed_work(cifsiod_wq, &server->echo, echo_interval);
>> +       queue_delayed_work(cifsiod_wq, &server->echo, server->echo_interval);
>>  }
>>
>>  static bool
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
>
> --
> Best regards,
> Pavel Shilovsky
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

Re: [v2] Call echo service immediately after socket reconnect

[Jonathan Liu]

Hi Sachin,

On 21 October 2016 at 10:52, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>

This commit is causing a flood of connections to Samba server as well
as high server CPU load when the Samba is restarted while CIFS share
is mounted on the client. This can cause the Samba server to become
slow or unresponsive resulting in denial of service to other users
connected to the Samba server.

Bug report: https://bugzilla.kernel.org/show_bug.cgi?id=194531

Could you please have a look?

Thanks.

Regards,
Jonathan

Re: [v2] Call echo service immediately after socket reconnect

[Pavel Shilovsky]

2017-04-15 8:38 GMT-07:00 Jonathan Liu <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hi Sachin,
>
> On 21 October 2016 at 10:52, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
>
> This commit is causing a flood of connections to Samba server as well
> as high server CPU load when the Samba is restarted while CIFS share
> is mounted on the client. This can cause the Samba server to become
> slow or unresponsive resulting in denial of service to other users
> connected to the Samba server.
>
> Bug report: https://bugzilla.kernel.org/show_bug.cgi?id=194531
>
> Could you please have a look?
>
> Thanks.
>
> Regards,
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Jonathan,

Thanks for reporting this.

It seems like we need to make CIFSSMBEcho() call as noop in case of
(server->tcpStatus == CifsNeedNegotiate) as we already did for
SMB2_echo().

--
Best regards,
Pavel Shilovsky

Re: [v2] Call echo service immediately after socket reconnect

[Sachin Prabhu]

On Sun, 2017-04-16 at 01:38 +1000, Jonathan Liu wrote:
> Hi Sachin,
> 
> On 21 October 2016 at 10:52, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> wrote:
> > 
> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> 
> This commit is causing a flood of connections to Samba server as well
> as high server CPU load when the Samba is restarted while CIFS share
> is mounted on the client. This can cause the Samba server to become
> slow or unresponsive resulting in denial of service to other users
> connected to the Samba server.
> 
> Bug report: https://bugzilla.kernel.org/show_bug.cgi?id=194531
> 
> Could you please have a look?
> 
> Thanks.
> 
> Regards,
> Jonathan

Hello Jonathan,

Thanks for bringing this to my attention. I've posted a patch to the
list. I've tested it out with the reproducer. Can you please test it
out  in your setup and let me know if it works for you.

Sachin Prabhu