diff mbox

cifs: consolidate reconnect logic in smb_init routines

Message ID 1250352387-5144-1-git-send-email-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton Aug. 15, 2009, 4:06 p.m. UTC 
There's a large cut and paste chunk of code in smb_init and
small_smb_init to handle reconnects. Break it out into a separate
function, clean it up and have both routines call it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifssmb.c |  314 +++++++++++++++++++++--------------------------------
 1 files changed, 124 insertions(+), 190 deletions(-)

Comments

Steve French Sept. 3, 2009, 1:38 a.m. UTC | #1 
On Sat, Aug 15, 2009 at 11:06 AM, Jeff Layton<jlayton@redhat.com> wrote:
> There's a large cut and paste chunk of code in smb_init and
> small_smb_init to handle reconnects. Break it out into a separate
> function, clean it up and have both routines call it.

As can be seen from the excerpt below, your patch does change behavior
(not just do cleanup) - not sure if that is what you intended.

In the old code, for handle based operations, when we have to
reconnect the socket in this path, after coming back from tcon
whether or not there was an error - the rc was converted
to EAGAIN - to signal the caller that he has to reopen the file (if
another reconnect is needed due to tcon failure that
will happen during the reopen).  The new code returns the tcon
error back to the caller for handle based operations (only returns
EAGAIN on handle based operations if tcon succeeds).

This will change behavior on paths when the session drops
and tcon reconnection fails - which may be ok - but needs to be
looked at.

> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 1866bc2..4bdca05 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
...
> +
> +       mark_open_files_invalid(tcon);
> +       rc = CIFSTCon(0, ses, tcon->treeName, tcon, nls_codepage);
> +       up(&ses->sesSem);
> +       cFYI(1, ("reconnect tcon rc = %d", rc));
> +
> +       if (rc)
> +               goto out;
....
> -                               mark_open_files_invalid(tcon);
> -                               rc = CIFSTCon(0, tcon->ses, tcon->treeName,
> -                                             tcon, nls_codepage);
> -                               up(&tcon->ses->sesSem);
> -                               /* BB FIXME add code to check if wsize needs
> -                               update due to negotiated smb buffer size
> -                               shrinking */
> -                               if (rc == 0) {
> -                                       atomic_inc(&tconInfoReconnectCount);
> -                                       /* tell server Unix caps we support */
> -                                       if (tcon->ses->capabilities & CAP_UNIX)
> -                                               reset_cifs_unix_caps(
> -                                               0 /* no xid */,
> -                                               tcon,
> -                                               NULL /* do not know sb */,
> -                                               NULL /* no vol info */);
> -                               }
> -
> -                               cFYI(1, ("reconnect tcon rc = %d", rc));
> -                               /* Removed call to reopen open files here.
> -                                  It is safer (and faster) to reopen files
> -                                  one at a time as needed in read and write */
> -
> -                               /* Check if handle based operation so we
> -                                  know whether we can continue or not without
> -                                  returning to caller to reset file handle */
> -                               switch (smb_command) {
> -                                       case SMB_COM_READ_ANDX:
> -                                       case SMB_COM_WRITE_ANDX:
> -                                       case SMB_COM_CLOSE:
> -                                       case SMB_COM_FIND_CLOSE2:
> -                                       case SMB_COM_LOCKING_ANDX: {
> -                                               unload_nls(nls_codepage);
> -                                               return -EAGAIN;
> -                                       }
Jeff Layton Sept. 3, 2009, 11:40 a.m. UTC | #2 
On Wed, 2 Sep 2009 20:38:49 -0500
Steve French <smfrench@gmail.com> wrote:

> On Sat, Aug 15, 2009 at 11:06 AM, Jeff Layton<jlayton@redhat.com> wrote:
> > There's a large cut and paste chunk of code in smb_init and
> > small_smb_init to handle reconnects. Break it out into a separate
> > function, clean it up and have both routines call it.
> 
> As can be seen from the excerpt below, your patch does change behavior
> (not just do cleanup) - not sure if that is what you intended.
> 
> In the old code, for handle based operations, when we have to
> reconnect the socket in this path, after coming back from tcon
> whether or not there was an error - the rc was converted
> to EAGAIN - to signal the caller that he has to reopen the file (if
> another reconnect is needed due to tcon failure that
> will happen during the reopen).  The new code returns the tcon
> error back to the caller for handle based operations (only returns
> EAGAIN on handle based operations if tcon succeeds).
> 
> This will change behavior on paths when the session drops
> and tcon reconnection fails - which may be ok - but needs to be
> looked at.
> 

Good catch. Yeah, that might be an issue. I'll move the "out:" label
above the switch statement and that should correct it. When I have some
time, I'll retest and resend.

That said, I'm not sure the design here is quite right. If it always
returns -EAGAIN to the caller on a handle-based operation when the tcon
fails isn't that going to loop indefinitely when there's a hard failure
with the tcon?

Also, I'm not sure we have the list of handle-based ops correct. What
about the SetFileInfo calls?

> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index 1866bc2..4bdca05 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> ...
> > +
> > +       mark_open_files_invalid(tcon);
> > +       rc = CIFSTCon(0, ses, tcon->treeName, tcon, nls_codepage);
> > +       up(&ses->sesSem);
> > +       cFYI(1, ("reconnect tcon rc = %d", rc));
> > +
> > +       if (rc)
> > +               goto out;
> ....
> > -                               mark_open_files_invalid(tcon);
> > -                               rc = CIFSTCon(0, tcon->ses, tcon->treeName,
> > -                                             tcon, nls_codepage);
> > -                               up(&tcon->ses->sesSem);
> > -                               /* BB FIXME add code to check if wsize needs
> > -                               update due to negotiated smb buffer size
> > -                               shrinking */
> > -                               if (rc == 0) {
> > -                                       atomic_inc(&tconInfoReconnectCount);
> > -                                       /* tell server Unix caps we support */
> > -                                       if (tcon->ses->capabilities & CAP_UNIX)
> > -                                               reset_cifs_unix_caps(
> > -                                               0 /* no xid */,
> > -                                               tcon,
> > -                                               NULL /* do not know sb */,
> > -                                               NULL /* no vol info */);
> > -                               }
> > -
> > -                               cFYI(1, ("reconnect tcon rc = %d", rc));
> > -                               /* Removed call to reopen open files here.
> > -                                  It is safer (and faster) to reopen files
> > -                                  one at a time as needed in read and write */
> > -
> > -                               /* Check if handle based operation so we
> > -                                  know whether we can continue or not without
> > -                                  returning to caller to reset file handle */
> > -                               switch (smb_command) {
> > -                                       case SMB_COM_READ_ANDX:
> > -                                       case SMB_COM_WRITE_ANDX:
> > -                                       case SMB_COM_CLOSE:
> > -                                       case SMB_COM_FIND_CLOSE2:
> > -                                       case SMB_COM_LOCKING_ANDX: {
> > -                                               unload_nls(nls_codepage);
> > -                                               return -EAGAIN;
> > -                                       }
> 
> 
> -- 
> Thanks,
> 
> Steve
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
Steve French Sept. 3, 2009, 2 p.m. UTC | #3 
On Thu, Sep 3, 2009 at 6:40 AM, Jeff Layton<jlayton@redhat.com> wrote:
> On Wed, 2 Sep 2009 20:38:49 -0500
> Steve French <smfrench@gmail.com> wrote:
>> EAGAIN on handle based operations if tcon succeeds).
>>
>> This will change behavior on paths when the session drops
>> and tcon reconnection fails - which may be ok - but needs to be
>> looked at.
>>
>
> Good catch. Yeah, that might be an issue. I'll move the "out:" label
> above the switch statement and that should correct it. When I have some
> time, I'll retest and resend.
>
> That said, I'm not sure the design here is quite right. If it always
> returns -EAGAIN to the caller on a handle-based operation when the tcon
> fails isn't that going to loop indefinitely when there's a hard failure
> with the tcon?
>
> Also, I'm not sure we have the list of handle-based ops correct. What
> about the SetFileInfo calls?

Yes - I forgot to mention a related point in the earlier email.
Are we handling checking for SMB close and SMB findclose correctly -
do we have code in the caller that turns EAGAIN into rc = 0
(if the session dropped, the handle is already closed).
Jeff Layton Sept. 3, 2009, 2:24 p.m. UTC | #4 
On Thu, 3 Sep 2009 09:00:35 -0500
Steve French <smfrench@gmail.com> wrote:

> On Thu, Sep 3, 2009 at 6:40 AM, Jeff Layton<jlayton@redhat.com> wrote:
> > On Wed, 2 Sep 2009 20:38:49 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >> EAGAIN on handle based operations if tcon succeeds).
> >>
> >> This will change behavior on paths when the session drops
> >> and tcon reconnection fails - which may be ok - but needs to be
> >> looked at.
> >>
> >
> > Good catch. Yeah, that might be an issue. I'll move the "out:" label
> > above the switch statement and that should correct it. When I have some
> > time, I'll retest and resend.
> >
> > That said, I'm not sure the design here is quite right. If it always
> > returns -EAGAIN to the caller on a handle-based operation when the tcon
> > fails isn't that going to loop indefinitely when there's a hard failure
> > with the tcon?
> >
> > Also, I'm not sure we have the list of handle-based ops correct. What
> > about the SetFileInfo calls?
> 
> Yes - I forgot to mention a related point in the earlier email.
> Are we handling checking for SMB close and SMB findclose correctly -
> do we have code in the caller that turns EAGAIN into rc = 0
> (if the session dropped, the handle is already closed).
> 
> 

Yes, that probably needs to be looked at as well. Either way though, I
suggest we shoot for no behavioral changes with the first patch and then
plan to fix what needs fixing once there aren't 2 copies of the
same code.
Steve French Sept. 3, 2009, 2:59 p.m. UTC | #5 
On Thu, Sep 3, 2009 at 9:24 AM, Jeff Layton<jlayton@redhat.com> wrote:
> On Thu, 3 Sep 2009 09:00:35 -0500
>> > Also, I'm not sure we have the list of handle-based ops correct. What
>> > about the SetFileInfo calls?
>>
>> Yes - I forgot to mention a related point in the earlier email.
>> Are we handling checking for SMB close and SMB findclose correctly -
>> do we have code in the caller that turns EAGAIN into rc = 0
>> (if the session dropped, the handle is already closed).
>>
>>
>
> Yes, that probably needs to be looked at as well. Either way though, I
> suggest we shoot for no behavioral changes with the first patch and then
> plan to fix what needs fixing once there aren't 2 copies of the
> same code.

Agreed
diff mbox

Patch

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 1866bc2..4bdca05 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -100,110 +100,138 @@  static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
 	   to this tcon */
 }
 
-/* Allocate and return pointer to an SMB request buffer, and set basic
-   SMB information in the SMB header.  If the return code is zero, this
-   function must have filled in request_buf pointer */
+/* reconnect the socket, tcon, and smb session if needed */
 static int
-small_smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
-		void **request_buf)
+cifs_reconnect_tcon(struct cifsTconInfo *tcon, int smb_command)
 {
 	int rc = 0;
+	struct cifsSesInfo *ses;
+	struct TCP_Server_Info *server;
+	struct nls_table *nls_codepage;
 
-	/* SMBs NegProt, SessSetup, uLogoff do not have tcon yet so
-	   check for tcp and smb session status done differently
-	   for those three - in the calling routine */
-	if (tcon) {
-		if (tcon->tidStatus == CifsExiting) {
-			/* only tree disconnect, open, and write,
-			(and ulogoff which does not have tcon)
-			are allowed as we start force umount */
-			if ((smb_command != SMB_COM_WRITE_ANDX) &&
-			   (smb_command != SMB_COM_OPEN_ANDX) &&
-			   (smb_command != SMB_COM_TREE_DISCONNECT)) {
-				cFYI(1, ("can not send cmd %d while umounting",
-					smb_command));
-				return -ENODEV;
-			}
+	/*
+	 * SMBs NegProt, SessSetup, uLogoff do not have tcon yet so check for
+	 * tcp and smb session status done differently for those three - in the
+	 * calling routine
+	 */
+	if (!tcon)
+		return 0;
+
+	ses = tcon->ses;
+	server = ses->server;
+
+	/*
+	 * only tree disconnect, open, and write, (and ulogoff which does not
+	 * have tcon) are allowed as we start force umount
+	 */
+	if (tcon->tidStatus == CifsExiting) {
+		if (smb_command != SMB_COM_WRITE_ANDX &&
+		    smb_command != SMB_COM_OPEN_ANDX &&
+		    smb_command != SMB_COM_TREE_DISCONNECT) {
+			cFYI(1, ("can not send cmd %d while umounting",
+				smb_command));
+			return -ENODEV;
 		}
-		if ((tcon->ses) && (tcon->ses->status != CifsExiting) &&
-				  (tcon->ses->server)) {
-			struct nls_table *nls_codepage;
-				/* Give Demultiplex thread up to 10 seconds to
-				   reconnect, should be greater than cifs socket
-				   timeout which is 7 seconds */
-			while (tcon->ses->server->tcpStatus ==
-							 CifsNeedReconnect) {
-				wait_event_interruptible_timeout(tcon->ses->server->response_q,
-					(tcon->ses->server->tcpStatus ==
-							CifsGood), 10 * HZ);
-				if (tcon->ses->server->tcpStatus ==
-							CifsNeedReconnect) {
-					/* on "soft" mounts we wait once */
-					if (!tcon->retry ||
-					   (tcon->ses->status == CifsExiting)) {
-						cFYI(1, ("gave up waiting on "
-						      "reconnect in smb_init"));
-						return -EHOSTDOWN;
-					} /* else "hard" mount - keep retrying
-					     until process is killed or server
-					     comes back on-line */
-				} else /* TCP session is reestablished now */
-					break;
-			}
+	}
 
-			nls_codepage = load_nls_default();
-		/* need to prevent multiple threads trying to
-		simultaneously reconnect the same SMB session */
-			down(&tcon->ses->sesSem);
-			if (tcon->ses->need_reconnect)
-				rc = cifs_setup_session(0, tcon->ses,
-							nls_codepage);
-			if (!rc && (tcon->need_reconnect)) {
-				mark_open_files_invalid(tcon);
-				rc = CIFSTCon(0, tcon->ses, tcon->treeName,
-					      tcon, nls_codepage);
-				up(&tcon->ses->sesSem);
-				/* BB FIXME add code to check if wsize needs
-				   update due to negotiated smb buffer size
-				   shrinking */
-				if (rc == 0) {
-					atomic_inc(&tconInfoReconnectCount);
-					/* tell server Unix caps we support */
-					if (tcon->ses->capabilities & CAP_UNIX)
-						reset_cifs_unix_caps(
-						0 /* no xid */,
-						tcon,
-						NULL /* we do not know sb */,
-						NULL /* no vol info */);
-				}
+	if (ses->status == CifsExiting)
+		return -EIO;
 
-				cFYI(1, ("reconnect tcon rc = %d", rc));
-				/* Removed call to reopen open files here.
-				   It is safer (and faster) to reopen files
-				   one at a time as needed in read and write */
-
-				/* Check if handle based operation so we
-				   know whether we can continue or not without
-				   returning to caller to reset file handle */
-				switch (smb_command) {
-					case SMB_COM_READ_ANDX:
-					case SMB_COM_WRITE_ANDX:
-					case SMB_COM_CLOSE:
-					case SMB_COM_FIND_CLOSE2:
-					case SMB_COM_LOCKING_ANDX: {
-						unload_nls(nls_codepage);
-						return -EAGAIN;
-					}
-				}
-			} else {
-				up(&tcon->ses->sesSem);
-			}
-			unload_nls(nls_codepage);
+	/*
+	 * Give demultiplex thread up to 10 seconds to reconnect, should be
+	 * greater than cifs socket timeout which is 7 seconds
+	 */
+	while (server->tcpStatus == CifsNeedReconnect) {
+		wait_event_interruptible_timeout(server->response_q,
+			(server->tcpStatus == CifsGood), 10 * HZ);
 
-		} else {
-			return -EIO;
+		/* is TCP session is reestablished now ?*/
+		if (server->tcpStatus != CifsNeedReconnect)
+			break;
+
+		/*
+		 * on "soft" mounts we wait once. Hard mounts keep
+		 * retrying until process is killed or server comes
+		 * back on-line
+		 */
+		if (!tcon->retry || ses->status == CifsExiting) {
+			cFYI(1, ("gave up waiting on reconnect in smb_init"));
+			return -EHOSTDOWN;
 		}
 	}
+
+	if (!ses->need_reconnect && !tcon->need_reconnect)
+		return 0;
+
+	nls_codepage = load_nls_default();
+
+	/*
+	 * need to prevent multiple threads trying to simultaneously
+	 * reconnect the same SMB session
+	 */
+	down(&ses->sesSem);
+	if (ses->need_reconnect)
+		rc = cifs_setup_session(0, ses, nls_codepage);
+
+	/* do we need to reconnect tcon? */
+	if (rc || !tcon->need_reconnect) {
+		up(&ses->sesSem);
+		goto out;
+	}
+
+	mark_open_files_invalid(tcon);
+	rc = CIFSTCon(0, ses, tcon->treeName, tcon, nls_codepage);
+	up(&ses->sesSem);
+	cFYI(1, ("reconnect tcon rc = %d", rc));
+
+	if (rc)
+		goto out;
+
+	/*
+	 * FIXME: check if wsize needs updated due to negotiated smb buffer
+	 * 	  size shrinking
+	 */
+	atomic_inc(&tconInfoReconnectCount);
+
+	/* tell server Unix caps we support */
+	if (ses->capabilities & CAP_UNIX)
+		reset_cifs_unix_caps(0, tcon, NULL, NULL);
+
+	/*
+	 * Removed call to reopen open files here. It is safer (and faster) to
+	 * reopen files one at a time as needed in read and write.
+	 *
+	 * FIXME: what about file locks? don't we need to reclaim them ASAP?
+	 */
+
+	/*
+	 * Check if handle based operation so we know whether we can continue
+	 * or not without returning to caller to reset file handle
+	 */
+	switch (smb_command) {
+		case SMB_COM_READ_ANDX:
+		case SMB_COM_WRITE_ANDX:
+		case SMB_COM_CLOSE:
+		case SMB_COM_FIND_CLOSE2:
+		case SMB_COM_LOCKING_ANDX:
+			rc = -EAGAIN;
+	}
+
+out:
+	unload_nls(nls_codepage);
+	return rc;
+}
+
+/* Allocate and return pointer to an SMB request buffer, and set basic
+   SMB information in the SMB header.  If the return code is zero, this
+   function must have filled in request_buf pointer */
+static int
+small_smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
+		void **request_buf)
+{
+	int rc = 0;
+
+	rc = cifs_reconnect_tcon(tcon, smb_command);
 	if (rc)
 		return rc;
 
@@ -256,101 +284,7 @@  smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
 {
 	int rc = 0;
 
-	/* SMBs NegProt, SessSetup, uLogoff do not have tcon yet so
-	   check for tcp and smb session status done differently
-	   for those three - in the calling routine */
-	if (tcon) {
-		if (tcon->tidStatus == CifsExiting) {
-			/* only tree disconnect, open, and write,
-			  (and ulogoff which does not have tcon)
-			  are allowed as we start force umount */
-			if ((smb_command != SMB_COM_WRITE_ANDX) &&
-			   (smb_command != SMB_COM_OPEN_ANDX) &&
-			   (smb_command != SMB_COM_TREE_DISCONNECT)) {
-				cFYI(1, ("can not send cmd %d while umounting",
-					smb_command));
-				return -ENODEV;
-			}
-		}
-
-		if ((tcon->ses) && (tcon->ses->status != CifsExiting) &&
-				  (tcon->ses->server)) {
-			struct nls_table *nls_codepage;
-				/* Give Demultiplex thread up to 10 seconds to
-				   reconnect, should be greater than cifs socket
-				   timeout which is 7 seconds */
-			while (tcon->ses->server->tcpStatus ==
-							CifsNeedReconnect) {
-				wait_event_interruptible_timeout(tcon->ses->server->response_q,
-					(tcon->ses->server->tcpStatus ==
-							CifsGood), 10 * HZ);
-				if (tcon->ses->server->tcpStatus ==
-						CifsNeedReconnect) {
-					/* on "soft" mounts we wait once */
-					if (!tcon->retry ||
-					   (tcon->ses->status == CifsExiting)) {
-						cFYI(1, ("gave up waiting on "
-						      "reconnect in smb_init"));
-						return -EHOSTDOWN;
-					} /* else "hard" mount - keep retrying
-					     until process is killed or server
-					     comes on-line */
-				} else /* TCP session is reestablished now */
-					break;
-			}
-			nls_codepage = load_nls_default();
-		/* need to prevent multiple threads trying to
-		simultaneously reconnect the same SMB session */
-			down(&tcon->ses->sesSem);
-			if (tcon->ses->need_reconnect)
-				rc = cifs_setup_session(0, tcon->ses,
-							nls_codepage);
-			if (!rc && (tcon->need_reconnect)) {
-				mark_open_files_invalid(tcon);
-				rc = CIFSTCon(0, tcon->ses, tcon->treeName,
-					      tcon, nls_codepage);
-				up(&tcon->ses->sesSem);
-				/* BB FIXME add code to check if wsize needs
-				update due to negotiated smb buffer size
-				shrinking */
-				if (rc == 0) {
-					atomic_inc(&tconInfoReconnectCount);
-					/* tell server Unix caps we support */
-					if (tcon->ses->capabilities & CAP_UNIX)
-						reset_cifs_unix_caps(
-						0 /* no xid */,
-						tcon,
-						NULL /* do not know sb */,
-						NULL /* no vol info */);
-				}
-
-				cFYI(1, ("reconnect tcon rc = %d", rc));
-				/* Removed call to reopen open files here.
-				   It is safer (and faster) to reopen files
-				   one at a time as needed in read and write */
-
-				/* Check if handle based operation so we
-				   know whether we can continue or not without
-				   returning to caller to reset file handle */
-				switch (smb_command) {
-					case SMB_COM_READ_ANDX:
-					case SMB_COM_WRITE_ANDX:
-					case SMB_COM_CLOSE:
-					case SMB_COM_FIND_CLOSE2:
-					case SMB_COM_LOCKING_ANDX: {
-						unload_nls(nls_codepage);
-						return -EAGAIN;
-					}
-				}
-			} else {
-				up(&tcon->ses->sesSem);
-			}
-			unload_nls(nls_codepage);
-
-		} else {
-			return -EIO;
-		}
-	}
+	rc = cifs_reconnect_tcon(tcon, smb_command);
 	if (rc)
 		return rc;