* [PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument
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
2019-03-08 2:58 ` [PATCH 2/6] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
and compute timeout and optyp from it.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/transport.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 849a45d7d732..5c4becefde4b 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -529,15 +529,20 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
}
static int
-wait_for_free_request(struct TCP_Server_Info *server, const int timeout,
- const int optype, unsigned int *instance)
+wait_for_free_request(struct TCP_Server_Info *server, const int flags,
+ unsigned int *instance)
{
int *val;
+ int timeout, optype;
+
+ timeout = flags & CIFS_TIMEOUT_MASK;
+ optype = flags & CIFS_OP_MASK;
val = server->ops->get_credits_field(server, optype);
/* Since an echo is already inflight, no need to wait to send another */
if (*val <= 0 && optype == CIFS_ECHO_OP)
return -EAGAIN;
+
return wait_for_free_credits(server, timeout, val, instance);
}
@@ -637,16 +642,16 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
mid_handle_t *handle, void *cbdata, const int flags,
const struct cifs_credits *exist_credits)
{
- int rc, timeout, optype;
+ int rc;
struct mid_q_entry *mid;
struct cifs_credits credits = { .value = 0, .instance = 0 };
unsigned int instance;
+ int optype;
- timeout = flags & CIFS_TIMEOUT_MASK;
optype = flags & CIFS_OP_MASK;
if ((flags & CIFS_HAS_CREDITS) == 0) {
- rc = wait_for_free_request(server, timeout, optype, &instance);
+ rc = wait_for_free_request(server, flags, &instance);
if (rc)
return rc;
credits.value = 1;
@@ -862,8 +867,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
const int flags, const int num_rqst, struct smb_rqst *rqst,
int *resp_buf_type, struct kvec *resp_iov)
{
- int i, j, rc = 0;
- int timeout, optype;
+ int i, j, optype, rc = 0;
struct mid_q_entry *midQ[MAX_COMPOUND];
bool cancelled_mid[MAX_COMPOUND] = {false};
struct cifs_credits credits[MAX_COMPOUND] = {
@@ -873,7 +877,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
unsigned int first_instance = 0;
char *buf;
- timeout = flags & CIFS_TIMEOUT_MASK;
optype = flags & CIFS_OP_MASK;
for (i = 0; i < num_rqst; i++)
@@ -924,8 +927,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
* Ensure we obtain 1 credit per request in the compound chain.
*/
for (i = 0; i < num_rqst; i++) {
- rc = wait_for_free_request(ses->server, timeout, optype,
- &instance);
+ rc = wait_for_free_request(ses->server, flags, &instance);
if (rc == 0) {
credits[i].value = 1;
@@ -1048,7 +1050,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
smb311_update_preauth_hash(ses, rqst[0].rq_iov,
rqst[0].rq_nvec);
- if (timeout == CIFS_ASYNC_OP)
+ if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
goto out;
for (i = 0; i < num_rqst; i++) {
@@ -1185,7 +1187,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
int
SendReceive(const unsigned int xid, struct cifs_ses *ses,
struct smb_hdr *in_buf, struct smb_hdr *out_buf,
- int *pbytes_returned, const int timeout)
+ int *pbytes_returned, const int flags)
{
int rc = 0;
struct mid_q_entry *midQ;
@@ -1216,7 +1218,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
return -EIO;
}
- rc = wait_for_free_request(ses->server, timeout, 0, &credits.instance);
+ rc = wait_for_free_request(ses->server, flags, &credits.instance);
if (rc)
return rc;
@@ -1255,7 +1257,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
if (rc < 0)
goto out;
- if (timeout == CIFS_ASYNC_OP)
+ if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
goto out;
rc = wait_for_response(ses->server, midQ);
@@ -1358,8 +1360,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
return -EIO;
}
- rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, 0,
- &instance);
+ rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, &instance);
if (rc)
return rc;
--
2.13.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument
2019-03-08 2:58 ` [PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
@ 2019-03-09 0:53 ` Pavel Shilovsky
0 siblings, 0 replies; 17+ 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>:
>
> and compute timeout and optyp from it.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/transport.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 849a45d7d732..5c4becefde4b 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -529,15 +529,20 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
> }
>
> static int
> -wait_for_free_request(struct TCP_Server_Info *server, const int timeout,
> - const int optype, unsigned int *instance)
> +wait_for_free_request(struct TCP_Server_Info *server, const int flags,
> + unsigned int *instance)
> {
> int *val;
> + int timeout, optype;
> +
> + timeout = flags & CIFS_TIMEOUT_MASK;
> + optype = flags & CIFS_OP_MASK;
>
> val = server->ops->get_credits_field(server, optype);
> /* Since an echo is already inflight, no need to wait to send another */
> if (*val <= 0 && optype == CIFS_ECHO_OP)
> return -EAGAIN;
> +
> return wait_for_free_credits(server, timeout, val, instance);
> }
>
> @@ -637,16 +642,16 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> mid_handle_t *handle, void *cbdata, const int flags,
> const struct cifs_credits *exist_credits)
> {
> - int rc, timeout, optype;
> + int rc;
> struct mid_q_entry *mid;
> struct cifs_credits credits = { .value = 0, .instance = 0 };
> unsigned int instance;
> + int optype;
>
> - timeout = flags & CIFS_TIMEOUT_MASK;
> optype = flags & CIFS_OP_MASK;
>
> if ((flags & CIFS_HAS_CREDITS) == 0) {
> - rc = wait_for_free_request(server, timeout, optype, &instance);
> + rc = wait_for_free_request(server, flags, &instance);
> if (rc)
> return rc;
> credits.value = 1;
> @@ -862,8 +867,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> const int flags, const int num_rqst, struct smb_rqst *rqst,
> int *resp_buf_type, struct kvec *resp_iov)
> {
> - int i, j, rc = 0;
> - int timeout, optype;
> + int i, j, optype, rc = 0;
> struct mid_q_entry *midQ[MAX_COMPOUND];
> bool cancelled_mid[MAX_COMPOUND] = {false};
> struct cifs_credits credits[MAX_COMPOUND] = {
> @@ -873,7 +877,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> unsigned int first_instance = 0;
> char *buf;
>
> - timeout = flags & CIFS_TIMEOUT_MASK;
> optype = flags & CIFS_OP_MASK;
>
> for (i = 0; i < num_rqst; i++)
> @@ -924,8 +927,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> * Ensure we obtain 1 credit per request in the compound chain.
> */
> for (i = 0; i < num_rqst; i++) {
> - rc = wait_for_free_request(ses->server, timeout, optype,
> - &instance);
> + rc = wait_for_free_request(ses->server, flags, &instance);
>
> if (rc == 0) {
> credits[i].value = 1;
> @@ -1048,7 +1050,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> smb311_update_preauth_hash(ses, rqst[0].rq_iov,
> rqst[0].rq_nvec);
>
> - if (timeout == CIFS_ASYNC_OP)
> + if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
> goto out;
>
> for (i = 0; i < num_rqst; i++) {
> @@ -1185,7 +1187,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
> int
> SendReceive(const unsigned int xid, struct cifs_ses *ses,
> struct smb_hdr *in_buf, struct smb_hdr *out_buf,
> - int *pbytes_returned, const int timeout)
> + int *pbytes_returned, const int flags)
> {
> int rc = 0;
> struct mid_q_entry *midQ;
> @@ -1216,7 +1218,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
> return -EIO;
> }
>
> - rc = wait_for_free_request(ses->server, timeout, 0, &credits.instance);
> + rc = wait_for_free_request(ses->server, flags, &credits.instance);
> if (rc)
> return rc;
>
> @@ -1255,7 +1257,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
> if (rc < 0)
> goto out;
>
> - if (timeout == CIFS_ASYNC_OP)
> + if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
> goto out;
>
> rc = wait_for_response(ses->server, midQ);
> @@ -1358,8 +1360,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
> return -EIO;
> }
>
> - rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, 0,
> - &instance);
> + rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP, &instance);
> if (rc)
> return rc;
>
> --
> 2.13.6
>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/6] cifs: pass flags down into wait_for_free_credits()
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
2019-03-08 2:58 ` [PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:54 ` Pavel Shilovsky
2019-03-08 2:58 ` [PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/transport.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c4becefde4b..8887db2cd582 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -477,15 +477,24 @@ 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 timeout,
- int *credits, unsigned int *instance)
+wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
+ unsigned int *instance)
{
int rc;
+ int *credits;
+ int optype;
+
+ optype = flags & CIFS_OP_MASK;
*instance = 0;
+ credits = server->ops->get_credits_field(server, optype);
+ /* Since an echo is already inflight, no need to wait to send another */
+ if (*credits <= 0 && optype == CIFS_ECHO_OP)
+ return -EAGAIN;
+
spin_lock(&server->req_lock);
- if (timeout == CIFS_ASYNC_OP) {
+ if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
/* oplock breaks must not be held up */
server->in_flight++;
*credits -= 1;
@@ -516,7 +525,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
*/
/* update # of requests on the wire to server */
- if (timeout != CIFS_BLOCKING_OP) {
+ if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
*credits -= 1;
server->in_flight++;
*instance = server->reconnect_instance;
@@ -532,18 +541,7 @@ static int
wait_for_free_request(struct TCP_Server_Info *server, const int flags,
unsigned int *instance)
{
- int *val;
- int timeout, optype;
-
- timeout = flags & CIFS_TIMEOUT_MASK;
- optype = flags & CIFS_OP_MASK;
-
- val = server->ops->get_credits_field(server, optype);
- /* Since an echo is already inflight, no need to wait to send another */
- if (*val <= 0 && optype == CIFS_ECHO_OP)
- return -EAGAIN;
-
- return wait_for_free_credits(server, timeout, val, instance);
+ return wait_for_free_credits(server, flags, instance);
}
int
--
2.13.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/6] cifs: pass flags down into wait_for_free_credits()
2019-03-08 2:58 ` [PATCH 2/6] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
@ 2019-03-09 0:54 ` Pavel Shilovsky
0 siblings, 0 replies; 17+ messages in thread
From: Pavel Shilovsky @ 2019-03-09 0:54 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 7 мар. 2019 г. в 19:35, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/transport.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c4becefde4b..8887db2cd582 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -477,15 +477,24 @@ 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 timeout,
> - int *credits, unsigned int *instance)
> +wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
> + unsigned int *instance)
> {
> int rc;
> + int *credits;
> + int optype;
> +
> + optype = flags & CIFS_OP_MASK;
>
> *instance = 0;
>
> + credits = server->ops->get_credits_field(server, optype);
> + /* Since an echo is already inflight, no need to wait to send another */
> + if (*credits <= 0 && optype == CIFS_ECHO_OP)
> + return -EAGAIN;
> +
> spin_lock(&server->req_lock);
> - if (timeout == CIFS_ASYNC_OP) {
> + if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
> /* oplock breaks must not be held up */
> server->in_flight++;
> *credits -= 1;
> @@ -516,7 +525,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int timeout,
> */
>
> /* update # of requests on the wire to server */
> - if (timeout != CIFS_BLOCKING_OP) {
> + if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
> *credits -= 1;
> server->in_flight++;
> *instance = server->reconnect_instance;
> @@ -532,18 +541,7 @@ static int
> wait_for_free_request(struct TCP_Server_Info *server, const int flags,
> unsigned int *instance)
> {
> - int *val;
> - int timeout, optype;
> -
> - timeout = flags & CIFS_TIMEOUT_MASK;
> - optype = flags & CIFS_OP_MASK;
> -
> - val = server->ops->get_credits_field(server, optype);
> - /* Since an echo is already inflight, no need to wait to send another */
> - if (*val <= 0 && optype == CIFS_ECHO_OP)
> - return -EAGAIN;
> -
> - return wait_for_free_credits(server, timeout, val, instance);
> + return wait_for_free_credits(server, flags, instance);
> }
>
> int
> --
> 2.13.6
>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
2019-03-08 2:58 ` [PATCH 1/6] cifs: change wait_for_free_request() to take flags as argument Ronnie Sahlberg
2019-03-08 2:58 ` [PATCH 2/6] cifs: pass flags down into wait_for_free_credits() Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:54 ` Pavel Shilovsky
2019-03-08 2:58 ` [PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
Change wait_for_free_credits() to allow waiting for >=1 credits instead of just
a single credit.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 4 ++--
fs/cifs/smb2ops.c | 2 +-
fs/cifs/transport.c | 14 +++++++-------
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 31c63e7437fd..bc2bb14b2180 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -734,13 +734,13 @@ in_flight(struct TCP_Server_Info *server)
}
static inline bool
-has_credits(struct TCP_Server_Info *server, int *credits)
+has_credits(struct TCP_Server_Info *server, int *credits, int num_credits)
{
int num;
spin_lock(&server->req_lock);
num = *credits;
spin_unlock(&server->req_lock);
- return num > 0;
+ return num >= num_credits;
}
static inline void
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1243407c9546..15bc7953b7a6 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -185,7 +185,7 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
spin_unlock(&server->req_lock);
cifs_num_waiters_inc(server);
rc = wait_event_killable(server->request_q,
- has_credits(server, &server->credits));
+ has_credits(server, &server->credits, 1));
cifs_num_waiters_dec(server);
if (rc)
return rc;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 8887db2cd582..baf15194aa3d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -477,8 +477,8 @@ 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 flags,
- unsigned int *instance)
+wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
+ const int flags, unsigned int *instance)
{
int rc;
int *credits;
@@ -504,11 +504,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
}
while (1) {
- if (*credits <= 0) {
+ 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));
+ has_credits(server, credits, num_credits));
cifs_num_waiters_dec(server);
if (rc)
return rc;
@@ -526,8 +526,8 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
/* update # of requests on the wire to server */
if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
- *credits -= 1;
- server->in_flight++;
+ *credits -= num_credits;
+ server->in_flight += num_credits;
*instance = server->reconnect_instance;
}
spin_unlock(&server->req_lock);
@@ -541,7 +541,7 @@ static int
wait_for_free_request(struct TCP_Server_Info *server, const int flags,
unsigned int *instance)
{
- return wait_for_free_credits(server, flags, instance);
+ return wait_for_free_credits(server, 1, flags, instance);
}
int
--
2.13.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits
2019-03-08 2:58 ` [PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
@ 2019-03-09 0:54 ` Pavel Shilovsky
0 siblings, 0 replies; 17+ messages in thread
From: Pavel Shilovsky @ 2019-03-09 0:54 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 7 мар. 2019 г. в 19:35, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Change wait_for_free_credits() to allow waiting for >=1 credits instead of just
> a single credit.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsglob.h | 4 ++--
> fs/cifs/smb2ops.c | 2 +-
> fs/cifs/transport.c | 14 +++++++-------
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 31c63e7437fd..bc2bb14b2180 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -734,13 +734,13 @@ in_flight(struct TCP_Server_Info *server)
> }
>
> static inline bool
> -has_credits(struct TCP_Server_Info *server, int *credits)
> +has_credits(struct TCP_Server_Info *server, int *credits, int num_credits)
> {
> int num;
> spin_lock(&server->req_lock);
> num = *credits;
> spin_unlock(&server->req_lock);
> - return num > 0;
> + return num >= num_credits;
> }
>
> static inline void
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 1243407c9546..15bc7953b7a6 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -185,7 +185,7 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
> spin_unlock(&server->req_lock);
> cifs_num_waiters_inc(server);
> rc = wait_event_killable(server->request_q,
> - has_credits(server, &server->credits));
> + has_credits(server, &server->credits, 1));
> cifs_num_waiters_dec(server);
> if (rc)
> return rc;
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 8887db2cd582..baf15194aa3d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -477,8 +477,8 @@ 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 flags,
> - unsigned int *instance)
> +wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
> + const int flags, unsigned int *instance)
> {
> int rc;
> int *credits;
> @@ -504,11 +504,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
> }
>
> while (1) {
> - if (*credits <= 0) {
> + 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));
> + has_credits(server, credits, num_credits));
> cifs_num_waiters_dec(server);
> if (rc)
> return rc;
> @@ -526,8 +526,8 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int flags,
>
> /* update # of requests on the wire to server */
> if ((flags & CIFS_TIMEOUT_MASK) != CIFS_BLOCKING_OP) {
> - *credits -= 1;
> - server->in_flight++;
> + *credits -= num_credits;
> + server->in_flight += num_credits;
> *instance = server->reconnect_instance;
> }
> spin_unlock(&server->req_lock);
> @@ -541,7 +541,7 @@ static int
> wait_for_free_request(struct TCP_Server_Info *server, const int flags,
> unsigned int *instance)
> {
> - return wait_for_free_credits(server, flags, instance);
> + return wait_for_free_credits(server, 1, flags, instance);
> }
>
> int
> --
> 2.13.6
>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
` (2 preceding siblings ...)
2019-03-08 2:58 ` [PATCH 3/6] cifs: wait_for_free_credits() make it possible to wait for >=1 credits Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:55 ` Pavel Shilovsky
2019-03-08 2:58 ` [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits Ronnie Sahlberg
2019-03-08 2:58 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
5 siblings, 1 reply; 17+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
Reserve the last MAX_COMPOUND credits for any request asking for >1 credit.
This is to prevent future compound requests from becoming starved while waiting
for potentially many requests is there is a large number of concurrent
singe-credit requests.
However, we need to protect from servers that are very slow to hand out
new credits on new sessions so we only do this IFF there are 2*MAX_COMPOUND
(arbitrary) credits already in flight.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/transport.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index baf15194aa3d..f4ec9635dab2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -520,6 +520,34 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
}
/*
+ * For normal commands, reserve the last MAX_COMPOUND
+ * credits to compound requests.
+ * Otherwise these compounds could be permanently
+ * starved for credits by single-credit requests.
+ *
+ * To prevent spinning CPU, block this thread until
+ * there are >MAX_COMPOUND credits available.
+ * But only do this is we already have a lot of
+ * credits in flight to avoid triggering this check
+ * for servers that are slow to hand out credits on
+ * new sessions.
+ */
+ if (!optype && num_credits == 1 &&
+ server->in_flight > 2 * MAX_COMPOUND &&
+ *credits <= MAX_COMPOUND) {
+ spin_unlock(&server->req_lock);
+ cifs_num_waiters_inc(server);
+ rc = wait_event_killable(server->request_q,
+ has_credits(server, credits,
+ MAX_COMPOUND + 1));
+ cifs_num_waiters_dec(server);
+ if (rc)
+ return rc;
+ spin_lock(&server->req_lock);
+ continue;
+ }
+
+ /*
* Can not count locking commands against total
* as they are allowed to block on server.
*/
--
2.13.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests
2019-03-08 2:58 ` [PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
@ 2019-03-09 0:55 ` Pavel Shilovsky
0 siblings, 0 replies; 17+ messages in thread
From: Pavel Shilovsky @ 2019-03-09 0:55 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 7 мар. 2019 г. в 19:35, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Reserve the last MAX_COMPOUND credits for any request asking for >1 credit.
> This is to prevent future compound requests from becoming starved while waiting
> for potentially many requests is there is a large number of concurrent
> singe-credit requests.
>
> However, we need to protect from servers that are very slow to hand out
> new credits on new sessions so we only do this IFF there are 2*MAX_COMPOUND
> (arbitrary) credits already in flight.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/transport.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index baf15194aa3d..f4ec9635dab2 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -520,6 +520,34 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
> }
>
> /*
> + * For normal commands, reserve the last MAX_COMPOUND
> + * credits to compound requests.
> + * Otherwise these compounds could be permanently
> + * starved for credits by single-credit requests.
> + *
> + * To prevent spinning CPU, block this thread until
> + * there are >MAX_COMPOUND credits available.
> + * But only do this is we already have a lot of
> + * credits in flight to avoid triggering this check
> + * for servers that are slow to hand out credits on
> + * new sessions.
> + */
> + if (!optype && num_credits == 1 &&
> + server->in_flight > 2 * MAX_COMPOUND &&
> + *credits <= MAX_COMPOUND) {
> + spin_unlock(&server->req_lock);
> + cifs_num_waiters_inc(server);
> + rc = wait_event_killable(server->request_q,
> + has_credits(server, credits,
> + MAX_COMPOUND + 1));
> + cifs_num_waiters_dec(server);
> + if (rc)
> + return rc;
> + spin_lock(&server->req_lock);
> + continue;
> + }
> +
> + /*
> * Can not count locking commands against total
> * as they are allowed to block on server.
> */
> --
> 2.13.6
>
We need to watch how the the approach above work in real workloads but
it looks promising.
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits
2019-03-08 2:58 [PATCH 0/6] cifs: simplify handling of credits for compounds Ronnie Sahlberg
` (3 preceding siblings ...)
2019-03-08 2:58 ` [PATCH 4/6] cifs: prevent starvation in wait_for_free_credits for multi-credit requests Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:58 ` Pavel Shilovsky
2019-03-08 2:58 ` [PATCH 6/6] cifs: simplify how we handle credits in compond_send_recv() Ronnie Sahlberg
5 siblings, 1 reply; 17+ messages in thread
From: Ronnie Sahlberg @ 2019-03-08 2:58 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
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..62c58ce80123 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 -ENOTSUPP;
+ }
+ 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 -ENOTSUPP;
+ }
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits
2019-03-08 2:58 ` [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits Ronnie Sahlberg
@ 2019-03-09 0:58 ` Pavel Shilovsky
[not found] ` <CAH2r5ms3METKgX9sMet44r0RKdVAEe_x=vuB_6JZobJn95x4_Q@mail.gmail.com>
2019-03-10 2:21 ` Steve French
0 siblings, 2 replies; 17+ messages in thread
From: Pavel Shilovsky @ 2019-03-09 0:58 UTC (permalink / raw)
To: Ronnie Sahlberg, Steve French; +Cc: linux-cifs, Pavel Shilovsky
чт, 7 мар. 2019 г. в 19:35, 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..62c58ce80123 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 -ENOTSUPP;
> + }
> + 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 -ENOTSUPP;
> + }
> + 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
>
We may consider adding trace points for cases where we timed out.
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <CAH2r5ms3METKgX9sMet44r0RKdVAEe_x=vuB_6JZobJn95x4_Q@mail.gmail.com>]
* Re: [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits
2019-03-09 0:58 ` Pavel Shilovsky
[not found] ` <CAH2r5ms3METKgX9sMet44r0RKdVAEe_x=vuB_6JZobJn95x4_Q@mail.gmail.com>
@ 2019-03-10 2:21 ` Steve French
1 sibling, 0 replies; 17+ messages in thread
From: Steve French @ 2019-03-10 2:21 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Ronnie Sahlberg, linux-cifs, Pavel Shilovsky
How about this to add the trace point you requested (I will send out a
patch for it on the list)
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index b3d04018195c..8731cfa66026 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -527,6 +527,8 @@ wait_for_free_credits(struct TCP_Server_Info
*server, const int num_credits,
has_credits(server, credits, num_credits), t);
cifs_num_waiters_dec(server);
if (!rc) {
+ trace_smb3_credit_timeout(server->CurrentMid,
+ server->hostname, num_credits);
cifs_dbg(VFS, "wait timed out after %d ms\n",
timeout);
return -ENOTSUPP;
@@ -565,6 +567,9 @@ wait_for_free_credits(struct TCP_Server_Info
*server, const int num_credits,
t);
cifs_num_waiters_dec(server);
if (!rc) {
+ trace_smb3_credit_timeout(
+ server->CurrentMid,
+ server->hostname, num_credits);
cifs_dbg(VFS, "wait timed out
after %d ms\n",
timeout);
return -ENOTSUPP;
On Fri, Mar 8, 2019 at 6:58 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 7 мар. 2019 г. в 19:35, 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..62c58ce80123 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 -ENOTSUPP;
> > + }
> > + 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 -ENOTSUPP;
> > + }
> > + 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
> >
>
> We may consider adding trace points for cases where we timed out.
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
--
Thanks,
Steve
^ permalink raw reply related [flat|nested] 17+ 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
` (4 preceding siblings ...)
2019-03-08 2:58 ` [PATCH 5/6] cifs: add a timeout argument to wait_for_free_credits Ronnie Sahlberg
@ 2019-03-08 2:58 ` Ronnie Sahlberg
2019-03-09 0:53 ` Pavel Shilovsky
5 siblings, 1 reply; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread