linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 4.20 38/42] cifs: Limit memory used by lock request calls to a page
       [not found] <20190209184734.125935-1-sashal@kernel.org>
@ 2019-02-09 18:47 ` Sasha Levin
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 39/42] CIFS: Fix credits calculation for cancelled requests Sasha Levin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Sasha Levin @ 2019-02-09 18:47 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Ross Lagerwall, Steve French, Sasha Levin, linux-cifs

From: Ross Lagerwall <ross.lagerwall@citrix.com>

[ Upstream commit 92a8109e4d3a34fb6b115c9098b51767dc933444 ]

The code tries to allocate a contiguous buffer with a size supplied by
the server (maxBuf). This could fail if memory is fragmented since it
results in high order allocations for commonly used server
implementations. It is also wasteful since there are probably
few locks in the usual case. Limit the buffer to be no larger than a
page to avoid memory allocation failures due to fragmentation.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/cifs/file.c     | 8 ++++++++
 fs/cifs/smb2file.c | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8431854b129f..116f8af0384f 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1139,6 +1139,10 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile)
 		return -EINVAL;
 	}
 
+	BUILD_BUG_ON(sizeof(struct smb_hdr) + sizeof(LOCKING_ANDX_RANGE) >
+		     PAGE_SIZE);
+	max_buf = min_t(unsigned int, max_buf - sizeof(struct smb_hdr),
+			PAGE_SIZE);
 	max_num = (max_buf - sizeof(struct smb_hdr)) /
 						sizeof(LOCKING_ANDX_RANGE);
 	buf = kcalloc(max_num, sizeof(LOCKING_ANDX_RANGE), GFP_KERNEL);
@@ -1477,6 +1481,10 @@ cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock,
 	if (max_buf < (sizeof(struct smb_hdr) + sizeof(LOCKING_ANDX_RANGE)))
 		return -EINVAL;
 
+	BUILD_BUG_ON(sizeof(struct smb_hdr) + sizeof(LOCKING_ANDX_RANGE) >
+		     PAGE_SIZE);
+	max_buf = min_t(unsigned int, max_buf - sizeof(struct smb_hdr),
+			PAGE_SIZE);
 	max_num = (max_buf - sizeof(struct smb_hdr)) /
 						sizeof(LOCKING_ANDX_RANGE);
 	buf = kcalloc(max_num, sizeof(LOCKING_ANDX_RANGE), GFP_KERNEL);
diff --git a/fs/cifs/smb2file.c b/fs/cifs/smb2file.c
index 2fc3d31967ee..b204e84b87fb 100644
--- a/fs/cifs/smb2file.c
+++ b/fs/cifs/smb2file.c
@@ -128,6 +128,8 @@ smb2_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock,
 	if (max_buf < sizeof(struct smb2_lock_element))
 		return -EINVAL;
 
+	BUILD_BUG_ON(sizeof(struct smb2_lock_element) > PAGE_SIZE);
+	max_buf = min_t(unsigned int, max_buf, PAGE_SIZE);
 	max_num = max_buf / sizeof(struct smb2_lock_element);
 	buf = kcalloc(max_num, sizeof(struct smb2_lock_element), GFP_KERNEL);
 	if (!buf)
@@ -264,6 +266,8 @@ smb2_push_mandatory_locks(struct cifsFileInfo *cfile)
 		return -EINVAL;
 	}
 
+	BUILD_BUG_ON(sizeof(struct smb2_lock_element) > PAGE_SIZE);
+	max_buf = min_t(unsigned int, max_buf, PAGE_SIZE);
 	max_num = max_buf / sizeof(struct smb2_lock_element);
 	buf = kcalloc(max_num, sizeof(struct smb2_lock_element), GFP_KERNEL);
 	if (!buf) {
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH AUTOSEL 4.20 39/42] CIFS: Fix credits calculation for cancelled requests
       [not found] <20190209184734.125935-1-sashal@kernel.org>
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 38/42] cifs: Limit memory used by lock request calls to a page Sasha Levin
@ 2019-02-09 18:47 ` Sasha Levin
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3 Sasha Levin
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 41/42] CIFS: Fix error paths in writeback code Sasha Levin
  3 siblings, 0 replies; 5+ messages in thread
From: Sasha Levin @ 2019-02-09 18:47 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Pavel Shilovsky, Steve French, Sasha Levin, linux-cifs

From: Pavel Shilovsky <pshilov@microsoft.com>

[ Upstream commit 8a26f0f781f56d3016b34a2217e346973d067e7b ]

If a request is cancelled, we can't assume that the server returns
1 credit back. Instead we need to wait for a response and process
the number of credits granted by the server.

Create a separate mid callback for cancelled request, parse the number
of credits in a response buffer and add them to the client's credits.
If the didn't get a response (no response buffer available) assume
0 credits granted. The latter most probably happens together with
session reconnect, so the client's credits are adjusted anyway.

Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/cifs/cifsglob.h  |  1 +
 fs/cifs/transport.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 38ab0fca49e1..7a4fae0dc566 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1426,6 +1426,7 @@ struct mid_q_entry {
 	int mid_state;	/* wish this were enum but can not pass to wait_event */
 	unsigned int mid_flags;
 	__le16 command;		/* smb command code */
+	unsigned int optype;	/* operation type */
 	bool large_buf:1;	/* if valid response, is pointer to large buf */
 	bool multiRsp:1;	/* multiple trans2 responses for one request  */
 	bool multiEnd:1;	/* both received */
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d51064c1ba42..4dbf62bb51b2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -785,6 +785,24 @@ cifs_noop_callback(struct mid_q_entry *mid)
 {
 }
 
+static void
+cifs_cancelled_callback(struct mid_q_entry *mid)
+{
+	struct TCP_Server_Info *server = mid->server;
+	unsigned int optype = mid->optype;
+	unsigned int credits_received = 0;
+
+	if (mid->mid_state == MID_RESPONSE_RECEIVED) {
+		if (mid->resp_buf)
+			credits_received = server->ops->get_credits(mid);
+		else
+			cifs_dbg(FYI, "Bad state for cancelled MID\n");
+	}
+
+	DeleteMidQEntry(mid);
+	add_credits(server, credits_received, optype);
+}
+
 int
 compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		   const int flags, const int num_rqst, struct smb_rqst *rqst,
@@ -860,6 +878,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		}
 
 		midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
+		midQ[i]->optype = optype;
 		/*
 		 * We don't invoke the callback compounds unless it is the last
 		 * request.
@@ -894,15 +913,20 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 
 	for (i = 0; i < num_rqst; i++) {
 		rc = wait_for_response(ses->server, midQ[i]);
-		if (rc != 0) {
+		if (rc != 0)
+			break;
+	}
+	if (rc != 0) {
+		for (; i < num_rqst; i++) {
 			cifs_dbg(VFS, "Cancelling wait for mid %llu cmd: %d\n",
 				 midQ[i]->mid, le16_to_cpu(midQ[i]->command));
 			send_cancel(ses->server, &rqst[i], midQ[i]);
 			spin_lock(&GlobalMid_Lock);
 			if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
 				midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
-				midQ[i]->callback = DeleteMidQEntry;
+				midQ[i]->callback = cifs_cancelled_callback;
 				cancelled_mid[i] = true;
+				credits[i] = 0;
 			}
 			spin_unlock(&GlobalMid_Lock);
 		}
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3
       [not found] <20190209184734.125935-1-sashal@kernel.org>
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 38/42] cifs: Limit memory used by lock request calls to a page Sasha Levin
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 39/42] CIFS: Fix credits calculation for cancelled requests Sasha Levin
@ 2019-02-09 18:47 ` Sasha Levin
  2019-02-12  1:48   ` Pavel Shilovskiy
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 41/42] CIFS: Fix error paths in writeback code Sasha Levin
  3 siblings, 1 reply; 5+ messages in thread
From: Sasha Levin @ 2019-02-09 18:47 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Pavel Shilovsky, Steve French, Sasha Levin, linux-cifs

From: Pavel Shilovsky <pshilov@microsoft.com>

[ Upstream commit ee258d79159afed52ca9372aeb9c1a51e89b32ee ]

Currently we account for credits in the thread initiating a request
and waiting for a response. The demultiplex thread receives the response,
wakes up the thread and the latter collects credits from the response
buffer and add them to the server structure on the client. This approach
is not accurate, because it may race with reconnect events in the
demultiplex thread which resets the number of credits.

Fix this by moving credit processing to new mid callbacks that collect
credits granted by the server from the response in the demultiplex thread.

Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/cifs/transport.c | 51 ++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 4dbf62bb51b2..0dab276eced8 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -781,12 +781,7 @@ cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
 }
 
 static void
-cifs_noop_callback(struct mid_q_entry *mid)
-{
-}
-
-static void
-cifs_cancelled_callback(struct mid_q_entry *mid)
+cifs_compound_callback(struct mid_q_entry *mid)
 {
 	struct TCP_Server_Info *server = mid->server;
 	unsigned int optype = mid->optype;
@@ -799,10 +794,23 @@ cifs_cancelled_callback(struct mid_q_entry *mid)
 			cifs_dbg(FYI, "Bad state for cancelled MID\n");
 	}
 
-	DeleteMidQEntry(mid);
 	add_credits(server, credits_received, optype);
 }
 
+static void
+cifs_compound_last_callback(struct mid_q_entry *mid)
+{
+	cifs_compound_callback(mid);
+	cifs_wake_up_task(mid);
+}
+
+static void
+cifs_cancelled_callback(struct mid_q_entry *mid)
+{
+	cifs_compound_callback(mid);
+	DeleteMidQEntry(mid);
+}
+
 int
 compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		   const int flags, const int num_rqst, struct smb_rqst *rqst,
@@ -880,11 +888,14 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
 		midQ[i]->optype = optype;
 		/*
-		 * We don't invoke the callback compounds unless it is the last
-		 * request.
+		 * Invoke callback for every part of the compound chain
+		 * to calculate credits properly. Wake up this thread only when
+		 * the last element is received.
 		 */
 		if (i < num_rqst - 1)
-			midQ[i]->callback = cifs_noop_callback;
+			midQ[i]->callback = cifs_compound_callback;
+		else
+			midQ[i]->callback = cifs_compound_last_callback;
 	}
 	cifs_in_send_inc(ses->server);
 	rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
@@ -898,8 +909,20 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 
 	mutex_unlock(&ses->server->srv_mutex);
 
-	if (rc < 0)
+	if (rc < 0) {
+		/* Sending failed for some reason - return credits back */
+		for (i = 0; i < num_rqst; i++)
+			add_credits(ses->server, credits[i], optype);
 		goto out;
+	}
+
+	/*
+	 * At this point the request is passed to the network stack - we assume
+	 * that any credits taken from the server structure on the client have
+	 * been spent and we can't return them back. Once we receive responses
+	 * we will collect credits granted by the server in the mid callbacks
+	 * and add those credits to the server structure.
+	 */
 
 	/*
 	 * Compounding is never used during session establish.
@@ -932,11 +955,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		}
 	}
 
-	for (i = 0; i < num_rqst; i++)
-		if (!cancelled_mid[i] && midQ[i]->resp_buf
-		    && (midQ[i]->mid_state == MID_RESPONSE_RECEIVED))
-			credits[i] = ses->server->ops->get_credits(midQ[i]);
-
 	for (i = 0; i < num_rqst; i++) {
 		if (rc < 0)
 			goto out;
@@ -995,7 +1013,6 @@ out:
 	for (i = 0; i < num_rqst; i++) {
 		if (!cancelled_mid[i])
 			cifs_delete_mid(midQ[i]);
-		add_credits(ses->server, credits[i], optype);
 	}
 
 	return rc;
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH AUTOSEL 4.20 41/42] CIFS: Fix error paths in writeback code
       [not found] <20190209184734.125935-1-sashal@kernel.org>
                   ` (2 preceding siblings ...)
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3 Sasha Levin
@ 2019-02-09 18:47 ` Sasha Levin
  3 siblings, 0 replies; 5+ messages in thread
From: Sasha Levin @ 2019-02-09 18:47 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Pavel Shilovsky, Steve French, Sasha Levin, linux-cifs

From: Pavel Shilovsky <pshilov@microsoft.com>

[ Upstream commit 9a66396f1857cc1de06f4f4771797315e1a4ea56 ]

This patch aims to address writeback code problems related to error
paths. In particular it respects EINTR and related error codes and
stores and returns the first error occurred during writeback.

Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Acked-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/cifs/cifsglob.h | 19 +++++++++++++++++++
 fs/cifs/cifssmb.c  |  7 ++++---
 fs/cifs/file.c     | 29 +++++++++++++++++++++++------
 fs/cifs/inode.c    | 10 ++++++++++
 4 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 7a4fae0dc566..373639199291 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1563,6 +1563,25 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 	kfree(param);
 }
 
+static inline bool is_interrupt_error(int error)
+{
+	switch (error) {
+	case -EINTR:
+	case -ERESTARTSYS:
+	case -ERESTARTNOHAND:
+	case -ERESTARTNOINTR:
+		return true;
+	}
+	return false;
+}
+
+static inline bool is_retryable_error(int error)
+{
+	if (is_interrupt_error(error) || error == -EAGAIN)
+		return true;
+	return false;
+}
+
 #define   MID_FREE 0
 #define   MID_REQUEST_ALLOCATED 1
 #define   MID_REQUEST_SUBMITTED 2
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index fce610f6cd24..327a101f7894 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2043,7 +2043,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata)
 
 		for (j = 0; j < nr_pages; j++) {
 			unlock_page(wdata2->pages[j]);
-			if (rc != 0 && rc != -EAGAIN) {
+			if (rc != 0 && !is_retryable_error(rc)) {
 				SetPageError(wdata2->pages[j]);
 				end_page_writeback(wdata2->pages[j]);
 				put_page(wdata2->pages[j]);
@@ -2052,7 +2052,7 @@ cifs_writev_requeue(struct cifs_writedata *wdata)
 
 		if (rc) {
 			kref_put(&wdata2->refcount, cifs_writedata_release);
-			if (rc == -EAGAIN)
+			if (is_retryable_error(rc))
 				continue;
 			break;
 		}
@@ -2061,7 +2061,8 @@ cifs_writev_requeue(struct cifs_writedata *wdata)
 		i += nr_pages;
 	} while (i < wdata->nr_pages);
 
-	mapping_set_error(inode->i_mapping, rc);
+	if (rc != 0 && !is_retryable_error(rc))
+		mapping_set_error(inode->i_mapping, rc);
 	kref_put(&wdata->refcount, cifs_writedata_release);
 }
 
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 116f8af0384f..c13effbaadba 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -732,7 +732,8 @@ reopen_success:
 
 	if (can_flush) {
 		rc = filemap_write_and_wait(inode->i_mapping);
-		mapping_set_error(inode->i_mapping, rc);
+		if (!is_interrupt_error(rc))
+			mapping_set_error(inode->i_mapping, rc);
 
 		if (tcon->unix_ext)
 			rc = cifs_get_inode_info_unix(&inode, full_path,
@@ -2117,6 +2118,7 @@ static int cifs_writepages(struct address_space *mapping,
 	pgoff_t end, index;
 	struct cifs_writedata *wdata;
 	int rc = 0;
+	int saved_rc = 0;
 	unsigned int xid;
 
 	/*
@@ -2145,8 +2147,10 @@ retry:
 
 		rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize,
 						   &wsize, &credits);
-		if (rc)
+		if (rc != 0) {
+			done = true;
 			break;
+		}
 
 		tofind = min((wsize / PAGE_SIZE) - 1, end - index) + 1;
 
@@ -2154,6 +2158,7 @@ retry:
 						  &found_pages);
 		if (!wdata) {
 			rc = -ENOMEM;
+			done = true;
 			add_credits_and_wake_if(server, credits, 0);
 			break;
 		}
@@ -2182,7 +2187,7 @@ retry:
 		if (rc != 0) {
 			add_credits_and_wake_if(server, wdata->credits, 0);
 			for (i = 0; i < nr_pages; ++i) {
-				if (rc == -EAGAIN)
+				if (is_retryable_error(rc))
 					redirty_page_for_writepage(wbc,
 							   wdata->pages[i]);
 				else
@@ -2190,7 +2195,7 @@ retry:
 				end_page_writeback(wdata->pages[i]);
 				put_page(wdata->pages[i]);
 			}
-			if (rc != -EAGAIN)
+			if (!is_retryable_error(rc))
 				mapping_set_error(mapping, rc);
 		}
 		kref_put(&wdata->refcount, cifs_writedata_release);
@@ -2200,6 +2205,15 @@ retry:
 			continue;
 		}
 
+		/* Return immediately if we received a signal during writing */
+		if (is_interrupt_error(rc)) {
+			done = true;
+			break;
+		}
+
+		if (rc != 0 && saved_rc == 0)
+			saved_rc = rc;
+
 		wbc->nr_to_write -= nr_pages;
 		if (wbc->nr_to_write <= 0)
 			done = true;
@@ -2217,6 +2231,9 @@ retry:
 		goto retry;
 	}
 
+	if (saved_rc != 0)
+		rc = saved_rc;
+
 	if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))
 		mapping->writeback_index = index;
 
@@ -2249,8 +2266,8 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc)
 	set_page_writeback(page);
 retry_write:
 	rc = cifs_partialpagewrite(page, 0, PAGE_SIZE);
-	if (rc == -EAGAIN) {
-		if (wbc->sync_mode == WB_SYNC_ALL)
+	if (is_retryable_error(rc)) {
+		if (wbc->sync_mode == WB_SYNC_ALL && rc == -EAGAIN)
 			goto retry_write;
 		redirty_page_for_writepage(wbc, page);
 	} else if (rc != 0) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a81a9df997c1..84d51ca91ef7 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2261,6 +2261,11 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	 * the flush returns error?
 	 */
 	rc = filemap_write_and_wait(inode->i_mapping);
+	if (is_interrupt_error(rc)) {
+		rc = -ERESTARTSYS;
+		goto out;
+	}
+
 	mapping_set_error(inode->i_mapping, rc);
 	rc = 0;
 
@@ -2404,6 +2409,11 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	 * the flush returns error?
 	 */
 	rc = filemap_write_and_wait(inode->i_mapping);
+	if (is_interrupt_error(rc)) {
+		rc = -ERESTARTSYS;
+		goto cifs_setattr_exit;
+	}
+
 	mapping_set_error(inode->i_mapping, rc);
 	rc = 0;
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3
  2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3 Sasha Levin
@ 2019-02-12  1:48   ` Pavel Shilovskiy
  0 siblings, 0 replies; 5+ messages in thread
From: Pavel Shilovskiy @ 2019-02-12  1:48 UTC (permalink / raw)
  To: Sasha Levin, linux-kernel, stable; +Cc: Steven French, linux-cifs


Sat, Feb 9 2019, 10:56AM, Sasha Levin <sashal@kernel.org> wrote:
>
> From: Pavel Shilovsky <pshilov@microsoft.com>
>
> [ Upstream commit ee258d79159afed52ca9372aeb9c1a51e89b32ee ]
>
> Currently we account for credits in the thread initiating a request
> and waiting for a response. The demultiplex thread receives the response,
> wakes up the thread and the latter collects credits from the response
> buffer and add them to the server structure on the client. This approach
> is not accurate, because it may race with reconnect events in the
> demultiplex thread which resets the number of credits.
>
> Fix this by moving credit processing to new mid callbacks that collect
> credits granted by the server from the response in the demultiplex thread.
>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  fs/cifs/transport.c | 51 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 4dbf62bb51b2..0dab276eced8 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -781,12 +781,7 @@ cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
>  }
>
>  static void
> -cifs_noop_callback(struct mid_q_entry *mid)
> -{
> -}
> -
> -static void
> -cifs_cancelled_callback(struct mid_q_entry *mid)
> +cifs_compound_callback(struct mid_q_entry *mid)
>  {
>         struct TCP_Server_Info *server = mid->server;
>         unsigned int optype = mid->optype;
> @@ -799,10 +794,23 @@ cifs_cancelled_callback(struct mid_q_entry *mid)
>                         cifs_dbg(FYI, "Bad state for cancelled MID\n");
>         }
>
> -       DeleteMidQEntry(mid);
>         add_credits(server, credits_received, optype);
>  }
>
> +static void
> +cifs_compound_last_callback(struct mid_q_entry *mid)
> +{
> +       cifs_compound_callback(mid);
> +       cifs_wake_up_task(mid);
> +}
> +
> +static void
> +cifs_cancelled_callback(struct mid_q_entry *mid)
> +{
> +       cifs_compound_callback(mid);
> +       DeleteMidQEntry(mid);
> +}
> +
>  int
>  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> @@ -880,11 +888,14 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                 midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
>                 midQ[i]->optype = optype;
>                 /*
> -                * We don't invoke the callback compounds unless it is the last
> -                * request.
> +                * Invoke callback for every part of the compound chain
> +                * to calculate credits properly. Wake up this thread only when
> +                * the last element is received.
>                  */
>                 if (i < num_rqst - 1)
> -                       midQ[i]->callback = cifs_noop_callback;
> +                       midQ[i]->callback = cifs_compound_callback;
> +               else
> +                       midQ[i]->callback = cifs_compound_last_callback;
>         }
>         cifs_in_send_inc(ses->server);
>         rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
> @@ -898,8 +909,20 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>
>         mutex_unlock(&ses->server->srv_mutex);
>
> -       if (rc < 0)
> +       if (rc < 0) {
> +               /* Sending failed for some reason - return credits back */
> +               for (i = 0; i < num_rqst; i++)
> +                       add_credits(ses->server, credits[i], optype);
>                 goto out;
> +       }
> +
> +       /*
> +        * At this point the request is passed to the network stack - we assume
> +        * that any credits taken from the server structure on the client have
> +        * been spent and we can't return them back. Once we receive responses
> +        * we will collect credits granted by the server in the mid callbacks
> +        * and add those credits to the server structure.
> +        */
>
>         /*
>          * Compounding is never used during session establish.
> @@ -932,11 +955,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                 }
>         }
>
> -       for (i = 0; i < num_rqst; i++)
> -               if (!cancelled_mid[i] && midQ[i]->resp_buf
> -                   && (midQ[i]->mid_state == MID_RESPONSE_RECEIVED))
> -                       credits[i] = ses->server->ops->get_credits(midQ[i]);
> -
>         for (i = 0; i < num_rqst; i++) {
>                 if (rc < 0)
>                         goto out;
> @@ -995,7 +1013,6 @@ out:
>         for (i = 0; i < num_rqst; i++) {
>                 if (!cancelled_mid[i])
>                         cifs_delete_mid(midQ[i]);
> -               add_credits(ses->server, credits[i], optype);
>         }
>
>         return rc;
> --
> 2.19.1
>

Please apply the following two patches too:

https://patchwork.ozlabs.org/patch/1030180/
https://patchwork.ozlabs.org/patch/1030181/

The 1st fixes some minor regressions introduces by the proposing patch and the 2nd adds the same logic to processing of async responses as the proposing patch is doing for sync ones.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-02-12  1:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190209184734.125935-1-sashal@kernel.org>
2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 38/42] cifs: Limit memory used by lock request calls to a page Sasha Levin
2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 39/42] CIFS: Fix credits calculation for cancelled requests Sasha Levin
2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 40/42] CIFS: Move credit processing to mid callbacks for SMB3 Sasha Levin
2019-02-12  1:48   ` Pavel Shilovskiy
2019-02-09 18:47 ` [PATCH AUTOSEL 4.20 41/42] CIFS: Fix error paths in writeback code Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).