Re: [PATCH 06/11] CIFS: Respect MaxMpxCount field

[Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

On Tue, 28 Feb 2012 15:18:02 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Feb 28, 2012 at 2:53 PM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> > 2012/2/29 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> Attempt to summarize after discussions with jra and Chris:
> >>
> >> - don't make default case to Samba worse performance than it already is
> >> - don't "fix" by adding restrictions on echo retries and blocking
> >> locks when it is not broken (windows allows these, and counts blocking
> >> locks against the pid not against the session).   For servers which
> >> allow few mpx, perhaps those for 10 or fewer - if you think they are
> >> buggy, I don't mind treating echo and blocking locks differently than
> >> we do now - but echo and blocking locks don't need to be restricted
> >> the way Pavel suggested to windows and samba and any normal server
> >> (and there is some risk in restricting them that way).   We could
> >> probably limit the number of blocking locks (out of resource error,
> >> returning ENOLOCK) when there are 50 (e.g.) on a process (or 10 if the
> >> server supports maxmpx of 10, and zero if maxmpx is less than 3 or
> >> less than 10).
> >> - for other smb requests (the ones we enforce today) enforce the limit
> >> the server returns (rather than 50 always)
> >>
> >
> > But for SMB2 we need to count both echos and blocking locks. It even
> > more compicated because the number of credits is dynamic: we need to
> > increment/decrement the maximum number of parallel blocking locks to
> > make sure we don't exceed available credits.
> 
> Yes - for smb2 the number of available requests (credits) can change
> on ANY smb2 response.  It is not related to how cifs requests are
> processed.
> 
> Also note that for cifs for Windows, apparently the smb echo is
> handled at a lower level (before checking mpx) - (and also Samba
> server does not enforce either).
> 

(cc'ing Chris and Jeremy to make sure I understand)

The description above is a good start but doesn't quite outline the
clear concise "rules" for this that I was looking for.

So if I understand what Steve wrote above, he's basically saying that
we should enforce the maxmpx for everything but blocking locks and
echoes? Is that correct? If so then I think that's wrong and won't fix
any of the problems that people have reported with the existing code.

It's all well and good that *some* servers allow you to exceed the
maxmpx in *some* cases, but we can't code to that assumption. We know
well that many servers do not handle exceeding the maxmpx gracefully at
all. As always we have to code to the lowest common denominator by
default, and then optionally allow people to exceed that if they choose
to do so.

I think this is a case where we need a good description in a human
language (preferably english) of how this should work (aka a
specification), and then write code to match that description (aka code
to the spec).

Anything else is going to send us down the rabbit hole where we are
today with cifs.ko -- a bunch of ad-hoc, broken code that no one really
understands.

While we may not like it, a hard cap on the number of outstanding
requests is required by the protocol docs that Chris helped write.

Allow me to quote from MS-CIFS:

MaxMpxCount (2 bytes): The maximum number of outstanding SMB operations
that the server supports. This value includes existing OpLocks, the
NT_TRANSACT_NOTIFY_CHANGE subcommand, and any other commands that are
pending on the server.

There's nothing in there that says anything about certain commands
being excepted from that value.
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>