[PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Jeff Layton]

Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
a reliable indicator of whether the socket has had a NEGOTIATE done on
it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
indicates this. It also cleans up cifs_reconnect_tcon a bit.

This is not the only way to fix this issue. As Steve pointed out to me,
we could also check to see whether there's a valid SMB session on the
list as an indicator of whether a NEGOTIATE has been done. I think the
approach I'm proposing clarifies the code better, but I'm willing to go
with his approach if that's the general concensus.

JG reported this problem and tested the patch and stated that it fixed
the problem for him.

Comments and suggestions welcome...

Jeff Layton (2):
  cifs: remove checks for ses->status == CifsExiting
  cifs: clarify the meaning of tcpStatus == CifsGood

 fs/cifs/cifsglob.h |    3 ++-
 fs/cifs/cifssmb.c  |    9 +++------
 fs/cifs/connect.c  |    8 ++++----
 3 files changed, 9 insertions(+), 11 deletions(-)

-- 
1.7.4

[PATCH 1/2] cifs: remove checks for ses->status == CifsExiting

[Jeff Layton]

ses->status is never set to CifsExiting, so these checks are
always false.

Tested-by: JG <jg-9fI4heB6ewM@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifssmb.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 46c66ed..904aa47 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -136,9 +136,6 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
 		}
 	}
 
-	if (ses->status == CifsExiting)
-		return -EIO;
-
 	/*
 	 * Give demultiplex thread up to 10 seconds to reconnect, should be
 	 * greater than cifs socket timeout which is 7 seconds
@@ -156,7 +153,7 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
 		 * retrying until process is killed or server comes
 		 * back on-line
 		 */
-		if (!tcon->retry || ses->status == CifsExiting) {
+		if (!tcon->retry) {
 			cFYI(1, "gave up waiting on reconnect in smb_init");
 			return -EHOSTDOWN;
 		}
-- 
1.7.4

[PATCH 2/2] cifs: clarify the meaning of tcpStatus == CifsGood

[Jeff Layton]

When the TCP_Server_Info is first allocated and connected, tcpStatus ==
CifsGood means that the NEGOTIATE_PROTOCOL request has completed and the
socket is ready for other calls. cifs_reconnect however sets tcpStatus
to CifsGood as soon as the socket is reconnected and the optional
RFC1001 session setup is done. We have no clear way to tell the
difference between these two states, and we need to know this in order
to know whether we can send an echo or not.

Resolve this by adding a new statusEnum value -- CifsNeedNegotiate. When
the socket has been connected but has not yet had a NEGOTIATE_PROTOCOL
request done, set it to this value. Once the NEGOTIATE is done,
cifs_negotiate_protocol will set tcpStatus to CifsGood.

This also fixes and cleans the logic in cifs_reconnect and
cifs_reconnect_tcon. The old code checked for specific states when what
it really wants to know is whether the state has actually changed from
CifsNeedReconnect.

Reported-and-Tested-by: JG <jg-9fI4heB6ewM@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |    3 ++-
 fs/cifs/cifssmb.c  |    4 ++--
 fs/cifs/connect.c  |    8 ++++----
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index edd5b29..5f333e0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -92,7 +92,8 @@ enum statusEnum {
 	CifsNew = 0,
 	CifsGood,
 	CifsExiting,
-	CifsNeedReconnect
+	CifsNeedReconnect,
+	CifsNeedNegotiate
 };
 
 enum securityEnum {
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 904aa47..f4bd502 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -142,9 +142,9 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
 	 */
 	while (server->tcpStatus == CifsNeedReconnect) {
 		wait_event_interruptible_timeout(server->response_q,
-			(server->tcpStatus == CifsGood), 10 * HZ);
+			(server->tcpStatus != CifsNeedReconnect), 10 * HZ);
 
-		/* is TCP session is reestablished now ?*/
+		/* are we still trying to reconnect? */
 		if (server->tcpStatus != CifsNeedReconnect)
 			break;
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 257b6d8..7deceae 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -199,8 +199,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 	}
 	spin_unlock(&GlobalMid_Lock);
 
-	while ((server->tcpStatus != CifsExiting) &&
-	       (server->tcpStatus != CifsGood)) {
+	while (server->tcpStatus == CifsNeedReconnect) {
 		try_to_freeze();
 
 		/* we should try only the port we connected to before */
@@ -212,7 +211,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 			atomic_inc(&tcpSesReconnectCount);
 			spin_lock(&GlobalMid_Lock);
 			if (server->tcpStatus != CifsExiting)
-				server->tcpStatus = CifsGood;
+				server->tcpStatus = CifsNeedNegotiate;
 			spin_unlock(&GlobalMid_Lock);
 		}
 	}
@@ -420,7 +419,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 		pdu_length = 4; /* enough to get RFC1001 header */
 
 incomplete_rcv:
-		if (echo_retries > 0 &&
+		if (echo_retries > 0 && server->tcpStatus == CifsGood &&
 		    time_after(jiffies, server->lstrp +
 					(echo_retries * SMB_ECHO_INTERVAL))) {
 			cERROR(1, "Server %s has not responded in %d seconds. "
@@ -1732,6 +1731,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		cERROR(1, "Error connecting to socket. Aborting operation");
 		goto out_err_crypto_release;
 	}
+	tcp_ses->tcpStatus = CifsNeedNegotiate;
 
 	/*
 	 * since we're in a cifs function already, we know that
-- 
1.7.4

Re: [PATCH 1/2] cifs: remove checks for ses->status == CifsExiting

[Steve French]

merged

On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> ses->status is never set to CifsExiting, so these checks are
> always false.
>
> Tested-by: JG <jg-9fI4heB6ewM@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifssmb.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 46c66ed..904aa47 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -136,9 +136,6 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
>                }
>        }
>
> -       if (ses->status == CifsExiting)
> -               return -EIO;
> -
>        /*
>         * Give demultiplex thread up to 10 seconds to reconnect, should be
>         * greater than cifs socket timeout which is 7 seconds
> @@ -156,7 +153,7 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
>                 * retrying until process is killed or server comes
>                 * back on-line
>                 */
> -               if (!tcon->retry || ses->status == CifsExiting) {
> +               if (!tcon->retry) {
>                        cFYI(1, "gave up waiting on reconnect in smb_init");
>                        return -EHOSTDOWN;
>                }
> --
> 1.7.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Steve French]

On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
> unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
> a reliable indicator of whether the socket has had a NEGOTIATE done on
> it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
> indicates this. It also cleans up cifs_reconnect_tcon a bit.
>
> This is not the only way to fix this issue. As Steve pointed out to me,
> we could also check to see whether there's a valid SMB session on the
> list as an indicator of whether a NEGOTIATE has been done. I think the
> approach I'm proposing clarifies the code better, but I'm willing to go
> with his approach if that's the general concensus.

Jeff may be right that the code is clearer with this approach (I may try
to code the alternative to check) but ... we really shouldn't be sending
an SMBecho between negprot and sessionsetup anyway (so checking for
sessionsetup may make sense).  If a negprot times out or
sessionsetup times out we don't need to be sending SMBecho
(on the other hand perhaps a really slow krb5 sessionsetup?)
Once a sessionsetup is sent, SMBechos make more sense.

-- 
Thanks,

Steve

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Jeff Layton]

On Mon, 7 Feb 2011 11:42:05 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
> > a reliable indicator of whether the socket has had a NEGOTIATE done on
> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
> >
> > This is not the only way to fix this issue. As Steve pointed out to me,
> > we could also check to see whether there's a valid SMB session on the
> > list as an indicator of whether a NEGOTIATE has been done. I think the
> > approach I'm proposing clarifies the code better, but I'm willing to go
> > with his approach if that's the general concensus.
> 
> Jeff may be right that the code is clearer with this approach (I may try
> to code the alternative to check) but ... we really shouldn't be sending
> an SMBecho between negprot and sessionsetup anyway (so checking for
> sessionsetup may make sense). 
>  If a negprot times out or
> sessionsetup times out we don't need to be sending SMBecho
> (on the other hand perhaps a really slow krb5 sessionsetup?)
> Once a sessionsetup is sent, SMBechos make more sense.
> 

Why shouldn't we send one between the negotiate and session setup? The
MS-CIFS spec makes no mention that a session setup is required prior to
doing an echo. You may be correct, but that's not really what the spec
says.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Steve French]

On Mon, Feb 7, 2011 at 12:20 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 7 Feb 2011 11:42:05 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
>> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
>> > a reliable indicator of whether the socket has had a NEGOTIATE done on
>> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
>> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
>> >
>> > This is not the only way to fix this issue. As Steve pointed out to me,
>> > we could also check to see whether there's a valid SMB session on the
>> > list as an indicator of whether a NEGOTIATE has been done. I think the
>> > approach I'm proposing clarifies the code better, but I'm willing to go
>> > with his approach if that's the general concensus.
>>
>> Jeff may be right that the code is clearer with this approach (I may try
>> to code the alternative to check) but ... we really shouldn't be sending
>> an SMBecho between negprot and sessionsetup anyway (so checking for
>> sessionsetup may make sense).
>>  If a negprot times out or
>> sessionsetup times out we don't need to be sending SMBecho
>> (on the other hand perhaps a really slow krb5 sessionsetup?)
>> Once a sessionsetup is sent, SMBechos make more sense.
>>
>
> Why shouldn't we send one between the negotiate and session setup? The
> MS-CIFS spec makes no mention that a session setup is required prior to
> doing an echo. You may be correct, but that's not really what the spec
> says.

I don't remember ever having seen one between negprot and sessionsetup
from other clients - but the broader point is that an SMBecho doesn't
serve any purpose in between negprot/sessionsetup (once at least one
session is setup, checking if the server stays up is more interesting)


-- 
Thanks,

Steve

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Jeff Layton]

On Mon, 7 Feb 2011 12:36:22 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Feb 7, 2011 at 12:20 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, 7 Feb 2011 11:42:05 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
> >> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
> >> > a reliable indicator of whether the socket has had a NEGOTIATE done on
> >> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
> >> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
> >> >
> >> > This is not the only way to fix this issue. As Steve pointed out to me,
> >> > we could also check to see whether there's a valid SMB session on the
> >> > list as an indicator of whether a NEGOTIATE has been done. I think the
> >> > approach I'm proposing clarifies the code better, but I'm willing to go
> >> > with his approach if that's the general concensus.
> >>
> >> Jeff may be right that the code is clearer with this approach (I may try
> >> to code the alternative to check) but ... we really shouldn't be sending
> >> an SMBecho between negprot and sessionsetup anyway (so checking for
> >> sessionsetup may make sense).
> >>  If a negprot times out or
> >> sessionsetup times out we don't need to be sending SMBecho
> >> (on the other hand perhaps a really slow krb5 sessionsetup?)
> >> Once a sessionsetup is sent, SMBechos make more sense.
> >>
> >
> > Why shouldn't we send one between the negotiate and session setup? The
> > MS-CIFS spec makes no mention that a session setup is required prior to
> > doing an echo. You may be correct, but that's not really what the spec
> > says.
> 
> I don't remember ever having seen one between negprot and sessionsetup
> from other clients - but the broader point is that an SMBecho doesn't
> serve any purpose in between negprot/sessionsetup (once at least one
> session is setup, checking if the server stays up is more interesting)
> 

I'm not sure I agree there. There's nothing that says that the
NEGOTIATE and SESSION_SETUP have to be done within a certain amount of
time of each other. Checking the validity of the connection there seems
reasonable.

...but at this point, if you think checking for a valid session is the
best approach then so be it.

Practically, how do you suggest that we check for a valid SMB session?
I assume the code will need to walk the smb_ses_list. The ses->status
doesn't appear to actually change on reconnect however. It only sets
the "need_reconnect" boolean.

It appears that the ses->status is set to CifsGood and never changes
after the initial session setup. Perhaps that field should be removed?

We could replace the "need_reconnect" flag with an "established" flag,
that's a clear indicator of whether the session setup conversation is
complete.

...or is there a better way to do this?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Steve French]

On Mon, Feb 7, 2011 at 3:14 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 7 Feb 2011 12:36:22 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Mon, Feb 7, 2011 at 12:20 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Mon, 7 Feb 2011 11:42:05 -0600
>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
>> >> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
>> >> > a reliable indicator of whether the socket has had a NEGOTIATE done on
>> >> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
>> >> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
>> >> >
>> >> > This is not the only way to fix this issue. As Steve pointed out to me,
>> >> > we could also check to see whether there's a valid SMB session on the
>> >> > list as an indicator of whether a NEGOTIATE has been done. I think the
>> >> > approach I'm proposing clarifies the code better, but I'm willing to go
>> >> > with his approach if that's the general concensus.
>> >>
>> >> Jeff may be right that the code is clearer with this approach (I may try
>> >> to code the alternative to check) but ... we really shouldn't be sending
>> >> an SMBecho between negprot and sessionsetup anyway (so checking for
>> >> sessionsetup may make sense).
>> >>  If a negprot times out or
>> >> sessionsetup times out we don't need to be sending SMBecho
>> >> (on the other hand perhaps a really slow krb5 sessionsetup?)
>> >> Once a sessionsetup is sent, SMBechos make more sense.
>> >>
>> >
>> > Why shouldn't we send one between the negotiate and session setup? The
>> > MS-CIFS spec makes no mention that a session setup is required prior to
>> > doing an echo. You may be correct, but that's not really what the spec
>> > says.
>>
>> I don't remember ever having seen one between negprot and sessionsetup
>> from other clients - but the broader point is that an SMBecho doesn't
>> serve any purpose in between negprot/sessionsetup (once at least one
>> session is setup, checking if the server stays up is more interesting)
>>
>
> I'm not sure I agree there. There's nothing that says that the
> NEGOTIATE and SESSION_SETUP have to be done within a certain amount of
> time of each other. Checking the validity of the connection there seems
> reasonable.
>
> ...but at this point, if you think checking for a valid session is the
> best approach then so be it.

I am not convinced which way is best and it likely doesn't matter.

> Practically, how do you suggest that we check for a valid SMB session?
> I assume the code will need to walk the smb_ses_list. The ses->status
> doesn't appear to actually change on reconnect however. It only sets
> the "need_reconnect" boolean.
>
> It appears that the ses->status is set to CifsGood and never changes
> after the initial session setup. Perhaps that field should be removed?

aah - that makes it tougher to check.  If we don't mark ses->Status ==
NEED_RECONNECT
or something other than CifsGood then your approach is easier.

> We could replace the "need_reconnect" flag with an "established" flag,
> that's a clear indicator of whether the session setup conversation is
> complete.
>
> ...or is there a better way to do this?




-- 
Thanks,

Steve

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Jeff Layton]

On Mon, 7 Feb 2011 15:29:05 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Feb 7, 2011 at 3:14 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, 7 Feb 2011 12:36:22 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Mon, Feb 7, 2011 at 12:20 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Mon, 7 Feb 2011 11:42:05 -0600
> >> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> >> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> >> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
> >> >> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
> >> >> > a reliable indicator of whether the socket has had a NEGOTIATE done on
> >> >> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
> >> >> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
> >> >> >
> >> >> > This is not the only way to fix this issue. As Steve pointed out to me,
> >> >> > we could also check to see whether there's a valid SMB session on the
> >> >> > list as an indicator of whether a NEGOTIATE has been done. I think the
> >> >> > approach I'm proposing clarifies the code better, but I'm willing to go
> >> >> > with his approach if that's the general concensus.
> >> >>
> >> >> Jeff may be right that the code is clearer with this approach (I may try
> >> >> to code the alternative to check) but ... we really shouldn't be sending
> >> >> an SMBecho between negprot and sessionsetup anyway (so checking for
> >> >> sessionsetup may make sense).
> >> >>  If a negprot times out or
> >> >> sessionsetup times out we don't need to be sending SMBecho
> >> >> (on the other hand perhaps a really slow krb5 sessionsetup?)
> >> >> Once a sessionsetup is sent, SMBechos make more sense.
> >> >>
> >> >
> >> > Why shouldn't we send one between the negotiate and session setup? The
> >> > MS-CIFS spec makes no mention that a session setup is required prior to
> >> > doing an echo. You may be correct, but that's not really what the spec
> >> > says.
> >>
> >> I don't remember ever having seen one between negprot and sessionsetup
> >> from other clients - but the broader point is that an SMBecho doesn't
> >> serve any purpose in between negprot/sessionsetup (once at least one
> >> session is setup, checking if the server stays up is more interesting)
> >>
> >
> > I'm not sure I agree there. There's nothing that says that the
> > NEGOTIATE and SESSION_SETUP have to be done within a certain amount of
> > time of each other. Checking the validity of the connection there seems
> > reasonable.
> >
> > ...but at this point, if you think checking for a valid session is the
> > best approach then so be it.
> 
> I am not convinced which way is best and it likely doesn't matter.
> 
> > Practically, how do you suggest that we check for a valid SMB session?
> > I assume the code will need to walk the smb_ses_list. The ses->status
> > doesn't appear to actually change on reconnect however. It only sets
> > the "need_reconnect" boolean.
> >
> > It appears that the ses->status is set to CifsGood and never changes
> > after the initial session setup. Perhaps that field should be removed?
> 
> aah - that makes it tougher to check.  If we don't mark ses->Status ==
> NEED_RECONNECT
> or something other than CifsGood then your approach is easier.
> 

Well, we do mark a need_reconnect boolean which we could check, but
what about sessions that are just being established for the first time?
They typically don't go on the list until they are established, but I
don't really want to depend on that since that's really an
implementation detail.

The ad hoc nature of a lot of this code makes me worry that future
changes to it could break things. The fields that are intended to track
state are not clearly defined, so it's hard to depend on any of this
without clearly defining what the states actually represent. The
locking around the state variables doesn't follow any clear rules
either.

This code is probably ripe for cleanup, but I'm not going to embark on
that for 2.6.38. Would it be reasonable to take the patches I've
already proposed and we can discuss approaches to clean this code up
when we're at Connectathon in a few weeks?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 0/2] cifs: don't perform SMB echoes on un-negotiated socket

[Steve French]

On Mon, Feb 7, 2011 at 3:57 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 7 Feb 2011 15:29:05 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Mon, Feb 7, 2011 at 3:14 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Mon, 7 Feb 2011 12:36:22 -0600
>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> On Mon, Feb 7, 2011 at 12:20 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >> > On Mon, 7 Feb 2011 11:42:05 -0600
>> >> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >
>> >> >> On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aJhl2p70BpVqQ@public.gmane.orgm> wrote:
>> >> >> > Commit 247ec9b4 makes the SMB echo workqueue job skip doing echoes
>> >> >> > unless the tcpStatus == CifsGood. Problem: tcpStatus == CifsGood is not
>> >> >> > a reliable indicator of whether the socket has had a NEGOTIATE done on
>> >> >> > it. This fixes that by adding a new tcpStatus of CifsNeedNegotiate that
>> >> >> > indicates this. It also cleans up cifs_reconnect_tcon a bit.
>> >> >> >
>> >> >> > This is not the only way to fix this issue. As Steve pointed out to me,
>> >> >> > we could also check to see whether there's a valid SMB session on the
>> >> >> > list as an indicator of whether a NEGOTIATE has been done. I think the
>> >> >> > approach I'm proposing clarifies the code better, but I'm willing to go
>> >> >> > with his approach if that's the general concensus.
>> >> >>
>> >> >> Jeff may be right that the code is clearer with this approach (I may try
>> >> >> to code the alternative to check) but ... we really shouldn't be sending
>> >> >> an SMBecho between negprot and sessionsetup anyway (so checking for
>> >> >> sessionsetup may make sense).
>> >> >>  If a negprot times out or
>> >> >> sessionsetup times out we don't need to be sending SMBecho
>> >> >> (on the other hand perhaps a really slow krb5 sessionsetup?)
>> >> >> Once a sessionsetup is sent, SMBechos make more sense.
>> >> >>
>> >> >
>> >> > Why shouldn't we send one between the negotiate and session setup? The
>> >> > MS-CIFS spec makes no mention that a session setup is required prior to
>> >> > doing an echo. You may be correct, but that's not really what the spec
>> >> > says.
>> >>
>> >> I don't remember ever having seen one between negprot and sessionsetup
>> >> from other clients - but the broader point is that an SMBecho doesn't
>> >> serve any purpose in between negprot/sessionsetup (once at least one
>> >> session is setup, checking if the server stays up is more interesting)
>> >>
>> >
>> > I'm not sure I agree there. There's nothing that says that the
>> > NEGOTIATE and SESSION_SETUP have to be done within a certain amount of
>> > time of each other. Checking the validity of the connection there seems
>> > reasonable.
>> >
>> > ...but at this point, if you think checking for a valid session is the
>> > best approach then so be it.
>>
>> I am not convinced which way is best and it likely doesn't matter.
>>
>> > Practically, how do you suggest that we check for a valid SMB session?
>> > I assume the code will need to walk the smb_ses_list. The ses->status
>> > doesn't appear to actually change on reconnect however. It only sets
>> > the "need_reconnect" boolean.
>> >
>> > It appears that the ses->status is set to CifsGood and never changes
>> > after the initial session setup. Perhaps that field should be removed?
>>
>> aah - that makes it tougher to check.  If we don't mark ses->Status ==
>> NEED_RECONNECT
>> or something other than CifsGood then your approach is easier.
>>
>
> Well, we do mark a need_reconnect boolean which we could check, but
> what about sessions that are just being established for the first time?
> They typically don't go on the list until they are established, but I
> don't really want to depend on that since that's really an
> implementation detail.
>
> The ad hoc nature of a lot of this code makes me worry that future
> changes to it could break things. The fields that are intended to track
> state are not clearly defined, so it's hard to depend on any of this
> without clearly defining what the states actually represent. The
> locking around the state variables doesn't follow any clear rules
> either.
>
> This code is probably ripe for cleanup, but I'm not going to embark on
> that for 2.6.38. Would it be reasonable to take the patches I've
> already proposed and we can discuss approaches to clean this code up
> when we're at Connectathon in a few weeks?

Yes - I am leaning toward simply merging the (apparently) working patch
(it shouldn't take me long to finish review) -  that makes sense
rather than overoptimizing.   That the tcp connection establishment
follows a different state model than the smb session is ok with me
as long as we don't have old bugs to fix in it.   There are lots
of higher priority things to fix/enhance (including improving uid mapping
support and enhancing your multiuser code which is turning out
to be useful but has restrictions that make it harder for some
common scenarios).   And of course - the really important things:
1) cifs_readpages and cifs_writepages dispatch of multiple
requests in parallel, asynchronously (which should make cifs
close to optimal for large file copy)
2) smb2:  ... and the only way to get much better performance will be with
smb2 (where larger i/o sizes, and credits, and a better
oplock model make it better performing)
to get



-- 
Thanks,

Steve

Re: [PATCH 2/2] cifs: clarify the meaning of tcpStatus == CifsGood

[Steve French]

I discussed a simpler alternative with Jeff today.
maxBuf is set by SMB Negotiate Protocol, and when
reconnecting is reset to zero to indicate
that Negotiate Protocol needs to be done, so it
is a smaller patch to just check that
(we could eliminate the tcpStatus == CifsGood check
as well, but it is harmless and as it was tested/works
I lean toward simply leaving that in, and adding
the most trivial change).  See below:

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 257b6d8..10011e9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -341,7 +341,7 @@ cifs_echo_request(struct work_struct *work)
 	 * We cannot send an echo until the NEGOTIATE_PROTOCOL request is done.
 	 * Also, no need to ping if we got a response recently
 	 */
-	if (server->tcpStatus != CifsGood ||
+	if ((server->tcpStatus != CifsGood) || (server->maxBuf == 0) ||
 	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
 		goto requeue_echo;

On Mon, Feb 7, 2011 at 7:54 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> When the TCP_Server_Info is first allocated and connected, tcpStatus ==
> CifsGood means that the NEGOTIATE_PROTOCOL request has completed and the
> socket is ready for other calls. cifs_reconnect however sets tcpStatus
> to CifsGood as soon as the socket is reconnected and the optional
> RFC1001 session setup is done. We have no clear way to tell the
> difference between these two states, and we need to know this in order
> to know whether we can send an echo or not.
>
> Resolve this by adding a new statusEnum value -- CifsNeedNegotiate. When
> the socket has been connected but has not yet had a NEGOTIATE_PROTOCOL
> request done, set it to this value. Once the NEGOTIATE is done,
> cifs_negotiate_protocol will set tcpStatus to CifsGood.
>
> This also fixes and cleans the logic in cifs_reconnect and
> cifs_reconnect_tcon. The old code checked for specific states when what
> it really wants to know is whether the state has actually changed from
> CifsNeedReconnect.
>
> Reported-and-Tested-by: JG <jg-9fI4heB6ewM@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h |    3 ++-
>  fs/cifs/cifssmb.c  |    4 ++--
>  fs/cifs/connect.c  |    8 ++++----
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index edd5b29..5f333e0 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -92,7 +92,8 @@ enum statusEnum {
>        CifsNew = 0,
>        CifsGood,
>        CifsExiting,
> -       CifsNeedReconnect
> +       CifsNeedReconnect,
> +       CifsNeedNegotiate
>  };
>
>  enum securityEnum {
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 904aa47..f4bd502 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -142,9 +142,9 @@ cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
>         */
>        while (server->tcpStatus == CifsNeedReconnect) {
>                wait_event_interruptible_timeout(server->response_q,
> -                       (server->tcpStatus == CifsGood), 10 * HZ);
> +                       (server->tcpStatus != CifsNeedReconnect), 10 * HZ);
>
> -               /* is TCP session is reestablished now ?*/
> +               /* are we still trying to reconnect? */
>                if (server->tcpStatus != CifsNeedReconnect)
>                        break;
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 257b6d8..7deceae 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -199,8 +199,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>        }
>        spin_unlock(&GlobalMid_Lock);
>
> -       while ((server->tcpStatus != CifsExiting) &&
> -              (server->tcpStatus != CifsGood)) {
> +       while (server->tcpStatus == CifsNeedReconnect) {
>                try_to_freeze();
>
>                /* we should try only the port we connected to before */
> @@ -212,7 +211,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                        atomic_inc(&tcpSesReconnectCount);
>                        spin_lock(&GlobalMid_Lock);
>                        if (server->tcpStatus != CifsExiting)
> -                               server->tcpStatus = CifsGood;
> +                               server->tcpStatus = CifsNeedNegotiate;
>                        spin_unlock(&GlobalMid_Lock);
>                }
>        }
> @@ -420,7 +419,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>                pdu_length = 4; /* enough to get RFC1001 header */
>
>  incomplete_rcv:
> -               if (echo_retries > 0 &&
> +               if (echo_retries > 0 && server->tcpStatus == CifsGood &&
>                    time_after(jiffies, server->lstrp +
>                                        (echo_retries * SMB_ECHO_INTERVAL))) {
>                        cERROR(1, "Server %s has not responded in %d seconds. "
> @@ -1732,6 +1731,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>                cERROR(1, "Error connecting to socket. Aborting operation");
>                goto out_err_crypto_release;
>        }
> +       tcp_ses->tcpStatus = CifsNeedNegotiate;
>
>        /*
>         * since we're in a cifs function already, we know that
> --
> 1.7.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 2/2] cifs: clarify the meaning of tcpStatus == CifsGood

[Jeff Layton]

On Tue, 8 Feb 2011 17:50:33 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I discussed a simpler alternative with Jeff today.
> maxBuf is set by SMB Negotiate Protocol, and when
> reconnecting is reset to zero to indicate
> that Negotiate Protocol needs to be done, so it
> is a smaller patch to just check that
> (we could eliminate the tcpStatus == CifsGood check
> as well, but it is harmless and as it was tested/works
> I lean toward simply leaving that in, and adding
> the most trivial change).  See below:
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 257b6d8..10011e9 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -341,7 +341,7 @@ cifs_echo_request(struct work_struct *work)
>  	 * We cannot send an echo until the NEGOTIATE_PROTOCOL request is done.
>  	 * Also, no need to ping if we got a response recently
>  	 */
> -	if (server->tcpStatus != CifsGood ||
> +	if ((server->tcpStatus != CifsGood) || (server->maxBuf == 0) ||
>  	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
>  		goto requeue_echo;
> 

It's a reasonable fix that should work. I think we should remove the
check for tcpStatus != CifsGood since it's meaningless. I'd also like
to see the comment altered to mention why you're checking for
maxBuf == 0 as it's not at all obvious.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH] cifs: clean up checks in cifs_echo_request

[Jeff Layton]

Follow-on patch to 7e90d705 which is already in Steve's tree...

The check for tcpStatus == CifsGood is not meaningful since it doesn't
indicate whether the NEGOTIATE request has been done. Also, clarify
why we're checking for maxBuf == 0.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 10011e9..161f24c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -338,10 +338,11 @@ cifs_echo_request(struct work_struct *work)
 					struct TCP_Server_Info, echo.work);
 
 	/*
-	 * We cannot send an echo until the NEGOTIATE_PROTOCOL request is done.
-	 * Also, no need to ping if we got a response recently
+	 * We cannot send an echo until the NEGOTIATE_PROTOCOL request is
+	 * done, which is indicated by maxBuf != 0. Also, no need to ping if
+	 * we got a response recently
 	 */
-	if ((server->tcpStatus != CifsGood) || (server->maxBuf == 0) ||
+	if (server->maxBuf == 0 ||
 	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
 		goto requeue_echo;
 
-- 
1.7.4