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@xxxxxxxxx> wrote:
>
> чт, 7 мар. 2019 г. в 19:35, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
>
> --
> Best regards,
> Pavel Shilovsky
--
Thanks,
Steve
