* [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
@ 2019-03-11 2:18 Ronnie Sahlberg
2019-03-11 17:54 ` Pavel Shilovsky
0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-03-11 2:18 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.
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-11 2:18 [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
@ 2019-03-11 17:54 ` Pavel Shilovsky
0 siblings, 0 replies; 7+ messages in thread
From: Pavel Shilovsky @ 2019-03-11 17:54 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
вс, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-09 0:53 ` Pavel Shilovsky
@ 2019-03-09 22:04 ` Steve French
0 siblings, 0 replies; 7+ messages in thread
From: Steve French @ 2019-03-09 22:04 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Ronnie Sahlberg, linux-cifs, Pavel Shilovsky
I will wait on minor update to this patch before merging it - but
tentatively merged the other 5 in the series to for-next pending more
testing
On Fri, Mar 8, 2019 at 6:53 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 7 мар. 2019 г. в 19:35, 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 | 106 +++++++++++++++++-----------------------------------
> > 1 file changed, 34 insertions(+), 72 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 62c58ce80123..3e30b6f9d7c6 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -592,6 +592,27 @@ 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)
> > +{
> > + spin_lock(&server->req_lock);
> > + if (server->credits < num) {
>
> The patch that introduced this behavior contained a minor error: we
> should check against
>
> credits = server->ops->get_credits_field(server, optype);
>
> not
>
> server->credits.
>
> There is absolutely no harm in that: the only possible operation that
> has different optype is oplock/lease break, it consumes 1 request
> (num=1) and there is no way that server->credits = 0 (normal bucket),
> server->oplock_credits = 1 (oplock bucket) and server->in_flight = 0 -
> credits are re-balanced every time we don't have requests in flight
> and are moved from the oplock bucket to the normal bucket.
>
> But it would be nice to fix that. Can you squash the fix into this patch please?
>
> > + /*
> > + * Return immediately if not too many requests in flight since
> > + * we will likely be stuck on waiting for credits.
> > + */
> > + if (server->in_flight < num - server->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 +941,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 +956,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 +984,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
> >
>
> Other than the minor comment above, the patchset looks good. Thanks!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-08 2:58 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
@ 2019-03-09 0:53 ` Pavel Shilovsky
2019-03-09 22:04 ` Steve French
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Shilovsky @ 2019-03-09 0:53 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 7 мар. 2019 г. в 19:35, 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 | 106 +++++++++++++++++-----------------------------------
> 1 file changed, 34 insertions(+), 72 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 62c58ce80123..3e30b6f9d7c6 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -592,6 +592,27 @@ 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)
> +{
> + spin_lock(&server->req_lock);
> + if (server->credits < num) {
The patch that introduced this behavior contained a minor error: we
should check against
credits = server->ops->get_credits_field(server, optype);
not
server->credits.
There is absolutely no harm in that: the only possible operation that
has different optype is oplock/lease break, it consumes 1 request
(num=1) and there is no way that server->credits = 0 (normal bucket),
server->oplock_credits = 1 (oplock bucket) and server->in_flight = 0 -
credits are re-balanced every time we don't have requests in flight
and are moved from the oplock bucket to the normal bucket.
But it would be nice to fix that. Can you squash the fix into this patch please?
> + /*
> + * Return immediately if not too many requests in flight since
> + * we will likely be stuck on waiting for credits.
> + */
> + if (server->in_flight < num - server->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 +941,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 +956,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 +984,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
>
Other than the minor comment above, the patchset looks good. Thanks!
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:53 ` Pavel Shilovsky
0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 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.
Set a default timeout of 60 seconds for compounded requests.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/transport.c | 106 +++++++++++++++++-----------------------------------
1 file changed, 34 insertions(+), 72 deletions(-)
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 62c58ce80123..3e30b6f9d7c6 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -592,6 +592,27 @@ 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)
+{
+ spin_lock(&server->req_lock);
+ if (server->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 - server->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 +941,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 +956,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 +984,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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-06 4:15 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
@ 2019-03-06 17:26 ` Pavel Shilovsky
0 siblings, 0 replies; 7+ messages in thread
From: Pavel Shilovsky @ 2019-03-06 17:26 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv()
2019-03-06 4:15 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
@ 2019-03-06 4:15 ` Ronnie Sahlberg
2019-03-06 17:26 ` Pavel Shilovsky
0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-03-06 4:15 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.
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-11 17:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-11 2:18 [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
2019-03-11 17:54 ` Pavel Shilovsky
-- strict thread matches above, loose matches on Subject: below --
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
2019-03-08 2:58 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
2019-03-09 0:53 ` Pavel Shilovsky
2019-03-09 22:04 ` Steve French
2019-03-06 4:15 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
2019-03-06 4:15 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
2019-03-06 17:26 ` Pavel Shilovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.