| Message ID | 20190311021858.1504-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
вс, 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
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);
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(-)