On Thu, Aug 30, 2018 at 9:01 AM, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@xxxxxxxxx>:
>>
>> вт, 28 авг. 2018 г. в 19:19, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
>> >
>> > 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@xxxxxxxxxx>
>> > ---
>> > 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
