| Message ID | 20190308025823.24382-7-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: simplify handling of credits for compounds | expand |
чт, 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
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
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);
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(-)