[PATCH 00/14] cifs: overhaul request timeout behavior in CIFS (try #2)

[PATCH 00/14] cifs: overhaul request timeout behavior in CIFS (try #2)

[Jeff Layton]

This is the second spin of the patchset to overhaul timeout behavior in
CIFS. The main differences are bugfixes, mainly to ensure that cifsd
isn't holding the GlobalMid_Lock when calling the mid callback functions.

I've also dropped the patch to change the default to "hard". I think it
would make for better data integrity in the face of reconnection events
but it's probably better to separate that patch from this set.

Finally, I've cleaned up the patch to handle -EAGAIN errors in
cifs_writepages. Rather than retrying in the WB_SYNC_NONE case, it has
cifs_writepages re-mark the page as dirty and just skip it. That should
prevent long hangs in cifs_writepages for non-data-integrity syncs.

This patchset is intended to fix the unreliable behavior in CIFS in the
face of a server that's taking a long time to process requests.  Much of
my rationale for this set has been outlined in the separate discussion
thread entitled:

    "cifs client timeouts and hard/soft mounts"

In general, the current code sets a timeout for all requests that are
sent on the wire. If the server doesn't respond to the request within
that timeout, the client performs a reconnect and retries the request.

This is dangerous and wasteful behavior for the client. Much of the
state of a CIFS mount is bound to the socket connection. Break the
socket connection and state is lost.

I believe this the root cause of some data corruption issues that have
been reported to me. We had a partner report that when they copied a
large file to a CIFS server and then compare the result to the original,
there is sometimes a mismatch. The problem is highly correlated to
messages in the ring buffer that indicate that the client reconnected
the socket during the test run.

Another problem that I can reliably reproduce -- I have win2k8
installed as a VM guest. When I run connectathon tests to that server,
it frequently fails on the test that writes 4GB past the EOF. The
storage on this server is slow, and it can take longer than 180s for
it to zero-fill the output file.

The intent of this patchset is to fundamentally change when the client
decides to reconnect the socket. Instead of the old behavior, this
patchset makes the client wait indefinitely for a response. Rather than
waiting in TASK_UNINTERRUPTIBLE sleep however, the client waits in
TASK_KILLABLE sleep so that fatal signals will end the sleep and
return -ERESTARTSYS to the caller.

In order to determine whether the server is completely dead or just
taking a long time to process requests, this patchset has the client do
an asynchronous SMB echo request every 30s when the client hasn't gotten
a reponse. If the server doesn't respond after 3 echo attempts, the
client will attempt to reconnect the socket.

With this patchset, I can reliably run the connectathon tests against my
slow server. Preliminary results using the proprietary test that was
seeing data corruption have also been promising.

I'd like to see this set considered for inclusion into 2.6.38. Timely
review would be appreciated so that I have time to make changes before
the merge window if they are needed.

Jeff Layton (13):
  cifs: don't fail writepages on -EAGAIN errors
  cifs: make wait_for_free_request take a TCP_Server_Info pointer
  cifs: move mid result processing into common function
  cifs: wait indefinitely for responses
  cifs: don't reconnect server when we don't get a response
  cifs: clean up handle_mid_response
  cifs: allow for different handling of received response
  cifs: handle cancelled requests better
  cifs: add cifs_call_async
  cifs: add ability to send an echo request
  cifs: set up recurring workqueue job to do SMB echo requests
  cifs: reconnect unresponsive servers
  cifs: remove code for setting timeouts on requests

 fs/cifs/cifs_debug.c |    8 +-
 fs/cifs/cifsglob.h   |   19 ++-
 fs/cifs/cifspdu.h    |   15 ++
 fs/cifs/cifsproto.h  |    7 +
 fs/cifs/cifssmb.c    |   55 ++++++++-
 fs/cifs/connect.c    |  146 +++++++++++++++++-----
 fs/cifs/file.c       |   67 +++-------
 fs/cifs/sess.c       |    2 +-
 fs/cifs/transport.c  |  345 ++++++++++++++++++++++----------------------------
 9 files changed, 375 insertions(+), 289 deletions(-)

-- 
1.7.3.2

[PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Jeff Layton]

If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
temporary. CIFS should retry the write instead of setting an error on
the mapping and returning.

For WB_SYNC_ALL, just retry the write immediately.

In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
pages that didn't get written out and then move on.

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

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index fe16f6d..8e57370 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1430,6 +1430,7 @@ retry:
 				break;
 		}
 		if (n_iov) {
+retry_write:
 			open_file = find_writable_file(CIFS_I(mapping->host),
 							false);
 			if (!open_file) {
@@ -1445,22 +1446,20 @@ retry:
 				cifs_update_eof(cifsi, offset, bytes_written);
 			}
 
-			if (rc || bytes_written < bytes_to_write) {
-				cERROR(1, "Write2 ret %d, wrote %d",
-					  rc, bytes_written);
-				mapping_set_error(mapping, rc);
-			} else {
+			/* retry on data-integrity flush */
+			if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
+				goto retry_write;
+
+			if (!rc)
 				cifs_stats_bytes_written(tcon, bytes_written);
-			}
+			else if (rc != -EAGAIN)
+				mapping_set_error(mapping, rc);
 
 			for (i = 0; i < n_iov; i++) {
 				page = pvec.pages[first + i];
-				/* Should we also set page error on
-				success rc but too little data written? */
-				/* BB investigate retry logic on temporary
-				server crash cases and how recovery works
-				when page marked as error */
-				if (rc)
+				if (rc == -EAGAIN)
+					redirty_page_for_writepage(wbc, page);
+				else if (rc != 0)
 					SetPageError(page);
 				kunmap(page);
 				unlock_page(page);
-- 
1.7.3.2

[PATCH 02/13] cifs: make wait_for_free_request take a TCP_Server_Info pointer

[Jeff Layton]

The cifsSesInfo pointer is only used to get at the server.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index e0588cd..9763f89 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -244,31 +244,31 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
 	return smb_sendv(server, &iov, 1);
 }
 
-static int wait_for_free_request(struct cifsSesInfo *ses, const int long_op)
+static int wait_for_free_request(struct TCP_Server_Info *server,
+				 const int long_op)
 {
 	if (long_op == CIFS_ASYNC_OP) {
 		/* oplock breaks must not be held up */
-		atomic_inc(&ses->server->inFlight);
+		atomic_inc(&server->inFlight);
 		return 0;
 	}
 
 	spin_lock(&GlobalMid_Lock);
 	while (1) {
-		if (atomic_read(&ses->server->inFlight) >=
-				cifs_max_pending){
+		if (atomic_read(&server->inFlight) >= cifs_max_pending) {
 			spin_unlock(&GlobalMid_Lock);
 #ifdef CONFIG_CIFS_STATS2
-			atomic_inc(&ses->server->num_waiters);
+			atomic_inc(&server->num_waiters);
 #endif
-			wait_event(ses->server->request_q,
-				   atomic_read(&ses->server->inFlight)
+			wait_event(server->request_q,
+				   atomic_read(&server->inFlight)
 				     < cifs_max_pending);
 #ifdef CONFIG_CIFS_STATS2
-			atomic_dec(&ses->server->num_waiters);
+			atomic_dec(&server->num_waiters);
 #endif
 			spin_lock(&GlobalMid_Lock);
 		} else {
-			if (ses->server->tcpStatus == CifsExiting) {
+			if (server->tcpStatus == CifsExiting) {
 				spin_unlock(&GlobalMid_Lock);
 				return -ENOENT;
 			}
@@ -278,7 +278,7 @@ static int wait_for_free_request(struct cifsSesInfo *ses, const int long_op)
 
 			/* update # of requests on the wire to server */
 			if (long_op != CIFS_BLOCKING_OP)
-				atomic_inc(&ses->server->inFlight);
+				atomic_inc(&server->inFlight);
 			spin_unlock(&GlobalMid_Lock);
 			break;
 		}
@@ -413,7 +413,7 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	   to the same server. We may make this configurable later or
 	   use ses->maxReq */
 
-	rc = wait_for_free_request(ses, long_op);
+	rc = wait_for_free_request(ses->server, long_op);
 	if (rc) {
 		cifs_small_buf_release(in_buf);
 		return rc;
@@ -610,7 +610,7 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 		return -EIO;
 	}
 
-	rc = wait_for_free_request(ses, long_op);
+	rc = wait_for_free_request(ses->server, long_op);
 	if (rc)
 		return rc;
 
@@ -845,7 +845,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 		return -EIO;
 	}
 
-	rc = wait_for_free_request(ses, CIFS_BLOCKING_OP);
+	rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP);
 	if (rc)
 		return rc;
 
-- 
1.7.3.2

[PATCH 03/13] cifs: move mid result processing into common function

[Jeff Layton]

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 9763f89..2d21bbd 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -382,6 +382,46 @@ SendReceiveNoRsp(const unsigned int xid, struct cifsSesInfo *ses,
 	return rc;
 }
 
+static int
+handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
+{
+	int rc = 0;
+
+	spin_lock(&GlobalMid_Lock);
+
+	if (mid->resp_buf) {
+		spin_unlock(&GlobalMid_Lock);
+		return rc;
+	}
+
+	cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
+	if (mid->midState == MID_REQUEST_SUBMITTED) {
+		if (server->tcpStatus == CifsExiting)
+			rc = -EHOSTDOWN;
+		else {
+			server->tcpStatus = CifsNeedReconnect;
+			mid->midState = MID_RETRY_NEEDED;
+		}
+	}
+
+	if (rc != -EHOSTDOWN) {
+		if (mid->midState == MID_RETRY_NEEDED) {
+			rc = -EAGAIN;
+			cFYI(1, "marking request for retry");
+		} else {
+			rc = -EIO;
+		}
+	}
+	spin_unlock(&GlobalMid_Lock);
+
+	DeleteMidQEntry(mid);
+	/* Update # of requests on wire to server */
+	atomic_dec(&server->inFlight);
+	wake_up(&server->request_q);
+
+	return rc;
+}
+
 int
 SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	     struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
@@ -485,37 +525,10 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	/* No user interrupts in wait - wreaks havoc with performance */
 	wait_for_response(ses, midQ, timeout, 10 * HZ);
 
-	spin_lock(&GlobalMid_Lock);
-
-	if (midQ->resp_buf == NULL) {
-		cERROR(1, "No response to cmd %d mid %d",
-			midQ->command, midQ->mid);
-		if (midQ->midState == MID_REQUEST_SUBMITTED) {
-			if (ses->server->tcpStatus == CifsExiting)
-				rc = -EHOSTDOWN;
-			else {
-				ses->server->tcpStatus = CifsNeedReconnect;
-				midQ->midState = MID_RETRY_NEEDED;
-			}
-		}
-
-		if (rc != -EHOSTDOWN) {
-			if (midQ->midState == MID_RETRY_NEEDED) {
-				rc = -EAGAIN;
-				cFYI(1, "marking request for retry");
-			} else {
-				rc = -EIO;
-			}
-		}
-		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
-		/* Update # of requests on wire to server */
-		atomic_dec(&ses->server->inFlight);
-		wake_up(&ses->server->request_q);
+	rc = handle_mid_result(midQ, ses->server);
+	if (rc != 0)
 		return rc;
-	}
 
-	spin_unlock(&GlobalMid_Lock);
 	receive_len = midQ->resp_buf->smb_buf_length;
 
 	if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
@@ -677,36 +690,10 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 	/* No user interrupts in wait - wreaks havoc with performance */
 	wait_for_response(ses, midQ, timeout, 10 * HZ);
 
-	spin_lock(&GlobalMid_Lock);
-	if (midQ->resp_buf == NULL) {
-		cERROR(1, "No response for cmd %d mid %d",
-			  midQ->command, midQ->mid);
-		if (midQ->midState == MID_REQUEST_SUBMITTED) {
-			if (ses->server->tcpStatus == CifsExiting)
-				rc = -EHOSTDOWN;
-			else {
-				ses->server->tcpStatus = CifsNeedReconnect;
-				midQ->midState = MID_RETRY_NEEDED;
-			}
-		}
-
-		if (rc != -EHOSTDOWN) {
-			if (midQ->midState == MID_RETRY_NEEDED) {
-				rc = -EAGAIN;
-				cFYI(1, "marking request for retry");
-			} else {
-				rc = -EIO;
-			}
-		}
-		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
-		/* Update # of requests on wire to server */
-		atomic_dec(&ses->server->inFlight);
-		wake_up(&ses->server->request_q);
+	rc = handle_mid_result(midQ, ses->server);
+	if (rc != 0)
 		return rc;
-	}
 
-	spin_unlock(&GlobalMid_Lock);
 	receive_len = midQ->resp_buf->smb_buf_length;
 
 	if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
@@ -926,35 +913,11 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 		}
 	}
 
-	spin_lock(&GlobalMid_Lock);
-	if (midQ->resp_buf) {
-		spin_unlock(&GlobalMid_Lock);
-		receive_len = midQ->resp_buf->smb_buf_length;
-	} else {
-		cERROR(1, "No response for cmd %d mid %d",
-			  midQ->command, midQ->mid);
-		if (midQ->midState == MID_REQUEST_SUBMITTED) {
-			if (ses->server->tcpStatus == CifsExiting)
-				rc = -EHOSTDOWN;
-			else {
-				ses->server->tcpStatus = CifsNeedReconnect;
-				midQ->midState = MID_RETRY_NEEDED;
-			}
-		}
-
-		if (rc != -EHOSTDOWN) {
-			if (midQ->midState == MID_RETRY_NEEDED) {
-				rc = -EAGAIN;
-				cFYI(1, "marking request for retry");
-			} else {
-				rc = -EIO;
-			}
-		}
-		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
+	rc = handle_mid_result(midQ, ses->server);
+	if (rc != 0)
 		return rc;
-	}
 
+	receive_len = midQ->resp_buf->smb_buf_length;
 	if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
 		cERROR(1, "Frame too large received.  Length: %d  Xid: %d",
 			receive_len, xid);
-- 
1.7.3.2

[PATCH 04/13] cifs: wait indefinitely for responses

[Jeff Layton]

The client should not be timing out on individual SMB requests. Too much
of the state between client and server is tied to the state of the
socket. If we time out requests and issue spurious disconnects then that
comprimises data integrity.

Instead of doing this complicated dance where we try to decide how long
to wait for a response for particular requests, have the client instead
wait indefinitely for a response. Also, use a TASK_KILLABLE sleep here
so that fatal signals will break out of this waiting.

Later patches will add support for detecting dead peers and forcing
reconnects based on that.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 2d21bbd..989674c 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -311,48 +311,17 @@ static int allocate_mid(struct cifsSesInfo *ses, struct smb_hdr *in_buf,
 	return 0;
 }
 
-static int wait_for_response(struct cifsSesInfo *ses,
-			struct mid_q_entry *midQ,
-			unsigned long timeout,
-			unsigned long time_to_wait)
+static int
+wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
 {
-	unsigned long curr_timeout;
-
-	for (;;) {
-		curr_timeout = timeout + jiffies;
-		wait_event_timeout(ses->server->response_q,
-			midQ->midState != MID_REQUEST_SUBMITTED, timeout);
-
-		if (time_after(jiffies, curr_timeout) &&
-			(midQ->midState == MID_REQUEST_SUBMITTED) &&
-			((ses->server->tcpStatus == CifsGood) ||
-			 (ses->server->tcpStatus == CifsNew))) {
-
-			unsigned long lrt;
+	int error;
 
-			/* We timed out. Is the server still
-			   sending replies ? */
-			spin_lock(&GlobalMid_Lock);
-			lrt = ses->server->lstrp;
-			spin_unlock(&GlobalMid_Lock);
+	error = wait_event_killable(server->response_q,
+				    midQ->midState != MID_REQUEST_SUBMITTED);
+	if (error < 0)
+		return -ERESTARTSYS;
 
-			/* Calculate time_to_wait past last receive time.
-			 Although we prefer not to time out if the
-			 server is still responding - we will time
-			 out if the server takes more than 15 (or 45
-			 or 180) seconds to respond to this request
-			 and has not responded to any request from
-			 other threads on the client within 10 seconds */
-			lrt += time_to_wait;
-			if (time_after(jiffies, lrt)) {
-				/* No replies for time_to_wait. */
-				cERROR(1, "server not responding");
-				return -1;
-			}
-		} else {
-			return 0;
-		}
-	}
+	return 0;
 }
 
 
@@ -430,7 +399,6 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	int rc = 0;
 	int long_op;
 	unsigned int receive_len;
-	unsigned long timeout;
 	struct mid_q_entry *midQ;
 	struct smb_hdr *in_buf = iov[0].iov_base;
 
@@ -497,33 +465,12 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	if (rc < 0)
 		goto out;
 
-	if (long_op == CIFS_STD_OP)
-		timeout = 15 * HZ;
-	else if (long_op == CIFS_VLONG_OP) /* e.g. slow writes past EOF */
-		timeout = 180 * HZ;
-	else if (long_op == CIFS_LONG_OP)
-		timeout = 45 * HZ; /* should be greater than
-			servers oplock break timeout (about 43 seconds) */
-	else if (long_op == CIFS_ASYNC_OP)
-		goto out;
-	else if (long_op == CIFS_BLOCKING_OP)
-		timeout = 0x7FFFFFFF; /*  large, but not so large as to wrap */
-	else {
-		cERROR(1, "unknown timeout flag %d", long_op);
-		rc = -EIO;
+	if (long_op == CIFS_ASYNC_OP)
 		goto out;
-	}
-
-	/* wait for 15 seconds or until woken up due to response arriving or
-	   due to last connection to this server being unmounted */
-	if (signal_pending(current)) {
-		/* if signal pending do not hold up user for full smb timeout
-		but we still give response a chance to complete */
-		timeout = 2 * HZ;
-	}
 
-	/* No user interrupts in wait - wreaks havoc with performance */
-	wait_for_response(ses, midQ, timeout, 10 * HZ);
+	rc = wait_for_response(ses->server, midQ);
+	if (rc != 0)
+		goto out;
 
 	rc = handle_mid_result(midQ, ses->server);
 	if (rc != 0)
@@ -598,7 +545,6 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 {
 	int rc = 0;
 	unsigned int receive_len;
-	unsigned long timeout;
 	struct mid_q_entry *midQ;
 
 	if (ses == NULL) {
@@ -662,33 +608,12 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 	if (rc < 0)
 		goto out;
 
-	if (long_op == CIFS_STD_OP)
-		timeout = 15 * HZ;
-	/* wait for 15 seconds or until woken up due to response arriving or
-	   due to last connection to this server being unmounted */
-	else if (long_op == CIFS_ASYNC_OP)
-		goto out;
-	else if (long_op == CIFS_VLONG_OP) /* writes past EOF can be slow */
-		timeout = 180 * HZ;
-	else if (long_op == CIFS_LONG_OP)
-		timeout = 45 * HZ; /* should be greater than
-			servers oplock break timeout (about 43 seconds) */
-	else if (long_op == CIFS_BLOCKING_OP)
-		timeout = 0x7FFFFFFF; /* large but no so large as to wrap */
-	else {
-		cERROR(1, "unknown timeout flag %d", long_op);
-		rc = -EIO;
+	if (long_op == CIFS_ASYNC_OP)
 		goto out;
-	}
 
-	if (signal_pending(current)) {
-		/* if signal pending do not hold up user for full smb timeout
-		but we still give response a chance to complete */
-		timeout = 2 * HZ;
-	}
-
-	/* No user interrupts in wait - wreaks havoc with performance */
-	wait_for_response(ses, midQ, timeout, 10 * HZ);
+	rc = wait_for_response(ses->server, midQ);
+	if (rc != 0)
+		goto out;
 
 	rc = handle_mid_result(midQ, ses->server);
 	if (rc != 0)
@@ -906,8 +831,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 			}
 		}
 
-		/* Wait 5 seconds for the response. */
-		if (wait_for_response(ses, midQ, 5 * HZ, 5 * HZ) == 0) {
+		if (wait_for_response(ses->server, midQ) == 0) {
 			/* We got the response - restart system call. */
 			rstart = 1;
 		}
-- 
1.7.3.2

[PATCH 05/13] cifs: don't reconnect server when we don't get a response

[Jeff Layton]

We only want to force a reconnect to the server under very limited and
specific circumstances. Now that we have processes waiting indefinitely
for responses, we shouldn't reach this point unless a reconnect is
already in process. Thus, there's no reason to re-mark the server for
reconnect here.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 989674c..0c0dadd 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -367,10 +367,8 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 	if (mid->midState == MID_REQUEST_SUBMITTED) {
 		if (server->tcpStatus == CifsExiting)
 			rc = -EHOSTDOWN;
-		else {
-			server->tcpStatus = CifsNeedReconnect;
+		else
 			mid->midState = MID_RETRY_NEEDED;
-		}
 	}
 
 	if (rc != -EHOSTDOWN) {
-- 
1.7.3.2

[PATCH 06/13] cifs: clean up handle_mid_response

[Jeff Layton]

Make it use a switch statement based on the value of the midStatus. If
the resp_buf is set, then MID_RESPONSE_RECEIVED is too.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 0c0dadd..05ced17 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -356,33 +356,33 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 {
 	int rc = 0;
 
-	spin_lock(&GlobalMid_Lock);
+	cFYI(1, "%s: cmd=%d mid=%d state=%d", __func__, mid->command,
+		mid->mid, mid->midState);
 
-	if (mid->resp_buf) {
+	spin_lock(&GlobalMid_Lock);
+	switch (mid->midState) {
+	case MID_RESPONSE_RECEIVED:
 		spin_unlock(&GlobalMid_Lock);
 		return rc;
-	}
-
-	cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
-	if (mid->midState == MID_REQUEST_SUBMITTED) {
-		if (server->tcpStatus == CifsExiting)
+	case MID_REQUEST_SUBMITTED:
+		/* socket is going down, reject all calls */
+		if (server->tcpStatus == CifsExiting) {
+			cERROR(1, "%s: canceling mid=%d cmd=0x%x state=%d",
+			       __func__, mid->mid, mid->command, mid->midState);
 			rc = -EHOSTDOWN;
-		else
-			mid->midState = MID_RETRY_NEEDED;
-	}
-
-	if (rc != -EHOSTDOWN) {
-		if (mid->midState == MID_RETRY_NEEDED) {
-			rc = -EAGAIN;
-			cFYI(1, "marking request for retry");
-		} else {
-			rc = -EIO;
+			break;
 		}
+		mid->midState = MID_RETRY_NEEDED;
+	case MID_RETRY_NEEDED:
+		rc = -EAGAIN;
+		break;
+	default:
+		cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
+			mid->mid, mid->midState);
 	}
 	spin_unlock(&GlobalMid_Lock);
 
 	DeleteMidQEntry(mid);
-	/* Update # of requests on wire to server */
 	atomic_dec(&server->inFlight);
 	wake_up(&server->request_q);
 
-- 
1.7.3.2

[PATCH 07/13] cifs: allow for different handling of received response

[Jeff Layton]

In order to incorporate async requests, we need to allow for a more
general way to do things on receive, rather than just waking up a
process.

Turn the task pointer in the mid_q_entry into a callback function and a
generic data pointer. When a response comes in, or the socket is
reconnected, cifsd can call the callback function in order to wake up
the process.

The default is to just wake up the current process which should mean no
change in behavior for existing code.

Also, clean up the locking in cifs_reconnect. There doesn't seem to be
any need to hold both the srv_mutex and GlobalMid_Lock when walking the
list of mids.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_debug.c |    8 +++---
 fs/cifs/cifsglob.h   |    7 +++++-
 fs/cifs/connect.c    |   54 ++++++++++++++++++++++++++++---------------------
 fs/cifs/transport.c  |   15 +++++++++++++-
 4 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index bd76527..c81934f 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -79,11 +79,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
 	spin_lock(&GlobalMid_Lock);
 	list_for_each(tmp, &server->pending_mid_q) {
 		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
-		cERROR(1, "State: %d Cmd: %d Pid: %d Tsk: %p Mid %d",
+		cERROR(1, "State: %d Cmd: %d Pid: %d Cbdata: %p Mid %d",
 			mid_entry->midState,
 			(int)mid_entry->command,
 			mid_entry->pid,
-			mid_entry->tsk,
+			mid_entry->callback_data,
 			mid_entry->mid);
 #ifdef CONFIG_CIFS_STATS2
 		cERROR(1, "IsLarge: %d buf: %p time rcv: %ld now: %ld",
@@ -218,11 +218,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 				mid_entry = list_entry(tmp3, struct mid_q_entry,
 					qhead);
 				seq_printf(m, "\tState: %d com: %d pid:"
-						" %d tsk: %p mid %d\n",
+						" %d cbdata: %p mid %d\n",
 						mid_entry->midState,
 						(int)mid_entry->command,
 						mid_entry->pid,
-						mid_entry->tsk,
+						mid_entry->callback_data,
 						mid_entry->mid);
 			}
 			spin_unlock(&GlobalMid_Lock);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index cf0dfda..cc43ada 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -511,6 +511,10 @@ static inline void cifs_stats_bytes_read(struct cifsTconInfo *tcon,
 
 #endif
 
+struct mid_q_entry;
+
+typedef void (mid_callback_t)(struct mid_q_entry *mid);
+
 /* one of these for every pending CIFS request to the server */
 struct mid_q_entry {
 	struct list_head qhead;	/* mids waiting on reply from this server */
@@ -522,7 +526,8 @@ struct mid_q_entry {
 	unsigned long when_sent; /* time when smb send finished */
 	unsigned long when_received; /* when demux complete (taken off wire) */
 #endif
-	struct task_struct *tsk;	/* task waiting for response */
+	mid_callback_t *callback; /* call completion callback */
+	void *callback_data;	  /* general purpose pointer for callback */
 	struct smb_hdr *resp_buf;	/* response buffer */
 	int midState;	/* wish this were enum but can not pass to wait_event */
 	__u8 command;	/* smb command code */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9fbe7c5..7e20ece 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -133,6 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 {
 	int rc = 0;
 	struct list_head *tmp, *tmp2;
+	struct list_head retry;
 	struct cifsSesInfo *ses;
 	struct cifsTconInfo *tcon;
 	struct mid_q_entry *mid_entry;
@@ -152,6 +153,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 
 	/* before reconnecting the tcp session, mark the smb session (uid)
 		and the tid bad so they are not used until reconnected */
+	cFYI(1, "%s: marking sessions and tcons for reconnect", __func__);
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each(tmp, &server->smb_ses_list) {
 		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
@@ -163,7 +165,9 @@ cifs_reconnect(struct TCP_Server_Info *server)
 		}
 	}
 	spin_unlock(&cifs_tcp_ses_lock);
+
 	/* do not want to be sending data on a socket we are freeing */
+	cFYI(1, "%s: tearing down socket", __func__);
 	mutex_lock(&server->srv_mutex);
 	if (server->ssocket) {
 		cFYI(1, "State: 0x%x Flags: 0x%lx", server->ssocket->state,
@@ -180,22 +184,21 @@ cifs_reconnect(struct TCP_Server_Info *server)
 	kfree(server->session_key.response);
 	server->session_key.response = NULL;
 	server->session_key.len = 0;
+	mutex_unlock(&server->srv_mutex);
 
+	/*
+	 * move in-progress mids to a private list so that we can walk it later
+	 * without needing a lock. We'll mark them for retry after reconnect.
+	 */
+	cFYI(1, "%s: moving mids to retry list", __func__);
+	INIT_LIST_HEAD(&retry);
 	spin_lock(&GlobalMid_Lock);
-	list_for_each(tmp, &server->pending_mid_q) {
-		mid_entry = list_entry(tmp, struct
-					mid_q_entry,
-					qhead);
-		if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
-				/* Mark other intransit requests as needing
-				   retry so we do not immediately mark the
-				   session bad again (ie after we reconnect
-				   below) as they timeout too */
-			mid_entry->midState = MID_RETRY_NEEDED;
-		}
+	list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
+		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+		if (mid_entry->midState == MID_REQUEST_SUBMITTED)
+			list_move(tmp, &retry);
 	}
 	spin_unlock(&GlobalMid_Lock);
-	mutex_unlock(&server->srv_mutex);
 
 	while ((server->tcpStatus != CifsExiting) &&
 	       (server->tcpStatus != CifsGood)) {
@@ -213,10 +216,17 @@ cifs_reconnect(struct TCP_Server_Info *server)
 			if (server->tcpStatus != CifsExiting)
 				server->tcpStatus = CifsGood;
 			spin_unlock(&GlobalMid_Lock);
-	/*		atomic_set(&server->inFlight,0);*/
-			wake_up(&server->response_q);
 		}
 	}
+
+	/* now, issue callback for all mids in flight */
+	list_for_each_safe(tmp, tmp2, &retry) {
+		list_del_init(tmp);
+		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+		mid_entry->midState = MID_RETRY_NEEDED;
+		mid_entry->callback(mid_entry);
+	}
+
 	return rc;
 }
 
@@ -560,8 +570,7 @@ incomplete_rcv:
 			continue;
 		}
 
-
-		task_to_wake = NULL;
+		mid_entry = NULL;
 		spin_lock(&GlobalMid_Lock);
 		list_for_each(tmp, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
@@ -604,7 +613,6 @@ incomplete_rcv:
 				mid_entry->resp_buf = smb_buffer;
 				mid_entry->largeBuf = isLargeBuf;
 multi_t2_fnd:
-				task_to_wake = mid_entry->tsk;
 				mid_entry->midState = MID_RESPONSE_RECEIVED;
 #ifdef CONFIG_CIFS_STATS2
 				mid_entry->when_received = jiffies;
@@ -615,9 +623,12 @@ multi_t2_fnd:
 				server->lstrp = jiffies;
 				break;
 			}
+			mid_entry = NULL;
 		}
 		spin_unlock(&GlobalMid_Lock);
-		if (task_to_wake) {
+
+		if (mid_entry != NULL) {
+			mid_entry->callback(mid_entry);
 			/* Was previous buf put in mpx struct for multi-rsp? */
 			if (!isMultiRsp) {
 				/* smb buffer will be freed by user thread */
@@ -626,7 +637,6 @@ multi_t2_fnd:
 				else
 					smallbuf = NULL;
 			}
-			wake_up_process(task_to_wake);
 		} else if (!is_valid_oplock_break(smb_buffer, server) &&
 			   !isMultiRsp) {
 			cERROR(1, "No task to wake, unknown frame received! "
@@ -707,11 +717,9 @@ multi_t2_fnd:
 		list_for_each(tmp, &server->pending_mid_q) {
 		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
 			if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
-				cFYI(1, "Clearing Mid 0x%x - waking up ",
+				cFYI(1, "Clearing Mid 0x%x - issuing callback",
 					 mid_entry->mid);
-				task_to_wake = mid_entry->tsk;
-				if (task_to_wake)
-					wake_up_process(task_to_wake);
+				mid_entry->callback(mid_entry);
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 05ced17..79647db 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -36,6 +36,12 @@
 
 extern mempool_t *cifs_mid_poolp;
 
+static void
+wake_up_task(struct mid_q_entry *mid)
+{
+	wake_up_process(mid->callback_data);
+}
+
 static struct mid_q_entry *
 AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 {
@@ -58,7 +64,13 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 	/*	do_gettimeofday(&temp->when_sent);*/ /* easier to use jiffies */
 		/* when mid allocated can be before when sent */
 		temp->when_alloc = jiffies;
-		temp->tsk = current;
+
+		/*
+		 * The default is for the mid to be synchronous, so the
+		 * default callback just wakes up the current task.
+		 */
+		temp->callback = wake_up_task;
+		temp->callback_data = current;
 	}
 
 	spin_lock(&GlobalMid_Lock);
@@ -374,6 +386,7 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 		}
 		mid->midState = MID_RETRY_NEEDED;
 	case MID_RETRY_NEEDED:
+		list_move(&mid->qhead, &server->pending_mid_q);
 		rc = -EAGAIN;
 		break;
 	default:
-- 
1.7.3.2

[PATCH 08/13] cifs: handle cancelled requests better

[Jeff Layton]

Currently, when a request is cancelled via signal, we delete the mid
immediately. If the request was already transmitted however, the client
is still likely to receive a response. When it does, it won't recognize
it however and will pop a printk.

It's also a little dangerous to just delete the mid entry like this. We
may end up reusing that mid. If we do then we could potentially get the
response from the first request confused with the later one.

Prevent the reuse of mids by marking them as cancelled and keeping them
on the pending_mid_q list. If the reply comes in, we'll delete it from
the list then. If it never comes, then we'll delete it at reconnect
or when cifsd comes down.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    2 +-
 fs/cifs/cifsproto.h |    1 +
 fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
 fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
 4 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index cc43ada..fb75f04 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 #define   MID_REQUEST_SUBMITTED 2
 #define   MID_RESPONSE_RECEIVED 4
 #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
-#define   MID_NO_RESP_NEEDED 0x10
+#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
 
 /* Types of response buffer returned from SendReceive2 */
 #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index fe77e69..a8fc606 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
 		const char *fullpath, const struct dfs_info3_param *ref,
 		char **devname);
 /* extern void renew_parental_timestamps(struct dentry *direntry);*/
+extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
 extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
 			struct smb_hdr * /* input */ ,
 			struct smb_hdr * /* out */ ,
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7e20ece..0feb592 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 {
 	int rc = 0;
 	struct list_head *tmp, *tmp2;
-	struct list_head retry;
+	struct list_head retry, dispose;
 	struct cifsSesInfo *ses;
 	struct cifsTconInfo *tcon;
 	struct mid_q_entry *mid_entry;
@@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
 	 */
 	cFYI(1, "%s: moving mids to retry list", __func__);
 	INIT_LIST_HEAD(&retry);
+	INIT_LIST_HEAD(&dispose);
 	spin_lock(&GlobalMid_Lock);
 	list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
 		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
 		if (mid_entry->midState == MID_REQUEST_SUBMITTED)
 			list_move(tmp, &retry);
+		else if (mid_entry->midState == MID_REQUEST_CANCELLED)
+			list_move(tmp, &dispose);
 	}
 	spin_unlock(&GlobalMid_Lock);
 
+	/* now walk private dispose list and delete entries */
+	list_for_each_safe(tmp, tmp2, &dispose) {
+		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+		DeleteMidQEntry(mid_entry);
+	}
+
 	while ((server->tcpStatus != CifsExiting) &&
 	       (server->tcpStatus != CifsGood)) {
 		try_to_freeze();
@@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 		}
 	}
 
-	/* now, issue callback for all mids in flight */
+	/* issue callback for all mids in flight */
 	list_for_each_safe(tmp, tmp2, &retry) {
 		list_del_init(tmp);
 		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
@@ -575,9 +584,13 @@ incomplete_rcv:
 		list_for_each(tmp, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
 
-			if ((mid_entry->mid == smb_buffer->Mid) &&
-			    (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
-			    (mid_entry->command == smb_buffer->Command)) {
+			if (mid_entry->mid != smb_buffer->Mid)
+				goto next_mid;
+			if (mid_entry->command != smb_buffer->Command)
+				goto next_mid;
+			if (mid_entry->midState == MID_REQUEST_CANCELLED)
+				break;
+			if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
 				if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
 					/* We have a multipart transact2 resp */
 					isMultiRsp = true;
@@ -623,11 +636,16 @@ multi_t2_fnd:
 				server->lstrp = jiffies;
 				break;
 			}
+next_mid:
 			mid_entry = NULL;
 		}
 		spin_unlock(&GlobalMid_Lock);
 
 		if (mid_entry != NULL) {
+			if (mid_entry->midState == MID_REQUEST_CANCELLED) {
+				DeleteMidQEntry(mid_entry);
+				continue;
+			}
 			mid_entry->callback(mid_entry);
 			/* Was previous buf put in mpx struct for multi-rsp? */
 			if (!isMultiRsp) {
@@ -704,6 +722,9 @@ multi_t2_fnd:
 		}
 		spin_unlock(&cifs_tcp_ses_lock);
 	} else {
+		struct mid_q_entry *tmp_mid;
+		struct list_head dispose;
+
 		/* although we can not zero the server struct pointer yet,
 		since there are active requests which may depnd on them,
 		mark the corresponding SMB sessions as exiting too */
@@ -713,17 +734,26 @@ multi_t2_fnd:
 			ses->status = CifsExiting;
 		}
 
+		INIT_LIST_HEAD(&dispose);
 		spin_lock(&GlobalMid_Lock);
-		list_for_each(tmp, &server->pending_mid_q) {
-		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+		list_for_each_entry_safe(mid_entry, tmp_mid,
+					 &server->pending_mid_q, qhead) {
 			if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
 				cFYI(1, "Clearing Mid 0x%x - issuing callback",
 					 mid_entry->mid);
 				mid_entry->callback(mid_entry);
+			} else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
+				cFYI(1, "Clearing Mid 0x%x - Cancelled",
+					mid_entry->mid);
+				list_move(&mid_entry->qhead, &dispose);
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
 		spin_unlock(&cifs_tcp_ses_lock);
+
+		/* now delete all of the cancelled mids */
+		list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
+			DeleteMidQEntry(mid_entry);
 		/* 1/8th of sec is more than enough time for them to exit */
 		msleep(125);
 	}
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 79647db..97a1170 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 	return temp;
 }
 
-static void
+void
 DeleteMidQEntry(struct mid_q_entry *midEntry)
 {
 #ifdef CONFIG_CIFS_STATS2
@@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 		goto out;
 
 	rc = wait_for_response(ses->server, midQ);
-	if (rc != 0)
-		goto out;
+	if (rc != 0) {
+		/* no longer considered to be "in-flight" */
+		midQ->midState = MID_REQUEST_CANCELLED;
+	        atomic_dec(&ses->server->inFlight);
+		wake_up(&ses->server->request_q);
+		return rc;
+	}
 
 	rc = handle_mid_result(midQ, ses->server);
 	if (rc != 0)
@@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 		goto out;
 
 	rc = wait_for_response(ses->server, midQ);
-	if (rc != 0)
-		goto out;
+	if (rc != 0) {
+		/* no longer considered to be "in-flight" */
+		midQ->midState = MID_REQUEST_CANCELLED;
+	        atomic_dec(&ses->server->inFlight);
+		wake_up(&ses->server->request_q);
+		return rc;
+	}
 
 	rc = handle_mid_result(midQ, ses->server);
 	if (rc != 0)
@@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 			}
 		}
 
-		if (wait_for_response(ses->server, midQ) == 0) {
-			/* We got the response - restart system call. */
-			rstart = 1;
+		rc = wait_for_response(ses->server, midQ);
+		if (rc) {
+			midQ->midState = MID_REQUEST_CANCELLED;
+			return rc;
 		}
+
+		/* We got the response - restart system call. */
+		rstart = 1;
 	}
 
 	rc = handle_mid_result(midQ, ses->server);
-- 
1.7.3.2

[PATCH 09/13] cifs: add cifs_call_async

[Jeff Layton]

Add a function that will send a request, and set up the mid for an
async reply.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsproto.h |    5 ++++
 fs/cifs/transport.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index a8fc606..6131c1b 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -61,7 +61,12 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
 		const char *fullpath, const struct dfs_info3_param *ref,
 		char **devname);
 /* extern void renew_parental_timestamps(struct dentry *direntry);*/
+extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
+					struct TCP_Server_Info *server);
 extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
+extern int cifs_call_async(struct TCP_Server_Info *server,
+			   struct smb_hdr *in_buf, mid_callback_t *callback,
+			   void *cbdata);
 extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
 			struct smb_hdr * /* input */ ,
 			struct smb_hdr * /* out */ ,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 97a1170..2a0b14d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -42,7 +42,7 @@ wake_up_task(struct mid_q_entry *mid)
 	wake_up_process(mid->callback_data);
 }
 
-static struct mid_q_entry *
+struct mid_q_entry *
 AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 {
 	struct mid_q_entry *temp;
@@ -338,6 +338,57 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
 
 
 /*
+ * Send a SMB request and set the callback function in the mid to handle
+ * the result. Caller is responsible for dealing with timeouts.
+ */
+int
+cifs_call_async(struct TCP_Server_Info *server, struct smb_hdr *in_buf,
+		mid_callback_t *callback, void *cbdata)
+{
+	int rc;
+	struct mid_q_entry *mid;
+
+	rc = wait_for_free_request(server, CIFS_ASYNC_OP);
+	if (rc)
+		return rc;
+
+	mutex_lock(&server->srv_mutex);
+	mid = AllocMidQEntry(in_buf, server);
+	if (mid == NULL) {
+		mutex_unlock(&server->srv_mutex);
+		return -ENOMEM;
+	}
+
+	rc = cifs_sign_smb(in_buf, server, &mid->sequence_number);
+	if (rc) {
+		mutex_unlock(&server->srv_mutex);
+		goto out_err;
+	}
+
+	mid->callback = callback;
+	mid->callback_data = cbdata;
+	mid->midState = MID_REQUEST_SUBMITTED;
+#ifdef CONFIG_CIFS_STATS2
+	atomic_inc(&server->inSend);
+#endif
+	rc = smb_send(server, in_buf, in_buf->smb_buf_length);
+#ifdef CONFIG_CIFS_STATS2
+	atomic_dec(&server->inSend);
+	mid->when_sent = jiffies;
+#endif
+	mutex_unlock(&server->srv_mutex);
+	if (rc)
+		goto out_err;
+
+	return rc;
+out_err:
+	DeleteMidQEntry(mid);
+	atomic_dec(&server->inFlight);
+	wake_up(&server->request_q);
+	return rc;
+}
+
+/*
  *
  * Send an SMB Request.  No response info (other than return code)
  * needs to be parsed.
-- 
1.7.3.2

[PATCH 10/13] cifs: add ability to send an echo request

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifspdu.h   |   15 +++++++++++++++
 fs/cifs/cifsproto.h |    1 +
 fs/cifs/cifssmb.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
index de36b09..ea205b4 100644
--- a/fs/cifs/cifspdu.h
+++ b/fs/cifs/cifspdu.h
@@ -50,6 +50,7 @@
 #define SMB_COM_SETATTR               0x09 /* trivial response */
 #define SMB_COM_LOCKING_ANDX          0x24 /* trivial response */
 #define SMB_COM_COPY                  0x29 /* trivial rsp, fail filename ignrd*/
+#define SMB_COM_ECHO                  0x2B /* echo request */
 #define SMB_COM_OPEN_ANDX             0x2D /* Legacy open for old servers */
 #define SMB_COM_READ_ANDX             0x2E
 #define SMB_COM_WRITE_ANDX            0x2F
@@ -760,6 +761,20 @@ typedef struct smb_com_tconx_rsp_ext {
  *
  */
 
+typedef struct smb_com_echo_req {
+	struct	smb_hdr hdr;
+	__le16	EchoCount;
+	__le16	ByteCount;
+	char	Data[1];
+} __attribute__((packed)) ECHO_REQ;
+
+typedef struct smb_com_echo_rsp {
+	struct	smb_hdr hdr;
+	__le16	SequenceNumber;
+	__le16	ByteCount;
+	char	Data[1];
+} __attribute__((packed)) ECHO_RSP;
+
 typedef struct smb_com_logoff_andx_req {
 	struct smb_hdr hdr;	/* wct = 2 */
 	__u8 AndXCommand;
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 6131c1b..3cd6474 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -353,6 +353,7 @@ extern int CIFSSMBPosixLock(const int xid, struct cifsTconInfo *tcon,
 			const __u64 len, struct file_lock *,
 			const __u16 lock_type, const bool waitFlag);
 extern int CIFSSMBTDis(const int xid, struct cifsTconInfo *tcon);
+extern int CIFSSMBEcho(struct TCP_Server_Info *server);
 extern int CIFSSMBLogoff(const int xid, struct cifsSesInfo *ses);
 
 extern struct cifsSesInfo *sesInfoAlloc(void);
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 9af98f6..fc8145f 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -706,6 +706,53 @@ CIFSSMBTDis(const int xid, struct cifsTconInfo *tcon)
 	return rc;
 }
 
+/*
+ * This is a no-op for now. We're not really interested in the reply, but
+ * rather in the fact that the server sent one and that server->lstrp
+ * gets updated.
+ *
+ * FIXME: maybe we should consider checking that the reply matches request?
+ */
+static void
+cifs_echo_callback(struct mid_q_entry *mid)
+{
+	struct TCP_Server_Info *server = mid->callback_data;
+
+	DeleteMidQEntry(mid);
+	atomic_dec(&server->inFlight);
+	wake_up(&server->request_q);
+}
+
+int
+CIFSSMBEcho(struct TCP_Server_Info *server)
+{
+	ECHO_REQ *smb;
+	int rc = 0;
+
+	cFYI(1, "In echo request");
+
+	rc = small_smb_init(SMB_COM_ECHO, 0, NULL, (void **)&smb);
+	if (rc)
+		return rc;
+
+	/* set up echo request */
+	smb->hdr.Tid = cpu_to_le16(0xffff);
+	smb->hdr.WordCount = cpu_to_le16(1);
+	smb->EchoCount = cpu_to_le16(1);
+	smb->ByteCount = cpu_to_le16(1);
+	smb->Data[0] = 'a';
+	smb->hdr.smb_buf_length += 3;
+
+	rc = cifs_call_async(server, (struct smb_hdr *)smb,
+				cifs_echo_callback, server);
+	if (rc)
+		cFYI(1, "Echo request failed: %d", rc);
+
+	cifs_small_buf_release(smb);
+
+	return rc;
+}
+
 int
 CIFSSMBLogoff(const int xid, struct cifsSesInfo *ses)
 {
-- 
1.7.3.2

[PATCH 11/13] cifs: set up recurring workqueue job to do SMB echo requests

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |    1 +
 fs/cifs/connect.c  |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index fb75f04..d924f3c 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -221,6 +221,7 @@ struct TCP_Server_Info {
 	bool	sec_kerberosu2u;	/* supports U2U Kerberos */
 	bool	sec_ntlmssp;		/* supports NTLMSSP */
 	bool session_estab; /* mark when very first sess is established */
+	struct delayed_work	echo; /* echo ping workqueue job */
 #ifdef CONFIG_CIFS_FSCACHE
 	struct fscache_cookie   *fscache; /* client index cache cookie */
 #endif
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 0feb592..98e1d38 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -52,6 +52,9 @@
 #define CIFS_PORT 445
 #define RFC1001_PORT 139
 
+/* SMB echo "timeout" -- FIXME: tunable? */
+#define SMB_ECHO_INTERVAL (30 * HZ)
+
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);
 
@@ -354,6 +357,26 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
 
 }
 
+static void
+cifs_echo_request(struct work_struct *work)
+{
+	int rc;
+	struct TCP_Server_Info *server = container_of(work,
+					struct TCP_Server_Info, echo.work);
+
+	/* no need to ping if we got a response recently */
+	if (time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
+		goto requeue_echo;
+
+	rc = CIFSSMBEcho(server);
+	if (rc)
+		cFYI(1, "Unable to send echo request to server: %s",
+			server->hostname);
+
+requeue_echo:
+	queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
+}
+
 static int
 cifs_demultiplex_thread(struct TCP_Server_Info *server)
 {
@@ -1610,6 +1633,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 	list_del_init(&server->tcp_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 
+	cancel_delayed_work_sync(&server->echo);
+
 	spin_lock(&GlobalMid_Lock);
 	server->tcpStatus = CifsExiting;
 	spin_unlock(&GlobalMid_Lock);
@@ -1701,6 +1726,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	tcp_ses->sequence_number = 0;
 	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
 	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
+	INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
 
 	/*
 	 * at this point we are the only ones with the pointer
@@ -1750,6 +1776,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 
 	cifs_fscache_get_client_cookie(tcp_ses);
 
+	/* queue echo request delayed work */
+	queue_delayed_work(system_nrt_wq, &tcp_ses->echo, SMB_ECHO_INTERVAL);
+
 	return tcp_ses;
 
 out_err_crypto_release:
-- 
1.7.3.2

[PATCH 12/13] cifs: reconnect unresponsive servers

[Jeff Layton]

If the server isn't responding to echoes, we don't want to leave tasks
hung waiting for it to reply. At that point, we'll want to reconnect
so that soft mounts can return an error to userspace quickly.

If the client hasn't received a reply after 3 echo intervals, assume
that the transport is down and attempt to reconnect the socket.

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

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 98e1d38..c596515 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -55,6 +55,9 @@
 /* SMB echo "timeout" -- FIXME: tunable? */
 #define SMB_ECHO_INTERVAL (30 * HZ)
 
+/* reconnect if no response from server in this time period */
+#define UNRESPONSIVE_SERVER_TIMEOUT (3 * SMB_ECHO_INTERVAL)
+
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);
 
@@ -187,6 +190,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 	kfree(server->session_key.response);
 	server->session_key.response = NULL;
 	server->session_key.len = 0;
+	server->lstrp = jiffies;
 	mutex_unlock(&server->srv_mutex);
 
 	/*
@@ -442,7 +446,19 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 		smb_msg.msg_control = NULL;
 		smb_msg.msg_controllen = 0;
 		pdu_length = 4; /* enough to get RFC1001 header */
+
 incomplete_rcv:
+		if (time_after(jiffies,
+			       server->lstrp + UNRESPONSIVE_SERVER_TIMEOUT)) {
+			cERROR(1, "Server %s has not responded in %d seconds. "
+				  "Reconnecting...", server->hostname,
+				  UNRESPONSIVE_SERVER_TIMEOUT / HZ);
+			cifs_reconnect(server);
+			csocket = server->ssocket;
+			wake_up(&server->response_q);
+			continue;
+		}
+
 		length =
 		    kernel_recvmsg(csocket, &smb_msg,
 				&iov, 1, pdu_length, 0 /* BB other flags? */);
@@ -603,6 +619,8 @@ incomplete_rcv:
 		}
 
 		mid_entry = NULL;
+		server->lstrp = jiffies;
+
 		spin_lock(&GlobalMid_Lock);
 		list_for_each(tmp, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
@@ -653,10 +671,6 @@ multi_t2_fnd:
 #ifdef CONFIG_CIFS_STATS2
 				mid_entry->when_received = jiffies;
 #endif
-				/* so we do not time out requests to  server
-				which is still responding (since server could
-				be busy but not dead) */
-				server->lstrp = jiffies;
 				break;
 			}
 next_mid:
@@ -1724,6 +1738,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		volume_info->target_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL);
 	tcp_ses->session_estab = false;
 	tcp_ses->sequence_number = 0;
+	tcp_ses->lstrp = jiffies;
 	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
 	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
 	INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
-- 
1.7.3.2

[PATCH 13/13] cifs: remove code for setting timeouts on requests

[Jeff Layton]

Since we don't time out individual requests anymore, remove the code
that we used to use for setting timeouts on different requests.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    9 +++------
 fs/cifs/cifssmb.c   |    8 ++++----
 fs/cifs/connect.c   |    2 +-
 fs/cifs/file.c      |   44 +++++++-------------------------------------
 fs/cifs/sess.c      |    2 +-
 fs/cifs/transport.c |    2 +-
 6 files changed, 17 insertions(+), 50 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index d924f3c..1e290fa 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -631,12 +631,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 #define   CIFS_IOVEC            4    /* array of response buffers */
 
 /* Type of Request to SendReceive2 */
-#define   CIFS_STD_OP	        0    /* normal request timeout */
-#define   CIFS_LONG_OP          1    /* long op (up to 45 sec, oplock time) */
-#define   CIFS_VLONG_OP         2    /* sloow op - can take up to 180 seconds */
-#define   CIFS_BLOCKING_OP      4    /* operation can block */
-#define   CIFS_ASYNC_OP         8    /* do not wait for response */
-#define   CIFS_TIMEOUT_MASK 0x00F    /* only one of 5 above set in req */
+#define   CIFS_BLOCKING_OP      1    /* operation can block */
+#define   CIFS_ASYNC_OP         2    /* do not wait for response */
+#define   CIFS_TIMEOUT_MASK 0x003    /* only one of above set in req */
 #define   CIFS_LOG_ERROR    0x010    /* log NT STATUS if non-zero */
 #define   CIFS_LARGE_BUF_OP 0x020    /* large request buffer */
 #define   CIFS_NO_RESP      0x040    /* no response buffer required */
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index fc8145f..606deba 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1240,7 +1240,7 @@ OldOpenRetry:
 	pSMB->ByteCount = cpu_to_le16(count);
 	/* long_op set to 1 to allow for oplock break timeouts */
 	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			(struct smb_hdr *)pSMBr, &bytes_returned, CIFS_LONG_OP);
+			(struct smb_hdr *)pSMBr, &bytes_returned, 0);
 	cifs_stats_inc(&tcon->num_opens);
 	if (rc) {
 		cFYI(1, "Error in Open = %d", rc);
@@ -1353,7 +1353,7 @@ openRetry:
 	pSMB->ByteCount = cpu_to_le16(count);
 	/* long_op set to 1 to allow for oplock break timeouts */
 	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			(struct smb_hdr *)pSMBr, &bytes_returned, CIFS_LONG_OP);
+			(struct smb_hdr *)pSMBr, &bytes_returned, 0);
 	cifs_stats_inc(&tcon->num_opens);
 	if (rc) {
 		cFYI(1, "Error in Open = %d", rc);
@@ -1435,7 +1435,7 @@ CIFSSMBRead(const int xid, struct cifsTconInfo *tcon, const int netfid,
 	iov[0].iov_base = (char *)pSMB;
 	iov[0].iov_len = pSMB->hdr.smb_buf_length + 4;
 	rc = SendReceive2(xid, tcon->ses, iov, 1 /* num iovecs */,
-			 &resp_buf_type, CIFS_STD_OP | CIFS_LOG_ERROR);
+			 &resp_buf_type, CIFS_LOG_ERROR);
 	cifs_stats_inc(&tcon->num_reads);
 	pSMBr = (READ_RSP *)iov[0].iov_base;
 	if (rc) {
@@ -3030,7 +3030,7 @@ CIFSSMBGetCIFSACL(const int xid, struct cifsTconInfo *tcon, __u16 fid,
 	iov[0].iov_len = pSMB->hdr.smb_buf_length + 4;
 
 	rc = SendReceive2(xid, tcon->ses, iov, 1 /* num iovec */, &buf_type,
-			 CIFS_STD_OP);
+			 0);
 	cifs_stats_inc(&tcon->num_acl_get);
 	if (rc) {
 		cFYI(1, "Send error in QuerySecDesc = %d", rc);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c596515..b8fb112 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3134,7 +3134,7 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses,
 	pSMB->ByteCount = cpu_to_le16(count);
 
 	rc = SendReceive(xid, ses, smb_buffer, smb_buffer_response, &length,
-			 CIFS_STD_OP);
+			 0);
 
 	/* above now done in SendReceive */
 	if ((rc == 0) && (tcon != NULL)) {
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8e57370..5c7b4f0 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -892,29 +892,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 	return rc;
 }
 
-/*
- * Set the timeout on write requests past EOF. For some servers (Windows)
- * these calls can be very long.
- *
- * If we're writing >10M past the EOF we give a 180s timeout. Anything less
- * than that gets a 45s timeout. Writes not past EOF get 15s timeouts.
- * The 10M cutoff is totally arbitrary. A better scheme for this would be
- * welcome if someone wants to suggest one.
- *
- * We may be able to do a better job with this if there were some way to
- * declare that a file should be sparse.
- */
-static int
-cifs_write_timeout(struct cifsInodeInfo *cifsi, loff_t offset)
-{
-	if (offset <= cifsi->server_eof)
-		return CIFS_STD_OP;
-	else if (offset > (cifsi->server_eof + (10 * 1024 * 1024)))
-		return CIFS_VLONG_OP;
-	else
-		return CIFS_LONG_OP;
-}
-
 /* update the file size (if needed) after a write */
 static void
 cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
@@ -935,7 +912,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 	unsigned int total_written;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *pTcon;
-	int xid, long_op;
+	int xid;
 	struct cifsFileInfo *open_file;
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
 
@@ -956,7 +933,6 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 
 	xid = GetXid();
 
-	long_op = cifs_write_timeout(cifsi, *poffset);
 	for (total_written = 0; write_size > total_written;
 	     total_written += bytes_written) {
 		rc = -EAGAIN;
@@ -984,7 +960,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 				min_t(const int, cifs_sb->wsize,
 				      write_size - total_written),
 				*poffset, &bytes_written,
-				NULL, write_data + total_written, long_op);
+				NULL, write_data + total_written, 0);
 		}
 		if (rc || (bytes_written == 0)) {
 			if (total_written)
@@ -997,8 +973,6 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 			cifs_update_eof(cifsi, *poffset, bytes_written);
 			*poffset += bytes_written;
 		}
-		long_op = CIFS_STD_OP; /* subsequent writes fast -
-				    15 seconds is plenty */
 	}
 
 	cifs_stats_bytes_written(pTcon, total_written);
@@ -1027,7 +1001,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 	unsigned int total_written;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *pTcon;
-	int xid, long_op;
+	int xid;
 	struct dentry *dentry = open_file->dentry;
 	struct cifsInodeInfo *cifsi = CIFS_I(dentry->d_inode);
 
@@ -1040,7 +1014,6 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 
 	xid = GetXid();
 
-	long_op = cifs_write_timeout(cifsi, *poffset);
 	for (total_written = 0; write_size > total_written;
 	     total_written += bytes_written) {
 		rc = -EAGAIN;
@@ -1070,7 +1043,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 				rc = CIFSSMBWrite2(xid, pTcon,
 						open_file->netfid, len,
 						*poffset, &bytes_written,
-						iov, 1, long_op);
+						iov, 1, 0);
 			} else
 				rc = CIFSSMBWrite(xid, pTcon,
 					 open_file->netfid,
@@ -1078,7 +1051,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 					       write_size - total_written),
 					 *poffset, &bytes_written,
 					 write_data + total_written,
-					 NULL, long_op);
+					 NULL, 0);
 		}
 		if (rc || (bytes_written == 0)) {
 			if (total_written)
@@ -1091,8 +1064,6 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 			cifs_update_eof(cifsi, *poffset, bytes_written);
 			*poffset += bytes_written;
 		}
-		long_op = CIFS_STD_OP; /* subsequent writes fast -
-				    15 seconds is plenty */
 	}
 
 	cifs_stats_bytes_written(pTcon, total_written);
@@ -1292,7 +1263,7 @@ static int cifs_writepages(struct address_space *mapping,
 	struct pagevec pvec;
 	int rc = 0;
 	int scanned = 0;
-	int xid, long_op;
+	int xid;
 
 	cifs_sb = CIFS_SB(mapping->host->i_sb);
 
@@ -1437,11 +1408,10 @@ retry_write:
 				cERROR(1, "No writable handles for inode");
 				rc = -EBADF;
 			} else {
-				long_op = cifs_write_timeout(cifsi, offset);
 				rc = CIFSSMBWrite2(xid, tcon, open_file->netfid,
 						   bytes_to_write, offset,
 						   &bytes_written, iov, n_iov,
-						   long_op);
+						   0);
 				cifsFileInfo_put(open_file);
 				cifs_update_eof(cifsi, offset, bytes_written);
 			}
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 2997533..50b74a5 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -881,7 +881,7 @@ ssetup_ntlmssp_authenticate:
 	BCC_LE(smb_buf) = cpu_to_le16(count);
 
 	rc = SendReceive2(xid, ses, iov, 3 /* num_iovecs */, &resp_buf_type,
-			  CIFS_STD_OP /* not long */ | CIFS_LOG_ERROR);
+			  CIFS_LOG_ERROR);
 	/* SMB request buf freed in SendReceive2 */
 
 	pSMB = (SESSION_SETUP_ANDX *)iov[0].iov_base;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 2a0b14d..70873ab 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -791,7 +791,7 @@ send_lock_cancel(const unsigned int xid, struct cifsTconInfo *tcon,
 	pSMB->hdr.Mid = GetNextMid(ses->server);
 
 	return SendReceive(xid, ses, in_buf, out_buf,
-			&bytes_returned, CIFS_STD_OP);
+			&bytes_returned, 0);
 }
 
 int
-- 
1.7.3.2

[PATCH 00/13] cifs: don't fail writepages on -EAGAIN errors (try #2)

[Jeff Layton]

If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
temporary. CIFS should retry the write instead of setting an error on
the mapping and returning.

For WB_SYNC_ALL, just retry the write immediately. In the WB_SYNC_NONE
case, call redirty_page_for_writeback on all of the pages that didn't
get written out and then move on.

Also, fix up the handling of a short write with a successful return
code. MS-CIFS says that 0 bytes_written means ENOSPC or EFBIG. It
doesn't mention what a short, but non-zero write means, so for now
treat it as we would an -EAGAIN return.

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

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index fe16f6d..8bef611 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1430,6 +1430,7 @@ retry:
 				break;
 		}
 		if (n_iov) {
+retry_write:
 			open_file = find_writable_file(CIFS_I(mapping->host),
 							false);
 			if (!open_file) {
@@ -1442,31 +1443,54 @@ retry:
 						   &bytes_written, iov, n_iov,
 						   long_op);
 				cifsFileInfo_put(open_file);
-				cifs_update_eof(cifsi, offset, bytes_written);
 			}
 
-			if (rc || bytes_written < bytes_to_write) {
-				cERROR(1, "Write2 ret %d, wrote %d",
-					  rc, bytes_written);
-				mapping_set_error(mapping, rc);
-			} else {
+			cFYI(1, "Write2 rc=%d, wrote=%u", rc, bytes_written);
+
+			/*
+			 * For now, treat a short write as if nothing got
+			 * written. A zero length write however indicates
+			 * ENOSPC or EFBIG. We have no way to know which
+			 * though, so call it ENOSPC for now. EFBIG would
+			 * get translated to AS_EIO anyway.
+			 *
+			 * FIXME: make it take into account the data that did
+			 *	  get written
+			 */
+			if (rc == 0) {
+				if (bytes_written == 0)
+					rc = -ENOSPC;
+				else if (bytes_written < bytes_to_write)
+					rc = -EAGAIN;
+			}
+
+			/* retry on data-integrity flush */
+			if (wbc->sync_mode == WB_SYNC_ALL &&
+			    (rc == -EAGAIN || bytes_written < bytes_to_write))
+				goto retry_write;
+
+			/* fix the stats and EOF */
+			if (bytes_written > 0) {
 				cifs_stats_bytes_written(tcon, bytes_written);
+				cifs_update_eof(cifsi, offset, bytes_written);
 			}
 
 			for (i = 0; i < n_iov; i++) {
 				page = pvec.pages[first + i];
-				/* Should we also set page error on
-				success rc but too little data written? */
-				/* BB investigate retry logic on temporary
-				server crash cases and how recovery works
-				when page marked as error */
-				if (rc)
+				/* on retryable write error, redirty page */
+				if (rc == -EAGAIN)
+					redirty_page_for_writepage(wbc, page);
+				else if (rc != 0)
 					SetPageError(page);
 				kunmap(page);
 				unlock_page(page);
 				end_page_writeback(page);
 				page_cache_release(page);
 			}
+
+			if (rc != -EAGAIN)
+				mapping_set_error(mapping, rc);
+
 			if ((wbc->nr_to_write -= n_iov) <= 0)
 				done = 1;
 			index = next;
-- 
1.7.3.2

Re: [PATCH 03/13] cifs: move mid result processing into common function

[Shirish Pargaonkar]

On Fri, Dec 10, 2010 at 9:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |  131 ++++++++++++++++++--------------------------------
>  1 files changed, 47 insertions(+), 84 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 9763f89..2d21bbd 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -382,6 +382,46 @@ SendReceiveNoRsp(const unsigned int xid, struct cifsSesInfo *ses,
>        return rc;
>  }
>
> +static int
> +handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
> +{
> +       int rc = 0;
> +
> +       spin_lock(&GlobalMid_Lock);
> +
> +       if (mid->resp_buf) {
> +               spin_unlock(&GlobalMid_Lock);
> +               return rc;
> +       }
> +
> +       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
> +       if (mid->midState == MID_REQUEST_SUBMITTED) {
> +               if (server->tcpStatus == CifsExiting)
> +                       rc = -EHOSTDOWN;
> +               else {
> +                       server->tcpStatus = CifsNeedReconnect;
> +                       mid->midState = MID_RETRY_NEEDED;
> +               }
> +       }
> +
> +       if (rc != -EHOSTDOWN) {
> +               if (mid->midState == MID_RETRY_NEEDED) {
> +                       rc = -EAGAIN;
> +                       cFYI(1, "marking request for retry");
> +               } else {
> +                       rc = -EIO;

I think this else part is redundant.  If rc is not EHOSTDOWN,
which means it is 0, midState will always be MID_RETRY_NEEDED.

> +               }
> +       }
> +       spin_unlock(&GlobalMid_Lock);
> +
> +       DeleteMidQEntry(mid);
> +       /* Update # of requests on wire to server */
> +       atomic_dec(&server->inFlight);
> +       wake_up(&server->request_q);
> +
> +       return rc;
> +}
> +
>  int
>  SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>             struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
> @@ -485,37 +525,10 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>        /* No user interrupts in wait - wreaks havoc with performance */
>        wait_for_response(ses, midQ, timeout, 10 * HZ);
>
> -       spin_lock(&GlobalMid_Lock);
> -
> -       if (midQ->resp_buf == NULL) {
> -               cERROR(1, "No response to cmd %d mid %d",
> -                       midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> -               /* Update # of requests on wire to server */
> -               atomic_dec(&ses->server->inFlight);
> -               wake_up(&ses->server->request_q);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> -       spin_unlock(&GlobalMid_Lock);
>        receive_len = midQ->resp_buf->smb_buf_length;
>
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> @@ -677,36 +690,10 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>        /* No user interrupts in wait - wreaks havoc with performance */
>        wait_for_response(ses, midQ, timeout, 10 * HZ);
>
> -       spin_lock(&GlobalMid_Lock);
> -       if (midQ->resp_buf == NULL) {
> -               cERROR(1, "No response for cmd %d mid %d",
> -                         midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> -               /* Update # of requests on wire to server */
> -               atomic_dec(&ses->server->inFlight);
> -               wake_up(&ses->server->request_q);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> -       spin_unlock(&GlobalMid_Lock);
>        receive_len = midQ->resp_buf->smb_buf_length;
>
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> @@ -926,35 +913,11 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>                }
>        }
>
> -       spin_lock(&GlobalMid_Lock);
> -       if (midQ->resp_buf) {
> -               spin_unlock(&GlobalMid_Lock);
> -               receive_len = midQ->resp_buf->smb_buf_length;
> -       } else {
> -               cERROR(1, "No response for cmd %d mid %d",
> -                         midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> +       receive_len = midQ->resp_buf->smb_buf_length;
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
>                cERROR(1, "Frame too large received.  Length: %d  Xid: %d",
>                        receive_len, xid);
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 06/13] cifs: clean up handle_mid_response

[Shirish Pargaonkar]

On Fri, Dec 10, 2010 at 9:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Make it use a switch statement based on the value of the midStatus. If
> the resp_buf is set, then MID_RESPONSE_RECEIVED is too.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   36 ++++++++++++++++++------------------
>  1 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 0c0dadd..05ced17 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -356,33 +356,33 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>  {
>        int rc = 0;
>
> -       spin_lock(&GlobalMid_Lock);
> +       cFYI(1, "%s: cmd=%d mid=%d state=%d", __func__, mid->command,
> +               mid->mid, mid->midState);
>
> -       if (mid->resp_buf) {
> +       spin_lock(&GlobalMid_Lock);
> +       switch (mid->midState) {
> +       case MID_RESPONSE_RECEIVED:
>                spin_unlock(&GlobalMid_Lock);
>                return rc;
> -       }
> -
> -       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
> -       if (mid->midState == MID_REQUEST_SUBMITTED) {
> -               if (server->tcpStatus == CifsExiting)
> +       case MID_REQUEST_SUBMITTED:
> +               /* socket is going down, reject all calls */
> +               if (server->tcpStatus == CifsExiting) {
> +                       cERROR(1, "%s: canceling mid=%d cmd=0x%x state=%d",
> +                              __func__, mid->mid, mid->command, mid->midState);
>                        rc = -EHOSTDOWN;
> -               else
> -                       mid->midState = MID_RETRY_NEEDED;
> -       }
> -
> -       if (rc != -EHOSTDOWN) {
> -               if (mid->midState == MID_RETRY_NEEDED) {
> -                       rc = -EAGAIN;
> -                       cFYI(1, "marking request for retry");
> -               } else {
> -                       rc = -EIO;
> +                       break;
>                }
> +               mid->midState = MID_RETRY_NEEDED;

Would it be cleaner to set rc to -EAGAIN instead of setting state here
to fall down and then setting rc as -EAGAIN?

> +       case MID_RETRY_NEEDED:
> +               rc = -EAGAIN;
> +               break;
> +       default:
> +               cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
> +                       mid->mid, mid->midState);
>        }
>        spin_unlock(&GlobalMid_Lock);
>
>        DeleteMidQEntry(mid);
> -       /* Update # of requests on wire to server */
>        atomic_dec(&server->inFlight);
>        wake_up(&server->request_q);
>
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 03/13] cifs: move mid result processing into common function

[Jeff Layton]

On Sun, 12 Dec 2010 17:29:58 -0600
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Dec 10, 2010 at 9:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/transport.c |  131 ++++++++++++++++++--------------------------------
> >  1 files changed, 47 insertions(+), 84 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 9763f89..2d21bbd 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -382,6 +382,46 @@ SendReceiveNoRsp(const unsigned int xid, struct cifsSesInfo *ses,
> >        return rc;
> >  }
> >
> > +static int
> > +handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
> > +{
> > +       int rc = 0;
> > +
> > +       spin_lock(&GlobalMid_Lock);
> > +
> > +       if (mid->resp_buf) {
> > +               spin_unlock(&GlobalMid_Lock);
> > +               return rc;
> > +       }
> > +
> > +       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
> > +       if (mid->midState == MID_REQUEST_SUBMITTED) {
> > +               if (server->tcpStatus == CifsExiting)
> > +                       rc = -EHOSTDOWN;
> > +               else {
> > +                       server->tcpStatus = CifsNeedReconnect;
> > +                       mid->midState = MID_RETRY_NEEDED;
> > +               }
> > +       }
> > +
> > +       if (rc != -EHOSTDOWN) {
> > +               if (mid->midState == MID_RETRY_NEEDED) {
> > +                       rc = -EAGAIN;
> > +                       cFYI(1, "marking request for retry");
> > +               } else {
> > +                       rc = -EIO;
> 
> I think this else part is redundant.  If rc is not EHOSTDOWN,
> which means it is 0, midState will always be MID_RETRY_NEEDED.
> 

I agree, but my goal with this patch is to consolidate the
cut-and-pasted code as-is into a single spot. A later patch will clean
it up.

> > +               }
> > +       }
> > +       spin_unlock(&GlobalMid_Lock);
> > +
> > +       DeleteMidQEntry(mid);
> > +       /* Update # of requests on wire to server */
> > +       atomic_dec(&server->inFlight);
> > +       wake_up(&server->request_q);
> > +
> > +       return rc;
> > +}
> > +
> >  int
> >  SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
> >             struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
> > @@ -485,37 +525,10 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
> >        /* No user interrupts in wait - wreaks havoc with performance */
> >        wait_for_response(ses, midQ, timeout, 10 * HZ);
> >
> > -       spin_lock(&GlobalMid_Lock);
> > -
> > -       if (midQ->resp_buf == NULL) {
> > -               cERROR(1, "No response to cmd %d mid %d",
> > -                       midQ->command, midQ->mid);
> > -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> > -                       if (ses->server->tcpStatus == CifsExiting)
> > -                               rc = -EHOSTDOWN;
> > -                       else {
> > -                               ses->server->tcpStatus = CifsNeedReconnect;
> > -                               midQ->midState = MID_RETRY_NEEDED;
> > -                       }
> > -               }
> > -
> > -               if (rc != -EHOSTDOWN) {
> > -                       if (midQ->midState == MID_RETRY_NEEDED) {
> > -                               rc = -EAGAIN;
> > -                               cFYI(1, "marking request for retry");
> > -                       } else {
> > -                               rc = -EIO;
> > -                       }
> > -               }
> > -               spin_unlock(&GlobalMid_Lock);
> > -               DeleteMidQEntry(midQ);
> > -               /* Update # of requests on wire to server */
> > -               atomic_dec(&ses->server->inFlight);
> > -               wake_up(&ses->server->request_q);
> > +       rc = handle_mid_result(midQ, ses->server);
> > +       if (rc != 0)
> >                return rc;
> > -       }
> >
> > -       spin_unlock(&GlobalMid_Lock);
> >        receive_len = midQ->resp_buf->smb_buf_length;
> >
> >        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> > @@ -677,36 +690,10 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
> >        /* No user interrupts in wait - wreaks havoc with performance */
> >        wait_for_response(ses, midQ, timeout, 10 * HZ);
> >
> > -       spin_lock(&GlobalMid_Lock);
> > -       if (midQ->resp_buf == NULL) {
> > -               cERROR(1, "No response for cmd %d mid %d",
> > -                         midQ->command, midQ->mid);
> > -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> > -                       if (ses->server->tcpStatus == CifsExiting)
> > -                               rc = -EHOSTDOWN;
> > -                       else {
> > -                               ses->server->tcpStatus = CifsNeedReconnect;
> > -                               midQ->midState = MID_RETRY_NEEDED;
> > -                       }
> > -               }
> > -
> > -               if (rc != -EHOSTDOWN) {
> > -                       if (midQ->midState == MID_RETRY_NEEDED) {
> > -                               rc = -EAGAIN;
> > -                               cFYI(1, "marking request for retry");
> > -                       } else {
> > -                               rc = -EIO;
> > -                       }
> > -               }
> > -               spin_unlock(&GlobalMid_Lock);
> > -               DeleteMidQEntry(midQ);
> > -               /* Update # of requests on wire to server */
> > -               atomic_dec(&ses->server->inFlight);
> > -               wake_up(&ses->server->request_q);
> > +       rc = handle_mid_result(midQ, ses->server);
> > +       if (rc != 0)
> >                return rc;
> > -       }
> >
> > -       spin_unlock(&GlobalMid_Lock);
> >        receive_len = midQ->resp_buf->smb_buf_length;
> >
> >        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> > @@ -926,35 +913,11 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
> >                }
> >        }
> >
> > -       spin_lock(&GlobalMid_Lock);
> > -       if (midQ->resp_buf) {
> > -               spin_unlock(&GlobalMid_Lock);
> > -               receive_len = midQ->resp_buf->smb_buf_length;
> > -       } else {
> > -               cERROR(1, "No response for cmd %d mid %d",
> > -                         midQ->command, midQ->mid);
> > -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> > -                       if (ses->server->tcpStatus == CifsExiting)
> > -                               rc = -EHOSTDOWN;
> > -                       else {
> > -                               ses->server->tcpStatus = CifsNeedReconnect;
> > -                               midQ->midState = MID_RETRY_NEEDED;
> > -                       }
> > -               }
> > -
> > -               if (rc != -EHOSTDOWN) {
> > -                       if (midQ->midState == MID_RETRY_NEEDED) {
> > -                               rc = -EAGAIN;
> > -                               cFYI(1, "marking request for retry");
> > -                       } else {
> > -                               rc = -EIO;
> > -                       }
> > -               }
> > -               spin_unlock(&GlobalMid_Lock);
> > -               DeleteMidQEntry(midQ);
> > +       rc = handle_mid_result(midQ, ses->server);
> > +       if (rc != 0)
> >                return rc;
> > -       }
> >
> > +       receive_len = midQ->resp_buf->smb_buf_length;
> >        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> >                cERROR(1, "Frame too large received.  Length: %d  Xid: %d",
> >                        receive_len, xid);
> > --
> > 1.7.3.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >


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

Re: [PATCH 06/13] cifs: clean up handle_mid_response

[Jeff Layton]

On Sun, 12 Dec 2010 17:52:26 -0600
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Dec 10, 2010 at 9:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Make it use a switch statement based on the value of the midStatus. If
> > the resp_buf is set, then MID_RESPONSE_RECEIVED is too.
> >
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/transport.c |   36 ++++++++++++++++++------------------
> >  1 files changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 0c0dadd..05ced17 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -356,33 +356,33 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
> >  {
> >        int rc = 0;
> >
> > -       spin_lock(&GlobalMid_Lock);
> > +       cFYI(1, "%s: cmd=%d mid=%d state=%d", __func__, mid->command,
> > +               mid->mid, mid->midState);
> >
> > -       if (mid->resp_buf) {
> > +       spin_lock(&GlobalMid_Lock);
> > +       switch (mid->midState) {
> > +       case MID_RESPONSE_RECEIVED:
> >                spin_unlock(&GlobalMid_Lock);
> >                return rc;
> > -       }
> > -
> > -       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
> > -       if (mid->midState == MID_REQUEST_SUBMITTED) {
> > -               if (server->tcpStatus == CifsExiting)
> > +       case MID_REQUEST_SUBMITTED:
> > +               /* socket is going down, reject all calls */
> > +               if (server->tcpStatus == CifsExiting) {
> > +                       cERROR(1, "%s: canceling mid=%d cmd=0x%x state=%d",
> > +                              __func__, mid->mid, mid->command, mid->midState);
> >                        rc = -EHOSTDOWN;
> > -               else
> > -                       mid->midState = MID_RETRY_NEEDED;
> > -       }
> > -
> > -       if (rc != -EHOSTDOWN) {
> > -               if (mid->midState == MID_RETRY_NEEDED) {
> > -                       rc = -EAGAIN;
> > -                       cFYI(1, "marking request for retry");
> > -               } else {
> > -                       rc = -EIO;
> > +                       break;
> >                }
> > +               mid->midState = MID_RETRY_NEEDED;
> 
> Would it be cleaner to set rc to -EAGAIN instead of setting state here
> to fall down and then setting rc as -EAGAIN?
> 

I'm not sure it'll be any more clear. It seems clear enough to me -- if
it's still SUBMITTED, then we want to treat it if it were
"RETRY_NEEDED".

Honestly, this really shouldn't happen. With this patchset, you should
never reach this function if the state is still 'SUBMITTED'. I left it
there for completeness sake and future-proofness.

> > +       case MID_RETRY_NEEDED:
> > +               rc = -EAGAIN;
> > +               break;
> > +       default:
> > +               cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
> > +                       mid->mid, mid->midState);
> >        }
> >        spin_unlock(&GlobalMid_Lock);
> >
> >        DeleteMidQEntry(mid);
> > -       /* Update # of requests on wire to server */
> >        atomic_dec(&server->inFlight);
> >        wake_up(&server->request_q);
> >
> > --
> > 1.7.3.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >


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

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
> temporary. CIFS should retry the write instead of setting an error on
> the mapping and returning.
>
> For WB_SYNC_ALL, just retry the write immediately.
>
> In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
> pages that didn't get written out and then move on.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/file.c |   23 +++++++++++------------
>  1 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index fe16f6d..8e57370 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1430,6 +1430,7 @@ retry:
>                                break;
>                }
>                if (n_iov) {
> +retry_write:
>                        open_file = find_writable_file(CIFS_I(mapping->host),
>                                                        false);
>                        if (!open_file) {
> @@ -1445,22 +1446,20 @@ retry:
>                                cifs_update_eof(cifsi, offset, bytes_written);
>                        }
>
> -                       if (rc || bytes_written < bytes_to_write) {
> -                               cERROR(1, "Write2 ret %d, wrote %d",
> -                                         rc, bytes_written);
> -                               mapping_set_error(mapping, rc);
> -                       } else {
> +                       /* retry on data-integrity flush */
> +                       if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
> +                               goto retry_write;
> +
> +                       if (!rc)
>                                cifs_stats_bytes_written(tcon, bytes_written);
> -                       }
> +                       else if (rc != -EAGAIN)
> +                               mapping_set_error(mapping, rc);
>
>                        for (i = 0; i < n_iov; i++) {
>                                page = pvec.pages[first + i];
> -                               /* Should we also set page error on
> -                               success rc but too little data written? */
> -                               /* BB investigate retry logic on temporary
> -                               server crash cases and how recovery works
> -                               when page marked as error */
> -                               if (rc)
> +                               if (rc == -EAGAIN)
> +                                       redirty_page_for_writepage(wbc, page);
> +                               else if (rc != 0)
>                                        SetPageError(page);
>                                kunmap(page);
>                                unlock_page(page);
> --
> 1.7.3.2
>

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 02/13] cifs: make wait_for_free_request take a TCP_Server_Info pointer

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> The cifsSesInfo pointer is only used to get at the server.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   26 +++++++++++++-------------
>  1 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index e0588cd..9763f89 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -244,31 +244,31 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
>        return smb_sendv(server, &iov, 1);
>  }
>
> -static int wait_for_free_request(struct cifsSesInfo *ses, const int long_op)
> +static int wait_for_free_request(struct TCP_Server_Info *server,
> +                                const int long_op)
>  {
>        if (long_op == CIFS_ASYNC_OP) {
>                /* oplock breaks must not be held up */
> -               atomic_inc(&ses->server->inFlight);
> +               atomic_inc(&server->inFlight);
>                return 0;
>        }
>
>        spin_lock(&GlobalMid_Lock);
>        while (1) {
> -               if (atomic_read(&ses->server->inFlight) >=
> -                               cifs_max_pending){
> +               if (atomic_read(&server->inFlight) >= cifs_max_pending) {
>                        spin_unlock(&GlobalMid_Lock);
>  #ifdef CONFIG_CIFS_STATS2
> -                       atomic_inc(&ses->server->num_waiters);
> +                       atomic_inc(&server->num_waiters);
>  #endif
> -                       wait_event(ses->server->request_q,
> -                                  atomic_read(&ses->server->inFlight)
> +                       wait_event(server->request_q,
> +                                  atomic_read(&server->inFlight)
>                                     < cifs_max_pending);
>  #ifdef CONFIG_CIFS_STATS2
> -                       atomic_dec(&ses->server->num_waiters);
> +                       atomic_dec(&server->num_waiters);
>  #endif
>                        spin_lock(&GlobalMid_Lock);
>                } else {
> -                       if (ses->server->tcpStatus == CifsExiting) {
> +                       if (server->tcpStatus == CifsExiting) {
>                                spin_unlock(&GlobalMid_Lock);
>                                return -ENOENT;
>                        }
> @@ -278,7 +278,7 @@ static int wait_for_free_request(struct cifsSesInfo *ses, const int long_op)
>
>                        /* update # of requests on the wire to server */
>                        if (long_op != CIFS_BLOCKING_OP)
> -                               atomic_inc(&ses->server->inFlight);
> +                               atomic_inc(&server->inFlight);
>                        spin_unlock(&GlobalMid_Lock);
>                        break;
>                }
> @@ -413,7 +413,7 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>           to the same server. We may make this configurable later or
>           use ses->maxReq */
>
> -       rc = wait_for_free_request(ses, long_op);
> +       rc = wait_for_free_request(ses->server, long_op);
>        if (rc) {
>                cifs_small_buf_release(in_buf);
>                return rc;
> @@ -610,7 +610,7 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>                return -EIO;
>        }
>
> -       rc = wait_for_free_request(ses, long_op);
> +       rc = wait_for_free_request(ses->server, long_op);
>        if (rc)
>                return rc;
>
> @@ -845,7 +845,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>                return -EIO;
>        }
>
> -       rc = wait_for_free_request(ses, CIFS_BLOCKING_OP);
> +       rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP);
>        if (rc)
>                return rc;
>
> --
> 1.7.3.2


Good cleanup.
Reviewed-by:  Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 04/13] cifs: wait indefinitely for responses

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> The client should not be timing out on individual SMB requests. Too much
> of the state between client and server is tied to the state of the
> socket. If we time out requests and issue spurious disconnects then that
> comprimises data integrity.
>
> Instead of doing this complicated dance where we try to decide how long
> to wait for a response for particular requests, have the client instead
> wait indefinitely for a response. Also, use a TASK_KILLABLE sleep here
> so that fatal signals will break out of this waiting.
>
> Later patches will add support for detecting dead peers and forcing
> reconnects based on that.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |  110 ++++++++-------------------------------------------
>  1 files changed, 17 insertions(+), 93 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 2d21bbd..989674c 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -311,48 +311,17 @@ static int allocate_mid(struct cifsSesInfo *ses, struct smb_hdr *in_buf,
>        return 0;
>  }
>
> -static int wait_for_response(struct cifsSesInfo *ses,
> -                       struct mid_q_entry *midQ,
> -                       unsigned long timeout,
> -                       unsigned long time_to_wait)
> +static int
> +wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>  {
> -       unsigned long curr_timeout;
> -
> -       for (;;) {
> -               curr_timeout = timeout + jiffies;
> -               wait_event_timeout(ses->server->response_q,
> -                       midQ->midState != MID_REQUEST_SUBMITTED, timeout);
> -
> -               if (time_after(jiffies, curr_timeout) &&
> -                       (midQ->midState == MID_REQUEST_SUBMITTED) &&
> -                       ((ses->server->tcpStatus == CifsGood) ||
> -                        (ses->server->tcpStatus == CifsNew))) {
> -
> -                       unsigned long lrt;
> +       int error;
>
> -                       /* We timed out. Is the server still
> -                          sending replies ? */
> -                       spin_lock(&GlobalMid_Lock);
> -                       lrt = ses->server->lstrp;
> -                       spin_unlock(&GlobalMid_Lock);
> +       error = wait_event_killable(server->response_q,
> +                                   midQ->midState != MID_REQUEST_SUBMITTED);
> +       if (error < 0)
> +               return -ERESTARTSYS;
>
> -                       /* Calculate time_to_wait past last receive time.
> -                        Although we prefer not to time out if the
> -                        server is still responding - we will time
> -                        out if the server takes more than 15 (or 45
> -                        or 180) seconds to respond to this request
> -                        and has not responded to any request from
> -                        other threads on the client within 10 seconds */
> -                       lrt += time_to_wait;
> -                       if (time_after(jiffies, lrt)) {
> -                               /* No replies for time_to_wait. */
> -                               cERROR(1, "server not responding");
> -                               return -1;
> -                       }
> -               } else {
> -                       return 0;
> -               }
> -       }
> +       return 0;
>  }
>
>
> @@ -430,7 +399,6 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>        int rc = 0;
>        int long_op;
>        unsigned int receive_len;
> -       unsigned long timeout;
>        struct mid_q_entry *midQ;
>        struct smb_hdr *in_buf = iov[0].iov_base;
>
> @@ -497,33 +465,12 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>        if (rc < 0)
>                goto out;
>
> -       if (long_op == CIFS_STD_OP)
> -               timeout = 15 * HZ;
> -       else if (long_op == CIFS_VLONG_OP) /* e.g. slow writes past EOF */
> -               timeout = 180 * HZ;
> -       else if (long_op == CIFS_LONG_OP)
> -               timeout = 45 * HZ; /* should be greater than
> -                       servers oplock break timeout (about 43 seconds) */
> -       else if (long_op == CIFS_ASYNC_OP)
> -               goto out;
> -       else if (long_op == CIFS_BLOCKING_OP)
> -               timeout = 0x7FFFFFFF; /*  large, but not so large as to wrap */
> -       else {
> -               cERROR(1, "unknown timeout flag %d", long_op);
> -               rc = -EIO;
> +       if (long_op == CIFS_ASYNC_OP)
>                goto out;
> -       }
> -
> -       /* wait for 15 seconds or until woken up due to response arriving or
> -          due to last connection to this server being unmounted */
> -       if (signal_pending(current)) {
> -               /* if signal pending do not hold up user for full smb timeout
> -               but we still give response a chance to complete */
> -               timeout = 2 * HZ;
> -       }
>
> -       /* No user interrupts in wait - wreaks havoc with performance */
> -       wait_for_response(ses, midQ, timeout, 10 * HZ);
> +       rc = wait_for_response(ses->server, midQ);
> +       if (rc != 0)
> +               goto out;
>
>        rc = handle_mid_result(midQ, ses->server);
>        if (rc != 0)
> @@ -598,7 +545,6 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>  {
>        int rc = 0;
>        unsigned int receive_len;
> -       unsigned long timeout;
>        struct mid_q_entry *midQ;
>
>        if (ses == NULL) {
> @@ -662,33 +608,12 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>        if (rc < 0)
>                goto out;
>
> -       if (long_op == CIFS_STD_OP)
> -               timeout = 15 * HZ;
> -       /* wait for 15 seconds or until woken up due to response arriving or
> -          due to last connection to this server being unmounted */
> -       else if (long_op == CIFS_ASYNC_OP)
> -               goto out;
> -       else if (long_op == CIFS_VLONG_OP) /* writes past EOF can be slow */
> -               timeout = 180 * HZ;
> -       else if (long_op == CIFS_LONG_OP)
> -               timeout = 45 * HZ; /* should be greater than
> -                       servers oplock break timeout (about 43 seconds) */
> -       else if (long_op == CIFS_BLOCKING_OP)
> -               timeout = 0x7FFFFFFF; /* large but no so large as to wrap */
> -       else {
> -               cERROR(1, "unknown timeout flag %d", long_op);
> -               rc = -EIO;
> +       if (long_op == CIFS_ASYNC_OP)
>                goto out;
> -       }
>
> -       if (signal_pending(current)) {
> -               /* if signal pending do not hold up user for full smb timeout
> -               but we still give response a chance to complete */
> -               timeout = 2 * HZ;
> -       }
> -
> -       /* No user interrupts in wait - wreaks havoc with performance */
> -       wait_for_response(ses, midQ, timeout, 10 * HZ);
> +       rc = wait_for_response(ses->server, midQ);
> +       if (rc != 0)
> +               goto out;
>
>        rc = handle_mid_result(midQ, ses->server);
>        if (rc != 0)
> @@ -906,8 +831,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>                        }
>                }
>
> -               /* Wait 5 seconds for the response. */
> -               if (wait_for_response(ses, midQ, 5 * HZ, 5 * HZ) == 0) {
> +               if (wait_for_response(ses->server, midQ) == 0) {
>                        /* We got the response - restart system call. */
>                        rstart = 1;
>                }
> --
> 1.7.3.2
>


Reviewed-by:  Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Jeff Layton]

On Mon, 13 Dec 2010 23:01:41 +0300
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
> > temporary. CIFS should retry the write instead of setting an error on
> > the mapping and returning.
> >
> > For WB_SYNC_ALL, just retry the write immediately.
> >
> > In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
> > pages that didn't get written out and then move on.
> >
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/file.c |   23 +++++++++++------------
> >  1 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index fe16f6d..8e57370 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -1430,6 +1430,7 @@ retry:
> >                                break;
> >                }
> >                if (n_iov) {
> > +retry_write:
> >                        open_file = find_writable_file(CIFS_I(mapping->host),
> >                                                        false);
> >                        if (!open_file) {
> > @@ -1445,22 +1446,20 @@ retry:
> >                                cifs_update_eof(cifsi, offset, bytes_written);
> >                        }
> >
> > -                       if (rc || bytes_written < bytes_to_write) {
> > -                               cERROR(1, "Write2 ret %d, wrote %d",
> > -                                         rc, bytes_written);
> > -                               mapping_set_error(mapping, rc);
> > -                       } else {
> > +                       /* retry on data-integrity flush */
> > +                       if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
> > +                               goto retry_write;
> > +
> > +                       if (!rc)
> >                                cifs_stats_bytes_written(tcon, bytes_written);
> > -                       }
> > +                       else if (rc != -EAGAIN)
> > +                               mapping_set_error(mapping, rc);
> >
> >                        for (i = 0; i < n_iov; i++) {
> >                                page = pvec.pages[first + i];
> > -                               /* Should we also set page error on
> > -                               success rc but too little data written? */
> > -                               /* BB investigate retry logic on temporary
> > -                               server crash cases and how recovery works
> > -                               when page marked as error */
> > -                               if (rc)
> > +                               if (rc == -EAGAIN)
> > +                                       redirty_page_for_writepage(wbc, page);
> > +                               else if (rc != 0)
> >                                        SetPageError(page);
> >                                kunmap(page);
> >                                unlock_page(page);
> > --
> > 1.7.3.2
> >
> 
> Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I actually sent a later version of this patch with the unfortunate
subject of:

[PATCH 00/13] cifs: don't fail writepages on -EAGAIN errors (try #2)

Mind reviewing that one instead? I think it fixes some cases that this
one doesn't...

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

Re: [PATCH 05/13] cifs: don't reconnect server when we don't get a response

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> We only want to force a reconnect to the server under very limited and
> specific circumstances. Now that we have processes waiting indefinitely
> for responses, we shouldn't reach this point unless a reconnect is
> already in process. Thus, there's no reason to re-mark the server for
> reconnect here.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 989674c..0c0dadd 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -367,10 +367,8 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>        if (mid->midState == MID_REQUEST_SUBMITTED) {
>                if (server->tcpStatus == CifsExiting)
>                        rc = -EHOSTDOWN;
> -               else {
> -                       server->tcpStatus = CifsNeedReconnect;
> +               else
>                        mid->midState = MID_RETRY_NEEDED;
> -               }
>        }
>
>        if (rc != -EHOSTDOWN) {
> --
> 1.7.3.2
>


Reviewed-by:  Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Pavel Shilovsky]

2010/12/13 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Mon, 13 Dec 2010 23:01:41 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
>> > temporary. CIFS should retry the write instead of setting an error on
>> > the mapping and returning.
>> >
>> > For WB_SYNC_ALL, just retry the write immediately.
>> >
>> > In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
>> > pages that didn't get written out and then move on.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/file.c |   23 +++++++++++------------
>> >  1 files changed, 11 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index fe16f6d..8e57370 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -1430,6 +1430,7 @@ retry:
>> >                                break;
>> >                }
>> >                if (n_iov) {
>> > +retry_write:
>> >                        open_file = find_writable_file(CIFS_I(mapping->host),
>> >                                                        false);
>> >                        if (!open_file) {
>> > @@ -1445,22 +1446,20 @@ retry:
>> >                                cifs_update_eof(cifsi, offset, bytes_written);
>> >                        }
>> >
>> > -                       if (rc || bytes_written < bytes_to_write) {
>> > -                               cERROR(1, "Write2 ret %d, wrote %d",
>> > -                                         rc, bytes_written);
>> > -                               mapping_set_error(mapping, rc);
>> > -                       } else {
>> > +                       /* retry on data-integrity flush */
>> > +                       if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
>> > +                               goto retry_write;
>> > +
>> > +                       if (!rc)
>> >                                cifs_stats_bytes_written(tcon, bytes_written);
>> > -                       }
>> > +                       else if (rc != -EAGAIN)
>> > +                               mapping_set_error(mapping, rc);
>> >
>> >                        for (i = 0; i < n_iov; i++) {
>> >                                page = pvec.pages[first + i];
>> > -                               /* Should we also set page error on
>> > -                               success rc but too little data written? */
>> > -                               /* BB investigate retry logic on temporary
>> > -                               server crash cases and how recovery works
>> > -                               when page marked as error */
>> > -                               if (rc)
>> > +                               if (rc == -EAGAIN)
>> > +                                       redirty_page_for_writepage(wbc, page);
>> > +                               else if (rc != 0)
>> >                                        SetPageError(page);
>> >                                kunmap(page);
>> >                                unlock_page(page);
>> > --
>> > 1.7.3.2
>> >
>>
>> Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> I actually sent a later version of this patch with the unfortunate
> subject of:
>
> [PATCH 00/13] cifs: don't fail writepages on -EAGAIN errors (try #2)
>
> Mind reviewing that one instead? I think it fixes some cases that this
> one doesn't...
>

Yes, I missed that one :) No problem - will look it new version as well.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 00/13] cifs: don't fail writepages on -EAGAIN errors (try #2)

[Pavel Shilovsky]

2010/12/11 Jeff Layton <jlayton@redhat.com>:
> If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
> temporary. CIFS should retry the write instead of setting an error on
> the mapping and returning.
>
> For WB_SYNC_ALL, just retry the write immediately. In the WB_SYNC_NONE
> case, call redirty_page_for_writeback on all of the pages that didn't
> get written out and then move on.
>
> Also, fix up the handling of a short write with a successful return
> code. MS-CIFS says that 0 bytes_written means ENOSPC or EFBIG. It
> doesn't mention what a short, but non-zero write means, so for now
> treat it as we would an -EAGAIN return.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/file.c |   48 ++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index fe16f6d..8bef611 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1430,6 +1430,7 @@ retry:
>                                break;
>                }
>                if (n_iov) {
> +retry_write:
>                        open_file = find_writable_file(CIFS_I(mapping->host),
>                                                        false);
>                        if (!open_file) {
> @@ -1442,31 +1443,54 @@ retry:
>                                                   &bytes_written, iov, n_iov,
>                                                   long_op);
>                                cifsFileInfo_put(open_file);
> -                               cifs_update_eof(cifsi, offset, bytes_written);
>                        }
>
> -                       if (rc || bytes_written < bytes_to_write) {
> -                               cERROR(1, "Write2 ret %d, wrote %d",
> -                                         rc, bytes_written);
> -                               mapping_set_error(mapping, rc);
> -                       } else {
> +                       cFYI(1, "Write2 rc=%d, wrote=%u", rc, bytes_written);
> +
> +                       /*
> +                        * For now, treat a short write as if nothing got
> +                        * written. A zero length write however indicates
> +                        * ENOSPC or EFBIG. We have no way to know which
> +                        * though, so call it ENOSPC for now. EFBIG would
> +                        * get translated to AS_EIO anyway.
> +                        *
> +                        * FIXME: make it take into account the data that did
> +                        *        get written
> +                        */
> +                       if (rc == 0) {
> +                               if (bytes_written == 0)
> +                                       rc = -ENOSPC;
> +                               else if (bytes_written < bytes_to_write)
> +                                       rc = -EAGAIN;
> +                       }
> +
> +                       /* retry on data-integrity flush */
> +                       if (wbc->sync_mode == WB_SYNC_ALL &&
> +                           (rc == -EAGAIN || bytes_written < bytes_to_write))
> +                               goto retry_write;
> +
> +                       /* fix the stats and EOF */
> +                       if (bytes_written > 0) {
>                                cifs_stats_bytes_written(tcon, bytes_written);
> +                               cifs_update_eof(cifsi, offset, bytes_written);
>                        }
>
>                        for (i = 0; i < n_iov; i++) {
>                                page = pvec.pages[first + i];
> -                               /* Should we also set page error on
> -                               success rc but too little data written? */
> -                               /* BB investigate retry logic on temporary
> -                               server crash cases and how recovery works
> -                               when page marked as error */
> -                               if (rc)
> +                               /* on retryable write error, redirty page */
> +                               if (rc == -EAGAIN)
> +                                       redirty_page_for_writepage(wbc, page);
> +                               else if (rc != 0)
>                                        SetPageError(page);
>                                kunmap(page);
>                                unlock_page(page);
>                                end_page_writeback(page);
>                                page_cache_release(page);
>                        }
> +
> +                       if (rc != -EAGAIN)
> +                               mapping_set_error(mapping, rc);
> +
>                        if ((wbc->nr_to_write -= n_iov) <= 0)
>                                done = 1;
>                        index = next;
> --
> 1.7.3.2


Looks good.

Reviewed-by:  Pavel Shilovsky <piastryyy@gmail.com>

I also think that not retrying on EAGAIN here can cause a error on
smb2 code (sometimes -one or two times for ten runs - I got file with
a size that was less than 400MB while I wrote exactly 400MB during my
test).

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 07/13] cifs: allow for different handling of received response

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> In order to incorporate async requests, we need to allow for a more
> general way to do things on receive, rather than just waking up a
> process.
>
> Turn the task pointer in the mid_q_entry into a callback function and a
> generic data pointer. When a response comes in, or the socket is
> reconnected, cifsd can call the callback function in order to wake up
> the process.
>
> The default is to just wake up the current process which should mean no
> change in behavior for existing code.
>
> Also, clean up the locking in cifs_reconnect. There doesn't seem to be
> any need to hold both the srv_mutex and GlobalMid_Lock when walking the
> list of mids.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifs_debug.c |    8 +++---
>  fs/cifs/cifsglob.h   |    7 +++++-
>  fs/cifs/connect.c    |   54 ++++++++++++++++++++++++++++---------------------
>  fs/cifs/transport.c  |   15 +++++++++++++-
>  4 files changed, 55 insertions(+), 29 deletions(-)
>
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index bd76527..c81934f 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -79,11 +79,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
>        spin_lock(&GlobalMid_Lock);
>        list_for_each(tmp, &server->pending_mid_q) {
>                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> -               cERROR(1, "State: %d Cmd: %d Pid: %d Tsk: %p Mid %d",
> +               cERROR(1, "State: %d Cmd: %d Pid: %d Cbdata: %p Mid %d",
>                        mid_entry->midState,
>                        (int)mid_entry->command,
>                        mid_entry->pid,
> -                       mid_entry->tsk,
> +                       mid_entry->callback_data,
>                        mid_entry->mid);
>  #ifdef CONFIG_CIFS_STATS2
>                cERROR(1, "IsLarge: %d buf: %p time rcv: %ld now: %ld",
> @@ -218,11 +218,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>                                mid_entry = list_entry(tmp3, struct mid_q_entry,
>                                        qhead);
>                                seq_printf(m, "\tState: %d com: %d pid:"
> -                                               " %d tsk: %p mid %d\n",
> +                                               " %d cbdata: %p mid %d\n",
>                                                mid_entry->midState,
>                                                (int)mid_entry->command,
>                                                mid_entry->pid,
> -                                               mid_entry->tsk,
> +                                               mid_entry->callback_data,
>                                                mid_entry->mid);
>                        }
>                        spin_unlock(&GlobalMid_Lock);
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index cf0dfda..cc43ada 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -511,6 +511,10 @@ static inline void cifs_stats_bytes_read(struct cifsTconInfo *tcon,
>
>  #endif
>
> +struct mid_q_entry;
> +
> +typedef void (mid_callback_t)(struct mid_q_entry *mid);
> +
>  /* one of these for every pending CIFS request to the server */
>  struct mid_q_entry {
>        struct list_head qhead; /* mids waiting on reply from this server */
> @@ -522,7 +526,8 @@ struct mid_q_entry {
>        unsigned long when_sent; /* time when smb send finished */
>        unsigned long when_received; /* when demux complete (taken off wire) */
>  #endif
> -       struct task_struct *tsk;        /* task waiting for response */
> +       mid_callback_t *callback; /* call completion callback */
> +       void *callback_data;      /* general purpose pointer for callback */
>        struct smb_hdr *resp_buf;       /* response buffer */
>        int midState;   /* wish this were enum but can not pass to wait_event */
>        __u8 command;   /* smb command code */
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9fbe7c5..7e20ece 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -133,6 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>  {
>        int rc = 0;
>        struct list_head *tmp, *tmp2;
> +       struct list_head retry;
>        struct cifsSesInfo *ses;
>        struct cifsTconInfo *tcon;
>        struct mid_q_entry *mid_entry;
> @@ -152,6 +153,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>
>        /* before reconnecting the tcp session, mark the smb session (uid)
>                and the tid bad so they are not used until reconnected */
> +       cFYI(1, "%s: marking sessions and tcons for reconnect", __func__);
>        spin_lock(&cifs_tcp_ses_lock);
>        list_for_each(tmp, &server->smb_ses_list) {
>                ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> @@ -163,7 +165,9 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                }
>        }
>        spin_unlock(&cifs_tcp_ses_lock);
> +
>        /* do not want to be sending data on a socket we are freeing */
> +       cFYI(1, "%s: tearing down socket", __func__);
>        mutex_lock(&server->srv_mutex);
>        if (server->ssocket) {
>                cFYI(1, "State: 0x%x Flags: 0x%lx", server->ssocket->state,
> @@ -180,22 +184,21 @@ cifs_reconnect(struct TCP_Server_Info *server)
>        kfree(server->session_key.response);
>        server->session_key.response = NULL;
>        server->session_key.len = 0;
> +       mutex_unlock(&server->srv_mutex);
>
> +       /*
> +        * move in-progress mids to a private list so that we can walk it later
> +        * without needing a lock. We'll mark them for retry after reconnect.
> +        */
> +       cFYI(1, "%s: moving mids to retry list", __func__);
> +       INIT_LIST_HEAD(&retry);
>        spin_lock(&GlobalMid_Lock);
> -       list_for_each(tmp, &server->pending_mid_q) {
> -               mid_entry = list_entry(tmp, struct
> -                                       mid_q_entry,
> -                                       qhead);
> -               if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> -                               /* Mark other intransit requests as needing
> -                                  retry so we do not immediately mark the
> -                                  session bad again (ie after we reconnect
> -                                  below) as they timeout too */
> -                       mid_entry->midState = MID_RETRY_NEEDED;
> -               }
> +       list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +               if (mid_entry->midState == MID_REQUEST_SUBMITTED)
> +                       list_move(tmp, &retry);
>        }
>        spin_unlock(&GlobalMid_Lock);
> -       mutex_unlock(&server->srv_mutex);
>
>        while ((server->tcpStatus != CifsExiting) &&
>               (server->tcpStatus != CifsGood)) {
> @@ -213,10 +216,17 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                        if (server->tcpStatus != CifsExiting)
>                                server->tcpStatus = CifsGood;
>                        spin_unlock(&GlobalMid_Lock);
> -       /*              atomic_set(&server->inFlight,0);*/
> -                       wake_up(&server->response_q);
>                }
>        }
> +
> +       /* now, issue callback for all mids in flight */
> +       list_for_each_safe(tmp, tmp2, &retry) {
> +               list_del_init(tmp);
> +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +               mid_entry->midState = MID_RETRY_NEEDED;
> +               mid_entry->callback(mid_entry);
> +       }
> +
>        return rc;
>  }
>
> @@ -560,8 +570,7 @@ incomplete_rcv:
>                        continue;
>                }
>
> -
> -               task_to_wake = NULL;
> +               mid_entry = NULL;
>                spin_lock(&GlobalMid_Lock);
>                list_for_each(tmp, &server->pending_mid_q) {
>                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> @@ -604,7 +613,6 @@ incomplete_rcv:
>                                mid_entry->resp_buf = smb_buffer;
>                                mid_entry->largeBuf = isLargeBuf;
>  multi_t2_fnd:
> -                               task_to_wake = mid_entry->tsk;
>                                mid_entry->midState = MID_RESPONSE_RECEIVED;
>  #ifdef CONFIG_CIFS_STATS2
>                                mid_entry->when_received = jiffies;
> @@ -615,9 +623,12 @@ multi_t2_fnd:
>                                server->lstrp = jiffies;
>                                break;
>                        }
> +                       mid_entry = NULL;
>                }
>                spin_unlock(&GlobalMid_Lock);
> -               if (task_to_wake) {
> +
> +               if (mid_entry != NULL) {
> +                       mid_entry->callback(mid_entry);
>                        /* Was previous buf put in mpx struct for multi-rsp? */
>                        if (!isMultiRsp) {
>                                /* smb buffer will be freed by user thread */
> @@ -626,7 +637,6 @@ multi_t2_fnd:
>                                else
>                                        smallbuf = NULL;
>                        }
> -                       wake_up_process(task_to_wake);
>                } else if (!is_valid_oplock_break(smb_buffer, server) &&
>                           !isMultiRsp) {
>                        cERROR(1, "No task to wake, unknown frame received! "
> @@ -707,11 +717,9 @@ multi_t2_fnd:
>                list_for_each(tmp, &server->pending_mid_q) {
>                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> -                               cFYI(1, "Clearing Mid 0x%x - waking up ",
> +                               cFYI(1, "Clearing Mid 0x%x - issuing callback",
>                                         mid_entry->mid);
> -                               task_to_wake = mid_entry->tsk;
> -                               if (task_to_wake)
> -                                       wake_up_process(task_to_wake);
> +                               mid_entry->callback(mid_entry);
>                        }
>                }
>                spin_unlock(&GlobalMid_Lock);
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 05ced17..79647db 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -36,6 +36,12 @@
>
>  extern mempool_t *cifs_mid_poolp;
>
> +static void
> +wake_up_task(struct mid_q_entry *mid)
> +{
> +       wake_up_process(mid->callback_data);
> +}
> +
>  static struct mid_q_entry *
>  AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>  {
> @@ -58,7 +64,13 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>        /*      do_gettimeofday(&temp->when_sent);*/ /* easier to use jiffies */
>                /* when mid allocated can be before when sent */
>                temp->when_alloc = jiffies;
> -               temp->tsk = current;
> +
> +               /*
> +                * The default is for the mid to be synchronous, so the
> +                * default callback just wakes up the current task.
> +                */
> +               temp->callback = wake_up_task;
> +               temp->callback_data = current;
>        }
>
>        spin_lock(&GlobalMid_Lock);
> @@ -374,6 +386,7 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>                }
>                mid->midState = MID_RETRY_NEEDED;
>        case MID_RETRY_NEEDED:
> +               list_move(&mid->qhead, &server->pending_mid_q);
>                rc = -EAGAIN;
>                break;
>        default:
> --
> 1.7.3.2
>

Very good idea to add default synchronous callback - it simplifies the
code greatly. Looks ok to me.

Reviewed-by:  Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 09/13] cifs: add cifs_call_async

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Add a function that will send a request, and set up the mid for an
> async reply.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsproto.h |    5 ++++
>  fs/cifs/transport.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index a8fc606..6131c1b 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -61,7 +61,12 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
>                const char *fullpath, const struct dfs_info3_param *ref,
>                char **devname);
>  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> +extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
> +                                       struct TCP_Server_Info *server);
>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> +extern int cifs_call_async(struct TCP_Server_Info *server,
> +                          struct smb_hdr *in_buf, mid_callback_t *callback,
> +                          void *cbdata);
>  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
>                        struct smb_hdr * /* input */ ,
>                        struct smb_hdr * /* out */ ,
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 97a1170..2a0b14d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -42,7 +42,7 @@ wake_up_task(struct mid_q_entry *mid)
>        wake_up_process(mid->callback_data);
>  }
>
> -static struct mid_q_entry *
> +struct mid_q_entry *
>  AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>  {
>        struct mid_q_entry *temp;
> @@ -338,6 +338,57 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>
>
>  /*
> + * Send a SMB request and set the callback function in the mid to handle
> + * the result. Caller is responsible for dealing with timeouts.
> + */
> +int
> +cifs_call_async(struct TCP_Server_Info *server, struct smb_hdr *in_buf,
> +               mid_callback_t *callback, void *cbdata)
> +{
> +       int rc;
> +       struct mid_q_entry *mid;
> +
> +       rc = wait_for_free_request(server, CIFS_ASYNC_OP);
> +       if (rc)
> +               return rc;
> +
> +       mutex_lock(&server->srv_mutex);
> +       mid = AllocMidQEntry(in_buf, server);
> +       if (mid == NULL) {
> +               mutex_unlock(&server->srv_mutex);
> +               return -ENOMEM;
> +       }
> +
> +       rc = cifs_sign_smb(in_buf, server, &mid->sequence_number);
> +       if (rc) {
> +               mutex_unlock(&server->srv_mutex);
> +               goto out_err;
> +       }
> +
> +       mid->callback = callback;
> +       mid->callback_data = cbdata;
> +       mid->midState = MID_REQUEST_SUBMITTED;
> +#ifdef CONFIG_CIFS_STATS2
> +       atomic_inc(&server->inSend);
> +#endif
> +       rc = smb_send(server, in_buf, in_buf->smb_buf_length);
> +#ifdef CONFIG_CIFS_STATS2
> +       atomic_dec(&server->inSend);
> +       mid->when_sent = jiffies;
> +#endif
> +       mutex_unlock(&server->srv_mutex);
> +       if (rc)
> +               goto out_err;
> +
> +       return rc;
> +out_err:
> +       DeleteMidQEntry(mid);
> +       atomic_dec(&server->inFlight);
> +       wake_up(&server->request_q);
> +       return rc;
> +}
> +
> +/*
>  *
>  * Send an SMB Request.  No response info (other than return code)
>  * needs to be parsed.
> --
> 1.7.3.2
>


Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 11/13] cifs: set up recurring workqueue job to do SMB echo requests

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/connect.c  |   29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index fb75f04..d924f3c 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -221,6 +221,7 @@ struct TCP_Server_Info {
>        bool    sec_kerberosu2u;        /* supports U2U Kerberos */
>        bool    sec_ntlmssp;            /* supports NTLMSSP */
>        bool session_estab; /* mark when very first sess is established */
> +       struct delayed_work     echo; /* echo ping workqueue job */
>  #ifdef CONFIG_CIFS_FSCACHE
>        struct fscache_cookie   *fscache; /* client index cache cookie */
>  #endif
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 0feb592..98e1d38 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -52,6 +52,9 @@
>  #define CIFS_PORT 445
>  #define RFC1001_PORT 139
>
> +/* SMB echo "timeout" -- FIXME: tunable? */
> +#define SMB_ECHO_INTERVAL (30 * HZ)
> +
>  extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
>                         unsigned char *p24);
>
> @@ -354,6 +357,26 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
>
>  }
>
> +static void
> +cifs_echo_request(struct work_struct *work)
> +{
> +       int rc;
> +       struct TCP_Server_Info *server = container_of(work,
> +                                       struct TCP_Server_Info, echo.work);
> +
> +       /* no need to ping if we got a response recently */
> +       if (time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
> +               goto requeue_echo;
> +
> +       rc = CIFSSMBEcho(server);
> +       if (rc)
> +               cFYI(1, "Unable to send echo request to server: %s",
> +                       server->hostname);
> +
> +requeue_echo:
> +       queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
> +}
> +
>  static int
>  cifs_demultiplex_thread(struct TCP_Server_Info *server)
>  {
> @@ -1610,6 +1633,8 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
>        list_del_init(&server->tcp_ses_list);
>        spin_unlock(&cifs_tcp_ses_lock);
>
> +       cancel_delayed_work_sync(&server->echo);
> +
>        spin_lock(&GlobalMid_Lock);
>        server->tcpStatus = CifsExiting;
>        spin_unlock(&GlobalMid_Lock);
> @@ -1701,6 +1726,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>        tcp_ses->sequence_number = 0;
>        INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
>        INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
> +       INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
>
>        /*
>         * at this point we are the only ones with the pointer
> @@ -1750,6 +1776,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>
>        cifs_fscache_get_client_cookie(tcp_ses);
>
> +       /* queue echo request delayed work */
> +       queue_delayed_work(system_nrt_wq, &tcp_ses->echo, SMB_ECHO_INTERVAL);
> +
>        return tcp_ses;
>
>  out_err_crypto_release:
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 12/13] cifs: reconnect unresponsive servers

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> If the server isn't responding to echoes, we don't want to leave tasks
> hung waiting for it to reply. At that point, we'll want to reconnect
> so that soft mounts can return an error to userspace quickly.
>
> If the client hasn't received a reply after 3 echo intervals, assume
> that the transport is down and attempt to reconnect the socket.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 98e1d38..c596515 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -55,6 +55,9 @@
>  /* SMB echo "timeout" -- FIXME: tunable? */
>  #define SMB_ECHO_INTERVAL (30 * HZ)
>
> +/* reconnect if no response from server in this time period */
> +#define UNRESPONSIVE_SERVER_TIMEOUT (3 * SMB_ECHO_INTERVAL)
> +
>  extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
>                         unsigned char *p24);
>
> @@ -187,6 +190,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>        kfree(server->session_key.response);
>        server->session_key.response = NULL;
>        server->session_key.len = 0;
> +       server->lstrp = jiffies;
>        mutex_unlock(&server->srv_mutex);
>
>        /*
> @@ -442,7 +446,19 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>                smb_msg.msg_control = NULL;
>                smb_msg.msg_controllen = 0;
>                pdu_length = 4; /* enough to get RFC1001 header */
> +
>  incomplete_rcv:
> +               if (time_after(jiffies,
> +                              server->lstrp + UNRESPONSIVE_SERVER_TIMEOUT)) {
> +                       cERROR(1, "Server %s has not responded in %d seconds. "
> +                                 "Reconnecting...", server->hostname,
> +                                 UNRESPONSIVE_SERVER_TIMEOUT / HZ);
> +                       cifs_reconnect(server);
> +                       csocket = server->ssocket;
> +                       wake_up(&server->response_q);
> +                       continue;
> +               }
> +
>                length =
>                    kernel_recvmsg(csocket, &smb_msg,
>                                &iov, 1, pdu_length, 0 /* BB other flags? */);
> @@ -603,6 +619,8 @@ incomplete_rcv:
>                }
>
>                mid_entry = NULL;
> +               server->lstrp = jiffies;
> +
>                spin_lock(&GlobalMid_Lock);
>                list_for_each(tmp, &server->pending_mid_q) {
>                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> @@ -653,10 +671,6 @@ multi_t2_fnd:
>  #ifdef CONFIG_CIFS_STATS2
>                                mid_entry->when_received = jiffies;
>  #endif
> -                               /* so we do not time out requests to  server
> -                               which is still responding (since server could
> -                               be busy but not dead) */
> -                               server->lstrp = jiffies;
>                                break;
>                        }
>  next_mid:
> @@ -1724,6 +1738,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>                volume_info->target_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL);
>        tcp_ses->session_estab = false;
>        tcp_ses->sequence_number = 0;
> +       tcp_ses->lstrp = jiffies;
>        INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
>        INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
>        INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
> --
> 1.7.3.2

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 10/13] cifs: add ability to send an echo request

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifspdu.h   |   15 +++++++++++++++
>  fs/cifs/cifsproto.h |    1 +
>  fs/cifs/cifssmb.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
> index de36b09..ea205b4 100644
> --- a/fs/cifs/cifspdu.h
> +++ b/fs/cifs/cifspdu.h
> @@ -50,6 +50,7 @@
>  #define SMB_COM_SETATTR               0x09 /* trivial response */
>  #define SMB_COM_LOCKING_ANDX          0x24 /* trivial response */
>  #define SMB_COM_COPY                  0x29 /* trivial rsp, fail filename ignrd*/
> +#define SMB_COM_ECHO                  0x2B /* echo request */
>  #define SMB_COM_OPEN_ANDX             0x2D /* Legacy open for old servers */
>  #define SMB_COM_READ_ANDX             0x2E
>  #define SMB_COM_WRITE_ANDX            0x2F
> @@ -760,6 +761,20 @@ typedef struct smb_com_tconx_rsp_ext {
>  *
>  */
>
> +typedef struct smb_com_echo_req {
> +       struct  smb_hdr hdr;
> +       __le16  EchoCount;
> +       __le16  ByteCount;
> +       char    Data[1];
> +} __attribute__((packed)) ECHO_REQ;
> +
> +typedef struct smb_com_echo_rsp {
> +       struct  smb_hdr hdr;
> +       __le16  SequenceNumber;
> +       __le16  ByteCount;
> +       char    Data[1];
> +} __attribute__((packed)) ECHO_RSP;
> +
>  typedef struct smb_com_logoff_andx_req {
>        struct smb_hdr hdr;     /* wct = 2 */
>        __u8 AndXCommand;
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 6131c1b..3cd6474 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -353,6 +353,7 @@ extern int CIFSSMBPosixLock(const int xid, struct cifsTconInfo *tcon,
>                        const __u64 len, struct file_lock *,
>                        const __u16 lock_type, const bool waitFlag);
>  extern int CIFSSMBTDis(const int xid, struct cifsTconInfo *tcon);
> +extern int CIFSSMBEcho(struct TCP_Server_Info *server);
>  extern int CIFSSMBLogoff(const int xid, struct cifsSesInfo *ses);
>
>  extern struct cifsSesInfo *sesInfoAlloc(void);
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 9af98f6..fc8145f 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -706,6 +706,53 @@ CIFSSMBTDis(const int xid, struct cifsTconInfo *tcon)
>        return rc;
>  }
>
> +/*
> + * This is a no-op for now. We're not really interested in the reply, but
> + * rather in the fact that the server sent one and that server->lstrp
> + * gets updated.
> + *
> + * FIXME: maybe we should consider checking that the reply matches request?
> + */
> +static void
> +cifs_echo_callback(struct mid_q_entry *mid)
> +{
> +       struct TCP_Server_Info *server = mid->callback_data;
> +
> +       DeleteMidQEntry(mid);
> +       atomic_dec(&server->inFlight);
> +       wake_up(&server->request_q);
> +}
> +
> +int
> +CIFSSMBEcho(struct TCP_Server_Info *server)
> +{
> +       ECHO_REQ *smb;
> +       int rc = 0;
> +
> +       cFYI(1, "In echo request");
> +
> +       rc = small_smb_init(SMB_COM_ECHO, 0, NULL, (void **)&smb);
> +       if (rc)
> +               return rc;
> +
> +       /* set up echo request */
> +       smb->hdr.Tid = cpu_to_le16(0xffff);
> +       smb->hdr.WordCount = cpu_to_le16(1);
> +       smb->EchoCount = cpu_to_le16(1);
> +       smb->ByteCount = cpu_to_le16(1);
> +       smb->Data[0] = 'a';
> +       smb->hdr.smb_buf_length += 3;
> +
> +       rc = cifs_call_async(server, (struct smb_hdr *)smb,
> +                               cifs_echo_callback, server);
> +       if (rc)
> +               cFYI(1, "Echo request failed: %d", rc);
> +
> +       cifs_small_buf_release(smb);
> +
> +       return rc;
> +}
> +
>  int
>  CIFSSMBLogoff(const int xid, struct cifsSesInfo *ses)
>  {
> --
> 1.7.3.2
>

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Currently, when a request is cancelled via signal, we delete the mid
> immediately. If the request was already transmitted however, the client
> is still likely to receive a response. When it does, it won't recognize
> it however and will pop a printk.
>
> It's also a little dangerous to just delete the mid entry like this. We
> may end up reusing that mid. If we do then we could potentially get the
> response from the first request confused with the later one.
>
> Prevent the reuse of mids by marking them as cancelled and keeping them
> on the pending_mid_q list. If the reply comes in, we'll delete it from
> the list then. If it never comes, then we'll delete it at reconnect
> or when cifsd comes down.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |    2 +-
>  fs/cifs/cifsproto.h |    1 +
>  fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
>  fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
>  4 files changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index cc43ada..fb75f04 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>  #define   MID_REQUEST_SUBMITTED 2
>  #define   MID_RESPONSE_RECEIVED 4
>  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
> -#define   MID_NO_RESP_NEEDED 0x10
> +#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
>
>  /* Types of response buffer returned from SendReceive2 */
>  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index fe77e69..a8fc606 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
>                const char *fullpath, const struct dfs_info3_param *ref,
>                char **devname);
>  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
>                        struct smb_hdr * /* input */ ,
>                        struct smb_hdr * /* out */ ,
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 7e20ece..0feb592 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>  {
>        int rc = 0;
>        struct list_head *tmp, *tmp2;
> -       struct list_head retry;
> +       struct list_head retry, dispose;
>        struct cifsSesInfo *ses;
>        struct cifsTconInfo *tcon;
>        struct mid_q_entry *mid_entry;
> @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         */
>        cFYI(1, "%s: moving mids to retry list", __func__);
>        INIT_LIST_HEAD(&retry);
> +       INIT_LIST_HEAD(&dispose);
>        spin_lock(&GlobalMid_Lock);
>        list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>                if (mid_entry->midState == MID_REQUEST_SUBMITTED)
>                        list_move(tmp, &retry);
> +               else if (mid_entry->midState == MID_REQUEST_CANCELLED)
> +                       list_move(tmp, &dispose);
>        }
>        spin_unlock(&GlobalMid_Lock);
>
> +       /* now walk private dispose list and delete entries */
> +       list_for_each_safe(tmp, tmp2, &dispose) {
> +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +               DeleteMidQEntry(mid_entry);
> +       }
> +
>        while ((server->tcpStatus != CifsExiting) &&
>               (server->tcpStatus != CifsGood)) {
>                try_to_freeze();
> @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                }
>        }
>
> -       /* now, issue callback for all mids in flight */
> +       /* issue callback for all mids in flight */
>        list_for_each_safe(tmp, tmp2, &retry) {
>                list_del_init(tmp);
>                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> @@ -575,9 +584,13 @@ incomplete_rcv:
>                list_for_each(tmp, &server->pending_mid_q) {
>                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>
> -                       if ((mid_entry->mid == smb_buffer->Mid) &&
> -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
> -                           (mid_entry->command == smb_buffer->Command)) {
> +                       if (mid_entry->mid != smb_buffer->Mid)
> +                               goto next_mid;
> +                       if (mid_entry->command != smb_buffer->Command)
> +                               goto next_mid;
> +                       if (mid_entry->midState == MID_REQUEST_CANCELLED)
> +                               break;
> +                       if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>                                if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
>                                        /* We have a multipart transact2 resp */
>                                        isMultiRsp = true;
> @@ -623,11 +636,16 @@ multi_t2_fnd:
>                                server->lstrp = jiffies;
>                                break;
>                        }
> +next_mid:
>                        mid_entry = NULL;
>                }
>                spin_unlock(&GlobalMid_Lock);
>
>                if (mid_entry != NULL) {
> +                       if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> +                               DeleteMidQEntry(mid_entry);
> +                               continue;
> +                       }
>                        mid_entry->callback(mid_entry);
>                        /* Was previous buf put in mpx struct for multi-rsp? */
>                        if (!isMultiRsp) {
> @@ -704,6 +722,9 @@ multi_t2_fnd:
>                }
>                spin_unlock(&cifs_tcp_ses_lock);
>        } else {
> +               struct mid_q_entry *tmp_mid;
> +               struct list_head dispose;
> +
>                /* although we can not zero the server struct pointer yet,
>                since there are active requests which may depnd on them,
>                mark the corresponding SMB sessions as exiting too */
> @@ -713,17 +734,26 @@ multi_t2_fnd:
>                        ses->status = CifsExiting;
>                }
>
> +               INIT_LIST_HEAD(&dispose);
>                spin_lock(&GlobalMid_Lock);
> -               list_for_each(tmp, &server->pending_mid_q) {
> -               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +               list_for_each_entry_safe(mid_entry, tmp_mid,
> +                                        &server->pending_mid_q, qhead) {
>                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>                                cFYI(1, "Clearing Mid 0x%x - issuing callback",
>                                         mid_entry->mid);
>                                mid_entry->callback(mid_entry);
> +                       } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> +                               cFYI(1, "Clearing Mid 0x%x - Cancelled",
> +                                       mid_entry->mid);
> +                               list_move(&mid_entry->qhead, &dispose);

Why do we need another list here? It seems to me that we can simply
delete cancelled mid.

>                        }
>                }
>                spin_unlock(&GlobalMid_Lock);
>                spin_unlock(&cifs_tcp_ses_lock);
> +
> +               /* now delete all of the cancelled mids */
> +               list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
> +                       DeleteMidQEntry(mid_entry);
>                /* 1/8th of sec is more than enough time for them to exit */
>                msleep(125);
>        }
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 79647db..97a1170 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>        return temp;
>  }
>
> -static void
> +void
>  DeleteMidQEntry(struct mid_q_entry *midEntry)
>  {
>  #ifdef CONFIG_CIFS_STATS2
> @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>                goto out;
>
>        rc = wait_for_response(ses->server, midQ);
> -       if (rc != 0)
> -               goto out;
> +       if (rc != 0) {
> +               /* no longer considered to be "in-flight" */
> +               midQ->midState = MID_REQUEST_CANCELLED;
> +               atomic_dec(&ses->server->inFlight);
> +               wake_up(&ses->server->request_q);
> +               return rc;
> +       }
>
>        rc = handle_mid_result(midQ, ses->server);
>        if (rc != 0)
> @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>                goto out;
>
>        rc = wait_for_response(ses->server, midQ);
> -       if (rc != 0)
> -               goto out;
> +       if (rc != 0) {
> +               /* no longer considered to be "in-flight" */
> +               midQ->midState = MID_REQUEST_CANCELLED;
> +               atomic_dec(&ses->server->inFlight);
> +               wake_up(&ses->server->request_q);
> +               return rc;
> +       }
>
>        rc = handle_mid_result(midQ, ses->server);
>        if (rc != 0)
> @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>                        }
>                }
>
> -               if (wait_for_response(ses->server, midQ) == 0) {
> -                       /* We got the response - restart system call. */
> -                       rstart = 1;
> +               rc = wait_for_response(ses->server, midQ);
> +               if (rc) {
> +                       midQ->midState = MID_REQUEST_CANCELLED;
> +                       return rc;
>                }
> +
> +               /* We got the response - restart system call. */
> +               rstart = 1;
>        }
>
>        rc = handle_mid_result(midQ, ses->server);
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 13/13] cifs: remove code for setting timeouts on requests

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Since we don't time out individual requests anymore, remove the code
> that we used to use for setting timeouts on different requests.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |    9 +++------
>  fs/cifs/cifssmb.c   |    8 ++++----
>  fs/cifs/connect.c   |    2 +-
>  fs/cifs/file.c      |   44 +++++++-------------------------------------
>  fs/cifs/sess.c      |    2 +-
>  fs/cifs/transport.c |    2 +-
>  6 files changed, 17 insertions(+), 50 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index d924f3c..1e290fa 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -631,12 +631,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>  #define   CIFS_IOVEC            4    /* array of response buffers */
>
>  /* Type of Request to SendReceive2 */
> -#define   CIFS_STD_OP          0    /* normal request timeout */
> -#define   CIFS_LONG_OP          1    /* long op (up to 45 sec, oplock time) */
> -#define   CIFS_VLONG_OP         2    /* sloow op - can take up to 180 seconds */
> -#define   CIFS_BLOCKING_OP      4    /* operation can block */
> -#define   CIFS_ASYNC_OP         8    /* do not wait for response */
> -#define   CIFS_TIMEOUT_MASK 0x00F    /* only one of 5 above set in req */
> +#define   CIFS_BLOCKING_OP      1    /* operation can block */
> +#define   CIFS_ASYNC_OP         2    /* do not wait for response */
> +#define   CIFS_TIMEOUT_MASK 0x003    /* only one of above set in req */
>  #define   CIFS_LOG_ERROR    0x010    /* log NT STATUS if non-zero */
>  #define   CIFS_LARGE_BUF_OP 0x020    /* large request buffer */
>  #define   CIFS_NO_RESP      0x040    /* no response buffer required */
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index fc8145f..606deba 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1240,7 +1240,7 @@ OldOpenRetry:
>        pSMB->ByteCount = cpu_to_le16(count);
>        /* long_op set to 1 to allow for oplock break timeouts */
>        rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> -                       (struct smb_hdr *)pSMBr, &bytes_returned, CIFS_LONG_OP);
> +                       (struct smb_hdr *)pSMBr, &bytes_returned, 0);
>        cifs_stats_inc(&tcon->num_opens);
>        if (rc) {
>                cFYI(1, "Error in Open = %d", rc);
> @@ -1353,7 +1353,7 @@ openRetry:
>        pSMB->ByteCount = cpu_to_le16(count);
>        /* long_op set to 1 to allow for oplock break timeouts */
>        rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> -                       (struct smb_hdr *)pSMBr, &bytes_returned, CIFS_LONG_OP);
> +                       (struct smb_hdr *)pSMBr, &bytes_returned, 0);
>        cifs_stats_inc(&tcon->num_opens);
>        if (rc) {
>                cFYI(1, "Error in Open = %d", rc);
> @@ -1435,7 +1435,7 @@ CIFSSMBRead(const int xid, struct cifsTconInfo *tcon, const int netfid,
>        iov[0].iov_base = (char *)pSMB;
>        iov[0].iov_len = pSMB->hdr.smb_buf_length + 4;
>        rc = SendReceive2(xid, tcon->ses, iov, 1 /* num iovecs */,
> -                        &resp_buf_type, CIFS_STD_OP | CIFS_LOG_ERROR);
> +                        &resp_buf_type, CIFS_LOG_ERROR);
>        cifs_stats_inc(&tcon->num_reads);
>        pSMBr = (READ_RSP *)iov[0].iov_base;
>        if (rc) {
> @@ -3030,7 +3030,7 @@ CIFSSMBGetCIFSACL(const int xid, struct cifsTconInfo *tcon, __u16 fid,
>        iov[0].iov_len = pSMB->hdr.smb_buf_length + 4;
>
>        rc = SendReceive2(xid, tcon->ses, iov, 1 /* num iovec */, &buf_type,
> -                        CIFS_STD_OP);
> +                        0);
>        cifs_stats_inc(&tcon->num_acl_get);
>        if (rc) {
>                cFYI(1, "Send error in QuerySecDesc = %d", rc);
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index c596515..b8fb112 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3134,7 +3134,7 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses,
>        pSMB->ByteCount = cpu_to_le16(count);
>
>        rc = SendReceive(xid, ses, smb_buffer, smb_buffer_response, &length,
> -                        CIFS_STD_OP);
> +                        0);
>
>        /* above now done in SendReceive */
>        if ((rc == 0) && (tcon != NULL)) {
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 8e57370..5c7b4f0 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -892,29 +892,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
>        return rc;
>  }
>
> -/*
> - * Set the timeout on write requests past EOF. For some servers (Windows)
> - * these calls can be very long.
> - *
> - * If we're writing >10M past the EOF we give a 180s timeout. Anything less
> - * than that gets a 45s timeout. Writes not past EOF get 15s timeouts.
> - * The 10M cutoff is totally arbitrary. A better scheme for this would be
> - * welcome if someone wants to suggest one.
> - *
> - * We may be able to do a better job with this if there were some way to
> - * declare that a file should be sparse.
> - */
> -static int
> -cifs_write_timeout(struct cifsInodeInfo *cifsi, loff_t offset)
> -{
> -       if (offset <= cifsi->server_eof)
> -               return CIFS_STD_OP;
> -       else if (offset > (cifsi->server_eof + (10 * 1024 * 1024)))
> -               return CIFS_VLONG_OP;
> -       else
> -               return CIFS_LONG_OP;
> -}
> -
>  /* update the file size (if needed) after a write */
>  static void
>  cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
> @@ -935,7 +912,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>        unsigned int total_written;
>        struct cifs_sb_info *cifs_sb;
>        struct cifsTconInfo *pTcon;
> -       int xid, long_op;
> +       int xid;
>        struct cifsFileInfo *open_file;
>        struct cifsInodeInfo *cifsi = CIFS_I(inode);
>
> @@ -956,7 +933,6 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>
>        xid = GetXid();
>
> -       long_op = cifs_write_timeout(cifsi, *poffset);
>        for (total_written = 0; write_size > total_written;
>             total_written += bytes_written) {
>                rc = -EAGAIN;
> @@ -984,7 +960,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>                                min_t(const int, cifs_sb->wsize,
>                                      write_size - total_written),
>                                *poffset, &bytes_written,
> -                               NULL, write_data + total_written, long_op);
> +                               NULL, write_data + total_written, 0);
>                }
>                if (rc || (bytes_written == 0)) {
>                        if (total_written)
> @@ -997,8 +973,6 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
>                        cifs_update_eof(cifsi, *poffset, bytes_written);
>                        *poffset += bytes_written;
>                }
> -               long_op = CIFS_STD_OP; /* subsequent writes fast -
> -                                   15 seconds is plenty */
>        }
>
>        cifs_stats_bytes_written(pTcon, total_written);
> @@ -1027,7 +1001,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
>        unsigned int total_written;
>        struct cifs_sb_info *cifs_sb;
>        struct cifsTconInfo *pTcon;
> -       int xid, long_op;
> +       int xid;
>        struct dentry *dentry = open_file->dentry;
>        struct cifsInodeInfo *cifsi = CIFS_I(dentry->d_inode);
>
> @@ -1040,7 +1014,6 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
>
>        xid = GetXid();
>
> -       long_op = cifs_write_timeout(cifsi, *poffset);
>        for (total_written = 0; write_size > total_written;
>             total_written += bytes_written) {
>                rc = -EAGAIN;
> @@ -1070,7 +1043,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
>                                rc = CIFSSMBWrite2(xid, pTcon,
>                                                open_file->netfid, len,
>                                                *poffset, &bytes_written,
> -                                               iov, 1, long_op);
> +                                               iov, 1, 0);
>                        } else
>                                rc = CIFSSMBWrite(xid, pTcon,
>                                         open_file->netfid,
> @@ -1078,7 +1051,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
>                                               write_size - total_written),
>                                         *poffset, &bytes_written,
>                                         write_data + total_written,
> -                                        NULL, long_op);
> +                                        NULL, 0);
>                }
>                if (rc || (bytes_written == 0)) {
>                        if (total_written)
> @@ -1091,8 +1064,6 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
>                        cifs_update_eof(cifsi, *poffset, bytes_written);
>                        *poffset += bytes_written;
>                }
> -               long_op = CIFS_STD_OP; /* subsequent writes fast -
> -                                   15 seconds is plenty */
>        }
>
>        cifs_stats_bytes_written(pTcon, total_written);
> @@ -1292,7 +1263,7 @@ static int cifs_writepages(struct address_space *mapping,
>        struct pagevec pvec;
>        int rc = 0;
>        int scanned = 0;
> -       int xid, long_op;
> +       int xid;
>
>        cifs_sb = CIFS_SB(mapping->host->i_sb);
>
> @@ -1437,11 +1408,10 @@ retry_write:
>                                cERROR(1, "No writable handles for inode");
>                                rc = -EBADF;
>                        } else {
> -                               long_op = cifs_write_timeout(cifsi, offset);
>                                rc = CIFSSMBWrite2(xid, tcon, open_file->netfid,
>                                                   bytes_to_write, offset,
>                                                   &bytes_written, iov, n_iov,
> -                                                  long_op);
> +                                                  0);
>                                cifsFileInfo_put(open_file);
>                                cifs_update_eof(cifsi, offset, bytes_written);
>                        }
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 2997533..50b74a5 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -881,7 +881,7 @@ ssetup_ntlmssp_authenticate:
>        BCC_LE(smb_buf) = cpu_to_le16(count);
>
>        rc = SendReceive2(xid, ses, iov, 3 /* num_iovecs */, &resp_buf_type,
> -                         CIFS_STD_OP /* not long */ | CIFS_LOG_ERROR);
> +                         CIFS_LOG_ERROR);
>        /* SMB request buf freed in SendReceive2 */
>
>        pSMB = (SESSION_SETUP_ANDX *)iov[0].iov_base;
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 2a0b14d..70873ab 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -791,7 +791,7 @@ send_lock_cancel(const unsigned int xid, struct cifsTconInfo *tcon,
>        pSMB->hdr.Mid = GetNextMid(ses->server);
>
>        return SendReceive(xid, ses, in_buf, out_buf,
> -                       &bytes_returned, CIFS_STD_OP);
> +                       &bytes_returned, 0);
>  }
>
>  int
> --
> 1.7.3.2
>

Good. Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 06/13] cifs: clean up handle_mid_response

[Pavel Shilovsky]

2010/12/13 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Sun, 12 Dec 2010 17:52:26 -0600
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Fri, Dec 10, 2010 at 9:44 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > Make it use a switch statement based on the value of the midStatus. If
>> > the resp_buf is set, then MID_RESPONSE_RECEIVED is too.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/transport.c |   36 ++++++++++++++++++------------------
>> >  1 files changed, 18 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> > index 0c0dadd..05ced17 100644
>> > --- a/fs/cifs/transport.c
>> > +++ b/fs/cifs/transport.c
>> > @@ -356,33 +356,33 @@ handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>> >  {
>> >        int rc = 0;
>> >
>> > -       spin_lock(&GlobalMid_Lock);
>> > +       cFYI(1, "%s: cmd=%d mid=%d state=%d", __func__, mid->command,
>> > +               mid->mid, mid->midState);
>> >
>> > -       if (mid->resp_buf) {
>> > +       spin_lock(&GlobalMid_Lock);
>> > +       switch (mid->midState) {
>> > +       case MID_RESPONSE_RECEIVED:
>> >                spin_unlock(&GlobalMid_Lock);
>> >                return rc;
>> > -       }
>> > -
>> > -       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
>> > -       if (mid->midState == MID_REQUEST_SUBMITTED) {
>> > -               if (server->tcpStatus == CifsExiting)
>> > +       case MID_REQUEST_SUBMITTED:
>> > +               /* socket is going down, reject all calls */
>> > +               if (server->tcpStatus == CifsExiting) {
>> > +                       cERROR(1, "%s: canceling mid=%d cmd=0x%x state=%d",
>> > +                              __func__, mid->mid, mid->command, mid->midState);
>> >                        rc = -EHOSTDOWN;
>> > -               else
>> > -                       mid->midState = MID_RETRY_NEEDED;
>> > -       }
>> > -
>> > -       if (rc != -EHOSTDOWN) {
>> > -               if (mid->midState == MID_RETRY_NEEDED) {
>> > -                       rc = -EAGAIN;
>> > -                       cFYI(1, "marking request for retry");
>> > -               } else {
>> > -                       rc = -EIO;
>> > +                       break;
>> >                }
>> > +               mid->midState = MID_RETRY_NEEDED;
>>
>> Would it be cleaner to set rc to -EAGAIN instead of setting state here
>> to fall down and then setting rc as -EAGAIN?
>>
>
> I'm not sure it'll be any more clear. It seems clear enough to me -- if
> it's still SUBMITTED, then we want to treat it if it were
> "RETRY_NEEDED".
>
> Honestly, this really shouldn't happen. With this patchset, you should
> never reach this function if the state is still 'SUBMITTED'. I left it
> there for completeness sake and future-proofness.
>
>> > +       case MID_RETRY_NEEDED:
>> > +               rc = -EAGAIN;
>> > +               break;
>> > +       default:
>> > +               cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
>> > +                       mid->mid, mid->midState);
>> >        }
>> >        spin_unlock(&GlobalMid_Lock);
>> >
>> >        DeleteMidQEntry(mid);
>> > -       /* Update # of requests on wire to server */
>> >        atomic_dec(&server->inFlight);
>> >        wake_up(&server->request_q);
>> >
>> > --
>> > 1.7.3.2
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 03/13] cifs: move mid result processing into common function

[Pavel Shilovsky]

2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |  131 ++++++++++++++++++--------------------------------
>  1 files changed, 47 insertions(+), 84 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 9763f89..2d21bbd 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -382,6 +382,46 @@ SendReceiveNoRsp(const unsigned int xid, struct cifsSesInfo *ses,
>        return rc;
>  }
>
> +static int
> +handle_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
> +{
> +       int rc = 0;
> +
> +       spin_lock(&GlobalMid_Lock);
> +
> +       if (mid->resp_buf) {
> +               spin_unlock(&GlobalMid_Lock);
> +               return rc;
> +       }
> +
> +       cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
> +       if (mid->midState == MID_REQUEST_SUBMITTED) {
> +               if (server->tcpStatus == CifsExiting)
> +                       rc = -EHOSTDOWN;
> +               else {
> +                       server->tcpStatus = CifsNeedReconnect;
> +                       mid->midState = MID_RETRY_NEEDED;
> +               }
> +       }
> +
> +       if (rc != -EHOSTDOWN) {
> +               if (mid->midState == MID_RETRY_NEEDED) {
> +                       rc = -EAGAIN;
> +                       cFYI(1, "marking request for retry");
> +               } else {
> +                       rc = -EIO;
> +               }
> +       }
> +       spin_unlock(&GlobalMid_Lock);
> +
> +       DeleteMidQEntry(mid);
> +       /* Update # of requests on wire to server */
> +       atomic_dec(&server->inFlight);
> +       wake_up(&server->request_q);
> +
> +       return rc;
> +}
> +
>  int
>  SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>             struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
> @@ -485,37 +525,10 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>        /* No user interrupts in wait - wreaks havoc with performance */
>        wait_for_response(ses, midQ, timeout, 10 * HZ);
>
> -       spin_lock(&GlobalMid_Lock);
> -
> -       if (midQ->resp_buf == NULL) {
> -               cERROR(1, "No response to cmd %d mid %d",
> -                       midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> -               /* Update # of requests on wire to server */
> -               atomic_dec(&ses->server->inFlight);
> -               wake_up(&ses->server->request_q);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> -       spin_unlock(&GlobalMid_Lock);
>        receive_len = midQ->resp_buf->smb_buf_length;
>
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> @@ -677,36 +690,10 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>        /* No user interrupts in wait - wreaks havoc with performance */
>        wait_for_response(ses, midQ, timeout, 10 * HZ);
>
> -       spin_lock(&GlobalMid_Lock);
> -       if (midQ->resp_buf == NULL) {
> -               cERROR(1, "No response for cmd %d mid %d",
> -                         midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> -               /* Update # of requests on wire to server */
> -               atomic_dec(&ses->server->inFlight);
> -               wake_up(&ses->server->request_q);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> -       spin_unlock(&GlobalMid_Lock);
>        receive_len = midQ->resp_buf->smb_buf_length;
>
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
> @@ -926,35 +913,11 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>                }
>        }
>
> -       spin_lock(&GlobalMid_Lock);
> -       if (midQ->resp_buf) {
> -               spin_unlock(&GlobalMid_Lock);
> -               receive_len = midQ->resp_buf->smb_buf_length;
> -       } else {
> -               cERROR(1, "No response for cmd %d mid %d",
> -                         midQ->command, midQ->mid);
> -               if (midQ->midState == MID_REQUEST_SUBMITTED) {
> -                       if (ses->server->tcpStatus == CifsExiting)
> -                               rc = -EHOSTDOWN;
> -                       else {
> -                               ses->server->tcpStatus = CifsNeedReconnect;
> -                               midQ->midState = MID_RETRY_NEEDED;
> -                       }
> -               }
> -
> -               if (rc != -EHOSTDOWN) {
> -                       if (midQ->midState == MID_RETRY_NEEDED) {
> -                               rc = -EAGAIN;
> -                               cFYI(1, "marking request for retry");
> -                       } else {
> -                               rc = -EIO;
> -                       }
> -               }
> -               spin_unlock(&GlobalMid_Lock);
> -               DeleteMidQEntry(midQ);
> +       rc = handle_mid_result(midQ, ses->server);
> +       if (rc != 0)
>                return rc;
> -       }
>
> +       receive_len = midQ->resp_buf->smb_buf_length;
>        if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
>                cERROR(1, "Frame too large received.  Length: %d  Xid: %d",
>                        receive_len, xid);
> --
> 1.7.3.2
>


Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Suresh Jayaraman]

On 12/10/2010 09:14 PM, Jeff Layton wrote:
> If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
> temporary. CIFS should retry the write instead of setting an error on
> the mapping and returning.
> 
> For WB_SYNC_ALL, just retry the write immediately.
> 
> In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
> pages that didn't get written out and then move on.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/file.c |   23 +++++++++++------------
>  1 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index fe16f6d..8e57370 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1430,6 +1430,7 @@ retry:
>  				break;
>  		}
>  		if (n_iov) {
> +retry_write:
>  			open_file = find_writable_file(CIFS_I(mapping->host),
>  							false);
>  			if (!open_file) {
> @@ -1445,22 +1446,20 @@ retry:
>  				cifs_update_eof(cifsi, offset, bytes_written);
>  			}
>  
> -			if (rc || bytes_written < bytes_to_write) {
> -				cERROR(1, "Write2 ret %d, wrote %d",
> -					  rc, bytes_written);
> -				mapping_set_error(mapping, rc);
> -			} else {
> +			/* retry on data-integrity flush */
> +			if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
> +				goto retry_write;
> +
> +			if (!rc)
>  				cifs_stats_bytes_written(tcon, bytes_written);
> -			}
> +			else if (rc != -EAGAIN)
> +				mapping_set_error(mapping, rc);
>  
>  			for (i = 0; i < n_iov; i++) {
>  				page = pvec.pages[first + i];
> -				/* Should we also set page error on
> -				success rc but too little data written? */
> -				/* BB investigate retry logic on temporary
> -				server crash cases and how recovery works
> -				when page marked as error */
> -				if (rc)
> +				if (rc == -EAGAIN)
> +					redirty_page_for_writepage(wbc, page);
> +				else if (rc != 0)
>  					SetPageError(page);
>  				kunmap(page);
>  				unlock_page(page);

Documentation/filesystems/Locking suggests that rc should be set to 0
after redirtying?


-- 
Suresh Jayaraman

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Jeff Layton]

On Tue, 14 Dec 2010 10:24:28 +0300
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > Currently, when a request is cancelled via signal, we delete the mid
> > immediately. If the request was already transmitted however, the client
> > is still likely to receive a response. When it does, it won't recognize
> > it however and will pop a printk.
> >
> > It's also a little dangerous to just delete the mid entry like this. We
> > may end up reusing that mid. If we do then we could potentially get the
> > response from the first request confused with the later one.
> >
> > Prevent the reuse of mids by marking them as cancelled and keeping them
> > on the pending_mid_q list. If the reply comes in, we'll delete it from
> > the list then. If it never comes, then we'll delete it at reconnect
> > or when cifsd comes down.
> >
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/cifsglob.h  |    2 +-
> >  fs/cifs/cifsproto.h |    1 +
> >  fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
> >  fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
> >  4 files changed, 61 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index cc43ada..fb75f04 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
> >  #define   MID_REQUEST_SUBMITTED 2
> >  #define   MID_RESPONSE_RECEIVED 4
> >  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
> > -#define   MID_NO_RESP_NEEDED 0x10
> > +#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
> >
> >  /* Types of response buffer returned from SendReceive2 */
> >  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index fe77e69..a8fc606 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
> >                const char *fullpath, const struct dfs_info3_param *ref,
> >                char **devname);
> >  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> > +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> >  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
> >                        struct smb_hdr * /* input */ ,
> >                        struct smb_hdr * /* out */ ,
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 7e20ece..0feb592 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >  {
> >        int rc = 0;
> >        struct list_head *tmp, *tmp2;
> > -       struct list_head retry;
> > +       struct list_head retry, dispose;
> >        struct cifsSesInfo *ses;
> >        struct cifsTconInfo *tcon;
> >        struct mid_q_entry *mid_entry;
> > @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >         */
> >        cFYI(1, "%s: moving mids to retry list", __func__);
> >        INIT_LIST_HEAD(&retry);
> > +       INIT_LIST_HEAD(&dispose);
> >        spin_lock(&GlobalMid_Lock);
> >        list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >                if (mid_entry->midState == MID_REQUEST_SUBMITTED)
> >                        list_move(tmp, &retry);
> > +               else if (mid_entry->midState == MID_REQUEST_CANCELLED)
> > +                       list_move(tmp, &dispose);
> >        }
> >        spin_unlock(&GlobalMid_Lock);
> >
> > +       /* now walk private dispose list and delete entries */
> > +       list_for_each_safe(tmp, tmp2, &dispose) {
> > +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> > +               DeleteMidQEntry(mid_entry);
> > +       }
> > +
> >        while ((server->tcpStatus != CifsExiting) &&
> >               (server->tcpStatus != CifsGood)) {
> >                try_to_freeze();
> > @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >                }
> >        }
> >
> > -       /* now, issue callback for all mids in flight */
> > +       /* issue callback for all mids in flight */
> >        list_for_each_safe(tmp, tmp2, &retry) {
> >                list_del_init(tmp);
> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> > @@ -575,9 +584,13 @@ incomplete_rcv:
> >                list_for_each(tmp, &server->pending_mid_q) {
> >                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >
> > -                       if ((mid_entry->mid == smb_buffer->Mid) &&
> > -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
> > -                           (mid_entry->command == smb_buffer->Command)) {
> > +                       if (mid_entry->mid != smb_buffer->Mid)
> > +                               goto next_mid;
> > +                       if (mid_entry->command != smb_buffer->Command)
> > +                               goto next_mid;
> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED)
> > +                               break;
> > +                       if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >                                if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
> >                                        /* We have a multipart transact2 resp */
> >                                        isMultiRsp = true;
> > @@ -623,11 +636,16 @@ multi_t2_fnd:
> >                                server->lstrp = jiffies;
> >                                break;
> >                        }
> > +next_mid:
> >                        mid_entry = NULL;
> >                }
> >                spin_unlock(&GlobalMid_Lock);
> >
> >                if (mid_entry != NULL) {
> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> > +                               DeleteMidQEntry(mid_entry);
> > +                               continue;
> > +                       }
> >                        mid_entry->callback(mid_entry);
> >                        /* Was previous buf put in mpx struct for multi-rsp? */
> >                        if (!isMultiRsp) {
> > @@ -704,6 +722,9 @@ multi_t2_fnd:
> >                }
> >                spin_unlock(&cifs_tcp_ses_lock);
> >        } else {
> > +               struct mid_q_entry *tmp_mid;
> > +               struct list_head dispose;
> > +
> >                /* although we can not zero the server struct pointer yet,
> >                since there are active requests which may depnd on them,
> >                mark the corresponding SMB sessions as exiting too */
> > @@ -713,17 +734,26 @@ multi_t2_fnd:
> >                        ses->status = CifsExiting;
> >                }
> >
> > +               INIT_LIST_HEAD(&dispose);
> >                spin_lock(&GlobalMid_Lock);
> > -               list_for_each(tmp, &server->pending_mid_q) {
> > -               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> > +               list_for_each_entry_safe(mid_entry, tmp_mid,
> > +                                        &server->pending_mid_q, qhead) {
> >                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >                                cFYI(1, "Clearing Mid 0x%x - issuing callback",
> >                                         mid_entry->mid);
> >                                mid_entry->callback(mid_entry);
> > +                       } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> > +                               cFYI(1, "Clearing Mid 0x%x - Cancelled",
> > +                                       mid_entry->mid);
> > +                               list_move(&mid_entry->qhead, &dispose);
> 
> Why do we need another list here? It seems to me that we can simply
> delete cancelled mid.
> 

Nope, we can't. DeleteMidQEntry will try to take the GlobalMid_Lock on
its own, so you can't call it there without deadlocking. You also can't
drop the spinlock and then call DeleteMidQEntry since that might mean
that the list will change in the middle of walking it. We could add a
"DeleteMidQEntry_locked", but dealing with locked and non-locked
variants gets messy.

Moving it to a private dispose list and then walking that list outside
of the lock takes care of those problems. I added the "retry" list in
this function for similar reasons.

> >                        }
> >                }
> >                spin_unlock(&GlobalMid_Lock);
> >                spin_unlock(&cifs_tcp_ses_lock);
> > +
> > +               /* now delete all of the cancelled mids */
> > +               list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
> > +                       DeleteMidQEntry(mid_entry);
> >                /* 1/8th of sec is more than enough time for them to exit */
> >                msleep(125);
> >        }
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 79647db..97a1170 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
> >        return temp;
> >  }
> >
> > -static void
> > +void
> >  DeleteMidQEntry(struct mid_q_entry *midEntry)
> >  {
> >  #ifdef CONFIG_CIFS_STATS2
> > @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
> >                goto out;
> >
> >        rc = wait_for_response(ses->server, midQ);
> > -       if (rc != 0)
> > -               goto out;
> > +       if (rc != 0) {
> > +               /* no longer considered to be "in-flight" */
> > +               midQ->midState = MID_REQUEST_CANCELLED;
> > +               atomic_dec(&ses->server->inFlight);
> > +               wake_up(&ses->server->request_q);
> > +               return rc;
> > +       }
> >
> >        rc = handle_mid_result(midQ, ses->server);
> >        if (rc != 0)
> > @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
> >                goto out;
> >
> >        rc = wait_for_response(ses->server, midQ);
> > -       if (rc != 0)
> > -               goto out;
> > +       if (rc != 0) {
> > +               /* no longer considered to be "in-flight" */
> > +               midQ->midState = MID_REQUEST_CANCELLED;
> > +               atomic_dec(&ses->server->inFlight);
> > +               wake_up(&ses->server->request_q);
> > +               return rc;
> > +       }
> >
> >        rc = handle_mid_result(midQ, ses->server);
> >        if (rc != 0)
> > @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
> >                        }
> >                }
> >
> > -               if (wait_for_response(ses->server, midQ) == 0) {
> > -                       /* We got the response - restart system call. */
> > -                       rstart = 1;
> > +               rc = wait_for_response(ses->server, midQ);
> > +               if (rc) {
> > +                       midQ->midState = MID_REQUEST_CANCELLED;
> > +                       return rc;
> >                }
> > +
> > +               /* We got the response - restart system call. */
> > +               rstart = 1;
> >        }
> >
> >        rc = handle_mid_result(midQ, ses->server);
> > --
> > 1.7.3.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> 
> 


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

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Jeff Layton]

On Tue, 14 Dec 2010 14:56:18 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/10/2010 09:14 PM, Jeff Layton wrote:
> > If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
> > temporary. CIFS should retry the write instead of setting an error on
> > the mapping and returning.
> > 
> > For WB_SYNC_ALL, just retry the write immediately.
> > 
> > In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
> > pages that didn't get written out and then move on.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/file.c |   23 +++++++++++------------
> >  1 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index fe16f6d..8e57370 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -1430,6 +1430,7 @@ retry:
> >  				break;
> >  		}
> >  		if (n_iov) {
> > +retry_write:
> >  			open_file = find_writable_file(CIFS_I(mapping->host),
> >  							false);
> >  			if (!open_file) {
> > @@ -1445,22 +1446,20 @@ retry:
> >  				cifs_update_eof(cifsi, offset, bytes_written);
> >  			}
> >  
> > -			if (rc || bytes_written < bytes_to_write) {
> > -				cERROR(1, "Write2 ret %d, wrote %d",
> > -					  rc, bytes_written);
> > -				mapping_set_error(mapping, rc);
> > -			} else {
> > +			/* retry on data-integrity flush */
> > +			if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
> > +				goto retry_write;
> > +
> > +			if (!rc)
> >  				cifs_stats_bytes_written(tcon, bytes_written);
> > -			}
> > +			else if (rc != -EAGAIN)
> > +				mapping_set_error(mapping, rc);
> >  
> >  			for (i = 0; i < n_iov; i++) {
> >  				page = pvec.pages[first + i];
> > -				/* Should we also set page error on
> > -				success rc but too little data written? */
> > -				/* BB investigate retry logic on temporary
> > -				server crash cases and how recovery works
> > -				when page marked as error */
> > -				if (rc)
> > +				if (rc == -EAGAIN)
> > +					redirty_page_for_writepage(wbc, page);
> > +				else if (rc != 0)
> >  					SetPageError(page);
> >  				kunmap(page);
> >  				unlock_page(page);
> 
> Documentation/filesystems/Locking suggests that rc should be set to 0
> after redirtying?
> 
> 

Ahh good catch. I'll fix it.

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

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Pavel Shilovsky]

2010/12/14 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 14 Dec 2010 10:24:28 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > Currently, when a request is cancelled via signal, we delete the mid
>> > immediately. If the request was already transmitted however, the client
>> > is still likely to receive a response. When it does, it won't recognize
>> > it however and will pop a printk.
>> >
>> > It's also a little dangerous to just delete the mid entry like this. We
>> > may end up reusing that mid. If we do then we could potentially get the
>> > response from the first request confused with the later one.
>> >
>> > Prevent the reuse of mids by marking them as cancelled and keeping them
>> > on the pending_mid_q list. If the reply comes in, we'll delete it from
>> > the list then. If it never comes, then we'll delete it at reconnect
>> > or when cifsd comes down.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/cifsglob.h  |    2 +-
>> >  fs/cifs/cifsproto.h |    1 +
>> >  fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
>> >  fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
>> >  4 files changed, 61 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > index cc43ada..fb75f04 100644
>> > --- a/fs/cifs/cifsglob.h
>> > +++ b/fs/cifs/cifsglob.h
>> > @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>> >  #define   MID_REQUEST_SUBMITTED 2
>> >  #define   MID_RESPONSE_RECEIVED 4
>> >  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
>> > -#define   MID_NO_RESP_NEEDED 0x10
>> > +#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
>> >
>> >  /* Types of response buffer returned from SendReceive2 */
>> >  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> > index fe77e69..a8fc606 100644
>> > --- a/fs/cifs/cifsproto.h
>> > +++ b/fs/cifs/cifsproto.h
>> > @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
>> >                const char *fullpath, const struct dfs_info3_param *ref,
>> >                char **devname);
>> >  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
>> > +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>> >  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
>> >                        struct smb_hdr * /* input */ ,
>> >                        struct smb_hdr * /* out */ ,
>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> > index 7e20ece..0feb592 100644
>> > --- a/fs/cifs/connect.c
>> > +++ b/fs/cifs/connect.c
>> > @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> >  {
>> >        int rc = 0;
>> >        struct list_head *tmp, *tmp2;
>> > -       struct list_head retry;
>> > +       struct list_head retry, dispose;
>> >        struct cifsSesInfo *ses;
>> >        struct cifsTconInfo *tcon;
>> >        struct mid_q_entry *mid_entry;
>> > @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> >         */
>> >        cFYI(1, "%s: moving mids to retry list", __func__);
>> >        INIT_LIST_HEAD(&retry);
>> > +       INIT_LIST_HEAD(&dispose);
>> >        spin_lock(&GlobalMid_Lock);
>> >        list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> >                if (mid_entry->midState == MID_REQUEST_SUBMITTED)
>> >                        list_move(tmp, &retry);
>> > +               else if (mid_entry->midState == MID_REQUEST_CANCELLED)
>> > +                       list_move(tmp, &dispose);
>> >        }
>> >        spin_unlock(&GlobalMid_Lock);
>> >
>> > +       /* now walk private dispose list and delete entries */
>> > +       list_for_each_safe(tmp, tmp2, &dispose) {
>> > +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> > +               DeleteMidQEntry(mid_entry);
>> > +       }
>> > +
>> >        while ((server->tcpStatus != CifsExiting) &&
>> >               (server->tcpStatus != CifsGood)) {
>> >                try_to_freeze();
>> > @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> >                }
>> >        }
>> >
>> > -       /* now, issue callback for all mids in flight */
>> > +       /* issue callback for all mids in flight */
>> >        list_for_each_safe(tmp, tmp2, &retry) {
>> >                list_del_init(tmp);
>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> > @@ -575,9 +584,13 @@ incomplete_rcv:
>> >                list_for_each(tmp, &server->pending_mid_q) {
>> >                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> >
>> > -                       if ((mid_entry->mid == smb_buffer->Mid) &&
>> > -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
>> > -                           (mid_entry->command == smb_buffer->Command)) {
>> > +                       if (mid_entry->mid != smb_buffer->Mid)
>> > +                               goto next_mid;
>> > +                       if (mid_entry->command != smb_buffer->Command)
>> > +                               goto next_mid;
>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED)
>> > +                               break;
>> > +                       if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>> >                                if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
>> >                                        /* We have a multipart transact2 resp */
>> >                                        isMultiRsp = true;
>> > @@ -623,11 +636,16 @@ multi_t2_fnd:
>> >                                server->lstrp = jiffies;
>> >                                break;
>> >                        }
>> > +next_mid:
>> >                        mid_entry = NULL;
>> >                }
>> >                spin_unlock(&GlobalMid_Lock);
>> >
>> >                if (mid_entry != NULL) {
>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED) {
>> > +                               DeleteMidQEntry(mid_entry);
>> > +                               continue;
>> > +                       }
>> >                        mid_entry->callback(mid_entry);
>> >                        /* Was previous buf put in mpx struct for multi-rsp? */
>> >                        if (!isMultiRsp) {
>> > @@ -704,6 +722,9 @@ multi_t2_fnd:
>> >                }
>> >                spin_unlock(&cifs_tcp_ses_lock);
>> >        } else {
>> > +               struct mid_q_entry *tmp_mid;
>> > +               struct list_head dispose;
>> > +
>> >                /* although we can not zero the server struct pointer yet,
>> >                since there are active requests which may depnd on them,
>> >                mark the corresponding SMB sessions as exiting too */
>> > @@ -713,17 +734,26 @@ multi_t2_fnd:
>> >                        ses->status = CifsExiting;
>> >                }
>> >
>> > +               INIT_LIST_HEAD(&dispose);
>> >                spin_lock(&GlobalMid_Lock);
>> > -               list_for_each(tmp, &server->pending_mid_q) {
>> > -               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> > +               list_for_each_entry_safe(mid_entry, tmp_mid,
>> > +                                        &server->pending_mid_q, qhead) {
>> >                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>> >                                cFYI(1, "Clearing Mid 0x%x - issuing callback",
>> >                                         mid_entry->mid);
>> >                                mid_entry->callback(mid_entry);
>> > +                       } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
>> > +                               cFYI(1, "Clearing Mid 0x%x - Cancelled",
>> > +                                       mid_entry->mid);
>> > +                               list_move(&mid_entry->qhead, &dispose);
>>
>> Why do we need another list here? It seems to me that we can simply
>> delete cancelled mid.
>>
>
> Nope, we can't. DeleteMidQEntry will try to take the GlobalMid_Lock on
> its own, so you can't call it there without deadlocking. You also can't
> drop the spinlock and then call DeleteMidQEntry since that might mean
> that the list will change in the middle of walking it. We could add a
> "DeleteMidQEntry_locked", but dealing with locked and non-locked
> variants gets messy.
>
> Moving it to a private dispose list and then walking that list outside
> of the lock takes care of those problems. I added the "retry" list in
> this function for similar reasons.
>
>> >                        }
>> >                }
>> >                spin_unlock(&GlobalMid_Lock);
>> >                spin_unlock(&cifs_tcp_ses_lock);
>> > +
>> > +               /* now delete all of the cancelled mids */
>> > +               list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
>> > +                       DeleteMidQEntry(mid_entry);
>> >                /* 1/8th of sec is more than enough time for them to exit */
>> >                msleep(125);
>> >        }
>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> > index 79647db..97a1170 100644
>> > --- a/fs/cifs/transport.c
>> > +++ b/fs/cifs/transport.c
>> > @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>> >        return temp;
>> >  }
>> >
>> > -static void
>> > +void
>> >  DeleteMidQEntry(struct mid_q_entry *midEntry)
>> >  {
>> >  #ifdef CONFIG_CIFS_STATS2
>> > @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>> >                goto out;
>> >
>> >        rc = wait_for_response(ses->server, midQ);
>> > -       if (rc != 0)
>> > -               goto out;
>> > +       if (rc != 0) {
>> > +               /* no longer considered to be "in-flight" */
>> > +               midQ->midState = MID_REQUEST_CANCELLED;
>> > +               atomic_dec(&ses->server->inFlight);
>> > +               wake_up(&ses->server->request_q);
>> > +               return rc;
>> > +       }
>> >
>> >        rc = handle_mid_result(midQ, ses->server);
>> >        if (rc != 0)
>> > @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>> >                goto out;
>> >
>> >        rc = wait_for_response(ses->server, midQ);
>> > -       if (rc != 0)
>> > -               goto out;
>> > +       if (rc != 0) {
>> > +               /* no longer considered to be "in-flight" */
>> > +               midQ->midState = MID_REQUEST_CANCELLED;
>> > +               atomic_dec(&ses->server->inFlight);
>> > +               wake_up(&ses->server->request_q);
>> > +               return rc;
>> > +       }
>> >
>> >        rc = handle_mid_result(midQ, ses->server);
>> >        if (rc != 0)
>> > @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>> >                        }
>> >                }
>> >
>> > -               if (wait_for_response(ses->server, midQ) == 0) {
>> > -                       /* We got the response - restart system call. */
>> > -                       rstart = 1;
>> > +               rc = wait_for_response(ses->server, midQ);
>> > +               if (rc) {
>> > +                       midQ->midState = MID_REQUEST_CANCELLED;
>> > +                       return rc;
>> >                }
>> > +
>> > +               /* We got the response - restart system call. */
>> > +               rstart = 1;
>> >        }
>> >
>> >        rc = handle_mid_result(midQ, ses->server);
>> > --
>> > 1.7.3.2
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>>
>>
>>
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Ok. Adding DeleteMidQEntry without lock/unlock looks better to me, but
anyway it's ok.

Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Steve French]

Why wouldn't we issue SMB NTCancel on these?  That way we only have to
wait until the timeout for the NTCancel (at worst) and can't leak midq
entries.

On Tue, Dec 14, 2010 at 2:40 PM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2010/12/14 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> On Tue, 14 Dec 2010 10:24:28 +0300
>> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>>
>>> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> > Currently, when a request is cancelled via signal, we delete the mid
>>> > immediately. If the request was already transmitted however, the client
>>> > is still likely to receive a response. When it does, it won't recognize
>>> > it however and will pop a printk.
>>> >
>>> > It's also a little dangerous to just delete the mid entry like this. We
>>> > may end up reusing that mid. If we do then we could potentially get the
>>> > response from the first request confused with the later one.
>>> >
>>> > Prevent the reuse of mids by marking them as cancelled and keeping them
>>> > on the pending_mid_q list. If the reply comes in, we'll delete it from
>>> > the list then. If it never comes, then we'll delete it at reconnect
>>> > or when cifsd comes down.
>>> >
>>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> > ---
>>> >  fs/cifs/cifsglob.h  |    2 +-
>>> >  fs/cifs/cifsproto.h |    1 +
>>> >  fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
>>> >  fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
>>> >  4 files changed, 61 insertions(+), 16 deletions(-)
>>> >
>>> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>> > index cc43ada..fb75f04 100644
>>> > --- a/fs/cifs/cifsglob.h
>>> > +++ b/fs/cifs/cifsglob.h
>>> > @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>>> >  #define   MID_REQUEST_SUBMITTED 2
>>> >  #define   MID_RESPONSE_RECEIVED 4
>>> >  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
>>> > -#define   MID_NO_RESP_NEEDED 0x10
>>> > +#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
>>> >
>>> >  /* Types of response buffer returned from SendReceive2 */
>>> >  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
>>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>>> > index fe77e69..a8fc606 100644
>>> > --- a/fs/cifs/cifsproto.h
>>> > +++ b/fs/cifs/cifsproto.h
>>> > @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
>>> >                const char *fullpath, const struct dfs_info3_param *ref,
>>> >                char **devname);
>>> >  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
>>> > +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>>> >  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
>>> >                        struct smb_hdr * /* input */ ,
>>> >                        struct smb_hdr * /* out */ ,
>>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>>> > index 7e20ece..0feb592 100644
>>> > --- a/fs/cifs/connect.c
>>> > +++ b/fs/cifs/connect.c
>>> > @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>> >  {
>>> >        int rc = 0;
>>> >        struct list_head *tmp, *tmp2;
>>> > -       struct list_head retry;
>>> > +       struct list_head retry, dispose;
>>> >        struct cifsSesInfo *ses;
>>> >        struct cifsTconInfo *tcon;
>>> >        struct mid_q_entry *mid_entry;
>>> > @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>> >         */
>>> >        cFYI(1, "%s: moving mids to retry list", __func__);
>>> >        INIT_LIST_HEAD(&retry);
>>> > +       INIT_LIST_HEAD(&dispose);
>>> >        spin_lock(&GlobalMid_Lock);
>>> >        list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>>> >                if (mid_entry->midState == MID_REQUEST_SUBMITTED)
>>> >                        list_move(tmp, &retry);
>>> > +               else if (mid_entry->midState == MID_REQUEST_CANCELLED)
>>> > +                       list_move(tmp, &dispose);
>>> >        }
>>> >        spin_unlock(&GlobalMid_Lock);
>>> >
>>> > +       /* now walk private dispose list and delete entries */
>>> > +       list_for_each_safe(tmp, tmp2, &dispose) {
>>> > +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>>> > +               DeleteMidQEntry(mid_entry);
>>> > +       }
>>> > +
>>> >        while ((server->tcpStatus != CifsExiting) &&
>>> >               (server->tcpStatus != CifsGood)) {
>>> >                try_to_freeze();
>>> > @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>> >                }
>>> >        }
>>> >
>>> > -       /* now, issue callback for all mids in flight */
>>> > +       /* issue callback for all mids in flight */
>>> >        list_for_each_safe(tmp, tmp2, &retry) {
>>> >                list_del_init(tmp);
>>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>>> > @@ -575,9 +584,13 @@ incomplete_rcv:
>>> >                list_for_each(tmp, &server->pending_mid_q) {
>>> >                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>>> >
>>> > -                       if ((mid_entry->mid == smb_buffer->Mid) &&
>>> > -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
>>> > -                           (mid_entry->command == smb_buffer->Command)) {
>>> > +                       if (mid_entry->mid != smb_buffer->Mid)
>>> > +                               goto next_mid;
>>> > +                       if (mid_entry->command != smb_buffer->Command)
>>> > +                               goto next_mid;
>>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED)
>>> > +                               break;
>>> > +                       if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>>> >                                if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
>>> >                                        /* We have a multipart transact2 resp */
>>> >                                        isMultiRsp = true;
>>> > @@ -623,11 +636,16 @@ multi_t2_fnd:
>>> >                                server->lstrp = jiffies;
>>> >                                break;
>>> >                        }
>>> > +next_mid:
>>> >                        mid_entry = NULL;
>>> >                }
>>> >                spin_unlock(&GlobalMid_Lock);
>>> >
>>> >                if (mid_entry != NULL) {
>>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED) {
>>> > +                               DeleteMidQEntry(mid_entry);
>>> > +                               continue;
>>> > +                       }
>>> >                        mid_entry->callback(mid_entry);
>>> >                        /* Was previous buf put in mpx struct for multi-rsp? */
>>> >                        if (!isMultiRsp) {
>>> > @@ -704,6 +722,9 @@ multi_t2_fnd:
>>> >                }
>>> >                spin_unlock(&cifs_tcp_ses_lock);
>>> >        } else {
>>> > +               struct mid_q_entry *tmp_mid;
>>> > +               struct list_head dispose;
>>> > +
>>> >                /* although we can not zero the server struct pointer yet,
>>> >                since there are active requests which may depnd on them,
>>> >                mark the corresponding SMB sessions as exiting too */
>>> > @@ -713,17 +734,26 @@ multi_t2_fnd:
>>> >                        ses->status = CifsExiting;
>>> >                }
>>> >
>>> > +               INIT_LIST_HEAD(&dispose);
>>> >                spin_lock(&GlobalMid_Lock);
>>> > -               list_for_each(tmp, &server->pending_mid_q) {
>>> > -               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>>> > +               list_for_each_entry_safe(mid_entry, tmp_mid,
>>> > +                                        &server->pending_mid_q, qhead) {
>>> >                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
>>> >                                cFYI(1, "Clearing Mid 0x%x - issuing callback",
>>> >                                         mid_entry->mid);
>>> >                                mid_entry->callback(mid_entry);
>>> > +                       } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
>>> > +                               cFYI(1, "Clearing Mid 0x%x - Cancelled",
>>> > +                                       mid_entry->mid);
>>> > +                               list_move(&mid_entry->qhead, &dispose);
>>>
>>> Why do we need another list here? It seems to me that we can simply
>>> delete cancelled mid.
>>>
>>
>> Nope, we can't. DeleteMidQEntry will try to take the GlobalMid_Lock on
>> its own, so you can't call it there without deadlocking. You also can't
>> drop the spinlock and then call DeleteMidQEntry since that might mean
>> that the list will change in the middle of walking it. We could add a
>> "DeleteMidQEntry_locked", but dealing with locked and non-locked
>> variants gets messy.
>>
>> Moving it to a private dispose list and then walking that list outside
>> of the lock takes care of those problems. I added the "retry" list in
>> this function for similar reasons.
>>
>>> >                        }
>>> >                }
>>> >                spin_unlock(&GlobalMid_Lock);
>>> >                spin_unlock(&cifs_tcp_ses_lock);
>>> > +
>>> > +               /* now delete all of the cancelled mids */
>>> > +               list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
>>> > +                       DeleteMidQEntry(mid_entry);
>>> >                /* 1/8th of sec is more than enough time for them to exit */
>>> >                msleep(125);
>>> >        }
>>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> > index 79647db..97a1170 100644
>>> > --- a/fs/cifs/transport.c
>>> > +++ b/fs/cifs/transport.c
>>> > @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
>>> >        return temp;
>>> >  }
>>> >
>>> > -static void
>>> > +void
>>> >  DeleteMidQEntry(struct mid_q_entry *midEntry)
>>> >  {
>>> >  #ifdef CONFIG_CIFS_STATS2
>>> > @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
>>> >                goto out;
>>> >
>>> >        rc = wait_for_response(ses->server, midQ);
>>> > -       if (rc != 0)
>>> > -               goto out;
>>> > +       if (rc != 0) {
>>> > +               /* no longer considered to be "in-flight" */
>>> > +               midQ->midState = MID_REQUEST_CANCELLED;
>>> > +               atomic_dec(&ses->server->inFlight);
>>> > +               wake_up(&ses->server->request_q);
>>> > +               return rc;
>>> > +       }
>>> >
>>> >        rc = handle_mid_result(midQ, ses->server);
>>> >        if (rc != 0)
>>> > @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
>>> >                goto out;
>>> >
>>> >        rc = wait_for_response(ses->server, midQ);
>>> > -       if (rc != 0)
>>> > -               goto out;
>>> > +       if (rc != 0) {
>>> > +               /* no longer considered to be "in-flight" */
>>> > +               midQ->midState = MID_REQUEST_CANCELLED;
>>> > +               atomic_dec(&ses->server->inFlight);
>>> > +               wake_up(&ses->server->request_q);
>>> > +               return rc;
>>> > +       }
>>> >
>>> >        rc = handle_mid_result(midQ, ses->server);
>>> >        if (rc != 0)
>>> > @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
>>> >                        }
>>> >                }
>>> >
>>> > -               if (wait_for_response(ses->server, midQ) == 0) {
>>> > -                       /* We got the response - restart system call. */
>>> > -                       rstart = 1;
>>> > +               rc = wait_for_response(ses->server, midQ);
>>> > +               if (rc) {
>>> > +                       midQ->midState = MID_REQUEST_CANCELLED;
>>> > +                       return rc;
>>> >                }
>>> > +
>>> > +               /* We got the response - restart system call. */
>>> > +               rstart = 1;
>>> >        }
>>> >
>>> >        rc = handle_mid_result(midQ, ses->server);
>>> > --
>>> > 1.7.3.2
>>> >
>>> > --
>>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> >
>>>
>>>
>>>
>>
>>
>> --
>> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Ok. Adding DeleteMidQEntry without lock/unlock looks better to me, but
> anyway it's ok.
>
> Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> --
> Best regards,
> Pavel Shilovsky.
>



-- 
Thanks,

Steve

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Jeff Layton]

On Tue, 14 Dec 2010 15:33:48 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Why wouldn't we issue SMB NTCancel on these?  That way we only have to
> wait until the timeout for the NTCancel (at worst) and can't leak midq
> entries.
> 

I suppose we could, but...

a) windows doesn't do it

b) we don't have appropriate infrastructure for it

...sounds like a good follow-on project, but not something I really
want to tackle in this set.

> On Tue, Dec 14, 2010 at 2:40 PM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> > 2010/12/14 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> On Tue, 14 Dec 2010 10:24:28 +0300
> >> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >>
> >>> 2010/12/10 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>> > Currently, when a request is cancelled via signal, we delete the mid
> >>> > immediately. If the request was already transmitted however, the client
> >>> > is still likely to receive a response. When it does, it won't recognize
> >>> > it however and will pop a printk.
> >>> >
> >>> > It's also a little dangerous to just delete the mid entry like this. We
> >>> > may end up reusing that mid. If we do then we could potentially get the
> >>> > response from the first request confused with the later one.
> >>> >
> >>> > Prevent the reuse of mids by marking them as cancelled and keeping them
> >>> > on the pending_mid_q list. If the reply comes in, we'll delete it from
> >>> > the list then. If it never comes, then we'll delete it at reconnect
> >>> > or when cifsd comes down.
> >>> >
> >>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> > ---
> >>> >  fs/cifs/cifsglob.h  |    2 +-
> >>> >  fs/cifs/cifsproto.h |    1 +
> >>> >  fs/cifs/connect.c   |   44 +++++++++++++++++++++++++++++++++++++-------
> >>> >  fs/cifs/transport.c |   30 ++++++++++++++++++++++--------
> >>> >  4 files changed, 61 insertions(+), 16 deletions(-)
> >>> >
> >>> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >>> > index cc43ada..fb75f04 100644
> >>> > --- a/fs/cifs/cifsglob.h
> >>> > +++ b/fs/cifs/cifsglob.h
> >>> > @@ -621,7 +621,7 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
> >>> >  #define   MID_REQUEST_SUBMITTED 2
> >>> >  #define   MID_RESPONSE_RECEIVED 4
> >>> >  #define   MID_RETRY_NEEDED      8 /* session closed while this request out */
> >>> > -#define   MID_NO_RESP_NEEDED 0x10
> >>> > +#define   MID_REQUEST_CANCELLED 0x10 /* discard any reply */
> >>> >
> >>> >  /* Types of response buffer returned from SendReceive2 */
> >>> >  #define   CIFS_NO_BUFFER        0    /* Response buffer not returned */
> >>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >>> > index fe77e69..a8fc606 100644
> >>> > --- a/fs/cifs/cifsproto.h
> >>> > +++ b/fs/cifs/cifsproto.h
> >>> > @@ -61,6 +61,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
> >>> >                const char *fullpath, const struct dfs_info3_param *ref,
> >>> >                char **devname);
> >>> >  /* extern void renew_parental_timestamps(struct dentry *direntry);*/
> >>> > +extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> >>> >  extern int SendReceive(const unsigned int /* xid */ , struct cifsSesInfo *,
> >>> >                        struct smb_hdr * /* input */ ,
> >>> >                        struct smb_hdr * /* out */ ,
> >>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >>> > index 7e20ece..0feb592 100644
> >>> > --- a/fs/cifs/connect.c
> >>> > +++ b/fs/cifs/connect.c
> >>> > @@ -133,7 +133,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> >  {
> >>> >        int rc = 0;
> >>> >        struct list_head *tmp, *tmp2;
> >>> > -       struct list_head retry;
> >>> > +       struct list_head retry, dispose;
> >>> >        struct cifsSesInfo *ses;
> >>> >        struct cifsTconInfo *tcon;
> >>> >        struct mid_q_entry *mid_entry;
> >>> > @@ -192,14 +192,23 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> >         */
> >>> >        cFYI(1, "%s: moving mids to retry list", __func__);
> >>> >        INIT_LIST_HEAD(&retry);
> >>> > +       INIT_LIST_HEAD(&dispose);
> >>> >        spin_lock(&GlobalMid_Lock);
> >>> >        list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> >>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> >                if (mid_entry->midState == MID_REQUEST_SUBMITTED)
> >>> >                        list_move(tmp, &retry);
> >>> > +               else if (mid_entry->midState == MID_REQUEST_CANCELLED)
> >>> > +                       list_move(tmp, &dispose);
> >>> >        }
> >>> >        spin_unlock(&GlobalMid_Lock);
> >>> >
> >>> > +       /* now walk private dispose list and delete entries */
> >>> > +       list_for_each_safe(tmp, tmp2, &dispose) {
> >>> > +               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > +               DeleteMidQEntry(mid_entry);
> >>> > +       }
> >>> > +
> >>> >        while ((server->tcpStatus != CifsExiting) &&
> >>> >               (server->tcpStatus != CifsGood)) {
> >>> >                try_to_freeze();
> >>> > @@ -219,7 +228,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>> >                }
> >>> >        }
> >>> >
> >>> > -       /* now, issue callback for all mids in flight */
> >>> > +       /* issue callback for all mids in flight */
> >>> >        list_for_each_safe(tmp, tmp2, &retry) {
> >>> >                list_del_init(tmp);
> >>> >                mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > @@ -575,9 +584,13 @@ incomplete_rcv:
> >>> >                list_for_each(tmp, &server->pending_mid_q) {
> >>> >                        mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> >
> >>> > -                       if ((mid_entry->mid == smb_buffer->Mid) &&
> >>> > -                           (mid_entry->midState == MID_REQUEST_SUBMITTED) &&
> >>> > -                           (mid_entry->command == smb_buffer->Command)) {
> >>> > +                       if (mid_entry->mid != smb_buffer->Mid)
> >>> > +                               goto next_mid;
> >>> > +                       if (mid_entry->command != smb_buffer->Command)
> >>> > +                               goto next_mid;
> >>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED)
> >>> > +                               break;
> >>> > +                       if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >>> >                                if (check2ndT2(smb_buffer,server->maxBuf) > 0) {
> >>> >                                        /* We have a multipart transact2 resp */
> >>> >                                        isMultiRsp = true;
> >>> > @@ -623,11 +636,16 @@ multi_t2_fnd:
> >>> >                                server->lstrp = jiffies;
> >>> >                                break;
> >>> >                        }
> >>> > +next_mid:
> >>> >                        mid_entry = NULL;
> >>> >                }
> >>> >                spin_unlock(&GlobalMid_Lock);
> >>> >
> >>> >                if (mid_entry != NULL) {
> >>> > +                       if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> >>> > +                               DeleteMidQEntry(mid_entry);
> >>> > +                               continue;
> >>> > +                       }
> >>> >                        mid_entry->callback(mid_entry);
> >>> >                        /* Was previous buf put in mpx struct for multi-rsp? */
> >>> >                        if (!isMultiRsp) {
> >>> > @@ -704,6 +722,9 @@ multi_t2_fnd:
> >>> >                }
> >>> >                spin_unlock(&cifs_tcp_ses_lock);
> >>> >        } else {
> >>> > +               struct mid_q_entry *tmp_mid;
> >>> > +               struct list_head dispose;
> >>> > +
> >>> >                /* although we can not zero the server struct pointer yet,
> >>> >                since there are active requests which may depnd on them,
> >>> >                mark the corresponding SMB sessions as exiting too */
> >>> > @@ -713,17 +734,26 @@ multi_t2_fnd:
> >>> >                        ses->status = CifsExiting;
> >>> >                }
> >>> >
> >>> > +               INIT_LIST_HEAD(&dispose);
> >>> >                spin_lock(&GlobalMid_Lock);
> >>> > -               list_for_each(tmp, &server->pending_mid_q) {
> >>> > -               mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> >>> > +               list_for_each_entry_safe(mid_entry, tmp_mid,
> >>> > +                                        &server->pending_mid_q, qhead) {
> >>> >                        if (mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >>> >                                cFYI(1, "Clearing Mid 0x%x - issuing callback",
> >>> >                                         mid_entry->mid);
> >>> >                                mid_entry->callback(mid_entry);
> >>> > +                       } else if (mid_entry->midState == MID_REQUEST_CANCELLED) {
> >>> > +                               cFYI(1, "Clearing Mid 0x%x - Cancelled",
> >>> > +                                       mid_entry->mid);
> >>> > +                               list_move(&mid_entry->qhead, &dispose);
> >>>
> >>> Why do we need another list here? It seems to me that we can simply
> >>> delete cancelled mid.
> >>>
> >>
> >> Nope, we can't. DeleteMidQEntry will try to take the GlobalMid_Lock on
> >> its own, so you can't call it there without deadlocking. You also can't
> >> drop the spinlock and then call DeleteMidQEntry since that might mean
> >> that the list will change in the middle of walking it. We could add a
> >> "DeleteMidQEntry_locked", but dealing with locked and non-locked
> >> variants gets messy.
> >>
> >> Moving it to a private dispose list and then walking that list outside
> >> of the lock takes care of those problems. I added the "retry" list in
> >> this function for similar reasons.
> >>
> >>> >                        }
> >>> >                }
> >>> >                spin_unlock(&GlobalMid_Lock);
> >>> >                spin_unlock(&cifs_tcp_ses_lock);
> >>> > +
> >>> > +               /* now delete all of the cancelled mids */
> >>> > +               list_for_each_entry_safe(mid_entry, tmp_mid, &dispose, qhead)
> >>> > +                       DeleteMidQEntry(mid_entry);
> >>> >                /* 1/8th of sec is more than enough time for them to exit */
> >>> >                msleep(125);
> >>> >        }
> >>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >>> > index 79647db..97a1170 100644
> >>> > --- a/fs/cifs/transport.c
> >>> > +++ b/fs/cifs/transport.c
> >>> > @@ -81,7 +81,7 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
> >>> >        return temp;
> >>> >  }
> >>> >
> >>> > -static void
> >>> > +void
> >>> >  DeleteMidQEntry(struct mid_q_entry *midEntry)
> >>> >  {
> >>> >  #ifdef CONFIG_CIFS_STATS2
> >>> > @@ -480,8 +480,13 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
> >>> >                goto out;
> >>> >
> >>> >        rc = wait_for_response(ses->server, midQ);
> >>> > -       if (rc != 0)
> >>> > -               goto out;
> >>> > +       if (rc != 0) {
> >>> > +               /* no longer considered to be "in-flight" */
> >>> > +               midQ->midState = MID_REQUEST_CANCELLED;
> >>> > +               atomic_dec(&ses->server->inFlight);
> >>> > +               wake_up(&ses->server->request_q);
> >>> > +               return rc;
> >>> > +       }
> >>> >
> >>> >        rc = handle_mid_result(midQ, ses->server);
> >>> >        if (rc != 0)
> >>> > @@ -623,8 +628,13 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
> >>> >                goto out;
> >>> >
> >>> >        rc = wait_for_response(ses->server, midQ);
> >>> > -       if (rc != 0)
> >>> > -               goto out;
> >>> > +       if (rc != 0) {
> >>> > +               /* no longer considered to be "in-flight" */
> >>> > +               midQ->midState = MID_REQUEST_CANCELLED;
> >>> > +               atomic_dec(&ses->server->inFlight);
> >>> > +               wake_up(&ses->server->request_q);
> >>> > +               return rc;
> >>> > +       }
> >>> >
> >>> >        rc = handle_mid_result(midQ, ses->server);
> >>> >        if (rc != 0)
> >>> > @@ -842,10 +852,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
> >>> >                        }
> >>> >                }
> >>> >
> >>> > -               if (wait_for_response(ses->server, midQ) == 0) {
> >>> > -                       /* We got the response - restart system call. */
> >>> > -                       rstart = 1;
> >>> > +               rc = wait_for_response(ses->server, midQ);
> >>> > +               if (rc) {
> >>> > +                       midQ->midState = MID_REQUEST_CANCELLED;
> >>> > +                       return rc;
> >>> >                }
> >>> > +
> >>> > +               /* We got the response - restart system call. */
> >>> > +               rstart = 1;
> >>> >        }
> >>> >
> >>> >        rc = handle_mid_result(midQ, ses->server);
> >>> > --
> >>> > 1.7.3.2
> >>> >
> >>> > --
> >>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>> >
> >>>
> >>>
> >>>
> >>
> >>
> >> --
> >> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
> > Ok. Adding DeleteMidQEntry without lock/unlock looks better to me, but
> > anyway it's ok.
> >
> > Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > --
> > Best regards,
> > Pavel Shilovsky.
> >
> 
> 
> 


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

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Steve French]

On Tue, Dec 14, 2010 at 3:44 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 14 Dec 2010 15:33:48 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Why wouldn't we issue SMB NTCancel on these?  That way we only have to
>> wait until the timeout for the NTCancel (at worst) and can't leak midq
>> entries.
>>
>
> I suppose we could, but...
>
> a) windows doesn't do it

Windows does issue cancel requests ... even if not for exactly the same case

-- 
Thanks,

Steve

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Jeff Layton]

On Tue, 14 Dec 2010 16:22:01 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Dec 14, 2010 at 3:44 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 14 Dec 2010 15:33:48 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> Why wouldn't we issue SMB NTCancel on these?  That way we only have to
> >> wait until the timeout for the NTCancel (at worst) and can't leak midq
> >> entries.
> >>
> >
> > I suppose we could, but...
> >
> > a) windows doesn't do it
> 
> Windows does issue cancel requests ... even if not for exactly the same case
> 

Hmm...maybe, but when I asked MS about timeout behavior, Edgar said this:

2) If it returns an error to the application, does the client send a
SMB_COM_NT_CANCEL to cancel the outstanding request?

Answer:
The client will not send a CANCEL request on any outstanding request;
it simply tears down the connection after the session times out.

...he may have been talking about timeouts specifically however and not
about NT cancel commands in general.

Still, I'm a little leery of doing this. It adds complexity to an
already very complex codepath. It's also not going to be necessary in
most cases. Well behaved servers eventually send a reply of some sort. A
server that doesn't is broken. A MID that hangs around until reconnect
or unmount is probably the least of your worries in that situation.

But in principle, we could do it. There is a send_nt_cancel() command
in the code already and we could call it from this codepath. It'll be
tricky however as we'll have a signal pending and that affects
kernel_sendmsg behavior.

There's also the problem that we'll potentially block while trying to
send the cancel, which could make it so that you stall userspace out
while trying to kill off the process. Not ideal. Maybe we'll need to
send the cancel from another context entirely?

In any case, I'd really prefer to not do that in the context of
this set. It requires some careful thought about how to do it right,
and adds complexity that I don't think is needed at this point in time.

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

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Steve French]

On Tue, Dec 14, 2010 at 5:18 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 14 Dec 2010 16:22:01 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Tue, Dec 14, 2010 at 3:44 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Tue, 14 Dec 2010 15:33:48 -0600
>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> Why wouldn't we issue SMB NTCancel on these?  That way we only have to
>> >> wait until the timeout for the NTCancel (at worst) and can't leak midq
>> >> entries.
>> >>
>> >
>> > I suppose we could, but...
>> >
>> > a) windows doesn't do it
>>
>> Windows does issue cancel requests ... even if not for exactly the same case
>>
>
> Hmm...maybe, but when I asked MS about timeout behavior, Edgar said this:
>
> 2) If it returns an error to the application, does the client send a
> SMB_COM_NT_CANCEL to cancel the outstanding request?
>
> Answer:
> The client will not send a CANCEL request on any outstanding request;
> it simply tears down the connection after the session times out.
>
> ...he may have been talking about timeouts specifically however and not
> about NT cancel commands in general.
>
> Still, I'm a little leery of doing this. It adds complexity to an
> already very complex codepath. It's also not going to be necessary in
> most cases. Well behaved servers eventually send a reply of some sort. A
> server that doesn't is broken. A MID that hangs around until reconnect
> or unmount is probably the least of your worries in that situation.
>
> But in principle, we could do it. There is a send_nt_cancel() command
> in the code already and we could call it from this codepath. It'll be
> tricky however as we'll have a signal pending and that affects
> kernel_sendmsg behavior.
>
> There's also the problem that we'll potentially block while trying to
> send the cancel, which could make it so that you stall userspace out
> while trying to kill off the process. Not ideal. Maybe we'll need to
> send the cancel from another context entirely?
>
> In any case, I'd really prefer to not do that in the context of
> this set. It requires some careful thought about how to do it right,
> and adds complexity that I don't think is needed at this point in time.

The logical problem is that certain operations can take many minutes
or hours (and those are precisely those which might be ctl-c) so there
is a very real possibility that without issuing cancels we could
exhaust resources (ctl-c, reissue, ctl-c reissue etc.)



-- 
Thanks,

Steve

Re: [PATCH 08/13] cifs: handle cancelled requests better

[Jeff Layton]

On Tue, 14 Dec 2010 22:05:53 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Dec 14, 2010 at 5:18 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 14 Dec 2010 16:22:01 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Tue, Dec 14, 2010 at 3:44 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Tue, 14 Dec 2010 15:33:48 -0600
> >> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> >> Why wouldn't we issue SMB NTCancel on these?  That way we only have to
> >> >> wait until the timeout for the NTCancel (at worst) and can't leak midq
> >> >> entries.
> >> >>
> >> >
> >> > I suppose we could, but...
> >> >
> >> > a) windows doesn't do it
> >>
> >> Windows does issue cancel requests ... even if not for exactly the same case
> >>
> >
> > Hmm...maybe, but when I asked MS about timeout behavior, Edgar said this:
> >
> > 2) If it returns an error to the application, does the client send a
> > SMB_COM_NT_CANCEL to cancel the outstanding request?
> >
> > Answer:
> > The client will not send a CANCEL request on any outstanding request;
> > it simply tears down the connection after the session times out.
> >
> > ...he may have been talking about timeouts specifically however and not
> > about NT cancel commands in general.
> >
> > Still, I'm a little leery of doing this. It adds complexity to an
> > already very complex codepath. It's also not going to be necessary in
> > most cases. Well behaved servers eventually send a reply of some sort. A
> > server that doesn't is broken. A MID that hangs around until reconnect
> > or unmount is probably the least of your worries in that situation.
> >
> > But in principle, we could do it. There is a send_nt_cancel() command
> > in the code already and we could call it from this codepath. It'll be
> > tricky however as we'll have a signal pending and that affects
> > kernel_sendmsg behavior.
> >
> > There's also the problem that we'll potentially block while trying to
> > send the cancel, which could make it so that you stall userspace out
> > while trying to kill off the process. Not ideal. Maybe we'll need to
> > send the cancel from another context entirely?
> >
> > In any case, I'd really prefer to not do that in the context of
> > this set. It requires some careful thought about how to do it right,
> > and adds complexity that I don't think is needed at this point in time.
> 
> The logical problem is that certain operations can take many minutes
> or hours (and those are precisely those which might be ctl-c) so there
> is a very real possibility that without issuing cancels we could
> exhaust resources (ctl-c, reissue, ctl-c reissue etc.)
> 

mid_q_entries are small. I don't think we'll have much of a problem in
practice. But, if you think it's enough of a problem to worry about I
can try to do this before the merge window. Assuming I do so, does the
rest of the set look ready for merge into 2.6.38?

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

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Steve French]

Jeff,
Sounds like this patch series is close to ready.  Are you respinning
[only] patch 1 of 13?  Is this the easiest series of yours to
review/merge (into cifs-2.6.git) first for 2.6.38-pre



On Tue, Dec 14, 2010 at 6:18 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 14 Dec 2010 14:56:18 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> On 12/10/2010 09:14 PM, Jeff Layton wrote:
>> > If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered
>> > temporary. CIFS should retry the write instead of setting an error on
>> > the mapping and returning.
>> >
>> > For WB_SYNC_ALL, just retry the write immediately.
>> >
>> > In the WB_SYNC_NONE case, call redirty_page_for_writeback on all of the
>> > pages that didn't get written out and then move on.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/file.c |   23 +++++++++++------------
>> >  1 files changed, 11 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index fe16f6d..8e57370 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -1430,6 +1430,7 @@ retry:
>> >                             break;
>> >             }
>> >             if (n_iov) {
>> > +retry_write:
>> >                     open_file = find_writable_file(CIFS_I(mapping->host),
>> >                                                     false);
>> >                     if (!open_file) {
>> > @@ -1445,22 +1446,20 @@ retry:
>> >                             cifs_update_eof(cifsi, offset, bytes_written);
>> >                     }
>> >
>> > -                   if (rc || bytes_written < bytes_to_write) {
>> > -                           cERROR(1, "Write2 ret %d, wrote %d",
>> > -                                     rc, bytes_written);
>> > -                           mapping_set_error(mapping, rc);
>> > -                   } else {
>> > +                   /* retry on data-integrity flush */
>> > +                   if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
>> > +                           goto retry_write;
>> > +
>> > +                   if (!rc)
>> >                             cifs_stats_bytes_written(tcon, bytes_written);
>> > -                   }
>> > +                   else if (rc != -EAGAIN)
>> > +                           mapping_set_error(mapping, rc);
>> >
>> >                     for (i = 0; i < n_iov; i++) {
>> >                             page = pvec.pages[first + i];
>> > -                           /* Should we also set page error on
>> > -                           success rc but too little data written? */
>> > -                           /* BB investigate retry logic on temporary
>> > -                           server crash cases and how recovery works
>> > -                           when page marked as error */
>> > -                           if (rc)
>> > +                           if (rc == -EAGAIN)
>> > +                                   redirty_page_for_writepage(wbc, page);
>> > +                           else if (rc != 0)
>> >                                     SetPageError(page);
>> >                             kunmap(page);
>> >                             unlock_page(page);
>>
>> Documentation/filesystems/Locking suggests that rc should be set to 0
>> after redirtying?
>>
>>
>
> Ahh good catch. I'll fix it.
>
> Thanks,
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH 01/13] cifs: don't fail writepages on -EAGAIN errors

[Jeff Layton]

On Thu, 16 Dec 2010 10:35:12 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Jeff,
> Sounds like this patch series is close to ready.  Are you respinning
> [only] patch 1 of 13?  Is this the easiest series of yours to
> review/merge (into cifs-2.6.git) first for 2.6.38-pre
> 

Getting close, yes. I've made some other changes and will end up
re-posting the set in the next week or so after a bit more testing.
FWIW, I made an effort to add in NT_CANCEL requests when the program is
killed, but it's really quite complicated.

Working on that has really demonstrated to me that there are some
significant problems with signal handling in the existing cifs code. I
don't think this patchset makes that any worse, but I'd like to be sure
before it goes in.

I've also got another set of patches that's probably easier to review
-- the one to clean up CONFIG_CIFS_EXPERIMENTAL. Let me know if
you want me to resend that one.

Beyond that, I have some one-off patches, at least one of which depends
on Pavel's cleanup of the open code. Some of those I haven't sent yet,
but will once you start merging things.

Finally, I'm still carrying this patch that I sent months ago:

    cifs: use CreationTime like an i_generation field

I'd like to know whether you plan to commit it or not.

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