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

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

[Vincent Whitchurch]

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
-- 
2.28.0

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

[Tom Talpey]

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.

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

[Rohith Surabattula]

This issue will not be seen once changes related to deferred close for
files is committed.
Currently, changes are in review. I will address review comments by this week.

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.

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

[Tom Talpey]

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.
> 

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

[Rohith Surabattula]

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.
Got it.

>> 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.

I mean, there is an active thread for patch "deferred close for
files". As I was busy with other activities, I didn't update the
thread. Will update the thread once i get to it.

Regards,
Rohith

On Wed, Mar 17, 2021 at 12:59 AM Tom Talpey <tom@talpey.com> wrote:
>
> 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.
> >