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

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

[Jeff Layton]

This is the third patchset to clean up request timeout behavior in cifs.
There are some siginficant and subtle changes since the last set, so
I've left off any "Reviewed-by" tags on the previous patches. Here's
an overview of what has changed:

- small bugfix in the writepages patch to make it return 0 when there
  we're redirtying the pages for the write

- addition of a cleanup patch for dead code in cifs_demultiplex_thread

- DeleteMidQEntry and AllocMidQEntry no longer take the GlobalMid_Lock.
  This allows them to be called with that lock held.

- clarification of the locking and state of the mid when the
  mid->callback function is called. It's now assumed to be called with
  the GlobalMid_Lock held and with it no longer on the pending_mid_q.
  Comments were added to clarify this.

- canceled mids are handled by setting the callback function instead
  of setting the state and handling them in a special way

- send_nt_cancel creates the cancel request in a different way so that
  it doesn't need to be called with the tcon

- NT_CANCEL requests are sent when a process waiting on a synchronous
  response catches a fatal signal.

The main impetus of this patchset is to change the way CIFS handles SMB
requests that are taking a long time. I won't rewrite all of that
information, but the description is available here:

    http://marc.info/?l=linux-cifs&m=129199588315592&w=2

Another discussion thread surrounding this change is also available
here:

    http://marc.info/?l=linux-cifs&m=129142970724511&w=2

I think there is some further work needed in this area, but this is a
good stopping point for 2.6.38 and it seems to fix the most glaring
problems. I'd like to see this merged during the next merge window if
possible.

Comments and suggestions welcome...

Jeff Layton (18):
  cifs: don't fail writepages on -EAGAIN errors
  cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is
    exiting
  cifs: make wait_for_free_request take a TCP_Server_Info pointer
  cifs: clean up accesses to midCount
  cifs: move locked sections out of DeleteMidQEntry and AllocMidQEntry
  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 sync_mid_result
  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
  cifs: mangle existing header for SMB_COM_NT_CANCEL
  cifs: send an NT_CANCEL request when a process is signalled

 fs/cifs/cifs_debug.c |   10 +-
 fs/cifs/cifsglob.h   |   25 ++-
 fs/cifs/cifspdu.h    |   15 ++
 fs/cifs/cifsproto.h  |    7 +
 fs/cifs/cifssmb.c    |   55 ++++++-
 fs/cifs/connect.c    |  150 +++++++++--------
 fs/cifs/file.c       |   93 +++++------
 fs/cifs/sess.c       |    2 +-
 fs/cifs/transport.c  |  463 ++++++++++++++++++++++++++------------------------
 9 files changed, 459 insertions(+), 361 deletions(-)

-- 
1.7.3.3

[PATCH 01/18] 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.

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 |   49 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 4007e60..400c948 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1377,6 +1377,7 @@ retry:
 				break;
 		}
 		if (n_iov) {
+retry_write:
 			open_file = find_writable_file(CIFS_I(mapping->host),
 							false);
 			if (!open_file) {
@@ -1389,31 +1390,55 @@ 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)
+				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);
+			else
+				rc = 0;
+
 			if ((wbc->nr_to_write -= n_iov) <= 0)
 				done = 1;
 			index = next;
-- 
1.7.3.3

[PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Jeff Layton]

The TCP_Server_Info is refcounted and every SMB session holds a
reference to it. Thus, smb_ses_list is always going to be empty when
cifsd is coming down. This is dead code.

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

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9fe620e2..67d3d0d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -346,7 +346,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	struct kvec iov;
 	struct socket *csocket = server->ssocket;
 	struct list_head *tmp;
-	struct cifsSesInfo *ses;
 	struct task_struct *task_to_wake = NULL;
 	struct mid_q_entry *mid_entry;
 	char temp;
@@ -677,44 +676,19 @@ multi_t2_fnd:
 	if (smallbuf) /* no sense logging a debug message if NULL */
 		cifs_small_buf_release(smallbuf);
 
-	/*
-	 * BB: we shouldn't have to do any of this. It shouldn't be
-	 * possible to exit from the thread with active SMB sessions
-	 */
-	spin_lock(&cifs_tcp_ses_lock);
-	if (list_empty(&server->pending_mid_q)) {
-		/* loop through server session structures attached to this and
-		    mark them dead */
-		list_for_each(tmp, &server->smb_ses_list) {
-			ses = list_entry(tmp, struct cifsSesInfo,
-					 smb_ses_list);
-			ses->status = CifsExiting;
-			ses->server = NULL;
-		}
-		spin_unlock(&cifs_tcp_ses_lock);
-	} else {
-		/* 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 */
-		list_for_each(tmp, &server->smb_ses_list) {
-			ses = list_entry(tmp, struct cifsSesInfo,
-					 smb_ses_list);
-			ses->status = CifsExiting;
-		}
-
+	if (!list_empty(&server->pending_mid_q)) {
 		spin_lock(&GlobalMid_Lock);
 		list_for_each(tmp, &server->pending_mid_q) {
-		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+			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 ",
-					 mid_entry->mid);
+					mid_entry->mid);
 				task_to_wake = mid_entry->tsk;
 				if (task_to_wake)
 					wake_up_process(task_to_wake);
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
-		spin_unlock(&cifs_tcp_ses_lock);
 		/* 1/8th of sec is more than enough time for them to exit */
 		msleep(125);
 	}
@@ -732,18 +706,6 @@ multi_t2_fnd:
 		coming home not much else we can do but free the memory */
 	}
 
-	/* last chance to mark ses pointers invalid
-	if there are any pointing to this (e.g
-	if a crazy root user tried to kill cifsd
-	kernel thread explicitly this might happen) */
-	/* BB: This shouldn't be necessary, see above */
-	spin_lock(&cifs_tcp_ses_lock);
-	list_for_each(tmp, &server->smb_ses_list) {
-		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
-		ses->server = NULL;
-	}
-	spin_unlock(&cifs_tcp_ses_lock);
-
 	kfree(server->hostname);
 	task_to_wake = xchg(&server->tsk, NULL);
 	kfree(server);
-- 
1.7.3.3

[PATCH 03/18] 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 59ca81b..9a14f77 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.3

[PATCH 04/18] cifs: clean up accesses to midCount

[Jeff Layton]

It's an atomic_t and the code accesses the "counter" field in it directly
instead of using atomic_read(). It also is sometimes accessed under a
spinlock and sometimes not. Move it out of the spinlock since we don't need
belt-and-suspenders for something that's just informational.

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

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index bd76527..ac7a38f 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -331,7 +331,7 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
 				atomic_read(&totSmBufAllocCount));
 #endif /* CONFIG_CIFS_STATS2 */
 
-	seq_printf(m, "Operations (MIDs): %d\n", midCount.counter);
+	seq_printf(m, "Operations (MIDs): %d\n", atomic_read(&midCount));
 	seq_printf(m,
 		"\n%d session %d share reconnects\n",
 		tcpSesReconnectCount.counter, tconInfoReconnectCount.counter);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 67d3d0d..515cafa 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -628,7 +628,7 @@ multi_t2_fnd:
 		} else if (!is_valid_oplock_break(smb_buffer, server) &&
 			   !isMultiRsp) {
 			cERROR(1, "No task to wake, unknown frame received! "
-				   "NumMids %d", midCount.counter);
+				   "NumMids %d", atomic_read(&midCount));
 			cifs_dump_mem("Received Data is: ", (char *)smb_buffer,
 				      sizeof(struct smb_hdr));
 #ifdef CONFIG_CIFS_DEBUG2
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 9a14f77..b9eb0cf 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -61,10 +61,10 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 		temp->tsk = current;
 	}
 
-	spin_lock(&GlobalMid_Lock);
-	list_add_tail(&temp->qhead, &server->pending_mid_q);
 	atomic_inc(&midCount);
 	temp->midState = MID_REQUEST_ALLOCATED;
+	spin_lock(&GlobalMid_Lock);
+	list_add_tail(&temp->qhead, &server->pending_mid_q);
 	spin_unlock(&GlobalMid_Lock);
 	return temp;
 }
@@ -78,8 +78,8 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
 	spin_lock(&GlobalMid_Lock);
 	midEntry->midState = MID_FREE;
 	list_del(&midEntry->qhead);
-	atomic_dec(&midCount);
 	spin_unlock(&GlobalMid_Lock);
+	atomic_dec(&midCount);
 	if (midEntry->largeBuf)
 		cifs_buf_release(midEntry->resp_buf);
 	else
-- 
1.7.3.3

[PATCH 05/18] cifs: move locked sections out of DeleteMidQEntry and AllocMidQEntry

[Jeff Layton]

In later patches, we're going to need to have finer-grained control
over the addition and removal of these structs from the pending_mid_q
and we'll need to be able to call the destructor while holding the
spinlock. Move the locked sections out of both routines and into
the callers. Fix up current callers of DeleteMidQEntry to call a new
routine that dequeues the entry and then destroys it.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index b9eb0cf..801726b 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -63,9 +63,6 @@ AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
 
 	atomic_inc(&midCount);
 	temp->midState = MID_REQUEST_ALLOCATED;
-	spin_lock(&GlobalMid_Lock);
-	list_add_tail(&temp->qhead, &server->pending_mid_q);
-	spin_unlock(&GlobalMid_Lock);
 	return temp;
 }
 
@@ -75,10 +72,7 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
 #ifdef CONFIG_CIFS_STATS2
 	unsigned long now;
 #endif
-	spin_lock(&GlobalMid_Lock);
 	midEntry->midState = MID_FREE;
-	list_del(&midEntry->qhead);
-	spin_unlock(&GlobalMid_Lock);
 	atomic_dec(&midCount);
 	if (midEntry->largeBuf)
 		cifs_buf_release(midEntry->resp_buf);
@@ -103,6 +97,16 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
 	mempool_free(midEntry, cifs_mid_poolp);
 }
 
+static void
+delete_mid(struct mid_q_entry *mid)
+{
+	spin_lock(&GlobalMid_Lock);
+	list_del(&mid->qhead);
+	spin_unlock(&GlobalMid_Lock);
+
+	DeleteMidQEntry(mid);
+}
+
 static int
 smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 {
@@ -308,6 +312,9 @@ static int allocate_mid(struct cifsSesInfo *ses, struct smb_hdr *in_buf,
 	*ppmidQ = AllocMidQEntry(in_buf, ses->server);
 	if (*ppmidQ == NULL)
 		return -ENOMEM;
+	spin_lock(&GlobalMid_Lock);
+	list_add_tail(&(*ppmidQ)->qhead, &ses->server->pending_mid_q);
+	spin_unlock(&GlobalMid_Lock);
 	return 0;
 }
 
@@ -508,7 +515,7 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
+		delete_mid(midQ);
 		/* Update # of requests on wire to server */
 		atomic_dec(&ses->server->inFlight);
 		wake_up(&ses->server->request_q);
@@ -564,14 +571,14 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 		if ((flags & CIFS_NO_RESP) == 0)
 			midQ->resp_buf = NULL;  /* mark it so buf will
 						   not be freed by
-						   DeleteMidQEntry */
+						   delete_mid */
 	} else {
 		rc = -EIO;
 		cFYI(1, "Bad MID state?");
 	}
 
 out:
-	DeleteMidQEntry(midQ);
+	delete_mid(midQ);
 	atomic_dec(&ses->server->inFlight);
 	wake_up(&ses->server->request_q);
 
@@ -699,7 +706,7 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
+		delete_mid(midQ);
 		/* Update # of requests on wire to server */
 		atomic_dec(&ses->server->inFlight);
 		wake_up(&ses->server->request_q);
@@ -755,7 +762,7 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 	}
 
 out:
-	DeleteMidQEntry(midQ);
+	delete_mid(midQ);
 	atomic_dec(&ses->server->inFlight);
 	wake_up(&ses->server->request_q);
 
@@ -863,7 +870,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 
 	rc = cifs_sign_smb(in_buf, ses->server, &midQ->sequence_number);
 	if (rc) {
-		DeleteMidQEntry(midQ);
+		delete_mid(midQ);
 		mutex_unlock(&ses->server->srv_mutex);
 		return rc;
 	}
@@ -880,7 +887,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 	mutex_unlock(&ses->server->srv_mutex);
 
 	if (rc < 0) {
-		DeleteMidQEntry(midQ);
+		delete_mid(midQ);
 		return rc;
 	}
 
@@ -902,7 +909,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 
 			rc = send_nt_cancel(tcon, in_buf, midQ);
 			if (rc) {
-				DeleteMidQEntry(midQ);
+				delete_mid(midQ);
 				return rc;
 			}
 		} else {
@@ -914,7 +921,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 			/* If we get -ENOLCK back the lock may have
 			   already been removed. Don't exit in this case. */
 			if (rc && rc != -ENOLCK) {
-				DeleteMidQEntry(midQ);
+				delete_mid(midQ);
 				return rc;
 			}
 		}
@@ -951,7 +958,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
-		DeleteMidQEntry(midQ);
+		delete_mid(midQ);
 		return rc;
 	}
 
@@ -1001,7 +1008,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 		BCC(out_buf) = le16_to_cpu(BCC_LE(out_buf));
 
 out:
-	DeleteMidQEntry(midQ);
+	delete_mid(midQ);
 	if (rstart && rc == -EACCES)
 		return -ERESTARTSYS;
 	return rc;
-- 
1.7.3.3

[PATCH 06/18] 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 |  121 ++++++++++++++++++---------------------------------
 1 files changed, 43 insertions(+), 78 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 801726b..15059c7 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -389,6 +389,42 @@ SendReceiveNoRsp(const unsigned int xid, struct cifsSesInfo *ses,
 	return rc;
 }
 
+static int
+sync_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);
+
+	delete_mid(mid);
+	return rc;
+}
+
 int
 SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	     struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
@@ -492,37 +528,13 @@ 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);
-		delete_mid(midQ);
-		/* Update # of requests on wire to server */
+	rc = sync_mid_result(midQ, ses->server);
+	if (rc != 0) {
 		atomic_dec(&ses->server->inFlight);
 		wake_up(&ses->server->request_q);
 		return rc;
 	}
 
-	spin_unlock(&GlobalMid_Lock);
 	receive_len = midQ->resp_buf->smb_buf_length;
 
 	if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
@@ -684,36 +696,13 @@ 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);
-		delete_mid(midQ);
-		/* Update # of requests on wire to server */
+	rc = sync_mid_result(midQ, ses->server);
+	if (rc != 0) {
 		atomic_dec(&ses->server->inFlight);
 		wake_up(&ses->server->request_q);
 		return rc;
 	}
 
-	spin_unlock(&GlobalMid_Lock);
 	receive_len = midQ->resp_buf->smb_buf_length;
 
 	if (receive_len > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE) {
@@ -933,35 +922,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);
-		delete_mid(midQ);
+	rc = sync_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.3

[PATCH 07/18] 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 15059c7..c41c9c4 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -318,48 +318,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;
 }
 
 
@@ -433,7 +402,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;
 
@@ -500,33 +468,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 = sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
@@ -604,7 +551,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) {
@@ -668,33 +614,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 = sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
@@ -915,8 +840,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.3

[PATCH 08/18] 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 c41c9c4..f65cdec 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -374,10 +374,8 @@ sync_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.3

[PATCH 09/18] cifs: clean up sync_mid_result

[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 |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index f65cdec..6abd144 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -363,28 +363,29 @@ sync_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;
 		}
+	case MID_RETRY_NEEDED:
+		rc = -EAGAIN;
+		break;
+	default:
+		cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
+			mid->mid, mid->midState);
+		rc = -EIO;
 	}
 	spin_unlock(&GlobalMid_Lock);
 
-- 
1.7.3.3

[PATCH 10/18] 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   |   15 ++++++++++++-
 fs/cifs/connect.c    |   56 +++++++++++++++++++++++++-------------------------
 fs/cifs/transport.c  |   19 +++++++++++++++-
 4 files changed, 63 insertions(+), 35 deletions(-)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index ac7a38f..30d01bc 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 8c3c165..db90fe0 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -508,6 +508,18 @@ static inline void cifs_stats_bytes_read(struct cifsTconInfo *tcon,
 
 #endif
 
+struct mid_q_entry;
+
+/*
+ * This is the prototype for the mid callback function. When creating one,
+ * take special care to avoid deadlocks. Things to bear in mind:
+ *
+ * - it will be called by cifsd
+ * - the GlobalMid_Lock will be held
+ * - the mid will be removed from the pending_mid_q list
+ */
+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 */
@@ -519,7 +531,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 515cafa..734fb09 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -152,6 +152,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 +164,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 +183,22 @@ 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: issuing mid callbacks", __func__);
 	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 */
+	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)
 			mid_entry->midState = MID_RETRY_NEEDED;
-		}
+		list_del_init(&mid_entry->qhead);
+		mid_entry->callback(mid_entry);
 	}
 	spin_unlock(&GlobalMid_Lock);
-	mutex_unlock(&server->srv_mutex);
 
 	while ((server->tcpStatus != CifsExiting) &&
 	       (server->tcpStatus != CifsGood)) {
@@ -212,10 +215,9 @@ 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);
 		}
 	}
+
 	return rc;
 }
 
@@ -345,7 +347,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	struct msghdr smb_msg;
 	struct kvec iov;
 	struct socket *csocket = server->ssocket;
-	struct list_head *tmp;
+	struct list_head *tmp, *tmp2;
 	struct task_struct *task_to_wake = NULL;
 	struct mid_q_entry *mid_entry;
 	char temp;
@@ -558,10 +560,9 @@ incomplete_rcv:
 			continue;
 		}
 
-
-		task_to_wake = NULL;
+		mid_entry = NULL;
 		spin_lock(&GlobalMid_Lock);
-		list_for_each(tmp, &server->pending_mid_q) {
+		list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
 
 			if ((mid_entry->mid == smb_buffer->Mid) &&
@@ -602,8 +603,9 @@ 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;
+				list_del_init(&mid_entry->qhead);
+				mid_entry->callback(mid_entry);
 #ifdef CONFIG_CIFS_STATS2
 				mid_entry->when_received = jiffies;
 #endif
@@ -613,9 +615,11 @@ multi_t2_fnd:
 				server->lstrp = jiffies;
 				break;
 			}
+			mid_entry = NULL;
 		}
 		spin_unlock(&GlobalMid_Lock);
-		if (task_to_wake) {
+
+		if (mid_entry != NULL) {
 			/* Was previous buf put in mpx struct for multi-rsp? */
 			if (!isMultiRsp) {
 				/* smb buffer will be freed by user thread */
@@ -624,7 +628,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! "
@@ -678,15 +681,12 @@ multi_t2_fnd:
 
 	if (!list_empty(&server->pending_mid_q)) {
 		spin_lock(&GlobalMid_Lock);
-		list_for_each(tmp, &server->pending_mid_q) {
+		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) {
-				cFYI(1, "Clearing Mid 0x%x - waking up ",
-					mid_entry->mid);
-				task_to_wake = mid_entry->tsk;
-				if (task_to_wake)
-					wake_up_process(task_to_wake);
-			}
+			cFYI(1, "Clearing Mid 0x%x - issuing callback",
+					 mid_entry->mid);
+			list_del_init(&mid_entry->qhead);
+			mid_entry->callback(mid_entry);
 		}
 		spin_unlock(&GlobalMid_Lock);
 		/* 1/8th of sec is more than enough time for them to exit */
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 6abd144..d77b615 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;
 	}
 
 	atomic_inc(&midCount);
@@ -367,6 +379,9 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 		mid->mid, mid->midState);
 
 	spin_lock(&GlobalMid_Lock);
+	/* ensure that it's no longer on the pending_mid_q */
+	list_del_init(&mid->qhead);
+
 	switch (mid->midState) {
 	case MID_RESPONSE_RECEIVED:
 		spin_unlock(&GlobalMid_Lock);
@@ -389,7 +404,7 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 	}
 	spin_unlock(&GlobalMid_Lock);
 
-	delete_mid(mid);
+	DeleteMidQEntry(mid);
 	return rc;
 }
 
-- 
1.7.3.3

[PATCH 11/18] 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/transport.c |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d77b615..ad78cbd 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -486,8 +486,17 @@ 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) {
+		spin_lock(&GlobalMid_Lock);
+		if (midQ->midState == MID_REQUEST_SUBMITTED) {
+			midQ->callback = DeleteMidQEntry;
+			spin_unlock(&GlobalMid_Lock);
+		        atomic_dec(&ses->server->inFlight);
+			wake_up(&ses->server->request_q);
+			return rc;
+		}
+		spin_unlock(&GlobalMid_Lock);
+	}
 
 	rc = sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
@@ -632,8 +641,18 @@ 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) {
+		spin_lock(&GlobalMid_Lock);
+		if (midQ->midState == MID_REQUEST_SUBMITTED) {
+			/* no longer considered to be "in-flight" */
+			midQ->callback = DeleteMidQEntry;
+			spin_unlock(&GlobalMid_Lock);
+		        atomic_dec(&ses->server->inFlight);
+			wake_up(&ses->server->request_q);
+			return rc;
+		}
+		spin_unlock(&GlobalMid_Lock);
+	}
 
 	rc = sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
@@ -854,10 +873,20 @@ 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) {
+			spin_lock(&GlobalMid_Lock);
+			if (midQ->midState == MID_REQUEST_SUBMITTED) {
+				/* no longer considered to be "in-flight" */
+				midQ->callback = DeleteMidQEntry;
+				spin_unlock(&GlobalMid_Lock);
+				return rc;
+			}
+			spin_unlock(&GlobalMid_Lock);
 		}
+
+		/* We got the response - restart system call. */
+		rstart = 1;
 	}
 
 	rc = sync_mid_result(midQ, ses->server);
-- 
1.7.3.3

[PATCH 12/18] 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 |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index fe77e69..4d81f0e 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -61,6 +61,11 @@ 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 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 ad78cbd..cf8c64d 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;
@@ -345,6 +345,62 @@ 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;
+	}
+
+	/* put it on the pending_mid_q */
+	spin_lock(&GlobalMid_Lock);
+	list_add_tail(&mid->qhead, &server->pending_mid_q);
+	spin_unlock(&GlobalMid_Lock);
+
+	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:
+	delete_mid(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.3

[PATCH 13/18] 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 |    2 ++
 fs/cifs/cifssmb.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/transport.c |    2 +-
 4 files changed, 65 insertions(+), 1 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 4d81f0e..3cd6474 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -63,6 +63,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
 /* 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);
@@ -352,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)
 {
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index cf8c64d..db29f80 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -78,7 +78,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
-- 
1.7.3.3

[PATCH 14/18] 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 db90fe0..64d69f9 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -218,6 +218,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 734fb09..20f6eda 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 (60 * HZ)
+
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);
 
@@ -336,6 +339,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)
 {
@@ -1571,6 +1594,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);
@@ -1662,6 +1687,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
@@ -1710,6 +1736,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.3

[PATCH 15/18] 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 20f6eda..2ad3c67 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -55,6 +55,9 @@
 /* SMB echo "timeout" -- FIXME: tunable? */
 #define SMB_ECHO_INTERVAL (60 * HZ)
 
+/* reconnect if no response from server in this time period */
+#define UNRESPONSIVE_SERVER_TIMEOUT (5 * SMB_ECHO_INTERVAL)
+
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);
 
@@ -186,6 +189,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);
 
 	/*
@@ -423,7 +427,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? */);
@@ -584,6 +600,8 @@ incomplete_rcv:
 		}
 
 		mid_entry = NULL;
+		server->lstrp = jiffies;
+
 		spin_lock(&GlobalMid_Lock);
 		list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
 			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
@@ -632,10 +650,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;
 			}
 			mid_entry = NULL;
@@ -1685,6 +1699,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.3

[PATCH 16/18] 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 64d69f9..d96f186 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -636,12 +636,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 2ad3c67..0c257d6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3024,7 +3024,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 400c948..0490f53 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -839,29 +839,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,
@@ -882,7 +859,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);
 
@@ -903,7 +880,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;
@@ -931,7 +907,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)
@@ -944,8 +920,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);
@@ -974,7 +948,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);
 
@@ -987,7 +961,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;
@@ -1017,7 +990,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,
@@ -1025,7 +998,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)
@@ -1038,8 +1011,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);
@@ -1239,7 +1210,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);
 
@@ -1384,11 +1355,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);
 			}
 
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 db29f80..f769047 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -817,7 +817,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.3

[PATCH 17/18] cifs: mangle existing header for SMB_COM_NT_CANCEL

[Jeff Layton]

The NT_CANCEL command looks just like the original command, except for a
few small differences. The send_nt_cancel function however currently takes
a tcon, which we don't have in SendReceive and SendReceive2.

Instead of "respinning" the entire header for an NT_CANCEL, just mangle
the existing header by replacing just the fields we need. This means we
don't need a tcon and allows us to call it from other places.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index f769047..1488b7e 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -464,6 +464,43 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 	return rc;
 }
 
+/*
+ * An NT cancel request header looks just like the original request except:
+ *
+ * The Command is SMB_COM_NT_CANCEL
+ * The WordCount is zeroed out
+ * The ByteCount is zeroed out
+ *
+ * This function mangles an existing request buffer into a
+ * SMB_COM_NT_CANCEL request and then sends it.
+ */
+static int
+send_nt_cancel(struct TCP_Server_Info *server, struct smb_hdr *in_buf,
+		struct mid_q_entry *mid)
+{
+	int rc = 0;
+
+	/* -4 for RFC1001 length and +2 for BCC field */
+	in_buf->smb_buf_length = sizeof(struct smb_hdr) - 4  + 2;
+	in_buf->Command = SMB_COM_NT_CANCEL;
+	in_buf->WordCount = 0;
+	BCC_LE(in_buf) = 0;
+
+	mutex_lock(&server->srv_mutex);
+	rc = cifs_sign_smb(in_buf, server, &mid->sequence_number);
+	if (rc) {
+		mutex_unlock(&server->srv_mutex);
+		return rc;
+	}
+	rc = smb_send(server, in_buf, in_buf->smb_buf_length);
+	mutex_unlock(&server->srv_mutex);
+
+	cFYI(1, "issued NT_CANCEL for mid %u, rc = %d",
+		in_buf->Mid, rc);
+
+	return rc;
+}
+
 int
 SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 	     struct kvec *iov, int n_vec, int *pRespBufType /* ret */,
@@ -772,29 +809,6 @@ out:
 	return rc;
 }
 
-/* Send an NT_CANCEL SMB to cause the POSIX blocking lock to return. */
-
-static int
-send_nt_cancel(struct cifsTconInfo *tcon, struct smb_hdr *in_buf,
-		struct mid_q_entry *midQ)
-{
-	int rc = 0;
-	struct cifsSesInfo *ses = tcon->ses;
-	__u16 mid = in_buf->Mid;
-
-	header_assemble(in_buf, SMB_COM_NT_CANCEL, tcon, 0);
-	in_buf->Mid = mid;
-	mutex_lock(&ses->server->srv_mutex);
-	rc = cifs_sign_smb(in_buf, ses->server, &midQ->sequence_number);
-	if (rc) {
-		mutex_unlock(&ses->server->srv_mutex);
-		return rc;
-	}
-	rc = smb_send(ses->server, in_buf, in_buf->smb_buf_length);
-	mutex_unlock(&ses->server->srv_mutex);
-	return rc;
-}
-
 /* We send a LOCKINGX_CANCEL_LOCK to cause the Windows
    blocking lock to return. */
 
@@ -909,8 +923,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 		if (in_buf->Command == SMB_COM_TRANSACTION2) {
 			/* POSIX lock. We send a NT_CANCEL SMB to cause the
 			   blocking lock to return. */
-
-			rc = send_nt_cancel(tcon, in_buf, midQ);
+			rc = send_nt_cancel(ses->server, in_buf, midQ);
 			if (rc) {
 				delete_mid(midQ);
 				return rc;
-- 
1.7.3.3

[PATCH 18/18] cifs: send an NT_CANCEL request when a process is signalled

[Jeff Layton]

Use the new send_nt_cancel function to send an NT_CANCEL when the
process is delivered a fatal signal. This is a "best effort" enterprise
however, so don't bother to check the return code. There's nothing we
can reasonably do if it fails anyway.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 1488b7e..e2e66f3 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -570,20 +570,25 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 #endif
 
 	mutex_unlock(&ses->server->srv_mutex);
-	cifs_small_buf_release(in_buf);
 
-	if (rc < 0)
+	if (rc < 0) {
+		cifs_small_buf_release(in_buf);
 		goto out;
+	}
 
-	if (long_op == CIFS_ASYNC_OP)
+	if (long_op == CIFS_ASYNC_OP) {
+		cifs_small_buf_release(in_buf);
 		goto out;
+	}
 
 	rc = wait_for_response(ses->server, midQ);
 	if (rc != 0) {
+		send_nt_cancel(ses->server, in_buf, midQ);
 		spin_lock(&GlobalMid_Lock);
 		if (midQ->midState == MID_REQUEST_SUBMITTED) {
 			midQ->callback = DeleteMidQEntry;
 			spin_unlock(&GlobalMid_Lock);
+			cifs_small_buf_release(in_buf);
 		        atomic_dec(&ses->server->inFlight);
 			wake_up(&ses->server->request_q);
 			return rc;
@@ -591,6 +596,8 @@ SendReceive2(const unsigned int xid, struct cifsSesInfo *ses,
 		spin_unlock(&GlobalMid_Lock);
 	}
 
+	cifs_small_buf_release(in_buf);
+
 	rc = sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
 		atomic_dec(&ses->server->inFlight);
@@ -735,6 +742,7 @@ SendReceive(const unsigned int xid, struct cifsSesInfo *ses,
 
 	rc = wait_for_response(ses->server, midQ);
 	if (rc != 0) {
+		send_nt_cancel(ses->server, in_buf, midQ);
 		spin_lock(&GlobalMid_Lock);
 		if (midQ->midState == MID_REQUEST_SUBMITTED) {
 			/* no longer considered to be "in-flight" */
@@ -944,6 +952,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifsTconInfo *tcon,
 
 		rc = wait_for_response(ses->server, midQ);
 		if (rc) {
+			send_nt_cancel(ses->server, in_buf, midQ);
 			spin_lock(&GlobalMid_Lock);
 			if (midQ->midState == MID_REQUEST_SUBMITTED) {
 				/* no longer considered to be "in-flight" */
-- 
1.7.3.3

Re: [PATCH 04/18] cifs: clean up accesses to midCount

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> It's an atomic_t and the code accesses the "counter" field in it directly
> instead of using atomic_read(). It also is sometimes accessed under a
> spinlock and sometimes not. Move it out of the spinlock since we don't need
> belt-and-suspenders for something that's just informational.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifs_debug.c |    2 +-
>  fs/cifs/connect.c    |    2 +-
>  fs/cifs/transport.c  |    6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> The TCP_Server_Info is refcounted and every SMB session holds a
> reference to it. Thus, smb_ses_list is always going to be empty when
> cifsd is coming down. This is dead code.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   44 +++-----------------------------------------
>  1 files changed, 3 insertions(+), 41 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 03/18] cifs: make wait_for_free_request take a TCP_Server_Info pointer

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

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

[Suresh Jayaraman]

On 12/17/2010 08:38 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.
> 
> 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 |   49 +++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 37 insertions(+), 12 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 05/18] cifs: move locked sections out of DeleteMidQEntry and AllocMidQEntry

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> In later patches, we're going to need to have finer-grained control
> over the addition and removal of these structs from the pending_mid_q
> and we'll need to be able to call the destructor while holding the
> spinlock. Move the locked sections out of both routines and into
> the callers. Fix up current callers of DeleteMidQEntry to call a new
> routine that dequeues the entry and then destroys it.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   41 ++++++++++++++++++++++++-----------------
>  1 files changed, 24 insertions(+), 17 deletions(-)
> 
Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 06/18] cifs: move mid result processing into common function

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |  121 ++++++++++++++++++---------------------------------
>  1 files changed, 43 insertions(+), 78 deletions(-)
> 

Nice cleanup.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

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

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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 c41c9c4..f65cdec 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -374,10 +374,8 @@ sync_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) {

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 09/18] cifs: clean up sync_mid_result

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton 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 |   35 ++++++++++++++++++-----------------
>  1 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index f65cdec..6abd144 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -363,28 +363,29 @@ sync_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;
> -	}

When midState is MID_REQUEST_SUBMITTED and tcpStatus is not CifsExiting,
the old code used to set midState to MID_RETRY_NEEDED. This is not set
now - is this intentional?

> -	if (rc != -EHOSTDOWN) {
> -		if (mid->midState == MID_RETRY_NEEDED) {
> -			rc = -EAGAIN;
> -			cFYI(1, "marking request for retry");
> -		} else {
> -			rc = -EIO;
> +			break;
>  		}
> +	case MID_RETRY_NEEDED:
> +		rc = -EAGAIN;
> +		break;
> +	default:
> +		cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
> +			mid->mid, mid->midState);
> +		rc = -EIO;
>  	}
>  	spin_unlock(&GlobalMid_Lock);
>  


-- 
Suresh Jayaraman

Re: [PATCH 09/18] cifs: clean up sync_mid_result

[Jeff Layton]

On Mon, 20 Dec 2010 16:57:41 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/17/2010 08:38 PM, Jeff Layton 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 |   35 ++++++++++++++++++-----------------
> >  1 files changed, 18 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index f65cdec..6abd144 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -363,28 +363,29 @@ sync_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;
> > -	}
> 
> When midState is MID_REQUEST_SUBMITTED and tcpStatus is not CifsExiting,
> the old code used to set midState to MID_RETRY_NEEDED. This is not set
> now - is this intentional?
> 

I don't think it matters much. wait_for_response does this:

        error = wait_event_killable(server->response_q,
                                    midQ->midState != MID_REQUEST_SUBMITTED);

...so you can only get out of that wait with MID_REQUEST_SUBMITTED by
fatal signal. At that point, the caller is going to do send the cancel
and return without calling handle_mid_result.

So TBH, I think that this MID_REQUEST_SUBMITTED clause is going to be
unused, but we could reset the value to MID_RETRY_NEEDED if we think
it'll be better for future-proofness.


> > -	if (rc != -EHOSTDOWN) {
> > -		if (mid->midState == MID_RETRY_NEEDED) {
> > -			rc = -EAGAIN;
> > -			cFYI(1, "marking request for retry");
> > -		} else {
> > -			rc = -EIO;
> > +			break;
> >  		}
> > +	case MID_RETRY_NEEDED:
> > +		rc = -EAGAIN;
> > +		break;
> > +	default:
> > +		cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__,
> > +			mid->mid, mid->midState);
> > +		rc = -EIO;
> >  	}
> >  	spin_unlock(&GlobalMid_Lock);
> >  
> 
> 


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

Re: [PATCH 09/18] cifs: clean up sync_mid_result

[Suresh Jayaraman]

On 12/20/2010 06:34 PM, Jeff Layton wrote:
> On Mon, 20 Dec 2010 16:57:41 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 12/17/2010 08:38 PM, Jeff Layton 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 |   35 ++++++++++++++++++-----------------
>>>  1 files changed, 18 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> index f65cdec..6abd144 100644
>>> --- a/fs/cifs/transport.c
>>> +++ b/fs/cifs/transport.c
>>> @@ -363,28 +363,29 @@ sync_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;
>>> -	}
>>
>> When midState is MID_REQUEST_SUBMITTED and tcpStatus is not CifsExiting,
>> the old code used to set midState to MID_RETRY_NEEDED. This is not set
>> now - is this intentional?
>>
> 
> I don't think it matters much. wait_for_response does this:
> 
>         error = wait_event_killable(server->response_q,
>                                     midQ->midState != MID_REQUEST_SUBMITTED);
> 
> ...so you can only get out of that wait with MID_REQUEST_SUBMITTED by
> fatal signal. At that point, the caller is going to do send the cancel
> and return without calling handle_mid_result.
> 
> So TBH, I think that this MID_REQUEST_SUBMITTED clause is going to be
> unused, but we could reset the value to MID_RETRY_NEEDED if we think
> it'll be better for future-proofness.
> 

I don't think we'll need it. But, if Steve or others see any need, it's
fine to keep it as well.


-- 
Suresh Jayaraman

Re: [PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> The TCP_Server_Info is refcounted and every SMB session holds a
> reference to it. Thus, smb_ses_list is always going to be empty when
> cifsd is coming down. This is dead code.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |   44 +++-----------------------------------------
>  1 files changed, 3 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9fe620e2..67d3d0d 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -346,7 +346,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>  	struct kvec iov;
>  	struct socket *csocket = server->ssocket;
>  	struct list_head *tmp;
> -	struct cifsSesInfo *ses;
>  	struct task_struct *task_to_wake = NULL;
>  	struct mid_q_entry *mid_entry;
>  	char temp;
> @@ -677,44 +676,19 @@ multi_t2_fnd:
>  	if (smallbuf) /* no sense logging a debug message if NULL */
>  		cifs_small_buf_release(smallbuf);
>  
> -	/*
> -	 * BB: we shouldn't have to do any of this. It shouldn't be
> -	 * possible to exit from the thread with active SMB sessions
> -	 */
> -	spin_lock(&cifs_tcp_ses_lock);
> -	if (list_empty(&server->pending_mid_q)) {
> -		/* loop through server session structures attached to this and
> -		    mark them dead */
> -		list_for_each(tmp, &server->smb_ses_list) {
> -			ses = list_entry(tmp, struct cifsSesInfo,
> -					 smb_ses_list);
> -			ses->status = CifsExiting;
> -			ses->server = NULL;
> -		}
> -		spin_unlock(&cifs_tcp_ses_lock);
> -	} else {
> -		/* 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 */
> -		list_for_each(tmp, &server->smb_ses_list) {
> -			ses = list_entry(tmp, struct cifsSesInfo,
> -					 smb_ses_list);
> -			ses->status = CifsExiting;
> -		}
> -
> +	if (!list_empty(&server->pending_mid_q)) {
>  		spin_lock(&GlobalMid_Lock);
>  		list_for_each(tmp, &server->pending_mid_q) {
> -		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +			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 ",
> -					 mid_entry->mid);
> +					mid_entry->mid);
>  				task_to_wake = mid_entry->tsk;
>  				if (task_to_wake)
>  					wake_up_process(task_to_wake);
>  			}
>  		}
>  		spin_unlock(&GlobalMid_Lock);
> -		spin_unlock(&cifs_tcp_ses_lock);
>  		/* 1/8th of sec is more than enough time for them to exit */
>  		msleep(125);
>  	}
> @@ -732,18 +706,6 @@ multi_t2_fnd:
>  		coming home not much else we can do but free the memory */
>  	}
>  
> -	/* last chance to mark ses pointers invalid
> -	if there are any pointing to this (e.g
> -	if a crazy root user tried to kill cifsd
> -	kernel thread explicitly this might happen) */

The above comment sounds little scary. Though, I don't see how this
could happen, I think it's better to test the above scenario to be sure
(I guess you would have already taken care?)

> -	/* BB: This shouldn't be necessary, see above */
> -	spin_lock(&cifs_tcp_ses_lock);
> -	list_for_each(tmp, &server->smb_ses_list) {
> -		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> -		ses->server = NULL;
> -	}
> -	spin_unlock(&cifs_tcp_ses_lock);
> -
>  	kfree(server->hostname);
>  	task_to_wake = xchg(&server->tsk, NULL);
>  	kfree(server);


-- 
Suresh Jayaraman

Re: [PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Jeff Layton]

On Mon, 20 Dec 2010 22:08:38 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/17/2010 08:38 PM, Jeff Layton wrote:
> > The TCP_Server_Info is refcounted and every SMB session holds a
> > reference to it. Thus, smb_ses_list is always going to be empty when
> > cifsd is coming down. This is dead code.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/connect.c |   44 +++-----------------------------------------
> >  1 files changed, 3 insertions(+), 41 deletions(-)
> > 
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 9fe620e2..67d3d0d 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -346,7 +346,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
> >  	struct kvec iov;
> >  	struct socket *csocket = server->ssocket;
> >  	struct list_head *tmp;
> > -	struct cifsSesInfo *ses;
> >  	struct task_struct *task_to_wake = NULL;
> >  	struct mid_q_entry *mid_entry;
> >  	char temp;
> > @@ -677,44 +676,19 @@ multi_t2_fnd:
> >  	if (smallbuf) /* no sense logging a debug message if NULL */
> >  		cifs_small_buf_release(smallbuf);
> >  
> > -	/*
> > -	 * BB: we shouldn't have to do any of this. It shouldn't be
> > -	 * possible to exit from the thread with active SMB sessions
> > -	 */
> > -	spin_lock(&cifs_tcp_ses_lock);
> > -	if (list_empty(&server->pending_mid_q)) {
> > -		/* loop through server session structures attached to this and
> > -		    mark them dead */
> > -		list_for_each(tmp, &server->smb_ses_list) {
> > -			ses = list_entry(tmp, struct cifsSesInfo,
> > -					 smb_ses_list);
> > -			ses->status = CifsExiting;
> > -			ses->server = NULL;
> > -		}
> > -		spin_unlock(&cifs_tcp_ses_lock);
> > -	} else {
> > -		/* 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 */
> > -		list_for_each(tmp, &server->smb_ses_list) {
> > -			ses = list_entry(tmp, struct cifsSesInfo,
> > -					 smb_ses_list);
> > -			ses->status = CifsExiting;
> > -		}
> > -
> > +	if (!list_empty(&server->pending_mid_q)) {
> >  		spin_lock(&GlobalMid_Lock);
> >  		list_for_each(tmp, &server->pending_mid_q) {
> > -		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> > +			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 ",
> > -					 mid_entry->mid);
> > +					mid_entry->mid);
> >  				task_to_wake = mid_entry->tsk;
> >  				if (task_to_wake)
> >  					wake_up_process(task_to_wake);
> >  			}
> >  		}
> >  		spin_unlock(&GlobalMid_Lock);
> > -		spin_unlock(&cifs_tcp_ses_lock);
> >  		/* 1/8th of sec is more than enough time for them to exit */
> >  		msleep(125);
> >  	}
> > @@ -732,18 +706,6 @@ multi_t2_fnd:
> >  		coming home not much else we can do but free the memory */
> >  	}
> >  
> > -	/* last chance to mark ses pointers invalid
> > -	if there are any pointing to this (e.g
> > -	if a crazy root user tried to kill cifsd
> > -	kernel thread explicitly this might happen) */
> 
> The above comment sounds little scary. Though, I don't see how this
> could happen, I think it's better to test the above scenario to be sure
> (I guess you would have already taken care?)
> 
> > -	/* BB: This shouldn't be necessary, see above */
> > -	spin_lock(&cifs_tcp_ses_lock);
> > -	list_for_each(tmp, &server->smb_ses_list) {
> > -		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> > -		ses->server = NULL;
> > -	}
> > -	spin_unlock(&cifs_tcp_ses_lock);
> > -
> >  	kfree(server->hostname);
> >  	task_to_wake = xchg(&server->tsk, NULL);
> >  	kfree(server);
> 
> 

That comment is a holdover from earlier, broken code that did not do
proper refcounting (see the changes in 2008 near commit e7ddee90 that
fixed this). You'll note that I added this comment around the same
time:

-       /*
-        * BB: we shouldn't have to do any of this. It shouldn't be
-        * possible to exit from the thread with active SMB sessions
-        */

...but I never got around to actually ripping out that bogus code.

Today, the only way to take down cifs_demultiplex_thread is for the
refcount (server->srv_count) to go to zero. The model is for each
cifsSesInfo struct to hold a single reference to a TCP_Server_Info. So,
if there are still entries on the server->smb_ses_list while the thread
is coming down, then there is something very, very wrong.

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

Re: [PATCH 10/18] cifs: allow for different handling of received response

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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   |   15 ++++++++++++-
>  fs/cifs/connect.c    |   56 +++++++++++++++++++++++++-------------------------
>  fs/cifs/transport.c  |   19 +++++++++++++++-
>  4 files changed, 63 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index ac7a38f..30d01bc 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 8c3c165..db90fe0 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -508,6 +508,18 @@ static inline void cifs_stats_bytes_read(struct cifsTconInfo *tcon,
>  
>  #endif
>  
> +struct mid_q_entry;
> +
> +/*
> + * This is the prototype for the mid callback function. When creating one,
> + * take special care to avoid deadlocks. Things to bear in mind:
> + *
> + * - it will be called by cifsd
> + * - the GlobalMid_Lock will be held
> + * - the mid will be removed from the pending_mid_q list
> + */
> +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 */
> @@ -519,7 +531,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 515cafa..734fb09 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -152,6 +152,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 +164,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 +183,22 @@ 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: issuing mid callbacks", __func__);

Is the above comment valid? This patchset doesn't use a separate list IIUC?


-- 
Suresh Jayaraman

Re: [PATCH 07/18] cifs: wait indefinitely for responses

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 10/18] cifs: allow for different handling of received response

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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   |   15 ++++++++++++-
>  fs/cifs/connect.c    |   56 +++++++++++++++++++++++++-------------------------
>  fs/cifs/transport.c  |   19 +++++++++++++++-
>  4 files changed, 63 insertions(+), 35 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 12/18] cifs: add cifs_call_async

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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 |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 62 insertions(+), 1 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

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

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifspdu.h   |   15 +++++++++++++++
>  fs/cifs/cifsproto.h |    2 ++
>  fs/cifs/cifssmb.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/transport.c |    2 +-
>  4 files changed, 65 insertions(+), 1 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

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

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 10/18] cifs: allow for different handling of received response

[Jeff Layton]

On Tue, 21 Dec 2010 21:20:43 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/17/2010 08:38 PM, Jeff Layton wrote:
> > 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   |   15 ++++++++++++-
> >  fs/cifs/connect.c    |   56 +++++++++++++++++++++++++-------------------------
> >  fs/cifs/transport.c  |   19 +++++++++++++++-
> >  4 files changed, 63 insertions(+), 35 deletions(-)
> > 
> > diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> > index ac7a38f..30d01bc 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 8c3c165..db90fe0 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -508,6 +508,18 @@ static inline void cifs_stats_bytes_read(struct cifsTconInfo *tcon,
> >  
> >  #endif
> >  
> > +struct mid_q_entry;
> > +
> > +/*
> > + * This is the prototype for the mid callback function. When creating one,
> > + * take special care to avoid deadlocks. Things to bear in mind:
> > + *
> > + * - it will be called by cifsd
> > + * - the GlobalMid_Lock will be held
> > + * - the mid will be removed from the pending_mid_q list
> > + */
> > +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 */
> > @@ -519,7 +531,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 515cafa..734fb09 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -152,6 +152,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 +164,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 +183,22 @@ 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: issuing mid callbacks", __func__);
> 
> Is the above comment valid? This patchset doesn't use a separate list IIUC?
> 
> 

You're correct. I'll fix and resend.

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

Re: [PATCH 15/18] cifs: reconnect unresponsive servers

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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 20f6eda..2ad3c67 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -55,6 +55,9 @@
>  /* SMB echo "timeout" -- FIXME: tunable? */
>  #define SMB_ECHO_INTERVAL (60 * HZ)
>  
> +/* reconnect if no response from server in this time period */
> +#define UNRESPONSIVE_SERVER_TIMEOUT (5 * SMB_ECHO_INTERVAL)
> +

It's not clear to me why is this timeout is set to be
5 * SMB_ECHO_INTERVAL?

>  extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
>  			 unsigned char *p24);
>  
> @@ -186,6 +189,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);
>  
>  	/*
> @@ -423,7 +427,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? */);
> @@ -584,6 +600,8 @@ incomplete_rcv:
>  		}
>  
>  		mid_entry = NULL;
> +		server->lstrp = jiffies;
> +
>  		spin_lock(&GlobalMid_Lock);
>  		list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>  			mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> @@ -632,10 +650,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;
>  			}
>  			mid_entry = NULL;
> @@ -1685,6 +1699,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);


-- 
Suresh Jayaraman

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

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> 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(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 17/18] cifs: mangle existing header for SMB_COM_NT_CANCEL

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> The NT_CANCEL command looks just like the original command, except for a
> few small differences. The send_nt_cancel function however currently takes
> a tcon, which we don't have in SendReceive and SendReceive2.
> 
> Instead of "respinning" the entire header for an NT_CANCEL, just mangle
> the existing header by replacing just the fields we need. This means we
> don't need a tcon and allows us to call it from other places.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   63 ++++++++++++++++++++++++++++++--------------------
>  1 files changed, 38 insertions(+), 25 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 18/18] cifs: send an NT_CANCEL request when a process is signalled

[Suresh Jayaraman]

On 12/17/2010 08:38 PM, Jeff Layton wrote:
> Use the new send_nt_cancel function to send an NT_CANCEL when the
> process is delivered a fatal signal. This is a "best effort" enterprise
> however, so don't bother to check the return code. There's nothing we
> can reasonably do if it fails anyway.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/transport.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 15/18] cifs: reconnect unresponsive servers

[Jeff Layton]

On Thu, 23 Dec 2010 16:01:08 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/17/2010 08:38 PM, Jeff Layton wrote:
> > 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 20f6eda..2ad3c67 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -55,6 +55,9 @@
> >  /* SMB echo "timeout" -- FIXME: tunable? */
> >  #define SMB_ECHO_INTERVAL (60 * HZ)
> >  
> > +/* reconnect if no response from server in this time period */
> > +#define UNRESPONSIVE_SERVER_TIMEOUT (5 * SMB_ECHO_INTERVAL)
> > +
> 
> It's not clear to me why is this timeout is set to be
> 5 * SMB_ECHO_INTERVAL?
> 

My bad... I should have updated the patch description. When testing
this I experimented with a range of 2 to 5 times.

There is no "correct" value for this. It really comes down to a
judgement call. Reducing that value means that you'll return an error
to the caller more quickly on a soft mount timeout occurs. The downside
is that you'll issue reconnects more frequently when there's a temporary
network partition.

I tend to think that we're better off running with a larger value for
this (around 5*). Reconnects are terribly expensive and lock
reclaimation in Linux cifs is non-existent. Avoiding reconnects as much
as possible is preferable so making this as high as we can get away
with is a good thing.

That said, I'm open to suggestions on this. I think we shouldn't go
below 3*,  and above 5* is probably too much. We could eventually make
it tunable, but we still need a default. I don't think it should be a
mount option since it's a per-socket setting (mounts with different
timeouts wouldn't be able to share sockets), so maybe a module option
would be best.

Thoughts?

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

Re: [PATCH 15/18] cifs: reconnect unresponsive servers

[Suresh Jayaraman]

On 12/23/2010 06:50 PM, Jeff Layton wrote:
> On Thu, 23 Dec 2010 16:01:08 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 12/17/2010 08:38 PM, Jeff Layton wrote:
>>> 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 20f6eda..2ad3c67 100644
>>> --- a/fs/cifs/connect.c
>>> +++ b/fs/cifs/connect.c
>>> @@ -55,6 +55,9 @@
>>>  /* SMB echo "timeout" -- FIXME: tunable? */
>>>  #define SMB_ECHO_INTERVAL (60 * HZ)
>>>  
>>> +/* reconnect if no response from server in this time period */
>>> +#define UNRESPONSIVE_SERVER_TIMEOUT (5 * SMB_ECHO_INTERVAL)
>>> +
>>
>> It's not clear to me why is this timeout is set to be
>> 5 * SMB_ECHO_INTERVAL?
>>
> 
> My bad... I should have updated the patch description. When testing
> this I experimented with a range of 2 to 5 times.
> 

Ah, ok. Got confused a bit trying to match the code with patch description.

> There is no "correct" value for this. It really comes down to a
> judgement call. Reducing that value means that you'll return an error
> to the caller more quickly on a soft mount timeout occurs. The downside
> is that you'll issue reconnects more frequently when there's a temporary
> network partition.

> I tend to think that we're better off running with a larger value for
> this (around 5*). Reconnects are terribly expensive and lock
> reclaimation in Linux cifs is non-existent. Avoiding reconnects as much
> as possible is preferable so making this as high as we can get away
> with is a good thing.

Agreed.

> That said, I'm open to suggestions on this. I think we shouldn't go
> below 3*,  and above 5* is probably too much. We could eventually make
> it tunable, but we still need a default. I don't think it should be a
> mount option since it's a per-socket setting (mounts with different
> timeouts wouldn't be able to share sockets), so maybe a module option
> would be best.
> 

Sounds reasonable.


-- 
Suresh Jayaraman

[PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Jeff Layton]

The TCP_Server_Info is refcounted and every SMB session holds a
reference to it. Thus, smb_ses_list is always going to be empty when
cifsd is coming down. This is dead code.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Reviewed-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   44 +++-----------------------------------------
 1 files changed, 3 insertions(+), 41 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a65d311..d77e488 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -346,7 +346,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	struct kvec iov;
 	struct socket *csocket = server->ssocket;
 	struct list_head *tmp;
-	struct cifsSesInfo *ses;
 	struct task_struct *task_to_wake = NULL;
 	struct mid_q_entry *mid_entry;
 	char temp;
@@ -677,44 +676,19 @@ multi_t2_fnd:
 	if (smallbuf) /* no sense logging a debug message if NULL */
 		cifs_small_buf_release(smallbuf);
 
-	/*
-	 * BB: we shouldn't have to do any of this. It shouldn't be
-	 * possible to exit from the thread with active SMB sessions
-	 */
-	spin_lock(&cifs_tcp_ses_lock);
-	if (list_empty(&server->pending_mid_q)) {
-		/* loop through server session structures attached to this and
-		    mark them dead */
-		list_for_each(tmp, &server->smb_ses_list) {
-			ses = list_entry(tmp, struct cifsSesInfo,
-					 smb_ses_list);
-			ses->status = CifsExiting;
-			ses->server = NULL;
-		}
-		spin_unlock(&cifs_tcp_ses_lock);
-	} else {
-		/* 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 */
-		list_for_each(tmp, &server->smb_ses_list) {
-			ses = list_entry(tmp, struct cifsSesInfo,
-					 smb_ses_list);
-			ses->status = CifsExiting;
-		}
-
+	if (!list_empty(&server->pending_mid_q)) {
 		spin_lock(&GlobalMid_Lock);
 		list_for_each(tmp, &server->pending_mid_q) {
-		mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+			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 ",
-					 mid_entry->mid);
+					mid_entry->mid);
 				task_to_wake = mid_entry->tsk;
 				if (task_to_wake)
 					wake_up_process(task_to_wake);
 			}
 		}
 		spin_unlock(&GlobalMid_Lock);
-		spin_unlock(&cifs_tcp_ses_lock);
 		/* 1/8th of sec is more than enough time for them to exit */
 		msleep(125);
 	}
@@ -732,18 +706,6 @@ multi_t2_fnd:
 		coming home not much else we can do but free the memory */
 	}
 
-	/* last chance to mark ses pointers invalid
-	if there are any pointing to this (e.g
-	if a crazy root user tried to kill cifsd
-	kernel thread explicitly this might happen) */
-	/* BB: This shouldn't be necessary, see above */
-	spin_lock(&cifs_tcp_ses_lock);
-	list_for_each(tmp, &server->smb_ses_list) {
-		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
-		ses->server = NULL;
-	}
-	spin_unlock(&cifs_tcp_ses_lock);
-
 	kfree(server->hostname);
 	task_to_wake = xchg(&server->tsk, NULL);
 	kfree(server);
-- 
1.7.3.4