linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Simplify credits handling for compound requests
@ 2019-02-28  6:24 Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 1/5] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky

Pavel, Steve,

Here is an updated series that tries to simplify the handling of 
allocating credits to compound requests by using a new method
to atomically wait for n>=1 credits.

Aside from simplifying compound_send_recv() it also solves a potential
(but very unlikely) deadlock that could happen in the current code.

The first two patches are just rearranging the code and changing signatures.
There should not be any change in behaviour from them.

The third patch makes it possible to wait for >1 credits atomically
however no codepath yet uses this.

Fourth patch is to address a potential starvation issue that could happen
IF there are requests for >1 credits.

Fifth patch changes compound_send_recv() to ask for the required n
credits in one atomic wait.



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

* [PATCH 1/5] cifs: change wait_for_free_request() to take flags as argument
  2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
@ 2019-02-28  6:24 ` Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 2/5] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

and compute timeout and optyp from it.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 849a45d7d732..5c4becefde4b 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -529,15 +529,20 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
 }
 
 static int
-wait_for_free_request(struct TCP_Server_Info *server, const int timeout,
-		      const int optype, unsigned int *instance)
+wait_for_free_request(struct TCP_Server_Info *server, const int flags,
+		      unsigned int *instance)
 {
 	int *val;
+	int timeout, optype;
+
+	timeout = flags & CIFS_TIMEOUT_MASK;
+	optype = flags & CIFS_OP_MASK;
 
 	val = server->ops->get_credits_field(server, optype);
 	/* Since an echo is already inflight, no need to wait to send another */
 	if (*val <= 0 && optype == CIFS_ECHO_OP)
 		return -EAGAIN;
+
 	return wait_for_free_credits(server, timeout, val, instance);
 }
 
@@ -637,16 +642,16 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
 		mid_handle_t *handle, void *cbdata, const int flags,
 		const struct cifs_credits *exist_credits)
 {
-	int rc, timeout, optype;
+	int rc;
 	struct mid_q_entry *mid;
 	struct cifs_credits credits = { .value = 0, .instance = 0 };
 	unsigned int instance;
+	int optype;
 
-	timeout = flags & CIFS_TIMEOUT_MASK;
 	optype = flags & CIFS_OP_MASK;
 
 	if ((flags & CIFS_HAS_CREDITS) == 0) {
-		rc = wait_for_free_request(server, timeout, optype, &instance);
+		rc = wait_for_free_request(server, flags, &instance);
 		if (rc)
 			return rc;
 		credits.value = 1;
@@ -862,8 +867,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		   const int flags, const int num_rqst, struct smb_rqst *rqst,
 		   int *resp_buf_type, struct kvec *resp_iov)
 {
-	int i, j, rc = 0;
-	int timeout, optype;
+	int i, j, optype, rc = 0;
 	struct mid_q_entry *midQ[MAX_COMPOUND];
 	bool cancelled_mid[MAX_COMPOUND] = {false};
 	struct cifs_credits credits[MAX_COMPOUND] = {
@@ -873,7 +877,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	unsigned int first_instance = 0;
 	char *buf;
 
-	timeout = flags & CIFS_TIMEOUT_MASK;
 	optype = flags & CIFS_OP_MASK;
 
 	for (i = 0; i < num_rqst; i++)
@@ -924,8 +927,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	 * Ensure we obtain 1 credit per request in the compound chain.
 	 */
 	for (i = 0; i < num_rqst; i++) {
-		rc = wait_for_free_request(ses->server, timeout, optype,
-					   &instance);
+		rc = wait_for_free_request(ses->server, flags, &instance);
 
 		if (rc == 0) {
 			credits[i].value = 1;
@@ -1048,7 +1050,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		smb311_update_preauth_hash(ses, rqst[0].rq_iov,
 					   rqst[0].rq_nvec);
 
-	if (timeout == CIFS_ASYNC_OP)
+	if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
 		goto out;
 
 	for (i = 0; i < num_rqst; i++) {
@@ -1185,7 +1187,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 int
 SendReceive(const unsigned int xid, struct cifs_ses *ses,
 	    struct smb_hdr *in_buf, struct smb_hdr *out_buf,
-	    int *pbytes_returned, const int timeout)
+	    int *pbytes_returned, const int flags)
 {
 	int rc = 0;
 	struct mid_q_entry *midQ;
@@ -1216,7 +1218,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 		return -EIO;
 	}
 
-	rc = wait_for_free_request(ses->server, timeout, 0, &credits.instance);
+	rc = wait_for_free_request(ses->server, flags, &credits.instance);
 	if (rc)
 		return rc;
 
@@ -1255,7 +1257,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 	if (rc < 0)
 		goto out;
 
-	if (timeout == CIFS_ASYNC_OP)
+	if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
 		goto out;
 
 	rc = wait_for_response(ses->server, midQ);
@@ -1358,8 +1360,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
 		return -EIO;
 	}
 
-	rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, 0,
-				   &instance);
+	rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, &instance);
 	if (rc)
 		return rc;
 
-- 
2.13.6


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

* [PATCH 2/5] cifs: pass flags down into wait_for_free_credits()
  2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 1/5] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
@ 2019-02-28  6:24 ` Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c4becefde4b..8887db2cd582 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -477,15 +477,24 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
 }
 
 static int
-wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
-		      int *credits, unsigned int *instance)
+wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
+		      unsigned int *instance)
 {
 	int rc;
+	int *credits;
+	int optype;
+
+	optype = flags & CIFS_OP_MASK;
 
 	*instance = 0;
 
+	credits = server->ops->get_credits_field(server, optype);
+	/* Since an echo is already inflight, no need to wait to send another */
+	if (*credits <= 0 && optype == CIFS_ECHO_OP)
+		return -EAGAIN;
+
 	spin_lock(&server->req_lock);
-	if (timeout == CIFS_ASYNC_OP) {
+	if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
 		/* oplock breaks must not be held up */
 		server->in_flight++;
 		*credits -= 1;
@@ -516,7 +525,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
 			 */
 
 			/* update # of requests on the wire to server */
-			if (timeout != CIFS_BLOCKING_OP) {
+			if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
 				*credits -= 1;
 				server->in_flight++;
 				*instance = server->reconnect_instance;
@@ -532,18 +541,7 @@ static int
 wait_for_free_request(struct TCP_Server_Info *server, const int flags,
 		      unsigned int *instance)
 {
-	int *val;
-	int timeout, optype;
-
-	timeout = flags & CIFS_TIMEOUT_MASK;
-	optype = flags & CIFS_OP_MASK;
-
-	val = server->ops->get_credits_field(server, optype);
-	/* Since an echo is already inflight, no need to wait to send another */
-	if (*val <= 0 && optype == CIFS_ECHO_OP)
-		return -EAGAIN;
-
-	return wait_for_free_credits(server, timeout, val, instance);
+	return wait_for_free_credits(server, flags, instance);
 }
 
 int
-- 
2.13.6


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

* [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits
  2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 1/5] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 2/5] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
@ 2019-02-28  6:24 ` Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
  2019-02-28  6:24 ` [PATCH 5/5] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
  4 siblings, 0 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

Change wait_for_free_credits() to allow waiting for >=1 credits instead of just
a single credit.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsglob.h  |  4 ++--
 fs/cifs/smb2ops.c   |  2 +-
 fs/cifs/transport.c | 14 +++++++-------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 31c63e7437fd..bc2bb14b2180 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -734,13 +734,13 @@ in_flight(struct TCP_Server_Info *server)
 }
 
 static inline bool
-has_credits(struct TCP_Server_Info *server, int *credits)
+has_credits(struct TCP_Server_Info *server, int *credits, int num_credits)
 {
 	int num;
 	spin_lock(&server->req_lock);
 	num = *credits;
 	spin_unlock(&server->req_lock);
-	return num > 0;
+	return num >= num_credits;
 }
 
 static inline void
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1243407c9546..15bc7953b7a6 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -185,7 +185,7 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 			spin_unlock(&server->req_lock);
 			cifs_num_waiters_inc(server);
 			rc = wait_event_killable(server->request_q,
-					has_credits(server, &server->credits));
+				has_credits(server, &server->credits, 1));
 			cifs_num_waiters_dec(server);
 			if (rc)
 				return rc;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 8887db2cd582..baf15194aa3d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -477,8 +477,8 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
 }
 
 static int
-wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
-		      unsigned int *instance)
+wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
+		      const int flags, unsigned int *instance)
 {
 	int rc;
 	int *credits;
@@ -504,11 +504,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
 	}
 
 	while (1) {
-		if (*credits <= 0) {
+		if (*credits < num_credits) {
 			spin_unlock(&server->req_lock);
 			cifs_num_waiters_inc(server);
 			rc = wait_event_killable(server->request_q,
-						 has_credits(server, credits));
+				has_credits(server, credits, num_credits));
 			cifs_num_waiters_dec(server);
 			if (rc)
 				return rc;
@@ -526,8 +526,8 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
 
 			/* update # of requests on the wire to server */
 			if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
-				*credits -= 1;
-				server->in_flight++;
+				*credits -= num_credits;
+				server->in_flight += num_credits;
 				*instance = server->reconnect_instance;
 			}
 			spin_unlock(&server->req_lock);
@@ -541,7 +541,7 @@ static int
 wait_for_free_request(struct TCP_Server_Info *server, const int flags,
 		      unsigned int *instance)
 {
-	return wait_for_free_credits(server, flags, instance);
+	return wait_for_free_credits(server, 1, flags, instance);
 }
 
 int
-- 
2.13.6


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

* [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests
  2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
                   ` (2 preceding siblings ...)
  2019-02-28  6:24 ` [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
@ 2019-02-28  6:24 ` Ronnie Sahlberg
  2019-03-02  1:41   ` Pavel Shilovsky
  2019-02-28  6:24 ` [PATCH 5/5] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
  4 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

Reserve the last MAX_COMPOUND credits for any request asking for >1 credit.
This is to prevent future compound requests from becoming starved while waiting
for potentially many requests is there is a large number of concurrent
singe-credit requests.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index baf15194aa3d..4ff832fce2e9 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -520,6 +520,29 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 			}
 
 			/*
+			 * For normal commands, reserve the last MAX_COMPOUND
+			 * credits to compound requests.
+			 * Otherwise these compounds could be permanently
+			 * starved for credits by single-credit requests.
+			 *
+			 * To prevent spinning CPU, block this thread until
+			 * there are >MAX_COMPOUND credits available.
+			 */
+			if (!optype && num_credits == 1 &&
+			    *credits <= MAX_COMPOUND) {
+				spin_unlock(&server->req_lock);
+				cifs_num_waiters_inc(server);
+				rc = wait_event_killable(server->request_q,
+					has_credits(server, credits,
+						    MAX_COMPOUND + 1));
+				cifs_num_waiters_dec(server);
+				if (rc)
+					return rc;
+				spin_lock(&server->req_lock);
+				continue;
+			}
+
+			/*
 			 * Can not count locking commands against total
 			 * as they are allowed to block on server.
 			 */
-- 
2.13.6


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

* [PATCH 5/5] cifs: simplify how we handle credits in compond_send_recv()
  2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
                   ` (3 preceding siblings ...)
  2019-02-28  6:24 ` [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
@ 2019-02-28  6:24 ` Ronnie Sahlberg
  4 siblings, 0 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-02-28  6:24 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

Since we can now wait for multiple requests atomically in
wait_for_free_request() we can now greatly simplify the handling
of the credits in this function.

This fixes a potential deadlock where many concurrent compound requests
could each have reserved 1 or 2 credits each but are all blocked
waiting for the final credits they need to be able to issue the requests
to the server.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 81 +++++++++++++----------------------------------------
 1 file changed, 20 insertions(+), 61 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 4ff832fce2e9..e46b534bf007 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -567,6 +567,13 @@ wait_for_free_request(struct TCP_Server_Info *server, const int flags,
 	return wait_for_free_credits(server, 1, flags, instance);
 }
 
+static int
+wait_for_compound_request(struct TCP_Server_Info *server, int num,
+			  const int flags, unsigned int *instance)
+{
+	return wait_for_free_credits(server, num, flags, instance);
+}
+
 int
 cifs_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 		      unsigned int *num, struct cifs_credits *credits)
@@ -895,7 +902,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		{ .value = 0, .instance = 0 }
 	};
 	unsigned int instance;
-	unsigned int first_instance = 0;
 	char *buf;
 
 	optype = flags & CIFS_OP_MASK;
@@ -921,70 +927,27 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 			spin_unlock(&ses->server->req_lock);
 			return -ENOTSUPP;
 		}
-	} else {
-		/* enough credits to send the whole compounded request */
-		ses->server->credits -= num_rqst;
-		ses->server->in_flight += num_rqst;
-		first_instance = ses->server->reconnect_instance;
 	}
 	spin_unlock(&ses->server->req_lock);
 
-	if (first_instance) {
-		cifs_dbg(FYI, "Acquired %d credits at once\n", num_rqst);
-		for (i = 0; i < num_rqst; i++) {
-			credits[i].value = 1;
-			credits[i].instance = first_instance;
-		}
-		goto setup_rqsts;
-	}
-
 	/*
-	 * There are not enough credits to send the whole compound request but
-	 * there are requests in flight that may bring credits from the server.
+	 * Wait for all the requests to become available.
 	 * This approach still leaves the possibility to be stuck waiting for
 	 * credits if the server doesn't grant credits to the outstanding
-	 * requests. This should be fixed by returning immediately and letting
-	 * a caller fallback to sequential commands instead of compounding.
-	 * Ensure we obtain 1 credit per request in the compound chain.
+	 * requests and if the client is completely idle, not generating any
+	 * other requests.
+	 * This can be handled by the eventual session reconnect.
 	 */
-	for (i = 0; i < num_rqst; i++) {
-		rc = wait_for_free_request(ses->server, flags, &instance);
-
-		if (rc == 0) {
-			credits[i].value = 1;
-			credits[i].instance = instance;
-			/*
-			 * All parts of the compound chain must get credits from
-			 * the same session, otherwise we may end up using more
-			 * credits than the server granted. If there were
-			 * reconnects in between, return -EAGAIN and let callers
-			 * handle it.
-			 */
-			if (i == 0)
-				first_instance = instance;
-			else if (first_instance != instance) {
-				i++;
-				rc = -EAGAIN;
-			}
-		}
+	rc = wait_for_compound_request(ses->server, num_rqst, flags,
+				       &instance);
+	if (rc)
+		return rc;
 
-		if (rc) {
-			/*
-			 * We haven't sent an SMB packet to the server yet but
-			 * we already obtained credits for i requests in the
-			 * compound chain - need to return those credits back
-			 * for future use. Note that we need to call add_credits
-			 * multiple times to match the way we obtained credits
-			 * in the first place and to account for in flight
-			 * requests correctly.
-			 */
-			for (j = 0; j < i; j++)
-				add_credits(ses->server, &credits[j], optype);
-			return rc;
-		}
+	for (i = 0; i < num_rqst; i++) {
+		credits[i].value = 1;
+		credits[i].instance = instance;
 	}
 
-setup_rqsts:
 	/*
 	 * Make sure that we sign in the same order that we send on this socket
 	 * and avoid races inside tcp sendmsg code that could cause corruption
@@ -995,17 +958,13 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 
 	/*
 	 * All the parts of the compound chain belong obtained credits from the
-	 * same session (see the appropriate checks above). In the same time
-	 * there might be reconnects after those checks but before we acquired
-	 * the srv_mutex. We can not use credits obtained from the previous
+	 * same session. We can not use credits obtained from the previous
 	 * session to send this request. Check if there were reconnects after
 	 * we obtained credits and return -EAGAIN in such cases to let callers
 	 * handle it.
 	 */
-	if (first_instance != ses->server->reconnect_instance) {
+	if (instance != ses->server->reconnect_instance) {
 		mutex_unlock(&ses->server->srv_mutex);
-		for (j = 0; j < num_rqst; j++)
-			add_credits(ses->server, &credits[j], optype);
 		return -EAGAIN;
 	}
 
-- 
2.13.6


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

* Re: [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests
  2019-02-28  6:24 ` [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
@ 2019-03-02  1:41   ` Pavel Shilovsky
  2019-03-06  0:07     ` ronnie sahlberg
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Shilovsky @ 2019-03-02  1:41 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky

ср, 27 февр. 2019 г. в 22:26, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Reserve the last MAX_COMPOUND credits for any request asking for >1 credit.
> This is to prevent future compound requests from becoming starved while waiting
> for potentially many requests is there is a large number of concurrent
> singe-credit requests.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index baf15194aa3d..4ff832fce2e9 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -520,6 +520,29 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
>                         }
>
>                         /*
> +                        * For normal commands, reserve the last MAX_COMPOUND
> +                        * credits to compound requests.
> +                        * Otherwise these compounds could be permanently
> +                        * starved for credits by single-credit requests.
> +                        *
> +                        * To prevent spinning CPU, block this thread until
> +                        * there are >MAX_COMPOUND credits available.
> +                        */
> +                       if (!optype && num_credits == 1 &&

What if server decides to grants us credits gradually? E.g. granted
only 2 during negotiate protocol and session setup. The tree connect
will wait forever here.

> +                           *credits <= MAX_COMPOUND) {
> +                               spin_unlock(&server->req_lock);
> +                               cifs_num_waiters_inc(server);
> +                               rc = wait_event_killable(server->request_q,
> +                                       has_credits(server, credits,
> +                                                   MAX_COMPOUND + 1));
> +                               cifs_num_waiters_dec(server);
> +                               if (rc)
> +                                       return rc;
> +                               spin_lock(&server->req_lock);
> +                               continue;
> +                       }
> +
> +                       /*
>                          * Can not count locking commands against total
>                          * as they are allowed to block on server.
>                          */
> --
> 2.13.6
>


--
Best regards,
Pavel Shilovsky

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

* Re: [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests
  2019-03-02  1:41   ` Pavel Shilovsky
@ 2019-03-06  0:07     ` ronnie sahlberg
  0 siblings, 0 replies; 9+ messages in thread
From: ronnie sahlberg @ 2019-03-06  0:07 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Ronnie Sahlberg, linux-cifs, Steve French, Pavel Shilovsky

On Sat, Mar 2, 2019 at 11:41 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> ср, 27 февр. 2019 г. в 22:26, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > Reserve the last MAX_COMPOUND credits for any request asking for >1 credit.
> > This is to prevent future compound requests from becoming starved while waiting
> > for potentially many requests is there is a large number of concurrent
> > singe-credit requests.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/transport.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index baf15194aa3d..4ff832fce2e9 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -520,6 +520,29 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
> >                         }
> >
> >                         /*
> > +                        * For normal commands, reserve the last MAX_COMPOUND
> > +                        * credits to compound requests.
> > +                        * Otherwise these compounds could be permanently
> > +                        * starved for credits by single-credit requests.
> > +                        *
> > +                        * To prevent spinning CPU, block this thread until
> > +                        * there are >MAX_COMPOUND credits available.
> > +                        */
> > +                       if (!optype && num_credits == 1 &&
>
> What if server decides to grants us credits gradually? E.g. granted
> only 2 during negotiate protocol and session setup. The tree connect
> will wait forever here.

Thanks. You are right.
That should be avoidable by checking for and only doing this if we
have <arbitrary large> number of credits already in flight.

I will send an updated series.

>
> > +                           *credits <= MAX_COMPOUND) {
> > +                               spin_unlock(&server->req_lock);
> > +                               cifs_num_waiters_inc(server);
> > +                               rc = wait_event_killable(server->request_q,
> > +                                       has_credits(server, credits,
> > +                                                   MAX_COMPOUND + 1));
> > +                               cifs_num_waiters_dec(server);
> > +                               if (rc)
> > +                                       return rc;
> > +                               spin_lock(&server->req_lock);
> > +                               continue;
> > +                       }
> > +
> > +                       /*
> >                          * Can not count locking commands against total
> >                          * as they are allowed to block on server.
> >                          */
> > --
> > 2.13.6
> >
>
>
> --
> Best regards,
> Pavel Shilovsky

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

* [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits
  2019-03-06  0:06 [PATCH 0/5] cifs: simplify how we allocate credits for compunds Ronnie Sahlberg
@ 2019-03-06  0:06 ` Ronnie Sahlberg
  0 siblings, 0 replies; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-03-06  0:06 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg

Change wait_for_free_credits() to allow waiting for >=1 credits instead of just
a single credit.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsglob.h  |  4 ++--
 fs/cifs/smb2ops.c   |  2 +-
 fs/cifs/transport.c | 14 +++++++-------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 31c63e7437fd..bc2bb14b2180 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -734,13 +734,13 @@ in_flight(struct TCP_Server_Info *server)
 }
 
 static inline bool
-has_credits(struct TCP_Server_Info *server, int *credits)
+has_credits(struct TCP_Server_Info *server, int *credits, int num_credits)
 {
 	int num;
 	spin_lock(&server->req_lock);
 	num = *credits;
 	spin_unlock(&server->req_lock);
-	return num > 0;
+	return num >= num_credits;
 }
 
 static inline void
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1243407c9546..15bc7953b7a6 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -185,7 +185,7 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 			spin_unlock(&server->req_lock);
 			cifs_num_waiters_inc(server);
 			rc = wait_event_killable(server->request_q,
-					has_credits(server, &server->credits));
+				has_credits(server, &server->credits, 1));
 			cifs_num_waiters_dec(server);
 			if (rc)
 				return rc;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 8887db2cd582..baf15194aa3d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -477,8 +477,8 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
 }
 
 static int
-wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
-		      unsigned int *instance)
+wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
+		      const int flags, unsigned int *instance)
 {
 	int rc;
 	int *credits;
@@ -504,11 +504,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
 	}
 
 	while (1) {
-		if (*credits <= 0) {
+		if (*credits < num_credits) {
 			spin_unlock(&server->req_lock);
 			cifs_num_waiters_inc(server);
 			rc = wait_event_killable(server->request_q,
-						 has_credits(server, credits));
+				has_credits(server, credits, num_credits));
 			cifs_num_waiters_dec(server);
 			if (rc)
 				return rc;
@@ -526,8 +526,8 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
 
 			/* update # of requests on the wire to server */
 			if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
-				*credits -= 1;
-				server->in_flight++;
+				*credits -= num_credits;
+				server->in_flight += num_credits;
 				*instance = server->reconnect_instance;
 			}
 			spin_unlock(&server->req_lock);
@@ -541,7 +541,7 @@ static int
 wait_for_free_request(struct TCP_Server_Info *server, const int flags,
 		      unsigned int *instance)
 {
-	return wait_for_free_credits(server, flags, instance);
+	return wait_for_free_credits(server, 1, flags, instance);
 }
 
 int
-- 
2.13.6


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

end of thread, other threads:[~2019-03-06  0:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28  6:24 [PATCH 0/5] Simplify credits handling for compound requests Ronnie Sahlberg
2019-02-28  6:24 ` [PATCH 1/5] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
2019-02-28  6:24 ` [PATCH 2/5] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
2019-02-28  6:24 ` [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
2019-02-28  6:24 ` [PATCH 4/5] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
2019-03-02  1:41   ` Pavel Shilovsky
2019-03-06  0:07     ` ronnie sahlberg
2019-02-28  6:24 ` [PATCH 5/5] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
2019-03-06  0:06 [PATCH 0/5] cifs: simplify how we allocate credits for compunds Ronnie Sahlberg
2019-03-06  0:06 ` [PATCH 3/5] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg

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).