diff mbox series

[2/2] cifs: only wake the thread for the very last PDU in a compound

Message ID 20180830001300.28082-3-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show
Series [1/2] cifs: add a warning if we try to to dequeue a deleted mid | expand

Commit Message

Ronnie Sahlberg Aug. 30, 2018, 12:13 a.m. UTC
For compounded PDUs we whould only wake the waiting thread for the
very last PDU of the compound.
We do this so that we are guaranteed that the demultiplex_thread will
not process or access any of those MIDs any more once the send/recv
thread starts processing.

Else there is a race where at the end of the send/recv processing we
will try to delete all the mids of the compound. If the multiplex
thread still has other mids to process at this point for this compound
this can lead to an oops.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Pavel Shilovsky Aug. 30, 2018, 12:24 a.m. UTC | #1
ср, 29 авг. 2018 г. в 17:13, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> For compounded PDUs we whould only wake the waiting thread for the
> very last PDU of the compound.
> We do this so that we are guaranteed that the demultiplex_thread will
> not process or access any of those MIDs any more once the send/recv
> thread starts processing.
>
> Else there is a race where at the end of the send/recv processing we
> will try to delete all the mids of the compound. If the multiplex
> thread still has other mids to process at this point for this compound
> this can lead to an oops.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 9cc9a127749e..b48f43963da6 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -773,6 +773,11 @@ cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
>         return mid;
>  }
>
> +static void
> +cifs_noop_callback(struct mid_q_entry *mid)
> +{
> +}
> +
>  int
>  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> @@ -827,8 +832,13 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                 }
>
>                 midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
> +               /*
> +                * We don't invoke the callback compounds unless it is the last
> +                * request.
> +                */
> +               if (i < num_rqst - 1)
> +                       midQ[i]->callback = cifs_noop_callback;
>         }
> -
>         cifs_in_send_inc(ses->server);
>         rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
>         cifs_in_send_dec(ses->server);
> @@ -909,6 +919,12 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                         midQ[i]->resp_buf = NULL;
>         }
>  out:
> +       /*
> +        * This will dequeue all mids. After this it is important that the
> +        * demultiplex_thread will not process any of these mids any futher.
> +        * This is prevented above by using a noop callback that will not
> +        * wake this thread except for the very last PDU.
> +        */
>         for (i = 0; i < num_rqst; i++)
>                 cifs_delete_mid(midQ[i]);
>         add_credits(ses->server, credits, optype);
> --
> 2.13.3
>

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky
Steve French Aug. 30, 2018, 9:48 p.m. UTC | #2
Merged into cifs-2.6.git along with the other patch in the series (and
reviewed-by tags for Pavel added)
On Wed, Aug 29, 2018 at 7:13 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> For compounded PDUs we whould only wake the waiting thread for the
> very last PDU of the compound.
> We do this so that we are guaranteed that the demultiplex_thread will
> not process or access any of those MIDs any more once the send/recv
> thread starts processing.
>
> Else there is a race where at the end of the send/recv processing we
> will try to delete all the mids of the compound. If the multiplex
> thread still has other mids to process at this point for this compound
> this can lead to an oops.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 9cc9a127749e..b48f43963da6 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -773,6 +773,11 @@ cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
>         return mid;
>  }
>
> +static void
> +cifs_noop_callback(struct mid_q_entry *mid)
> +{
> +}
> +
>  int
>  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> @@ -827,8 +832,13 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                 }
>
>                 midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
> +               /*
> +                * We don't invoke the callback compounds unless it is the last
> +                * request.
> +                */
> +               if (i < num_rqst - 1)
> +                       midQ[i]->callback = cifs_noop_callback;
>         }
> -
>         cifs_in_send_inc(ses->server);
>         rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
>         cifs_in_send_dec(ses->server);
> @@ -909,6 +919,12 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                         midQ[i]->resp_buf = NULL;
>         }
>  out:
> +       /*
> +        * This will dequeue all mids. After this it is important that the
> +        * demultiplex_thread will not process any of these mids any futher.
> +        * This is prevented above by using a noop callback that will not
> +        * wake this thread except for the very last PDU.
> +        */
>         for (i = 0; i < num_rqst; i++)
>                 cifs_delete_mid(midQ[i]);
>         add_credits(ses->server, credits, optype);
> --
> 2.13.3
>
diff mbox series

Patch

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 9cc9a127749e..b48f43963da6 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -773,6 +773,11 @@  cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
 	return mid;
 }
 
+static void
+cifs_noop_callback(struct mid_q_entry *mid)
+{
+}
+
 int
 compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		   const int flags, const int num_rqst, struct smb_rqst *rqst,
@@ -827,8 +832,13 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		}
 
 		midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
+		/*
+		 * We don't invoke the callback compounds unless it is the last
+		 * request.
+		 */
+		if (i < num_rqst - 1)
+			midQ[i]->callback = cifs_noop_callback;
 	}
-
 	cifs_in_send_inc(ses->server);
 	rc = smb_send_rqst(ses->server, num_rqst, rqst, flags);
 	cifs_in_send_dec(ses->server);
@@ -909,6 +919,12 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 			midQ[i]->resp_buf = NULL;
 	}
 out:
+	/*
+	 * This will dequeue all mids. After this it is important that the
+	 * demultiplex_thread will not process any of these mids any futher.
+	 * This is prevented above by using a noop callback that will not
+	 * wake this thread except for the very last PDU.
+	 */
 	for (i = 0; i < num_rqst; i++)
 		cifs_delete_mid(midQ[i]);
 	add_credits(ses->server, credits, optype);