[PATCH 0/6] cifs: simplify handling of credits for compounds

[PATCH 0/6] cifs: simplify handling of credits for compounds

[Ronnie Sahlberg]

Pavel, Steve

Version 3:
- Add a timeout argument to the wait function and abort with -EAGAIN
upon timeout.

[PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument

[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

[PATCH 2/6] cifs: pass flags down into wait_for_free_credits()

[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

[PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits

[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

[PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests

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

However, we need to protect from servers that are very slow to hand out
new credits on new sessions so we only do this IFF there are 2*MAX_COMPOUND
(arbitrary) credits already in flight.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index baf15194aa3d..f4ec9635dab2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -520,6 +520,34 @@ 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.
+			 * But only do this is we already have a lot of
+			 * credits in flight to avoid triggering this check
+			 * for servers that are slow to hand out credits on
+			 * new sessions.
+			 */
+			if (!optype && num_credits == 1 &&
+			    server->in_flight > 2 * MAX_COMPOUND &&
+			    *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

[PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits

[Ronnie Sahlberg]

A negative timeout is the same as the current behaviour, i.e. no timeout.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index f4ec9635dab2..3263e8b3a57d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -478,11 +478,18 @@ 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 num_credits,
-		      const int flags, unsigned int *instance)
+		      const int timeout, const int flags,
+		      unsigned int *instance)
 {
 	int rc;
 	int *credits;
 	int optype;
+	long int t;
+
+	if (timeout < 0)
+		t = MAX_JIFFY_OFFSET;
+	else
+		t = msecs_to_jiffies(timeout);
 
 	optype = flags & CIFS_OP_MASK;
 
@@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 		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, num_credits));
+			rc = wait_event_killable_timeout(server->request_q,
+				has_credits(server, credits, num_credits), t);
 			cifs_num_waiters_dec(server);
-			if (rc)
-				return rc;
+			if (!rc) {
+				cifs_dbg(VFS, "wait timed out after %d ms\n",
+					 timeout);
+				return -EAGAIN;
+			}
+			if (rc == -ERESTARTSYS)
+				return -ERESTARTSYS;
 			spin_lock(&server->req_lock);
 		} else {
 			if (server->tcpStatus == CifsExiting) {
@@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 			    *credits <= MAX_COMPOUND) {
 				spin_unlock(&server->req_lock);
 				cifs_num_waiters_inc(server);
-				rc = wait_event_killable(server->request_q,
+				rc = wait_event_killable_timeout(
+					server->request_q,
 					has_credits(server, credits,
-						    MAX_COMPOUND + 1));
+						    MAX_COMPOUND + 1),
+					t);
 				cifs_num_waiters_dec(server);
-				if (rc)
-					return rc;
+				if (!rc) {
+					cifs_dbg(VFS, "wait timed out after %d ms\n",
+						 timeout);
+					return -EAGAIN;
+				}
+				if (rc == -ERESTARTSYS)
+					return -ERESTARTSYS;
 				spin_lock(&server->req_lock);
 				continue;
 			}
@@ -569,7 +588,8 @@ static int
 wait_for_free_request(struct TCP_Server_Info *server, const int flags,
 		      unsigned int *instance)
 {
-	return wait_for_free_credits(server, 1, flags, instance);
+	return wait_for_free_credits(server, 1, -1, flags,
+				     instance);
 }
 
 int
-- 
2.13.6

[PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()

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

Set a default timeout of 60 seconds for compounded requests.

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 3263e8b3a57d..f759822dd2f2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -592,6 +592,14 @@ wait_for_free_request(struct TCP_Server_Info *server, const int 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, 60000, flags,
+				     instance);
+}
+
 int
 cifs_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 		      unsigned int *num, struct cifs_credits *credits)
@@ -920,7 +928,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;
@@ -946,70 +953,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
@@ -1020,17 +984,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

Re: [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()

[Pavel Shilovsky]

вт, 5 мар. 2019 г. в 21:44, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> 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.
>
> Set a default timeout of 60 seconds for compounded requests.

Thanks for simplifying the compound_send_recv() and adding the
timeout. Although 60sec seems rather big interval, until we have
sequential fallback, it is the best we can do I guess.

>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 82 ++++++++++++++---------------------------------------
>  1 file changed, 21 insertions(+), 61 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 3263e8b3a57d..f759822dd2f2 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -592,6 +592,14 @@ wait_for_free_request(struct TCP_Server_Info *server, const int 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, 60000, flags,
> +                                    instance);
> +}
> +
>  int
>  cifs_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
>                       unsigned int *num, struct cifs_credits *credits)
> @@ -920,7 +928,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;
> @@ -946,70 +953,27 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                         spin_unlock(&ses->server->req_lock);
>                         return -ENOTSUPP;

The code above could also be moved into wait_for_free_credits() to
make compound_send_recv() even smaller.

>                 }
> -       } 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
> @@ -1020,17 +984,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);

Why removing this? We obtained credits and increased the number of
requests in flight but are about to return without sending the request
- need to return those credits back and adjust the number of requests
in flight.

I understand it may be confusing because the session changed and
add_credits won't actually add any credits *but* it will adjust the
number of requests in flight and possibly re-balance the remaining
credits in the current session, thus it is still needed to call
add_credits for all parts of compound chain here.

>                 return -EAGAIN;
>         }
>
> --
> 2.13.6
>


--
Best regards,
Pavel Shilovsky

Re: [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits

[Pavel Shilovsky]

вт, 5 мар. 2019 г. в 21:45, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> A negative timeout is the same as the current behaviour, i.e. no timeout.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index f4ec9635dab2..3263e8b3a57d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -478,11 +478,18 @@ 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 num_credits,
> -                     const int flags, unsigned int *instance)
> +                     const int timeout, const int flags,
> +                     unsigned int *instance)
>  {
>         int rc;
>         int *credits;
>         int optype;
> +       long int t;
> +
> +       if (timeout < 0)
> +               t = MAX_JIFFY_OFFSET;
> +       else
> +               t = msecs_to_jiffies(timeout);
>
>         optype = flags & CIFS_OP_MASK;
>
> @@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
>                 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, num_credits));
> +                       rc = wait_event_killable_timeout(server->request_q,
> +                               has_credits(server, credits, num_credits), t);
>                         cifs_num_waiters_dec(server);
> -                       if (rc)
> -                               return rc;
> +                       if (!rc) {
> +                               cifs_dbg(VFS, "wait timed out after %d ms\n",
> +                                        timeout);
> +                               return -EAGAIN;

It would be hard to distinguish different EAGAIN reasons in the upper
layers. As of now we use EAGAIN as retryable error indicating the
upper layer to retry. This case is different because the retry most
likely won't help and the sequential fallback is needed. In
compound_send_recv() we return -ENOTSUPP if there is not enough
credits to satisfy compounding and the number of requests in flight
doesn't promise to get new credits in future.

I suggest to return the same error code here (-ENOTSUPP). Probably
this particular error code isn't ideal choice and suggestions to
change it are welcome.

> +                       }
> +                       if (rc == -ERESTARTSYS)
> +                               return -ERESTARTSYS;
>                         spin_lock(&server->req_lock);
>                 } else {
>                         if (server->tcpStatus == CifsExiting) {
> @@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
>                             *credits <= MAX_COMPOUND) {
>                                 spin_unlock(&server->req_lock);
>                                 cifs_num_waiters_inc(server);
> -                               rc = wait_event_killable(server->request_q,
> +                               rc = wait_event_killable_timeout(
> +                                       server->request_q,
>                                         has_credits(server, credits,
> -                                                   MAX_COMPOUND + 1));
> +                                                   MAX_COMPOUND + 1),
> +                                       t);
>                                 cifs_num_waiters_dec(server);
> -                               if (rc)
> -                                       return rc;
> +                               if (!rc) {
> +                                       cifs_dbg(VFS, "wait timed out after %d ms\n",
> +                                                timeout);
> +                                       return -EAGAIN;
> +                               }
> +                               if (rc == -ERESTARTSYS)
> +                                       return -ERESTARTSYS;
>                                 spin_lock(&server->req_lock);
>                                 continue;
>                         }
> @@ -569,7 +588,8 @@ static int
>  wait_for_free_request(struct TCP_Server_Info *server, const int flags,
>                       unsigned int *instance)
>  {
> -       return wait_for_free_credits(server, 1, flags, instance);
> +       return wait_for_free_credits(server, 1, -1, flags,
> +                                    instance);
>  }
>
>  int
> --
> 2.13.6
>

--
Best regards,
Pavel Shilovsky

Re: [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits

[ronnie sahlberg]

On Thu, Mar 7, 2019 at 4:33 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вт, 5 мар. 2019 г. в 21:45, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > A negative timeout is the same as the current behaviour, i.e. no timeout.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/transport.c | 40 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index f4ec9635dab2..3263e8b3a57d 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -478,11 +478,18 @@ 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 num_credits,
> > -                     const int flags, unsigned int *instance)
> > +                     const int timeout, const int flags,
> > +                     unsigned int *instance)
> >  {
> >         int rc;
> >         int *credits;
> >         int optype;
> > +       long int t;
> > +
> > +       if (timeout < 0)
> > +               t = MAX_JIFFY_OFFSET;
> > +       else
> > +               t = msecs_to_jiffies(timeout);
> >
> >         optype = flags & CIFS_OP_MASK;
> >
> > @@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
> >                 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, num_credits));
> > +                       rc = wait_event_killable_timeout(server->request_q,
> > +                               has_credits(server, credits, num_credits), t);
> >                         cifs_num_waiters_dec(server);
> > -                       if (rc)
> > -                               return rc;
> > +                       if (!rc) {
> > +                               cifs_dbg(VFS, "wait timed out after %d ms\n",
> > +                                        timeout);
> > +                               return -EAGAIN;
>
> It would be hard to distinguish different EAGAIN reasons in the upper
> layers. As of now we use EAGAIN as retryable error indicating the
> upper layer to retry. This case is different because the retry most
> likely won't help and the sequential fallback is needed. In
> compound_send_recv() we return -ENOTSUPP if there is not enough
> credits to satisfy compounding and the number of requests in flight
> doesn't promise to get new credits in future.
>
> I suggest to return the same error code here (-ENOTSUPP). Probably
> this particular error code isn't ideal choice and suggestions to
> change it are welcome.

I will do that change.  ENOTSUPP might not be ideal but should be fine for now.
If/when we need to take special action and start serializing these
requests we can
pick a better errno.


>
> > +                       }
> > +                       if (rc == -ERESTARTSYS)
> > +                               return -ERESTARTSYS;
> >                         spin_lock(&server->req_lock);
> >                 } else {
> >                         if (server->tcpStatus == CifsExiting) {
> > @@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
> >                             *credits <= MAX_COMPOUND) {
> >                                 spin_unlock(&server->req_lock);
> >                                 cifs_num_waiters_inc(server);
> > -                               rc = wait_event_killable(server->request_q,
> > +                               rc = wait_event_killable_timeout(
> > +                                       server->request_q,
> >                                         has_credits(server, credits,
> > -                                                   MAX_COMPOUND + 1));
> > +                                                   MAX_COMPOUND + 1),
> > +                                       t);
> >                                 cifs_num_waiters_dec(server);
> > -                               if (rc)
> > -                                       return rc;
> > +                               if (!rc) {
> > +                                       cifs_dbg(VFS, "wait timed out after %d ms\n",
> > +                                                timeout);
> > +                                       return -EAGAIN;
> > +                               }
> > +                               if (rc == -ERESTARTSYS)
> > +                                       return -ERESTARTSYS;
> >                                 spin_lock(&server->req_lock);
> >                                 continue;
> >                         }
> > @@ -569,7 +588,8 @@ static int
> >  wait_for_free_request(struct TCP_Server_Info *server, const int flags,
> >                       unsigned int *instance)
> >  {
> > -       return wait_for_free_credits(server, 1, flags, instance);
> > +       return wait_for_free_credits(server, 1, -1, flags,
> > +                                    instance);
> >  }
> >
> >  int
> > --
> > 2.13.6
> >
>
> --
> Best regards,
> Pavel Shilovsky

Re: [PATCH 2/6] cifs: pass flags down into wait_for_free_credits()

[Pavel Shilovsky]

чт, 7 мар. 2019 г. в 19:35, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> 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
>

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky

[PATCH 2/6] cifs: pass flags down into wait_for_free_credits()

[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