diff mbox

[1/5] CIFS: Move buffer allocation to a separate function

Message ID 1308898317-7667-2-git-send-email-piastryyy@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky June 24, 2011, 6:51 a.m. UTC 
Signed-off-by: Pavel Shilovsky <piastryyy@gmail.com>
---
 fs/cifs/connect.c |   84 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 50 insertions(+), 34 deletions(-)

Comments

Christoph Hellwig June 24, 2011, 7:11 a.m. UTC | #1 
On Fri, Jun 24, 2011 at 10:51:53AM +0400, Pavel Shilovsky wrote:
> +static bool
> +allocate_buffers(char **pbigbuf, char **psmallbuf, unsigned int size,
> +		 bool isLargeBuf)

No p-prefixes for pointers, and no camelCase, please.

> +{
> +	char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
> +
> +	if (bigbuf == NULL) {
> +		bigbuf = (char *)cifs_buf_get();
> +		if (!bigbuf) {
> +			cERROR(1, "No memory for large SMB response");
> +			msleep(3000);
> +			/* retry will check if exiting */
> +			return false;
> +		}
> +	} else if (isLargeBuf) {
> +		/* we are reusing a dirty large buf, clear its start */
> +		memset(bigbuf, 0, size);
> +	}
> +
> +	if (smallbuf == NULL) {
> +		smallbuf = (char *)cifs_small_buf_get();
> +		if (!smallbuf) {
> +			cERROR(1, "No memory for SMB response");
> +			msleep(1000);
> +			/* retry will check if exiting */
> +			return false;
> +		}
> +		/* beginning of smb buffer is cleared in our buf_get */
> +	} else /* if existing small buf clear beginning */
> +		memset(smallbuf, 0, size);
> +
> +	*pbigbuf = bigbuf;
> +	*psmallbuf = smallbuf;
> +
> +	return true;

But in the end this whole code doesn't make any sense at all.  Just
sleeping when failing to memory is a bad idea, as is all this partial
reuse stuff.

Someone needs to rearchitect this code to actually make sense.

--
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
Pavel Shilovsky June 24, 2011, 7:36 a.m. UTC | #2 
2011/6/24 Christoph Hellwig <hch@infradead.org>:
> On Fri, Jun 24, 2011 at 10:51:53AM +0400, Pavel Shilovsky wrote:
>> +static bool
>> +allocate_buffers(char **pbigbuf, char **psmallbuf, unsigned int size,
>> +              bool isLargeBuf)
>
> No p-prefixes for pointers, and no camelCase, please.
>
>> +{
>> +     char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
>> +
>> +     if (bigbuf == NULL) {
>> +             bigbuf = (char *)cifs_buf_get();
>> +             if (!bigbuf) {
>> +                     cERROR(1, "No memory for large SMB response");
>> +                     msleep(3000);
>> +                     /* retry will check if exiting */
>> +                     return false;
>> +             }
>> +     } else if (isLargeBuf) {
>> +             /* we are reusing a dirty large buf, clear its start */
>> +             memset(bigbuf, 0, size);
>> +     }
>> +
>> +     if (smallbuf == NULL) {
>> +             smallbuf = (char *)cifs_small_buf_get();
>> +             if (!smallbuf) {
>> +                     cERROR(1, "No memory for SMB response");
>> +                     msleep(1000);
>> +                     /* retry will check if exiting */
>> +                     return false;
>> +             }
>> +             /* beginning of smb buffer is cleared in our buf_get */
>> +     } else /* if existing small buf clear beginning */
>> +             memset(smallbuf, 0, size);
>> +
>> +     *pbigbuf = bigbuf;
>> +     *psmallbuf = smallbuf;
>> +
>> +     return true;
>
> But in the end this whole code doesn't make any sense at all.  Just
> sleeping when failing to memory is a bad idea, as is all this partial
> reuse stuff.
>
> Someone needs to rearchitect this code to actually make sense.
>
>

Thanks for your comments! I think that moving this code to a separate
function is a good point to start with and further we can rethink it.
diff mbox

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 19fdbda..566080d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -317,14 +317,50 @@  requeue_echo:
 	queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
 }
 
+static bool
+allocate_buffers(char **pbigbuf, char **psmallbuf, unsigned int size,
+		 bool isLargeBuf)
+{
+	char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
+
+	if (bigbuf == NULL) {
+		bigbuf = (char *)cifs_buf_get();
+		if (!bigbuf) {
+			cERROR(1, "No memory for large SMB response");
+			msleep(3000);
+			/* retry will check if exiting */
+			return false;
+		}
+	} else if (isLargeBuf) {
+		/* we are reusing a dirty large buf, clear its start */
+		memset(bigbuf, 0, size);
+	}
+
+	if (smallbuf == NULL) {
+		smallbuf = (char *)cifs_small_buf_get();
+		if (!smallbuf) {
+			cERROR(1, "No memory for SMB response");
+			msleep(1000);
+			/* retry will check if exiting */
+			return false;
+		}
+		/* beginning of smb buffer is cleared in our buf_get */
+	} else /* if existing small buf clear beginning */
+		memset(smallbuf, 0, size);
+
+	*pbigbuf = bigbuf;
+	*psmallbuf = smallbuf;
+
+	return true;
+}
+
 static int
 cifs_demultiplex_thread(struct TCP_Server_Info *server)
 {
 	int length;
 	unsigned int pdu_length, total_read;
+	char *buf = NULL, *bigbuf = NULL, *smallbuf = NULL;
 	struct smb_hdr *smb_buffer = NULL;
-	struct smb_hdr *bigbuf = NULL;
-	struct smb_hdr *smallbuf = NULL;
 	struct msghdr smb_msg;
 	struct kvec iov;
 	struct socket *csocket = server->ssocket;
@@ -348,34 +384,15 @@  cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	while (server->tcpStatus != CifsExiting) {
 		if (try_to_freeze())
 			continue;
-		if (bigbuf == NULL) {
-			bigbuf = cifs_buf_get();
-			if (!bigbuf) {
-				cERROR(1, "No memory for large SMB response");
-				msleep(3000);
-				/* retry will check if exiting */
-				continue;
-			}
-		} else if (isLargeBuf) {
-			/* we are reusing a dirty large buf, clear its start */
-			memset(bigbuf, 0, sizeof(struct smb_hdr));
-		}
 
-		if (smallbuf == NULL) {
-			smallbuf = cifs_small_buf_get();
-			if (!smallbuf) {
-				cERROR(1, "No memory for SMB response");
-				msleep(1000);
-				/* retry will check if exiting */
-				continue;
-			}
-			/* beginning of smb buffer is cleared in our buf_get */
-		} else /* if existing small buf clear beginning */
-			memset(smallbuf, 0, sizeof(struct smb_hdr));
+		if (!allocate_buffers(&bigbuf, &smallbuf,
+				      sizeof(struct smb_hdr), isLargeBuf))
+			continue;
 
 		isLargeBuf = false;
 		isMultiRsp = false;
-		smb_buffer = smallbuf;
+		smb_buffer = (struct smb_hdr *)smallbuf;
+		buf = smallbuf;
 		iov.iov_base = smb_buffer;
 		iov.iov_len = 4;
 		smb_msg.msg_control = NULL;
@@ -414,8 +431,7 @@  incomplete_rcv:
 				allowing socket to clear and app threads to set
 				tcpStatus CifsNeedReconnect if server hung */
 			if (pdu_length < 4) {
-				iov.iov_base = (4 - pdu_length) +
-							(char *)smb_buffer;
+				iov.iov_base = (4 - pdu_length) + buf;
 				iov.iov_len = pdu_length;
 				smb_msg.msg_control = NULL;
 				smb_msg.msg_controllen = 0;
@@ -443,7 +459,7 @@  incomplete_rcv:
 		/* the first byte big endian of the length field,
 		is actually not part of the length but the type
 		with the most common, zero, as regular data */
-		temp = *((char *) smb_buffer);
+		temp = *buf;
 
 		/* Note that FC 1001 length is big endian on the wire,
 		but we convert it here so it is always manipulated
@@ -477,8 +493,7 @@  incomplete_rcv:
 			continue;
 		} else if (temp != (char) 0) {
 			cERROR(1, "Unknown RFC 1002 frame");
-			cifs_dump_mem(" Received Data: ", (char *)smb_buffer,
-				      length);
+			cifs_dump_mem(" Received Data: ", buf, length);
 			cifs_reconnect(server);
 			csocket = server->ssocket;
 			continue;
@@ -501,10 +516,11 @@  incomplete_rcv:
 		if (pdu_length > MAX_CIFS_SMALL_BUFFER_SIZE - 4) {
 			isLargeBuf = true;
 			memcpy(bigbuf, smallbuf, 4);
-			smb_buffer = bigbuf;
+			smb_buffer = (struct smb_hdr *)bigbuf;
+			buf = bigbuf;
 		}
 		length = 0;
-		iov.iov_base = 4 + (char *)smb_buffer;
+		iov.iov_base = 4 + buf;
 		iov.iov_len = pdu_length;
 		for (total_read = 0; total_read < pdu_length;
 		     total_read += length) {
@@ -645,7 +661,7 @@  multi_t2_fnd:
 			   !isMultiRsp) {
 			cERROR(1, "No task to wake, unknown frame received! "
 				   "NumMids %d", atomic_read(&midCount));
-			cifs_dump_mem("Received Data is: ", (char *)smb_buffer,
+			cifs_dump_mem("Received Data is: ", buf,
 				      sizeof(struct smb_hdr));
 #ifdef CONFIG_CIFS_DEBUG2
 			cifs_dump_detail(smb_buffer);