| Message ID | 1490743614-5439-2-git-send-email-germano.percossi@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-03-29 at 00:26 +0100, Germano Percossi wrote: > All the other threads are queue with a delay, no reason > why this one need to be so aggressive. Is there any reason to queue with a delay? Is there a problem with reconnecting immediately? Sachin Prabhu > > Signed-off-by: Germano Percossi <germano.percossi@citrix.com> > --- > fs/cifs/smb2pdu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 7446496..efe167c 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -258,7 +258,7 @@ smb2_reconnect(__le16 smb2_command, struct > cifs_tcon *tcon) > goto out; > > if (smb2_command != SMB2_INTERNAL_CMD) > - queue_delayed_work(cifsiod_wq, &server->reconnect, > 0); > + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 > * HZ); > > atomic_inc(&tconInfoReconnectCount); > out: > @@ -2231,7 +2231,7 @@ SMB2_echo(struct TCP_Server_Info *server) > > if (server->tcpStatus == CifsNeedNegotiate) { > /* No need to send echo on newly established > connections */ > - queue_delayed_work(cifsiod_wq, &server->reconnect, > 0); > + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 > * HZ); > return rc; > } > -- 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
Some servers (Microsoft for example) reply to reconnection requests with errors during the failover phase. Asking repeatedly to reconnect (hundreds times in few seconds) is not gonna solve the problem. This is would happen (reconnection bugs aside, fix later) without self-rescheduling and delay: * one node fails over during I/O * reconnect thread is immediately started * server is not ready and send an error * reconnect fails and exit * echo will notice connection is down and schedule reconnect * reconnect (hopefully succeeds) The problem here is that the echo timeout will make the difference: 60 seconds for some applications are not tolerable and making echo more frequent just to solve a once-off problem is sub-optimal. The reason for rescheduling by itself is to avoid relying on the echo thread and make possible to tune parameters separately: echo can still be used with its default (60s) without impacting the reconnect that usually happens when there is pending I/O. My initial idea was to make it configurable like echo_interval. Happy to do it if the whole idea of a delay and self rescheduling is accepted. Cheers, Germano P.S: there is a coding style error in this patch, I am sending the correct version. On 04/05/2017 07:07 PM, Sachin Prabhu wrote: > On Wed, 2017-03-29 at 00:26 +0100, Germano Percossi wrote: >> All the other threads are queue with a delay, no reason >> why this one need to be so aggressive. > > Is there any reason to queue with a delay? Is there a problem with > reconnecting immediately? > > Sachin Prabhu > >> >> Signed-off-by: Germano Percossi <germano.percossi@citrix.com> >> --- >> fs/cifs/smb2pdu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >> index 7446496..efe167c 100644 >> --- a/fs/cifs/smb2pdu.c >> +++ b/fs/cifs/smb2pdu.c >> @@ -258,7 +258,7 @@ smb2_reconnect(__le16 smb2_command, struct >> cifs_tcon *tcon) >> goto out; >> >> if (smb2_command != SMB2_INTERNAL_CMD) >> - queue_delayed_work(cifsiod_wq, &server->reconnect, >> 0); >> + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 >> * HZ); >> >> atomic_inc(&tconInfoReconnectCount); >> out: >> @@ -2231,7 +2231,7 @@ SMB2_echo(struct TCP_Server_Info *server) >> >> if (server->tcpStatus == CifsNeedNegotiate) { >> /* No need to send echo on newly established >> connections */ >> - queue_delayed_work(cifsiod_wq, &server->reconnect, >> 0); >> + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 >> * HZ); >> return rc; >> } >> > -- 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
Sorry Sachin, I just noticed your question was for the first patch. For this one probably the motivations are not as strong as for the following patch. No strong feelings for this one. We can drop it. Germano On 04/06/2017 03:03 PM, Germano Percossi wrote: > Some servers (Microsoft for example) reply to reconnection > requests with errors during the failover phase. > Asking repeatedly to reconnect (hundreds times in few > seconds) is not gonna solve the problem. > > This is would happen (reconnection bugs aside, fix later) > without self-rescheduling and delay: > * one node fails over during I/O > * reconnect thread is immediately started > * server is not ready and send an error > * reconnect fails and exit > * echo will notice connection is down and schedule reconnect > * reconnect (hopefully succeeds) > > The problem here is that the echo timeout will make the difference: > 60 seconds for some applications are not tolerable and > making echo more frequent just to solve a once-off problem is > sub-optimal. > > The reason for rescheduling by itself is to avoid relying > on the echo thread and make possible to tune parameters > separately: echo can still be used with its default (60s) > without impacting the reconnect that usually happens when > there is pending I/O. > > My initial idea was to make it configurable like > echo_interval. > Happy to do it if the whole idea of a delay and self rescheduling > is accepted. > > Cheers, > Germano > > P.S: there is a coding style error in this patch, I am > sending the correct version. > > On 04/05/2017 07:07 PM, Sachin Prabhu wrote: >> On Wed, 2017-03-29 at 00:26 +0100, Germano Percossi wrote: >>> All the other threads are queue with a delay, no reason >>> why this one need to be so aggressive. >> >> Is there any reason to queue with a delay? Is there a problem with >> reconnecting immediately? >> >> Sachin Prabhu >> >>> >>> Signed-off-by: Germano Percossi <germano.percossi@citrix.com> >>> --- >>> fs/cifs/smb2pdu.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index 7446496..efe167c 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -258,7 +258,7 @@ smb2_reconnect(__le16 smb2_command, struct >>> cifs_tcon *tcon) >>> goto out; >>> >>> if (smb2_command != SMB2_INTERNAL_CMD) >>> - queue_delayed_work(cifsiod_wq, &server->reconnect, >>> 0); >>> + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 >>> * HZ); >>> >>> atomic_inc(&tconInfoReconnectCount); >>> out: >>> @@ -2231,7 +2231,7 @@ SMB2_echo(struct TCP_Server_Info *server) >>> >>> if (server->tcpStatus == CifsNeedNegotiate) { >>> /* No need to send echo on newly established >>> connections */ >>> - queue_delayed_work(cifsiod_wq, &server->reconnect, >>> 0); >>> + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 >>> * HZ); >>> return rc; >>> } >>> >> > -- > 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 > -- 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
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 7446496..efe167c 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -258,7 +258,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon) goto out; if (smb2_command != SMB2_INTERNAL_CMD) - queue_delayed_work(cifsiod_wq, &server->reconnect, 0); + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 * HZ); atomic_inc(&tconInfoReconnectCount); out: @@ -2231,7 +2231,7 @@ SMB2_echo(struct TCP_Server_Info *server) if (server->tcpStatus == CifsNeedNegotiate) { /* No need to send echo on newly established connections */ - queue_delayed_work(cifsiod_wq, &server->reconnect, 0); + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 * HZ); return rc; }
All the other threads are queue with a delay, no reason why this one need to be so aggressive. Signed-off-by: Germano Percossi <germano.percossi@citrix.com> --- fs/cifs/smb2pdu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)