пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> ---
> fs/cifs/transport.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c59c498f56a..3b80884ddf0f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -971,7 +971,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, optype, rc = 0;
> + int i, j, optype, opmask, rc = 0;
> struct mid_q_entry *midQ[MAX_COMPOUND];
> bool cancelled_mid[MAX_COMPOUND] = {false};
> struct cifs_credits credits[MAX_COMPOUND] = {
> @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> char *buf;
>
> optype = flags & CIFS_OP_MASK;
> + opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
>
> for (i = 0; i < num_rqst; i++)
> resp_buf_type[i] = CIFS_NO_BUFFER; /* no response buf yet */
> @@ -1073,8 +1074,13 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>
> mutex_unlock(&ses->server->srv_mutex);
>
> - if (rc < 0) {
> - /* Sending failed for some reason - return credits back */
> + /*
> + * If sending failed for some reason or it is an oplock break that we
> + * will not receive a response to - return credits back
> + */
> + if (rc < 0 ||
> + (optype == CIFS_OBREAK_OP &&
> + opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
> for (i = 0; i < num_rqst; i++)
> add_credits(ses->server, &credits[i], optype);
> goto out;
> --
> 2.13.6
>
Thanks for looking into it. Let's me clarify the defines here because
their names are not straightforward and require changes:
#define CIFS_ASYNC_OP 2 /* do not wait for response */
meaning that we don't need a response - probably there won't be a one
(in SMB1 Oplock case). This is confusing because we also have
cifs_call_async that does asynchronous processing of the responses.
#define CIFS_NO_RESP 0x040 /* no response buffer required */
meaning that a response buffer is not needed but the error code is.
The problem with SMB1 oplocks was introduced when credits calculation
was moved to the demultiplex thread. The server might not respond to
such a request at all - that's why it is a special case.
So, CIFS_NO_RESP flags should be handled like we do today - no changes
are needed here. We wait for a response but just do not return a
buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
to add credits back once we send such a request. Prior to credit
movement to the demultiplex thread label "out" has the code to add
credits back. So I think the only fix that is needed here is:
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c59c49..26e249d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1095,8 +1095,11 @@ 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 ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
+ if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
+ for (i = 0; i < num_rqst; i++)
+ add_credits(ses->server, &credits[i], optype);
goto out;
+ }
for (i = 0; i < num_rqst; i++) {
rc = wait_for_response(ses->server, midQ[i]);
It worked locally against Samba server for me.
I would also suggest as a separate change to rename:
CIFS_ASYNC_OP to CIFS_NO_WAIT, and
CIFS_NO_RESP to CIFS_NO_RESP_BUF
to reflect the exact meanings.
--
Best regards,
Pavel Shilovsky
