| Message ID | 20190205025644.24613-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: add credits from unmatched responses/messages | expand |
пн, 4 февр. 2019 г. в 18:58, Ronnie Sahlberg <lsahlber@redhat.com>: > > We should add any credits granted to us from unmatched server responses. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/connect.c | 22 ++++++++++++++++++++++ > fs/cifs/smb2misc.c | 7 ------- > 2 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 431ca65b71e7..787d2c39b6c0 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -1064,6 +1064,26 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid) > return 0; > } > > +static void > +smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server) > +{ > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buffer; > + > + /* > + * SMB1 does not use credits. > + */ > + if (server->vals->header_preamble_size) > + return; > + > + if (shdr->CreditRequest) { > + spin_lock(&server->req_lock); > + server->credits += le16_to_cpu(shdr->CreditRequest); > + spin_unlock(&server->req_lock); > + wake_up(&server->request_q); > + } > +} > + > + > static int > cifs_demultiplex_thread(void *p) > { > @@ -1193,6 +1213,7 @@ cifs_demultiplex_thread(void *p) > } else if (server->ops->is_oplock_break && > server->ops->is_oplock_break(bufs[i], > server)) { > + smb2_add_credits_from_hdr(bufs[i], server); > cifs_dbg(FYI, "Received oplock break\n"); > } else { > cifs_dbg(VFS, "No task to wake, unknown frame " > @@ -1204,6 +1225,7 @@ cifs_demultiplex_thread(void *p) > if (server->ops->dump_detail) > server->ops->dump_detail(bufs[i], > server); > + smb2_add_credits_from_hdr(bufs[i], server); > cifs_dump_mids(server); > #endif /* CIFS_DEBUG2 */ > } > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index 7b8b58fb4d3f..6a9c47541c53 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -648,13 +648,6 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) > if (rsp->sync_hdr.Command != SMB2_OPLOCK_BREAK) > return false; > > - if (rsp->sync_hdr.CreditRequest) { > - spin_lock(&server->req_lock); > - server->credits += le16_to_cpu(rsp->sync_hdr.CreditRequest); > - spin_unlock(&server->req_lock); > - wake_up(&server->request_q); > - } > - > if (rsp->StructureSize != > smb2_rsp_struct_sizes[SMB2_OPLOCK_BREAK_HE]) { > if (le16_to_cpu(rsp->StructureSize) == 44) > -- > 2.13.6 > So, this will count credits not only for oplock/lease breaks but for any unmatched response even if it is unexpected on a wire. Probably won't hurt to do so. Have you ever see any evidence of unexpected responses during testing? Those cases should be investigated at least. Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> -- Best regards, Pavel Shilovsky
----- Original Message ----- > From: "Pavel Shilovsky" <piastryyy@gmail.com> > To: "Ronnie Sahlberg" <lsahlber@redhat.com> > Cc: "linux-cifs" <linux-cifs@vger.kernel.org>, "Steve French" <smfrench@gmail.com>, "Pavel Shilovsky" > <pshilov@microsoft.com> > Sent: Wednesday, 6 February, 2019 4:28:38 AM > Subject: Re: [PATCH] cifs: add credits from unmatched responses/messages > > пн, 4 февр. 2019 г. в 18:58, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > We should add any credits granted to us from unmatched server responses. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/connect.c | 22 ++++++++++++++++++++++ > > fs/cifs/smb2misc.c | 7 ------- > > 2 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 431ca65b71e7..787d2c39b6c0 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -1064,6 +1064,26 @@ cifs_handle_standard(struct TCP_Server_Info *server, > > struct mid_q_entry *mid) > > return 0; > > } > > > > +static void > > +smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server) > > +{ > > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buffer; > > + > > + /* > > + * SMB1 does not use credits. > > + */ > > + if (server->vals->header_preamble_size) > > + return; > > + > > + if (shdr->CreditRequest) { > > + spin_lock(&server->req_lock); > > + server->credits += le16_to_cpu(shdr->CreditRequest); > > + spin_unlock(&server->req_lock); > > + wake_up(&server->request_q); > > + } > > +} > > + > > + > > static int > > cifs_demultiplex_thread(void *p) > > { > > @@ -1193,6 +1213,7 @@ cifs_demultiplex_thread(void *p) > > } else if (server->ops->is_oplock_break && > > server->ops->is_oplock_break(bufs[i], > > server)) { > > + smb2_add_credits_from_hdr(bufs[i], server); > > cifs_dbg(FYI, "Received oplock break\n"); > > } else { > > cifs_dbg(VFS, "No task to wake, unknown > > frame " > > @@ -1204,6 +1225,7 @@ cifs_demultiplex_thread(void *p) > > if (server->ops->dump_detail) > > server->ops->dump_detail(bufs[i], > > server); > > + smb2_add_credits_from_hdr(bufs[i], server); > > cifs_dump_mids(server); > > #endif /* CIFS_DEBUG2 */ > > } > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > > index 7b8b58fb4d3f..6a9c47541c53 100644 > > --- a/fs/cifs/smb2misc.c > > +++ b/fs/cifs/smb2misc.c > > @@ -648,13 +648,6 @@ smb2_is_valid_oplock_break(char *buffer, struct > > TCP_Server_Info *server) > > if (rsp->sync_hdr.Command != SMB2_OPLOCK_BREAK) > > return false; > > > > - if (rsp->sync_hdr.CreditRequest) { > > - spin_lock(&server->req_lock); > > - server->credits += > > le16_to_cpu(rsp->sync_hdr.CreditRequest); > > - spin_unlock(&server->req_lock); > > - wake_up(&server->request_q); > > - } > > - > > if (rsp->StructureSize != > > smb2_rsp_struct_sizes[SMB2_OPLOCK_BREAK_HE]) > > { > > if (le16_to_cpu(rsp->StructureSize) == 44) > > -- > > 2.13.6 > > > > So, this will count credits not only for oplock/lease breaks but for > any unmatched response even if it is unexpected on a wire. Probably > won't hurt to do so. Have you ever see any evidence of unexpected > responses during testing? Those cases should be investigated at least. I have seen the message "No task to wake" occasionally but do not have a way to trigger/reproduce it yet. > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > > -- > Best regards, > Pavel Shilovsky >
merged into cifs-2.6.git for-next pending more testing On Tue, Feb 5, 2019 at 12:28 PM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > пн, 4 февр. 2019 г. в 18:58, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > We should add any credits granted to us from unmatched server responses. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/connect.c | 22 ++++++++++++++++++++++ > > fs/cifs/smb2misc.c | 7 ------- > > 2 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 431ca65b71e7..787d2c39b6c0 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -1064,6 +1064,26 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid) > > return 0; > > } > > > > +static void > > +smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server) > > +{ > > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buffer; > > + > > + /* > > + * SMB1 does not use credits. > > + */ > > + if (server->vals->header_preamble_size) > > + return; > > + > > + if (shdr->CreditRequest) { > > + spin_lock(&server->req_lock); > > + server->credits += le16_to_cpu(shdr->CreditRequest); > > + spin_unlock(&server->req_lock); > > + wake_up(&server->request_q); > > + } > > +} > > + > > + > > static int > > cifs_demultiplex_thread(void *p) > > { > > @@ -1193,6 +1213,7 @@ cifs_demultiplex_thread(void *p) > > } else if (server->ops->is_oplock_break && > > server->ops->is_oplock_break(bufs[i], > > server)) { > > + smb2_add_credits_from_hdr(bufs[i], server); > > cifs_dbg(FYI, "Received oplock break\n"); > > } else { > > cifs_dbg(VFS, "No task to wake, unknown frame " > > @@ -1204,6 +1225,7 @@ cifs_demultiplex_thread(void *p) > > if (server->ops->dump_detail) > > server->ops->dump_detail(bufs[i], > > server); > > + smb2_add_credits_from_hdr(bufs[i], server); > > cifs_dump_mids(server); > > #endif /* CIFS_DEBUG2 */ > > } > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > > index 7b8b58fb4d3f..6a9c47541c53 100644 > > --- a/fs/cifs/smb2misc.c > > +++ b/fs/cifs/smb2misc.c > > @@ -648,13 +648,6 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) > > if (rsp->sync_hdr.Command != SMB2_OPLOCK_BREAK) > > return false; > > > > - if (rsp->sync_hdr.CreditRequest) { > > - spin_lock(&server->req_lock); > > - server->credits += le16_to_cpu(rsp->sync_hdr.CreditRequest); > > - spin_unlock(&server->req_lock); > > - wake_up(&server->request_q); > > - } > > - > > if (rsp->StructureSize != > > smb2_rsp_struct_sizes[SMB2_OPLOCK_BREAK_HE]) { > > if (le16_to_cpu(rsp->StructureSize) == 44) > > -- > > 2.13.6 > > > > So, this will count credits not only for oplock/lease breaks but for > any unmatched response even if it is unexpected on a wire. Probably > won't hurt to do so. Have you ever see any evidence of unexpected > responses during testing? Those cases should be investigated at least. > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > > -- > Best regards, > Pavel Shilovsky
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 431ca65b71e7..787d2c39b6c0 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1064,6 +1064,26 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid) return 0; } +static void +smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server) +{ + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buffer; + + /* + * SMB1 does not use credits. + */ + if (server->vals->header_preamble_size) + return; + + if (shdr->CreditRequest) { + spin_lock(&server->req_lock); + server->credits += le16_to_cpu(shdr->CreditRequest); + spin_unlock(&server->req_lock); + wake_up(&server->request_q); + } +} + + static int cifs_demultiplex_thread(void *p) { @@ -1193,6 +1213,7 @@ cifs_demultiplex_thread(void *p) } else if (server->ops->is_oplock_break && server->ops->is_oplock_break(bufs[i], server)) { + smb2_add_credits_from_hdr(bufs[i], server); cifs_dbg(FYI, "Received oplock break\n"); } else { cifs_dbg(VFS, "No task to wake, unknown frame " @@ -1204,6 +1225,7 @@ cifs_demultiplex_thread(void *p) if (server->ops->dump_detail) server->ops->dump_detail(bufs[i], server); + smb2_add_credits_from_hdr(bufs[i], server); cifs_dump_mids(server); #endif /* CIFS_DEBUG2 */ } diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index 7b8b58fb4d3f..6a9c47541c53 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -648,13 +648,6 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) if (rsp->sync_hdr.Command != SMB2_OPLOCK_BREAK) return false; - if (rsp->sync_hdr.CreditRequest) { - spin_lock(&server->req_lock); - server->credits += le16_to_cpu(rsp->sync_hdr.CreditRequest); - spin_unlock(&server->req_lock); - wake_up(&server->request_q); - } - if (rsp->StructureSize != smb2_rsp_struct_sizes[SMB2_OPLOCK_BREAK_HE]) { if (le16_to_cpu(rsp->StructureSize) == 44)
We should add any credits granted to us from unmatched server responses. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/connect.c | 22 ++++++++++++++++++++++ fs/cifs/smb2misc.c | 7 ------- 2 files changed, 22 insertions(+), 7 deletions(-)