| Message ID | 20190306041546.10419-6-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: simplify handling of credits for compounds | expand |
вт, 5 мар. 2019 г. в 21:45, Ronnie Sahlberg <lsahlber@redhat.com>: > > A negative timeout is the same as the current behaviour, i.e. no timeout. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/transport.c | 40 ++++++++++++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index f4ec9635dab2..3263e8b3a57d 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -478,11 +478,18 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, > > static int > wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > - const int flags, unsigned int *instance) > + const int timeout, const int flags, > + unsigned int *instance) > { > int rc; > int *credits; > int optype; > + long int t; > + > + if (timeout < 0) > + t = MAX_JIFFY_OFFSET; > + else > + t = msecs_to_jiffies(timeout); > > optype = flags & CIFS_OP_MASK; > > @@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > if (*credits < num_credits) { > spin_unlock(&server->req_lock); > cifs_num_waiters_inc(server); > - rc = wait_event_killable(server->request_q, > - has_credits(server, credits, num_credits)); > + rc = wait_event_killable_timeout(server->request_q, > + has_credits(server, credits, num_credits), t); > cifs_num_waiters_dec(server); > - if (rc) > - return rc; > + if (!rc) { > + cifs_dbg(VFS, "wait timed out after %d ms\n", > + timeout); > + return -EAGAIN; It would be hard to distinguish different EAGAIN reasons in the upper layers. As of now we use EAGAIN as retryable error indicating the upper layer to retry. This case is different because the retry most likely won't help and the sequential fallback is needed. In compound_send_recv() we return -ENOTSUPP if there is not enough credits to satisfy compounding and the number of requests in flight doesn't promise to get new credits in future. I suggest to return the same error code here (-ENOTSUPP). Probably this particular error code isn't ideal choice and suggestions to change it are welcome. > + } > + if (rc == -ERESTARTSYS) > + return -ERESTARTSYS; > spin_lock(&server->req_lock); > } else { > if (server->tcpStatus == CifsExiting) { > @@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > *credits <= MAX_COMPOUND) { > spin_unlock(&server->req_lock); > cifs_num_waiters_inc(server); > - rc = wait_event_killable(server->request_q, > + rc = wait_event_killable_timeout( > + server->request_q, > has_credits(server, credits, > - MAX_COMPOUND + 1)); > + MAX_COMPOUND + 1), > + t); > cifs_num_waiters_dec(server); > - if (rc) > - return rc; > + if (!rc) { > + cifs_dbg(VFS, "wait timed out after %d ms\n", > + timeout); > + return -EAGAIN; > + } > + if (rc == -ERESTARTSYS) > + return -ERESTARTSYS; > spin_lock(&server->req_lock); > continue; > } > @@ -569,7 +588,8 @@ static int > wait_for_free_request(struct TCP_Server_Info *server, const int flags, > unsigned int *instance) > { > - return wait_for_free_credits(server, 1, flags, instance); > + return wait_for_free_credits(server, 1, -1, flags, > + instance); > } > > int > -- > 2.13.6 > -- Best regards, Pavel Shilovsky
On Thu, Mar 7, 2019 at 4:33 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > вт, 5 мар. 2019 г. в 21:45, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > A negative timeout is the same as the current behaviour, i.e. no timeout. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/transport.c | 40 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 30 insertions(+), 10 deletions(-) > > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > index f4ec9635dab2..3263e8b3a57d 100644 > > --- a/fs/cifs/transport.c > > +++ b/fs/cifs/transport.c > > @@ -478,11 +478,18 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, > > > > static int > > wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > > - const int flags, unsigned int *instance) > > + const int timeout, const int flags, > > + unsigned int *instance) > > { > > int rc; > > int *credits; > > int optype; > > + long int t; > > + > > + if (timeout < 0) > > + t = MAX_JIFFY_OFFSET; > > + else > > + t = msecs_to_jiffies(timeout); > > > > optype = flags & CIFS_OP_MASK; > > > > @@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > > if (*credits < num_credits) { > > spin_unlock(&server->req_lock); > > cifs_num_waiters_inc(server); > > - rc = wait_event_killable(server->request_q, > > - has_credits(server, credits, num_credits)); > > + rc = wait_event_killable_timeout(server->request_q, > > + has_credits(server, credits, num_credits), t); > > cifs_num_waiters_dec(server); > > - if (rc) > > - return rc; > > + if (!rc) { > > + cifs_dbg(VFS, "wait timed out after %d ms\n", > > + timeout); > > + return -EAGAIN; > > It would be hard to distinguish different EAGAIN reasons in the upper > layers. As of now we use EAGAIN as retryable error indicating the > upper layer to retry. This case is different because the retry most > likely won't help and the sequential fallback is needed. In > compound_send_recv() we return -ENOTSUPP if there is not enough > credits to satisfy compounding and the number of requests in flight > doesn't promise to get new credits in future. > > I suggest to return the same error code here (-ENOTSUPP). Probably > this particular error code isn't ideal choice and suggestions to > change it are welcome. I will do that change. ENOTSUPP might not be ideal but should be fine for now. If/when we need to take special action and start serializing these requests we can pick a better errno. > > > + } > > + if (rc == -ERESTARTSYS) > > + return -ERESTARTSYS; > > spin_lock(&server->req_lock); > > } else { > > if (server->tcpStatus == CifsExiting) { > > @@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, > > *credits <= MAX_COMPOUND) { > > spin_unlock(&server->req_lock); > > cifs_num_waiters_inc(server); > > - rc = wait_event_killable(server->request_q, > > + rc = wait_event_killable_timeout( > > + server->request_q, > > has_credits(server, credits, > > - MAX_COMPOUND + 1)); > > + MAX_COMPOUND + 1), > > + t); > > cifs_num_waiters_dec(server); > > - if (rc) > > - return rc; > > + if (!rc) { > > + cifs_dbg(VFS, "wait timed out after %d ms\n", > > + timeout); > > + return -EAGAIN; > > + } > > + if (rc == -ERESTARTSYS) > > + return -ERESTARTSYS; > > spin_lock(&server->req_lock); > > continue; > > } > > @@ -569,7 +588,8 @@ static int > > wait_for_free_request(struct TCP_Server_Info *server, const int flags, > > unsigned int *instance) > > { > > - return wait_for_free_credits(server, 1, flags, instance); > > + return wait_for_free_credits(server, 1, -1, flags, > > + instance); > > } > > > > int > > -- > > 2.13.6 > > > > -- > Best regards, > Pavel Shilovsky
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index f4ec9635dab2..3263e8b3a57d 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -478,11 +478,18 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, static int wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, - const int flags, unsigned int *instance) + const int timeout, const int flags, + unsigned int *instance) { int rc; int *credits; int optype; + long int t; + + if (timeout < 0) + t = MAX_JIFFY_OFFSET; + else + t = msecs_to_jiffies(timeout); optype = flags & CIFS_OP_MASK; @@ -507,11 +514,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, if (*credits < num_credits) { spin_unlock(&server->req_lock); cifs_num_waiters_inc(server); - rc = wait_event_killable(server->request_q, - has_credits(server, credits, num_credits)); + rc = wait_event_killable_timeout(server->request_q, + has_credits(server, credits, num_credits), t); cifs_num_waiters_dec(server); - if (rc) - return rc; + if (!rc) { + cifs_dbg(VFS, "wait timed out after %d ms\n", + timeout); + return -EAGAIN; + } + if (rc == -ERESTARTSYS) + return -ERESTARTSYS; spin_lock(&server->req_lock); } else { if (server->tcpStatus == CifsExiting) { @@ -537,12 +549,19 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits, *credits <= MAX_COMPOUND) { spin_unlock(&server->req_lock); cifs_num_waiters_inc(server); - rc = wait_event_killable(server->request_q, + rc = wait_event_killable_timeout( + server->request_q, has_credits(server, credits, - MAX_COMPOUND + 1)); + MAX_COMPOUND + 1), + t); cifs_num_waiters_dec(server); - if (rc) - return rc; + if (!rc) { + cifs_dbg(VFS, "wait timed out after %d ms\n", + timeout); + return -EAGAIN; + } + if (rc == -ERESTARTSYS) + return -ERESTARTSYS; spin_lock(&server->req_lock); continue; } @@ -569,7 +588,8 @@ static int wait_for_free_request(struct TCP_Server_Info *server, const int flags, unsigned int *instance) { - return wait_for_free_credits(server, 1, flags, instance); + return wait_for_free_credits(server, 1, -1, flags, + instance); } int
A negative timeout is the same as the current behaviour, i.e. no timeout. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/transport.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-)