I Think Pavel is right. Thanks for this.
I base this on reading the code and his comment.
My work laptop is still busted so I can not run proper tests until I
fix it proper.
On Sat, Sep 29, 2018 at 8:22 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
> пт, 28 сент. 2018 г. в 0:35, Steve French <smfrench@xxxxxxxxx>:
> >
> > Fixes problem (discovered by Aurelien) introduced by recent commit:
> > commit b24df3e30cbf48255db866720fb71f14bf9d2f39
> > ("cifs: update receive_encrypted_standard to handle compounded responses")
> >
> > which broke the ability to respond to some lease breaks
> > (lease breaks being ignored is a problem since can block
> > server response for duration of the lease break timeout).
> >
> > Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
> > ---
> > fs/cifs/connect.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index f765b20985cd..4307d635bc5d 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -969,6 +969,9 @@ cifs_demultiplex_thread(void *p)
> >
> > server->lstrp = jiffies;
> >
> > + if ((num_mids == 0) && (server->ops->is_oplock_break))
> > + server->ops->is_oplock_break(bufs[0], server);
> > +
> > for (i = 0; i < num_mids; i++) {
> > if (mids[i] != NULL) {
> > mids[i]->resp_buf_size = server->pdu_size;
> > --
> > 2.17.1
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
> adding Ronnie...
>
> The problem here is that we don't increment num_mid for non-enctypted
> connections for lease/oplock breaks (no waiting MID). This should not
> affect encrypted connections - we always increment num_mid there
> event if MID is not found in the list of waiting MIDs.
>
> This itself looks correct but I think there is a cleaner way:
>
> if (server->ops->is_transform_hdr &&
> server->ops->receive_transform &&
> server->ops->is_transform_hdr(buf)) {
> length = server->ops->receive_transform(server, mids, bufs,
> &num_mids); <--- here we always get a correct num_mid
> } else {
> mids[0] = server->ops->find_mid(server, buf);
> bufs[0] = buf;
> if (mids[0]) <--- this check is not needed and we should set num_mids
> to 1 here, so the for loop below will handle it right
> num_mids = 1;
>
> if (!mids[0] || !mids[0]->receive)
> length = standard_receive3(server, mids[0]);
> else
> length = mids[0]->receive(server, mids[0]);
> }
>
> .....
>
> server->lstrp = jiffies;
>
> for (i = 0; i < num_mids; i++) {
> if (mids[i] != NULL) {
>
> ....
>
> } else if (server->ops->is_oplock_break &&
> server->ops->is_oplock_break(bufs[i], server)) { <--- if we eliminate
> the check above this if block will process lease/oplock breaks
> cifs_dbg(FYI, "Received oplock break\n");
> .....
> }
>
> So, I would prefer to remove the check mentioned above. Thoughts?
>
> --
> Best regards,
> Pavel Shilovsky
