From: Pavel Shilovsky <piastryyy@gmail.com>
To: linux-cifs <linux-cifs@vger.kernel.org>,
Steve French <smfrench@gmail.com>,
Ronnie Sahlberg <lsahlber@redhat.com>
Subject: Re: [PATCH v2] CIFS: Do not skip SMB2 message IDs on send failures
Date: Fri, 1 Mar 2019 18:29:58 -0800 [thread overview]
Message-ID: <CAKywueRnhxyLaqL5sEHBf=i79tabk3drTRk36Q121ViGFOdFZQ@mail.gmail.com> (raw)
In-Reply-To: <1550779366-38445-1-git-send-email-pshilov@microsoft.com>
чт, 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
prev parent reply other threads:[~2019-03-02 2:30 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-21 20:02 [PATCH v2] CIFS: Do not skip SMB2 message IDs on send failures Pavel Shilovsky
2019-03-02 2:29 ` Pavel Shilovsky [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKywueRnhxyLaqL5sEHBf=i79tabk3drTRk36Q121ViGFOdFZQ@mail.gmail.com' \
--to=piastryyy@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=lsahlber@redhat.com \
--cc=smfrench@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).