Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Tom Talpey <tom@talpey.com>]

On 6/26/2023 2:33 AM, Shyam Prasad N wrote:
> On Fri, Jun 23, 2023 at 9:52 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 6/11/2023 4:01 AM, Shyam Prasad N wrote:
>>> On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote:
>>>>
>>>> should this be a warn once? Could it get very noisy?
>>>>
>>>> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>>>>
>>>>> We've seen the in-flight count go into negative with some
>>>>> internal stress testing in Microsoft.
>>>>>
>>>>> Adding a WARN when this happens, in hope of understanding
>>>>> why this happens when it happens.
>>>>>
>>>>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
>>>>> ---
>>>>>    fs/smb/client/smb2ops.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>>>>> index 6e3be58cfe49..43162915e03c 100644
>>>>> --- a/fs/smb/client/smb2ops.c
>>>>> +++ b/fs/smb/client/smb2ops.c
>>>>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
>>>>>                                               server->conn_id, server->hostname, *val,
>>>>>                                               add, server->in_flight);
>>>>>           }
>>>>> +       WARN_ON(server->in_flight == 0);
>>>>>           server->in_flight--;
>>>>>           if (server->in_flight == 0 &&
>>>>>              ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
>>>>> --
>>>>> 2.34.1
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>>
>>>> Steve
>>>
>>> Makes sense. We can have a warn once.
>>
>> Which sounds great, but isn't this connection basically toast?
>> It's not super helpful to just whine. Why not clamp it at zero?
>>
>> Tom.
> 
> So there's no "legal" way that this count can go negative.
> If it has, that's definitely because there's a bug. The WARN will
> hopefully help us catch and fix the bug.

To be clear, I'm ok with the warn, it's the fact that the code
goes to all the trouble to say it but doesn't do anything to
recover.

> We could also have a clamp at 0. I'll send an updated patch.

Sounds good.

Tom.