[6/6] cifs: simplify how we handle credits in compond_send_recv()
diff mbox series

Message ID 20190311021858.1504-1-lsahlber@redhat.com
State New
Headers show
Series
  • Untitled series #89905
Related show

Commit Message

Ronnie Sahlberg March 11, 2019, 2:18 a.m. UTC
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 | 110 ++++++++++++++++++----------------------------------
 1 file changed, 38 insertions(+), 72 deletions(-)

Comments

Pavel Shilovsky March 11, 2019, 5:54 p.m. UTC | #1
вс, 10 мар. 2019 г. в 20:20, 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.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 110 ++++++++++++++++++----------------------------------
>  1 file changed, 38 insertions(+), 72 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 62c58ce80123..5539c5c8ed75 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -592,6 +592,31 @@ 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)
> +{
> +       int *credits;
> +
> +       credits = server->ops->get_credits_field(server, flags & CIFS_OP_MASK);
> +
> +       spin_lock(&server->req_lock);
> +       if (*credits < num) {
> +               /*
> +                * Return immediately if not too many requests in flight since
> +                * we will likely be stuck on waiting for credits.
> +                */
> +               if (server->in_flight < num - *credits) {
> +                       spin_unlock(&server->req_lock);
> +                       return -ENOTSUPP;
> +               }
> +       }
> +       spin_unlock(&server->req_lock);
> +
> +       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 +945,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;
> @@ -936,80 +960,24 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>         if (ses->server->tcpStatus == CifsExiting)
>                 return -ENOENT;
>
> -       spin_lock(&ses->server->req_lock);
> -       if (ses->server->credits < num_rqst) {
> -               /*
> -                * Return immediately if not too many requests in flight since
> -                * we will likely be stuck on waiting for credits.
> -                */
> -               if (ses->server->in_flight < num_rqst - ses->server->credits) {
> -                       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,14 +988,12 @@ 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);
> --
> 2.13.6
>

Thanks for fixing it!

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

--
Best regards,
Pavel Shilovsky

Patch
diff mbox series

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 62c58ce80123..5539c5c8ed75 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -592,6 +592,31 @@  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)
+{
+	int *credits;
+
+	credits = server->ops->get_credits_field(server, flags & CIFS_OP_MASK);
+
+	spin_lock(&server->req_lock);
+	if (*credits < num) {
+		/*
+		 * Return immediately if not too many requests in flight since
+		 * we will likely be stuck on waiting for credits.
+		 */
+		if (server->in_flight < num - *credits) {
+			spin_unlock(&server->req_lock);
+			return -ENOTSUPP;
+		}
+	}
+	spin_unlock(&server->req_lock);
+
+	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 +945,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;
@@ -936,80 +960,24 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	if (ses->server->tcpStatus == CifsExiting)
 		return -ENOENT;
 
-	spin_lock(&ses->server->req_lock);
-	if (ses->server->credits < num_rqst) {
-		/*
-		 * Return immediately if not too many requests in flight since
-		 * we will likely be stuck on waiting for credits.
-		 */
-		if (ses->server->in_flight < num_rqst - ses->server->credits) {
-			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,14 +988,12 @@  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);