[v2] CIFS: Do not skip SMB2 message IDs on send failures
diff mbox series

Message ID 1550779366-38445-1-git-send-email-pshilov@microsoft.com
State New
Headers show
Series
  • [v2] CIFS: Do not skip SMB2 message IDs on send failures
Related show

Commit Message

Pavel Shilovsky Feb. 21, 2019, 8:02 p.m. UTC
When we hit failures during constructing MIDs or sending PDUs
through the network, we end up not using 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      | 18 ++++++++++++++++++
 fs/cifs/smb2ops.c       | 12 ++++++++++++
 fs/cifs/smb2transport.c | 12 ++++++++++--
 fs/cifs/transport.c     |  6 +++++-
 4 files changed, 45 insertions(+), 3 deletions(-)

Comments

Pavel Shilovsky March 2, 2019, 2:29 a.m. UTC | #1
чт, 21 февр. 2019 г. в 12:02, Pavel Shilovsky <piastryyy@gmail.com>:
>
> When we hit failures during constructing MIDs or sending PDUs
> through the network, we end up not using 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      | 18 ++++++++++++++++++
>  fs/cifs/smb2ops.c       | 12 ++++++++++++
>  fs/cifs/smb2transport.c | 12 ++++++++++--
>  fs/cifs/transport.c     |  6 +++++-
>  4 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 5bf463c..31c63e7 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,22 @@ 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
> +revert_current_mid_from_hdr(struct TCP_Server_Info *server,
> +                           const struct smb2_sync_hdr *shdr)
> +{
> +       unsigned int num = le16_to_cpu(shdr->CreditCharge);
> +
> +       return revert_current_mid(server, num > 0 ? num : 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..5609c0b 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) {
> +               revert_current_mid_from_hdr(ses->server, shdr);
>                 return ERR_PTR(rc);
> +       }
> +
>         rc = smb2_sign_rqst(rqst, ses->server);
>         if (rc) {
> +               revert_current_mid_from_hdr(ses->server, shdr);
>                 cifs_delete_mid(mid);
>                 return ERR_PTR(rc);
>         }
> +
>         return mid;
>  }
>
> @@ -695,11 +700,14 @@ smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>         smb2_seq_num_into_buf(server, shdr);
>
>         mid = smb2_mid_entry_alloc(shdr, server);
> -       if (mid == NULL)
> +       if (mid == NULL) {
> +               revert_current_mid_from_hdr(server, shdr);
>                 return ERR_PTR(-ENOMEM);
> +       }
>
>         rc = smb2_sign_rqst(rqst, server);
>         if (rc) {
> +               revert_current_mid_from_hdr(server, shdr);
>                 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
>

Posted v3 to the list:

replaced revert_current_mid with revert_current_mid_from_hdr in
cifs_call_async(), so it is independent from other credit patches and
can be marked for stable and be applied to 4.19.y, 4.20.y and 5.0.y
kernels.

--
Best regards,
Pavel Shilovsky

Patch
diff mbox series

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 5bf463c..31c63e7 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,22 @@  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
+revert_current_mid_from_hdr(struct TCP_Server_Info *server,
+			    const struct smb2_sync_hdr *shdr)
+{
+	unsigned int num = le16_to_cpu(shdr->CreditCharge);
+
+	return revert_current_mid(server, num > 0 ? num : 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..5609c0b 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) {
+		revert_current_mid_from_hdr(ses->server, shdr);
 		return ERR_PTR(rc);
+	}
+
 	rc = smb2_sign_rqst(rqst, ses->server);
 	if (rc) {
+		revert_current_mid_from_hdr(ses->server, shdr);
 		cifs_delete_mid(mid);
 		return ERR_PTR(rc);
 	}
+
 	return mid;
 }
 
@@ -695,11 +700,14 @@  smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 	smb2_seq_num_into_buf(server, shdr);
 
 	mid = smb2_mid_entry_alloc(shdr, server);
-	if (mid == NULL)
+	if (mid == NULL) {
+		revert_current_mid_from_hdr(server, shdr);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	rc = smb2_sign_rqst(rqst, server);
 	if (rc) {
+		revert_current_mid_from_hdr(server, shdr);
 		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);