[v1,04/13] cifs: split out a cifs_connect_session_locked() helper function

Commit Message

Stefan Metzmacher Feb. 24, 2020, 1:15 p.m. UTC

Signed-off-by: Stefan Metzmacher <[e-mail]>
---
 fs/cifs/cifsproto.h |  8 +++----
 fs/cifs/cifssmb.c   | 13 +++---------
 fs/cifs/connect.c   | 51 +++++++++++++++++++++++++++++----------------
 fs/cifs/sess.c      |  9 ++++----
 fs/cifs/smb2pdu.c   | 21 +++++--------------
 5 files changed, 49 insertions(+), 53 deletions(-)

Comments

Aurélien Aptel March 6, 2020, 5:05 p.m. UTC | #1

This LGTM but I think we should really test it on the
buildbot. Especially DFS failover and multichannel.

Stefan Metzmacher <[e-mail]> writes:
> +int
> +cifs_connect_session_locked(const unsigned int xid,
> +			    struct cifs_ses *ses,
> +			    struct nls_table *nls_info,
> +			    bool retry)
> +{
> +	int rc;
> +
> +	if (ses->server->tcpStatus == CifsNeedReconnect) {
> +		return -EHOSTDOWN;
> +	}

This check is now done everytime. Probably it's correct, but worth
pointing out.

Patch

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 89eaaf46d1ca..e2624ebad189 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -263,10 +263,10 @@  extern void cifs_move_llist(struct list_head *source, struct list_head *dest);
 extern void cifs_free_llist(struct list_head *llist);
 extern void cifs_del_lock_waiters(struct cifsLockInfo *lock);
 
-extern int cifs_negotiate_protocol(const unsigned int xid,
-				   struct cifs_ses *ses);
-extern int cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
-			      struct nls_table *nls_info);
+extern int cifs_connect_session_locked(const unsigned int xid,
+				       struct cifs_ses *ses,
+				       struct nls_table *nls_info,
+				       bool retry);
 extern int cifs_enable_signing(struct TCP_Server_Info *server, bool mnt_sign_required);
 extern int CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses);
 
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index d2b92770384b..dcb009fade8c 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -291,16 +291,9 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	 * and the server never sends an answer the socket will be closed
 	 * and tcpStatus set to reconnect.
 	 */
-	if (server->tcpStatus == CifsNeedReconnect) {
-		rc = -EHOSTDOWN;
-		mutex_unlock(&ses->session_mutex);
-		goto out;
-	}
-
-	rc = cifs_negotiate_protocol(0, ses);
-	if (rc == 0 && ses->need_reconnect)
-		rc = cifs_setup_session(0, ses, nls_codepage);
-
+	rc = cifs_connect_session_locked(0, ses,
+					 nls_codepage,
+					 tcon->retry);
 	/* do we need to reconnect tcon? */
 	if (rc || !tcon->need_reconnect) {
 		mutex_unlock(&ses->session_mutex);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 25df928b3ca0..34269ef40774 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3288,7 +3288,9 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 			 ses->status);
 
 		mutex_lock(&ses->session_mutex);
-		rc = cifs_negotiate_protocol(xid, ses);
+		rc = cifs_connect_session_locked(xid, ses,
+						 volume_info->local_nls,
+						 true /* retry */);
 		if (rc) {
 			mutex_unlock(&ses->session_mutex);
 			/* problem -- put our ses reference */
@@ -3296,18 +3298,6 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 			free_xid(xid);
 			return ERR_PTR(rc);
 		}
-		if (ses->need_reconnect) {
-			cifs_dbg(FYI, "Session needs reconnect\n");
-			rc = cifs_setup_session(xid, ses,
-						volume_info->local_nls);
-			if (rc) {
-				mutex_unlock(&ses->session_mutex);
-				/* problem -- put our reference */
-				cifs_put_smb_ses(ses);
-				free_xid(xid);
-				return ERR_PTR(rc);
-			}
-		}
 		mutex_unlock(&ses->session_mutex);
 
 		/* existing SMB ses has a server reference already */
@@ -3359,9 +3349,10 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 	ses->chan_count = 1;
 	ses->chan_max = volume_info->multichannel ? volume_info->max_channels:1;
 
-	rc = cifs_negotiate_protocol(xid, ses);
-	if (!rc)
-		rc = cifs_setup_session(xid, ses, volume_info->local_nls);
+	ses->need_reconnect = true;
+	rc = cifs_connect_session_locked(xid, ses,
+					 volume_info->local_nls,
+					 true /* retry */);
 
 	/* each channel uses a different signing key */
 	memcpy(ses->chans[0].signkey, ses->smb3signingkey,
@@ -5240,7 +5231,7 @@  cifs_umount(struct cifs_sb_info *cifs_sb)
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }
 
-int
+static int
 cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses)
 {
 	int rc = 0;
@@ -5266,7 +5257,7 @@  cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses)
 	return rc;
 }
 
-int
+static int
 cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
 		   struct nls_table *nls_info)
 {
@@ -5299,6 +5290,30 @@  cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
 	return rc;
 }
 
+int
+cifs_connect_session_locked(const unsigned int xid,
+			    struct cifs_ses *ses,
+			    struct nls_table *nls_info,
+			    bool retry)
+{
+	int rc;
+
+	if (ses->server->tcpStatus == CifsNeedReconnect) {
+		return -EHOSTDOWN;
+	}
+
+	rc = cifs_negotiate_protocol(xid, ses);
+	if (!rc && (ses->need_reconnect || ses->binding)) {
+		cifs_dbg(FYI, "Session needs reconnect\n");
+		rc = cifs_setup_session(xid, ses, nls_info);
+		if ((rc == -EACCES) && !retry) {
+			return -EHOSTDOWN;
+		}
+	}
+
+	return rc;
+}
+
 static int
 cifs_set_vol_auth(struct smb_vol *vol, struct cifs_ses *ses)
 {
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 43a88e26d26b..d38a6769d8aa 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -252,11 +252,10 @@  cifs_ses_add_channel(struct cifs_ses *ses, struct cifs_server_iface *iface)
 	}
 
 	ses->binding = true;
-	rc = cifs_negotiate_protocol(xid, ses);
-	if (rc)
-		goto out;
-
-	rc = cifs_setup_session(xid, ses, vol.local_nls);
+	rc = cifs_connect_session_locked(xid,
+					 ses,
+					 vol.local_nls,
+					 true /* retry */);
 	if (rc)
 		goto out;
 
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 2495c3021772..a15a53d2e808 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -343,21 +343,10 @@  smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon)
 	 * and the server never sends an answer the socket will be closed
 	 * and tcpStatus set to reconnect.
 	 */
-	if (server->tcpStatus == CifsNeedReconnect) {
-		rc = -EHOSTDOWN;
-		mutex_unlock(&tcon->ses->session_mutex);
-		goto out;
-	}
-
-	rc = cifs_negotiate_protocol(0, tcon->ses);
-	if (!rc && tcon->ses->need_reconnect) {
-		rc = cifs_setup_session(0, tcon->ses, nls_codepage);
-		if ((rc == -EACCES) && !tcon->retry) {
-			rc = -EHOSTDOWN;
-			mutex_unlock(&tcon->ses->session_mutex);
-			goto failed;
-		}
-	}
+	rc = cifs_connect_session_locked(0, tcon->ses,
+					 nls_codepage,
+					 tcon->retry);
+	/* do we need to reconnect tcon? */
 	if (rc || !tcon->need_reconnect) {
 		mutex_unlock(&tcon->ses->session_mutex);
 		goto out;
@@ -402,7 +391,7 @@  smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon)
 	case SMB2_SET_INFO:
 		rc = -EAGAIN;
 	}
-failed:
+
 	unload_nls(nls_codepage);
 	return rc;
 }