| Message ID | 20180829021920.811-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: only wake the thread for the very last PDU in a compound | expand |
tentatively merged into cifs-2.6.git for-next On Tue, Aug 28, 2018 at 9:19 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 | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 9e3a1e87ae1c..49a800ce75d6 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, > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, > return PTR_ERR(midQ[i]); > } > > + /* > + * We don't invoke the callback compounds unless it is the last > + * request. > + */ > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; > + if (ses->server->ops->next_header && > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) > + 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 +920,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 >
вт, 28 авг. 2018 г. в 19:19, 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 | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 9e3a1e87ae1c..49a800ce75d6 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, > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, > return PTR_ERR(midQ[i]); > } > > + /* > + * We don't invoke the callback compounds unless it is the last > + * request. > + */ > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; > + if (ses->server->ops->next_header && > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) > + midQ[i]->callback = cifs_noop_callback; Wouldn't it be easier to just check for i < num_rqst - 1 ? Having ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false should be a bug here. SMB1 code doesn't support compounding at all, so num_rqst is always 1. > } > - > cifs_in_send_inc(ses->server); > rc = smb_send_rqst(ses->server, num_rqst, rqst, flags); > cifs_in_send_dec(ses->server); > @@ -909,6 +920,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 > -- Best regards, Pavel Shilovsky
ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@gmail.com>: > > вт, 28 авг. 2018 г. в 19:19, 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 | 19 ++++++++++++++++++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > index 9e3a1e87ae1c..49a800ce75d6 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, > > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, > > return PTR_ERR(midQ[i]); > > } > > > > + /* > > + * We don't invoke the callback compounds unless it is the last > > + * request. > > + */ > > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; > > + if (ses->server->ops->next_header && > > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) > > + midQ[i]->callback = cifs_noop_callback; > > Wouldn't it be easier to just check for i < num_rqst - 1 ? Having > ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false > should be a bug here. SMB1 code doesn't support compounding at all, so > num_rqst is always 1. Also I thought that we may use a similar change you did in WIP patch - just set mids[i]->multiRsp and mids[i]->multiEnd properly for compounded requests. I mean the following: if (num_req > 1) { mids[i]->multiRsp = 1; if (i == num_req - 1) { mids[i]->multiEnd = 1; } } This will work for both compound and non-compound requests and utilize the existing multiRsp/multiEnd fields. Thoughts? -- Best regards, Pavel Shilovsky
ср, 29 авг. 2018 г. в 16:01, Pavel Shilovsky <piastryyy@gmail.com>: > > ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@gmail.com>: > > > > вт, 28 авг. 2018 г. в 19:19, 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 | 19 ++++++++++++++++++- > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > > index 9e3a1e87ae1c..49a800ce75d6 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, > > > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, > > > return PTR_ERR(midQ[i]); > > > } > > > > > > + /* > > > + * We don't invoke the callback compounds unless it is the last > > > + * request. > > > + */ > > > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; > > > + if (ses->server->ops->next_header && > > > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) > > > + midQ[i]->callback = cifs_noop_callback; > > > > Wouldn't it be easier to just check for i < num_rqst - 1 ? Having > > ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false > > should be a bug here. SMB1 code doesn't support compounding at all, so > > num_rqst is always 1. > > Also I thought that we may use a similar change you did in WIP patch - > just set mids[i]->multiRsp and mids[i]->multiEnd properly for > compounded requests. I mean the following: > > if (num_req > 1) { > mids[i]->multiRsp = 1; > if (i == num_req - 1) { > mids[i]->multiEnd = 1; > } > } > > This will work for both compound and non-compound requests and utilize > the existing multiRsp/multiEnd fields. Thoughts? > Just noticed that you proposed the same idea in another email thread.
On Thu, Aug 30, 2018 at 9:01 AM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@gmail.com>: >> >> вт, 28 авг. 2018 г. в 19:19, 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 | 19 ++++++++++++++++++- >> > 1 file changed, 18 insertions(+), 1 deletion(-) >> > >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> > index 9e3a1e87ae1c..49a800ce75d6 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, >> > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, >> > return PTR_ERR(midQ[i]); >> > } >> > >> > + /* >> > + * We don't invoke the callback compounds unless it is the last >> > + * request. >> > + */ >> > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; >> > + if (ses->server->ops->next_header && >> > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) >> > + midQ[i]->callback = cifs_noop_callback; >> >> Wouldn't it be easier to just check for i < num_rqst - 1 ? Having >> ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false >> should be a bug here. SMB1 code doesn't support compounding at all, so >> num_rqst is always 1. > > Also I thought that we may use a similar change you did in WIP patch - > just set mids[i]->multiRsp and mids[i]->multiEnd properly for > compounded requests. I mean the following: > > if (num_req > 1) { > mids[i]->multiRsp = 1; > if (i == num_req - 1) { > mids[i]->multiEnd = 1; > } > } > > This will work for both compound and non-compound requests and utilize > the existing multiRsp/multiEnd fields. Thoughts? I don't think we can set those flags already in compound_send_recv since that will affect other codepaths during send and receive, at least handle_mid() possibly other places too. But your other suggestion to just look at num_rqst instead of checking for and calling ->next_header makes sense. I will do that and re-send. regards ronnie sahlberg > > -- > Best regards, > Pavel Shilovsky
ср, 29 авг. 2018 г. в 16:13, ronnie sahlberg <ronniesahlberg@gmail.com>: > > On Thu, Aug 30, 2018 at 9:01 AM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > > ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@gmail.com>: > >> > >> вт, 28 авг. 2018 г. в 19:19, 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 | 19 ++++++++++++++++++- > >> > 1 file changed, 18 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > >> > index 9e3a1e87ae1c..49a800ce75d6 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, > >> > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, > >> > return PTR_ERR(midQ[i]); > >> > } > >> > > >> > + /* > >> > + * We don't invoke the callback compounds unless it is the last > >> > + * request. > >> > + */ > >> > midQ[i]->mid_state = MID_REQUEST_SUBMITTED; > >> > + if (ses->server->ops->next_header && > >> > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) > >> > + midQ[i]->callback = cifs_noop_callback; > >> > >> Wouldn't it be easier to just check for i < num_rqst - 1 ? Having > >> ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false > >> should be a bug here. SMB1 code doesn't support compounding at all, so > >> num_rqst is always 1. > > > > Also I thought that we may use a similar change you did in WIP patch - > > just set mids[i]->multiRsp and mids[i]->multiEnd properly for > > compounded requests. I mean the following: > > > > if (num_req > 1) { > > mids[i]->multiRsp = 1; > > if (i == num_req - 1) { > > mids[i]->multiEnd = 1; > > } > > } > > > > This will work for both compound and non-compound requests and utilize > > the existing multiRsp/multiEnd fields. Thoughts? > > I don't think we can set those flags already in compound_send_recv > since that will affect other codepaths during send and receive, at > least handle_mid() > possibly other places too. > > But your other suggestion to just look at num_rqst instead of checking > for and calling ->next_header makes sense. I will do that and re-send. > Actually, it does seem that handle_mid() is the only place that behaves differently depending on those flags. We could add another mid->compound flag to check in handle_mid(). But anyway, that will probably complicate things more because for SMB1 multiRsp/multiEnd flags serve slightly another purpose. Agree, let's do indices check instead. -- Best regards, Pavel Shilovsky
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 9e3a1e87ae1c..49a800ce75d6 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, @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses, return PTR_ERR(midQ[i]); } + /* + * We don't invoke the callback compounds unless it is the last + * request. + */ midQ[i]->mid_state = MID_REQUEST_SUBMITTED; + if (ses->server->ops->next_header && + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base)) + 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 +920,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);
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 | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)