| Message ID | 20190423032749.30799-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix credits leak for SMB1 oplock breaks | expand |
пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>: > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > 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
On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > Thanks for the thoughtful review. Some comments below > пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > 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. CIFS_ASYNC_OP is overloaded. In two places it means we will get a response eventually (in one of which "eventually" might mean unbounded time) but don't wait for it. In the third place it means that there is no response to this command. > > #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; > + } The reason I did it the way I did was that I wanted this change to only affect the smb1 oplock breaks and leave all other commands and behavior unchanged. Maybe we need a new flag that means that there will never be a response at all to this command so we must add all credits back immediately once we have sent the pdu out on the wire. We use CIFS_ASYNC_OP for three different cases. When sending an oplock break, in which case we know we will never get a response from the server (and thus we need to add the credits back somehow which is not the demultiplex loop) but also when we send an Echo request or when we send a Notify request both cases where we WILL eventually receive a reply back from the server and where we should add the credits back in the demultiplex loop. So if we add the credits back already here for all CIFS_ASYNC_OPs, doesn't that mean that we could possibly add the credits back twice? Once here and then once again in the demultiplex loop when we get the response? Since CIFS_ASYNC_OP suggests that it is an async operation but that we will eventually get a reply (well that is how I parse the name) I feel uncomfortable with that change since it conflates async operations with send-only-and-never-get-a-response operations. Maybe we need a new flag to better describe the unique case for requests we know will never get a reply ? CIFS_NO_SERVER_RESPONSE or something? What do you think? > > 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 Not sure about that. I think "async operation" and "don't wait for reply" are pretty equivalent so I don't see a need to rename here. What is unique with smb1 oplock releases is not that they are async (I could argue that they are not) but that they are requests we send that will never have a response pdu from the server. > > CIFS_NO_RESP to CIFS_NO_RESP_BUF I think this would be a good rename. But lets put it in a different patch that only does a rename but does not change any logic. > > to reflect the exact meanings. > > -- > Best regards, > Pavel Shilovsky
вт, 23 апр. 2019 г. в 17:45, ronnie sahlberg <ronniesahlberg@gmail.com>: > > On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > > Thanks for the thoughtful review. Some comments below > > > пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > 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. > > CIFS_ASYNC_OP is overloaded. > In two places it means we will get a response eventually (in one of > which "eventually" might mean unbounded time) but don't wait for it. > In the third place it means that there is no response to this command. Agree, we should define another one. > > > > > #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; > > + } > > The reason I did it the way I did was that I wanted this change to > only affect the smb1 oplock breaks and leave all other commands and > behavior unchanged. > Maybe we need a new flag that means that there will never be a > response at all to this command so we must add all credits back > immediately once we have sent the pdu out on the wire. > > We use CIFS_ASYNC_OP for three different cases. > When sending an oplock break, in which case we know we will never get > a response from the server (and thus we need to add the credits back > somehow which is not the demultiplex loop) > but also when we send an Echo request or when we send a Notify request > both cases where we WILL eventually receive a reply back from the > server and where we should add the credits back in the demultiplex > loop. Only SMB1 oplocks uses compound_send_recv() to send CIFS_ASYNC_OP. Notify uses SendReceive() but Notify itself is not being called anywhere. Echo uses cifs_call_async which is a completely different code path. > So if we add the credits back already here for all CIFS_ASYNC_OPs, > doesn't that mean that we could possibly add the credits back twice? > Once here and then once again in the demultiplex loop when we > get the response? You are right. In most cases we will manage to delete the mid going to "out" label but it is still possible that the server will respond too fast for SMB1 oplock and the demultiplex thread will add the credits twice. We need a special no-op callback to set for such mid in the same loop I propose, so we do not add credits twice. > Since CIFS_ASYNC_OP suggests that it is an async operation but that we > will eventually get a reply (well that is how I parse the name) I feel > uncomfortable with that change since it conflates async operations > with > send-only-and-never-get-a-response operations. Agree. CIFS_ASYNC_OP is being parsed for me as "return immediately after sending a request and handle a response asynchronously". We should use this one only in cifs_call_async(), I guess. > > Maybe we need a new flag to better describe the unique case for > requests we know will never get a reply ? > CIFS_NO_SERVER_RESPONSE or something? Let's do CIFS_NO_SRV_RSP to make it shorter - it will be set by SMB1 oplock only and compound_send_recv won't be handle CIFS_ASYNC_OP operations at all truly reflecting its name - we have cifs_call_async for those purposes. > What do you think? > > > > > 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 > > Not sure about that. I think "async operation" and "don't wait for > reply" are pretty equivalent so I don't see a need to rename here. > What is unique with smb1 oplock releases is not that they are async (I > could argue that they are not) but that they are requests we send that > will never have a response pdu from the server. Ok. Another option is to let cifs_call_async know about the new CIFS_NO_SRV_RSP flag and pass a combination of CIFS_ASYNC_OP | CIFS_NO_SRV_RSP to the function that will just send a request and then delete a mid. No-op callback is still needed there. > > > > > CIFS_NO_RESP to CIFS_NO_RESP_BUF > > I think this would be a good rename. But lets put it in a different > patch that only does a rename but does not change any logic. > +1 for a separate patch. Probably RSP instead of RESP, but don't have strong preferences here. -- Best regards, Pavel Shilovsky
On Thu, Apr 25, 2019 at 5:28 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > вт, 23 апр. 2019 г. в 17:45, ronnie sahlberg <ronniesahlberg@gmail.com>: > > > > On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > > > > > Thanks for the thoughtful review. Some comments below > > > > > пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > > --- > > > > 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. > > > > CIFS_ASYNC_OP is overloaded. > > In two places it means we will get a response eventually (in one of > > which "eventually" might mean unbounded time) but don't wait for it. > > In the third place it means that there is no response to this command. > > Agree, we should define another one. I added a new flag for this in the respin I sent. There are more things we need to do but that should wait until the next merge window I think. > > > > > > > > > #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; > > > + } > > > > The reason I did it the way I did was that I wanted this change to > > only affect the smb1 oplock breaks and leave all other commands and > > behavior unchanged. > > Maybe we need a new flag that means that there will never be a > > response at all to this command so we must add all credits back > > immediately once we have sent the pdu out on the wire. > > > > We use CIFS_ASYNC_OP for three different cases. > > When sending an oplock break, in which case we know we will never get > > a response from the server (and thus we need to add the credits back > > somehow which is not the demultiplex loop) > > but also when we send an Echo request or when we send a Notify request > > both cases where we WILL eventually receive a reply back from the > > server and where we should add the credits back in the demultiplex > > loop. > > Only SMB1 oplocks uses compound_send_recv() to send CIFS_ASYNC_OP. > Notify uses SendReceive() but Notify itself is not being called > anywhere. Echo uses cifs_call_async which is a completely different > code path. This is all for a separate patch for the next merge window, but: We can remove Notify() since it is not used. Since this is the only callsite for SendReceive() that uses CIFS_ASYNC_OP we can then also remove the check for it in SendReceive() With the new flag I added we no longer need to check for CIFS_ASYNC_OP in compound_send_recv() either so I removed it in the patch. This leaves CIFS_ASYNC_OP used in only two places and the only thing it is used for is to indicate that we do not want to block in wait_for_credits(). Thus possibly we should rename this to CIFS_NON_BLOCK. The semantics then become : * CIFS_NO_SRV_RSP : indicating that we will never get a response. I.e. smb1 oplock break. * cifs_call_async() : this is what is used to request async processing of the reply. * CIFS_NON_BLOCK : in both/all cases used to indicate we don't want to block in wait_for_credit() and we might also rename CIFS_NO_RSP to CIFS_NO_SRV_RSP to make it clearer what this flag indicates. > > > So if we add the credits back already here for all CIFS_ASYNC_OPs, > > doesn't that mean that we could possibly add the credits back twice? > > Once here and then once again in the demultiplex loop when we > > get the response? > > You are right. In most cases we will manage to delete the mid going to > "out" label but it is still possible that the server will respond too > fast for SMB1 oplock and the demultiplex thread will add the credits > twice. We need a special no-op callback to set for such mid in the > same loop I propose, so we do not add credits twice. > > > Since CIFS_ASYNC_OP suggests that it is an async operation but that we > > will eventually get a reply (well that is how I parse the name) I feel > > uncomfortable with that change since it conflates async operations > > with > > send-only-and-never-get-a-response operations. > > Agree. CIFS_ASYNC_OP is being parsed for me as "return immediately > after sending a request and handle a response asynchronously". We > should use this one only in cifs_call_async(), I guess. > > > > > Maybe we need a new flag to better describe the unique case for > > requests we know will never get a reply ? > > CIFS_NO_SERVER_RESPONSE or something? > > Let's do CIFS_NO_SRV_RSP to make it shorter - it will be set by SMB1 > oplock only and compound_send_recv won't be handle CIFS_ASYNC_OP > operations at all truly reflecting its name - we have cifs_call_async > for those purposes. > > > What do you think? > > > > > > > > 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 > > > > Not sure about that. I think "async operation" and "don't wait for > > reply" are pretty equivalent so I don't see a need to rename here. > > What is unique with smb1 oplock releases is not that they are async (I > > could argue that they are not) but that they are requests we send that > > will never have a response pdu from the server. > > Ok. Another option is to let cifs_call_async know about the new > CIFS_NO_SRV_RSP flag and pass a combination of > CIFS_ASYNC_OP | CIFS_NO_SRV_RSP to the function that will just send a > request and then delete a mid. No-op callback is still needed there. > > > > > > > > > CIFS_NO_RESP to CIFS_NO_RESP_BUF > > > > I think this would be a good rename. But lets put it in a different > > patch that only does a rename but does not change any logic. > > > > +1 for a separate patch. Probably RSP instead of RESP, but don't have > strong preferences here. > > -- > Best regards, > Pavel Shilovsky
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;
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/transport.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)