Re: [PATCH v2] cifs: Silently ignore unknown oplock break handle

[Tom Talpey <tom@talpey.com>]

On 3/16/2021 1:36 PM, Rohith Surabattula wrote:
> This issue will not be seen once changes related to deferred close for
> files is committed.

That may be, but it's irrelevant to this.

> Currently, changes are in review. I will address review comments by this week.

What do you mean by "in review"? Both threads are active on the
mailing list. If you or others have something to discuss, please
post it and don't leave us out of the discussion.

Tom.


> Regards,
> Rohith
> 
> On Tue, Mar 16, 2021 at 9:33 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 3/16/2021 8:48 AM, Vincent Whitchurch via samba-technical wrote:
>>> Make SMB2 not print out an error when an oplock break is received for an
>>> unknown handle, similar to SMB1.  The SMB2 lease break path is not
>>> affected by this patch.
>>>
>>> Without this, a program which writes to a file from one thread, and
>>> opens, reads, and writes the same file from another thread triggers the
>>> below errors several times a minute when run against a Samba server
>>> configured with "smb2 leases = no".
>>>
>>>    CIFS: VFS: \\192.168.0.1 No task to wake, unknown frame received! NumMids 2
>>>    00000000: 424d53fe 00000040 00000000 00000012  .SMB@...........
>>>    00000010: 00000001 00000000 ffffffff ffffffff  ................
>>>    00000020: 00000000 00000000 00000000 00000000  ................
>>>    00000030: 00000000 00000000 00000000 00000000  ................
>>>
>>> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>>> ---
>>>
>>> Notes:
>>>       v2:
>>>       - Drop change to lease break
>>>       - Rewrite commit message
>>>
>>>    fs/cifs/smb2misc.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
>>> index 60d4bd1eae2b..4d8576e202e3 100644
>>> --- a/fs/cifs/smb2misc.c
>>> +++ b/fs/cifs/smb2misc.c
>>> @@ -755,7 +755,7 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server)
>>>        }
>>>        spin_unlock(&cifs_tcp_ses_lock);
>>>        cifs_dbg(FYI, "Can not process oplock break for non-existent connection\n");
>>> -     return false;
>>> +     return true;
>>>    }
>>>
>>>    void
>>>
>>
>> As an oplock-only approach, it looks good. But the old cifs_dbg message
>> "non-existent connection" is possibly misleading, since the connection
>> may be perfectly fine.
>>
>> When breaking the loop successfully, the code emits
>>          cifs_dbg(FYI, "file id match, oplock break\n");
>> so perhaps
>>          cifs_dbg(FYI, "No file id matched, oplock break ignored\n");
>> ?
>>
>> Tom.
>