* [PATCH] CIFS: Reset current mid to next value in sequence on send failures
@ 2019-02-20 23:35 Pavel Shilovsky
2019-02-21 0:06 ` ronnie sahlberg
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Shilovsky @ 2019-02-20 23:35 UTC (permalink / raw)
To: linux-cifs, smfrench
When we hit failures during constructing MIDs or sending PDUs
through the network, we end up not using the message IDs assigned
to the packet. The next SMB packet will skip those message IDs and
continue with the next one. This behavior may lead to a server not
granting us credits until we use the skipped IDs. Fix this by
reverting the current ID to the original value if any errors occur
before we push the packet through the network stack.
This patch fixes the generic/310 test from the xfs-tests.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
fs/cifs/cifsglob.h | 15 +++++++++++++++
fs/cifs/smb2ops.c | 12 ++++++++++++
fs/cifs/smb2transport.c | 13 +++++++++++--
fs/cifs/transport.c | 6 +++++-
4 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 5bf463c..10be31e 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -238,6 +238,8 @@ struct smb_version_operations {
int * (*get_credits_field)(struct TCP_Server_Info *, const int);
unsigned int (*get_credits)(struct mid_q_entry *);
__u64 (*get_next_mid)(struct TCP_Server_Info *);
+ void (*revert_current_mid)(struct TCP_Server_Info *server,
+ const unsigned int val);
/* data offset from read response message */
unsigned int (*read_data_offset)(char *);
/*
@@ -789,6 +791,19 @@ get_next_mid(struct TCP_Server_Info *server)
return cpu_to_le16(mid);
}
+static inline void
+revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
+{
+ if (server->ops->revert_current_mid)
+ server->ops->revert_current_mid(server, val);
+}
+
+static inline void
+dec_current_mid(struct TCP_Server_Info *server)
+{
+ return revert_current_mid(server, 1);
+}
+
static inline __u16
get_mid(const struct smb_hdr *smb)
{
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 080929a..1243407 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -265,6 +265,14 @@ smb2_get_next_mid(struct TCP_Server_Info *server)
return mid;
}
+static void
+smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
+{
+ spin_lock(&GlobalMid_Lock);
+ server->CurrentMid -= val;
+ spin_unlock(&GlobalMid_Lock);
+}
+
static struct mid_q_entry *
smb2_find_mid(struct TCP_Server_Info *server, char *buf)
{
@@ -3606,6 +3614,7 @@ struct smb_version_operations smb20_operations = {
.get_credits = smb2_get_credits,
.wait_mtu_credits = cifs_wait_mtu_credits,
.get_next_mid = smb2_get_next_mid,
+ .revert_current_mid = smb2_revert_current_mid,
.read_data_offset = smb2_read_data_offset,
.read_data_length = smb2_read_data_length,
.map_error = map_smb2_to_linux_error,
@@ -3702,6 +3711,7 @@ struct smb_version_operations smb21_operations = {
.wait_mtu_credits = smb2_wait_mtu_credits,
.adjust_credits = smb2_adjust_credits,
.get_next_mid = smb2_get_next_mid,
+ .revert_current_mid = smb2_revert_current_mid,
.read_data_offset = smb2_read_data_offset,
.read_data_length = smb2_read_data_length,
.map_error = map_smb2_to_linux_error,
@@ -3799,6 +3809,7 @@ struct smb_version_operations smb30_operations = {
.wait_mtu_credits = smb2_wait_mtu_credits,
.adjust_credits = smb2_adjust_credits,
.get_next_mid = smb2_get_next_mid,
+ .revert_current_mid = smb2_revert_current_mid,
.read_data_offset = smb2_read_data_offset,
.read_data_length = smb2_read_data_length,
.map_error = map_smb2_to_linux_error,
@@ -3905,6 +3916,7 @@ struct smb_version_operations smb311_operations = {
.wait_mtu_credits = smb2_wait_mtu_credits,
.adjust_credits = smb2_adjust_credits,
.get_next_mid = smb2_get_next_mid,
+ .revert_current_mid = smb2_revert_current_mid,
.read_data_offset = smb2_read_data_offset,
.read_data_length = smb2_read_data_length,
.map_error = map_smb2_to_linux_error,
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index 7b351c6..36baf1b 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -674,13 +674,18 @@ smb2_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
smb2_seq_num_into_buf(ses->server, shdr);
rc = smb2_get_mid_entry(ses, shdr, &mid);
- if (rc)
+ if (rc) {
+ dec_current_mid(ses->server);
return ERR_PTR(rc);
+ }
+
rc = smb2_sign_rqst(rqst, ses->server);
if (rc) {
+ dec_current_mid(ses->server);
cifs_delete_mid(mid);
return ERR_PTR(rc);
}
+
return mid;
}
@@ -690,16 +695,20 @@ smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
int rc;
struct smb2_sync_hdr *shdr =
(struct smb2_sync_hdr *)rqst->rq_iov[0].iov_base;
+ unsigned int num = le16_to_cpu(shdr->CreditCharge);
struct mid_q_entry *mid;
smb2_seq_num_into_buf(server, shdr);
mid = smb2_mid_entry_alloc(shdr, server);
- if (mid == NULL)
+ if (mid == NULL) {
+ revert_current_mid(server, num);
return ERR_PTR(-ENOMEM);
+ }
rc = smb2_sign_rqst(rqst, server);
if (rc) {
+ revert_current_mid(server, num);
DeleteMidQEntry(mid);
return ERR_PTR(rc);
}
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d4f1224f..849a45d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -695,6 +695,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
cifs_in_send_dec(server);
if (rc < 0) {
+ revert_current_mid(server, credits.value);
server->sequence_number -= 2;
cifs_delete_mid(mid);
}
@@ -988,6 +989,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
for (i = 0; i < num_rqst; i++) {
midQ[i] = ses->server->ops->setup_request(ses, &rqst[i]);
if (IS_ERR(midQ[i])) {
+ revert_current_mid(ses->server, i);
for (j = 0; j < i; j++)
cifs_delete_mid(midQ[j]);
mutex_unlock(&ses->server->srv_mutex);
@@ -1017,8 +1019,10 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
for (i = 0; i < num_rqst; i++)
cifs_save_when_sent(midQ[i]);
- if (rc < 0)
+ if (rc < 0) {
+ revert_current_mid(ses->server, num_rqst);
ses->server->sequence_number -= 2;
+ }
mutex_unlock(&ses->server->srv_mutex);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] CIFS: Reset current mid to next value in sequence on send failures
2019-02-20 23:35 [PATCH] CIFS: Reset current mid to next value in sequence on send failures Pavel Shilovsky
@ 2019-02-21 0:06 ` ronnie sahlberg
2019-02-21 20:04 ` Pavel Shilovsky
0 siblings, 1 reply; 5+ messages in thread
From: ronnie sahlberg @ 2019-02-21 0:06 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs, Steve French
Awesome!
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
On Thu, Feb 21, 2019 at 9:38 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> When we hit failures during constructing MIDs or sending PDUs
> through the network, we end up not using the message IDs assigned
> to the packet. The next SMB packet will skip those message IDs and
> continue with the next one. This behavior may lead to a server not
> granting us credits until we use the skipped IDs. Fix this by
> reverting the current ID to the original value if any errors occur
> before we push the packet through the network stack.
>
> This patch fixes the generic/310 test from the xfs-tests.
>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
> fs/cifs/cifsglob.h | 15 +++++++++++++++
> fs/cifs/smb2ops.c | 12 ++++++++++++
> fs/cifs/smb2transport.c | 13 +++++++++++--
> fs/cifs/transport.c | 6 +++++-
> 4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 5bf463c..10be31e 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -238,6 +238,8 @@ struct smb_version_operations {
> int * (*get_credits_field)(struct TCP_Server_Info *, const int);
> unsigned int (*get_credits)(struct mid_q_entry *);
> __u64 (*get_next_mid)(struct TCP_Server_Info *);
> + void (*revert_current_mid)(struct TCP_Server_Info *server,
> + const unsigned int val);
> /* data offset from read response message */
> unsigned int (*read_data_offset)(char *);
> /*
> @@ -789,6 +791,19 @@ get_next_mid(struct TCP_Server_Info *server)
> return cpu_to_le16(mid);
> }
>
> +static inline void
> +revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
> +{
> + if (server->ops->revert_current_mid)
> + server->ops->revert_current_mid(server, val);
> +}
> +
> +static inline void
> +dec_current_mid(struct TCP_Server_Info *server)
> +{
> + return revert_current_mid(server, 1);
> +}
> +
> static inline __u16
> get_mid(const struct smb_hdr *smb)
> {
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 080929a..1243407 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -265,6 +265,14 @@ smb2_get_next_mid(struct TCP_Server_Info *server)
> return mid;
> }
>
> +static void
> +smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
> +{
> + spin_lock(&GlobalMid_Lock);
> + server->CurrentMid -= val;
> + spin_unlock(&GlobalMid_Lock);
> +}
> +
> static struct mid_q_entry *
> smb2_find_mid(struct TCP_Server_Info *server, char *buf)
> {
> @@ -3606,6 +3614,7 @@ struct smb_version_operations smb20_operations = {
> .get_credits = smb2_get_credits,
> .wait_mtu_credits = cifs_wait_mtu_credits,
> .get_next_mid = smb2_get_next_mid,
> + .revert_current_mid = smb2_revert_current_mid,
> .read_data_offset = smb2_read_data_offset,
> .read_data_length = smb2_read_data_length,
> .map_error = map_smb2_to_linux_error,
> @@ -3702,6 +3711,7 @@ struct smb_version_operations smb21_operations = {
> .wait_mtu_credits = smb2_wait_mtu_credits,
> .adjust_credits = smb2_adjust_credits,
> .get_next_mid = smb2_get_next_mid,
> + .revert_current_mid = smb2_revert_current_mid,
> .read_data_offset = smb2_read_data_offset,
> .read_data_length = smb2_read_data_length,
> .map_error = map_smb2_to_linux_error,
> @@ -3799,6 +3809,7 @@ struct smb_version_operations smb30_operations = {
> .wait_mtu_credits = smb2_wait_mtu_credits,
> .adjust_credits = smb2_adjust_credits,
> .get_next_mid = smb2_get_next_mid,
> + .revert_current_mid = smb2_revert_current_mid,
> .read_data_offset = smb2_read_data_offset,
> .read_data_length = smb2_read_data_length,
> .map_error = map_smb2_to_linux_error,
> @@ -3905,6 +3916,7 @@ struct smb_version_operations smb311_operations = {
> .wait_mtu_credits = smb2_wait_mtu_credits,
> .adjust_credits = smb2_adjust_credits,
> .get_next_mid = smb2_get_next_mid,
> + .revert_current_mid = smb2_revert_current_mid,
> .read_data_offset = smb2_read_data_offset,
> .read_data_length = smb2_read_data_length,
> .map_error = map_smb2_to_linux_error,
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 7b351c6..36baf1b 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -674,13 +674,18 @@ smb2_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
> smb2_seq_num_into_buf(ses->server, shdr);
>
> rc = smb2_get_mid_entry(ses, shdr, &mid);
> - if (rc)
> + if (rc) {
> + dec_current_mid(ses->server);
> return ERR_PTR(rc);
> + }
> +
> rc = smb2_sign_rqst(rqst, ses->server);
> if (rc) {
> + dec_current_mid(ses->server);
> cifs_delete_mid(mid);
> return ERR_PTR(rc);
> }
> +
> return mid;
> }
>
> @@ -690,16 +695,20 @@ smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> int rc;
> struct smb2_sync_hdr *shdr =
> (struct smb2_sync_hdr *)rqst->rq_iov[0].iov_base;
> + unsigned int num = le16_to_cpu(shdr->CreditCharge);
> struct mid_q_entry *mid;
>
> smb2_seq_num_into_buf(server, shdr);
>
> mid = smb2_mid_entry_alloc(shdr, server);
> - if (mid == NULL)
> + if (mid == NULL) {
> + revert_current_mid(server, num);
> return ERR_PTR(-ENOMEM);
> + }
>
> rc = smb2_sign_rqst(rqst, server);
> if (rc) {
> + revert_current_mid(server, num);
> DeleteMidQEntry(mid);
> return ERR_PTR(rc);
> }
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index d4f1224f..849a45d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -695,6 +695,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> cifs_in_send_dec(server);
>
> if (rc < 0) {
> + revert_current_mid(server, credits.value);
> server->sequence_number -= 2;
> cifs_delete_mid(mid);
> }
> @@ -988,6 +989,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> for (i = 0; i < num_rqst; i++) {
> midQ[i] = ses->server->ops->setup_request(ses, &rqst[i]);
> if (IS_ERR(midQ[i])) {
> + revert_current_mid(ses->server, i);
> for (j = 0; j < i; j++)
> cifs_delete_mid(midQ[j]);
> mutex_unlock(&ses->server->srv_mutex);
> @@ -1017,8 +1019,10 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> for (i = 0; i < num_rqst; i++)
> cifs_save_when_sent(midQ[i]);
>
> - if (rc < 0)
> + if (rc < 0) {
> + revert_current_mid(ses->server, num_rqst);
> ses->server->sequence_number -= 2;
> + }
>
> mutex_unlock(&ses->server->srv_mutex);
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] CIFS: Reset current mid to next value in sequence on send failures
2019-02-21 0:06 ` ronnie sahlberg
@ 2019-02-21 20:04 ` Pavel Shilovsky
2019-02-21 20:15 ` Pavel Shilovsky
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Shilovsky @ 2019-02-21 20:04 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: linux-cifs, Steve French
ср, 20 февр. 2019 г. в 16:06, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> Awesome!
>
> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
>
Thanks for the review. I posted the V2 today and changed the title to
more self-explanatory one:
[PATCH v2] CIFS: Do not skip SMB2 message IDs on send failures
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] CIFS: Reset current mid to next value in sequence on send failures
2019-02-21 20:04 ` Pavel Shilovsky
@ 2019-02-21 20:15 ` Pavel Shilovsky
2019-02-21 21:54 ` Steve French
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Shilovsky @ 2019-02-21 20:15 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: linux-cifs, Steve French
чт, 21 февр. 2019 г. в 12:04, Pavel Shilovsky <piastryyy@gmail.com>:
>
> ср, 20 февр. 2019 г. в 16:06, ronnie sahlberg <ronniesahlberg@gmail.com>:
> >
> > Awesome!
> >
> > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> >
>
> Thanks for the review. I posted the V2 today and changed the title to
> more self-explanatory one:
>
> [PATCH v2] CIFS: Do not skip SMB2 message IDs on send failures
Changes since V1:
- respect MTU requests in setup_sync_request (this is for the future,
we don't do MTU for sync requests now);
- properly revert MID for SMB 2.0 or ECHO command failures (since they
have CreditCharge as 0).
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] CIFS: Reset current mid to next value in sequence on send failures
2019-02-21 20:15 ` Pavel Shilovsky
@ 2019-02-21 21:54 ` Steve French
0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2019-02-21 21:54 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: ronnie sahlberg, linux-cifs
Merged into cifs-2.6.git for-next
On Thu, Feb 21, 2019 at 2:15 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 21 февр. 2019 г. в 12:04, Pavel Shilovsky <piastryyy@gmail.com>:
> >
> > ср, 20 февр. 2019 г. в 16:06, ronnie sahlberg <ronniesahlberg@gmail.com>:
> > >
> > > Awesome!
> > >
> > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > >
> >
> > Thanks for the review. I posted the V2 today and changed the title to
> > more self-explanatory one:
> >
> > [PATCH v2] CIFS: Do not skip SMB2 message IDs on send failures
>
> Changes since V1:
> - respect MTU requests in setup_sync_request (this is for the future,
> we don't do MTU for sync requests now);
> - properly revert MID for SMB 2.0 or ECHO command failures (since they
> have CreditCharge as 0).
>
> --
> Best regards,
> Pavel Shilovsky
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-21 21:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-20 23:35 [PATCH] CIFS: Reset current mid to next value in sequence on send failures Pavel Shilovsky
2019-02-21 0:06 ` ronnie sahlberg
2019-02-21 20:04 ` Pavel Shilovsky
2019-02-21 20:15 ` Pavel Shilovsky
2019-02-21 21:54 ` Steve French
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.