Minor cleanup of compound_send_recv

Commit Message

Steve French June 24, 2019, 6:29 p.m. UTC

In Aurelien's earlier patch series I noticed a cleanup (converting
ses->server to a local variable server=ses->server) which made code
easier to read in this function.  This doesn't require compounding but
probably helps his

Comments

ronnie sahlberg June 24, 2019, 9:41 p.m. UTC | #1

On Tue, Jun 25, 2019 at 7:24 AM Steve French <[e-mail]> wrote:
>
> In Aurelien's earlier patch series I noticed a cleanup (converting
> ses->server to a local variable server=ses->server) which made code
> easier to read in this function.  This doesn't require compounding but
> probably helps his
>

LGTM.

Possibly initialize server where you declare it as we do that for
*server almost everywhere else :

+ struct TCP_Server_Info *server = ses->server;

Reviewed-by: Ronnie Sahlberg <[e-mail]>

>
>
> --
> Thanks,
>
> Steve

Steve French June 24, 2019, 11:53 p.m. UTC | #2

On Mon, Jun 24, 2019 at 4:41 PM ronnie sahlberg
<[e-mail]> wrote:
>
> On Tue, Jun 25, 2019 at 7:24 AM Steve French <[e-mail]> wrote:
> >
> > In Aurelien's earlier patch series I noticed a cleanup (converting
> > ses->server to a local variable server=ses->server) which made code
> > easier to read in this function.  This doesn't require compounding but
> > probably helps his
> >
>
> LGTM.
>
> Possibly initialize server where you declare it as we do that for
> *server almost everywhere else :
>
> + struct TCP_Server_Info *server = ses->server;

I didn't set it there because later in the function we check:

           if ((ses == NULL) || (ses->server == NULL))

so I need to make sure the assignment is after the null check
(even if the null check is superfluous it would trigger a static check
warning to initialize server before the null check)

Aurélien Aptel July 1, 2019, 12:46 p.m. UTC | #3

Hi,

I was away for 2 weeks, reading through all my emails now.

Steve French <[e-mail]> writes:
> In Aurelien's earlier patch series I noticed a cleanup (converting
> ses->server to a local variable server=ses->server) which made code
> easier to read in this function.  This doesn't require compounding but
> probably helps his

Assuming we keep my approach that is a good idea, thanks.

Patch

From 1135a5fc08f45f918a68b3a604ae347cd70f3921 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <[e-mail]>
Date: Mon, 24 Jun 2019 13:00:12 -0500
Subject: [PATCH] smb3: minor cleanup of compound_send_recv

Trivial cleanup. Will make future multichannel code smaller
as well.

Signed-off-by: Aurelien Aptel <[e-mail]>
---
 fs/cifs/transport.c | 46 +++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 60661b3f983a..5d6d44bfe10a 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -979,6 +979,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	};
 	unsigned int instance;
 	char *buf;
+	struct TCP_Server_Info *server;
 
 	optype = flags & CIFS_OP_MASK;
 
@@ -990,7 +991,8 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		return -EIO;
 	}
 
-	if (ses->server->tcpStatus == CifsExiting)
+	server = ses->server;
+	if (server->tcpStatus == CifsExiting)
 		return -ENOENT;
 
 	/*
@@ -1001,7 +1003,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	 * other requests.
 	 * This can be handled by the eventual session reconnect.
 	 */
-	rc = wait_for_compound_request(ses->server, num_rqst, flags,
+	rc = wait_for_compound_request(server, num_rqst, flags,
 				       &instance);
 	if (rc)
 		return rc;
@@ -1017,7 +1019,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	 * of smb data.
 	 */
 
-	mutex_lock(&ses->server->srv_mutex);
+	mutex_lock(&server->srv_mutex);
 
 	/*
 	 * All the parts of the compound chain belong obtained credits from the
@@ -1026,24 +1028,24 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	 * we obtained credits and return -EAGAIN in such cases to let callers
 	 * handle it.
 	 */
-	if (instance != ses->server->reconnect_instance) {
-		mutex_unlock(&ses->server->srv_mutex);
+	if (instance != server->reconnect_instance) {
+		mutex_unlock(&server->srv_mutex);
 		for (j = 0; j < num_rqst; j++)
-			add_credits(ses->server, &credits[j], optype);
+			add_credits(server, &credits[j], optype);
 		return -EAGAIN;
 	}
 
 	for (i = 0; i < num_rqst; i++) {
-		midQ[i] = ses->server->ops->setup_request(ses, &rqst[i]);
+		midQ[i] = server->ops->setup_request(ses, &rqst[i]);
 		if (IS_ERR(midQ[i])) {
-			revert_current_mid(ses->server, i);
+			revert_current_mid(server, i);
 			for (j = 0; j < i; j++)
 				cifs_delete_mid(midQ[j]);
-			mutex_unlock(&ses->server->srv_mutex);
+			mutex_unlock(&server->srv_mutex);
 
 			/* Update # of requests on wire to server */
 			for (j = 0; j < num_rqst; j++)
-				add_credits(ses->server, &credits[j], optype);
+				add_credits(server, &credits[j], optype);
 			return PTR_ERR(midQ[i]);
 		}
 
@@ -1059,19 +1061,19 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		else
 			midQ[i]->callback = cifs_compound_last_callback;
 	}
-	cifs_in_send_inc(ses->server);
-	rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
-	cifs_in_send_dec(ses->server);
+	cifs_in_send_inc(server);
+	rc = smb_send_rqst(server, num_rqst, rqst, flags);
+	cifs_in_send_dec(server);
 
 	for (i = 0; i < num_rqst; i++)
 		cifs_save_when_sent(midQ[i]);
 
 	if (rc < 0) {
-		revert_current_mid(ses->server, num_rqst);
-		ses->server->sequence_number -= 2;
+		revert_current_mid(server, num_rqst);
+		server->sequence_number -= 2;
 	}
 
-	mutex_unlock(&ses->server->srv_mutex);
+	mutex_unlock(&server->srv_mutex);
 
 	/*
 	 * If sending failed for some reason or it is an oplock break that we
@@ -1079,7 +1081,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	 */
 	if (rc < 0 || (flags & CIFS_NO_SRV_RSP)) {
 		for (i = 0; i < num_rqst; i++)
-			add_credits(ses->server, &credits[i], optype);
+			add_credits(server, &credits[i], optype);
 		goto out;
 	}
 
@@ -1099,7 +1101,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 					   rqst[0].rq_nvec);
 
 	for (i = 0; i < num_rqst; i++) {
-		rc = wait_for_response(ses->server, midQ[i]);
+		rc = wait_for_response(server, midQ[i]);
 		if (rc != 0)
 			break;
 	}
@@ -1107,7 +1109,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		for (; i < num_rqst; i++) {
 			cifs_dbg(VFS, "Cancelling wait for mid %llu cmd: %d\n",
 				 midQ[i]->mid, le16_to_cpu(midQ[i]->command));
-			send_cancel(ses->server, &rqst[i], midQ[i]);
+			send_cancel(server, &rqst[i], midQ[i]);
 			spin_lock(&GlobalMid_Lock);
 			if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
 				midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
@@ -1123,7 +1125,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		if (rc < 0)
 			goto out;
 
-		rc = cifs_sync_mid_result(midQ[i], ses->server);
+		rc = cifs_sync_mid_result(midQ[i], server);
 		if (rc != 0) {
 			/* mark this mid as cancelled to not free it below */
 			cancelled_mid[i] = true;
@@ -1140,14 +1142,14 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		buf = (char *)midQ[i]->resp_buf;
 		resp_iov[i].iov_base = buf;
 		resp_iov[i].iov_len = midQ[i]->resp_buf_size +
-			ses->server->vals->header_preamble_size;
+			server->vals->header_preamble_size;
 
 		if (midQ[i]->large_buf)
 			resp_buf_type[i] = CIFS_LARGE_BUFFER;
 		else
 			resp_buf_type[i] = CIFS_SMALL_BUFFER;
 
-		rc = ses->server->ops->check_receive(midQ[i], ses->server,
+		rc = server->ops->check_receive(midQ[i], server,
 						     flags & CIFS_LOG_ERROR);
 
 		/* mark it so buf will not be freed by cifs_delete_mid */
-- 
2.20.1