On Wed, Jun 21, 2023 at 1:25 PM Tom Talpey <tom@xxxxxxxxxx> wrote:
>
> On 6/20/2023 11:57 PM, Steve French wrote:
> >> Why this value of 10? I would go with 1, since we already reserve 1
> > credit for oplocks. If the reasoning is to have enough credits to send multiple
> > lease/oplock acks, we should change the reserved count altogether.
> >
> > I think there could be some value in sending multiple lease break
> > responses (ie allow oplock credits to be a few more than 1), but my
> > main reasoning for this was to pick some number that was "safe"
> > (allowing 10 oplock/lease-break credits while in flight count is
> > non-zero is unlikely to be a problem) and would be unlikely to change
> > existing behavior.
> >
> > My thinking was that today's code allows oplock credits to be above 1
> > (and keep growing in the server scenario you noticed) while multiple
> > requests continue to be in flight - so there could potentially be a
> > performance benefit during this period of high activity in having a
> > few lease breaks in flight at one time and unlikely to hurt anything -
> > but more importantly if we change the code to never allow oplock/lease
> > credits to be above one we could (unlikely but possible) have subtle
> > behavior changes that trigger a bug (since we would then have cases to
> > at least some servers where we never have two lease breaks in flight).
> > It seemed harmless to set the threshold to something slightly more
> > than one (so multiple lease breaks in flight would still be possible
> > and thus behavior would not change - but risk of credit starvation is
> > gone). If you prefer - I could pick a number like 2 or 3 credits
> > instead of 10. My intent was just to make it extremely unlikely that
> > any behavior would change (but would still fix the possible credit
> > starvation scenario) - so 2 or 3 would also probably be fine.
>
> What do you mean by "oplock credits"? There's no such field in the
> protocol. Is this some sort of reserved pool
The client divides the total credits granted by the server into three
buckets (see struct TCP_Server_Info)
int echo_credits; /* echo reserved slots */
int oplock_credits; /* lease/oplock break reserved slots */
int credits; /* credits reserved for all other operation types */
If we run low on credits we can disable (temporarily) leases and
sending echo requests so we can continue to send other requests (open,
read, write, close etc.). As an example, if the server has granted
us 512 credits (total) if there were 4 large writes that were
responded to very slowly (and used up all of our credits), we could
time out if the write responses were very slow - since we would have
no way of sending an echo request periodically to see if the server
were still alive) - since we have 1 credit reserved for echo requests
though, even if the responses to the writes were slow, we would be
able to ping the server with an echo request to make sure it is still
alive. Similarly (and this can be very important with some servers
who could hold up granting credits if a lease break is pending) - we
have to be able to respond to a lease break even if all of our credits
are used up with large reads or writes so we reserve at least one
credit for sending lease break responses.
The easiest way to think about this is that we reserve 1 credit for
echo and 1 credit for leases (although it can grow larger as we saw in
some server scenarios when they grant us more credits on lease break
responses than we asked for), but all the reset (all other remaining
credits) are available to send read or write or open or close (etc.).
When requests in flight goes back to zero we rebalance credits to
make sure we still have 1 reserved for echo and 1 reserved for
oplock/lease break responses (and everything else for the other
operation types).
The scenario we were having a problem with though was one in which
requests inflight stayed above 0 for a long time and the server often
granted more credits than we asked for on lease break responses - this
caused the number of oplock/lease credits reserved to be larger than
the amount of credits for everything else and eventually starved the
client credits needed for normal operations (this would normally not
be an issue but the number of requests in flight stayed above zero for
a long time which kept us from rebalancing credits, and moving credits
back into the main credit pool from the oplock/lease break reserved
category - which could significantly hurt performance).
There is normally not a problem with having more than one credit in
the lease break pool, but when it grows particularly large it could
hurt performance (or even hang).
> If so, I really don't think any constant is appropriate. If the client
> can't calculate an expected number, we should keep it quite small.
