diff mbox series

SMB3: Do not send lease break acknowledgment if all file handles have been closed

Message ID 20230619033436.808928-1-bharathsm@microsoft.com (mailing list archive)
State New, archived
Headers show
Series SMB3: Do not send lease break acknowledgment if all file handles have been closed | expand

Commit Message

Bharath SM June 19, 2023, 3:34 a.m. UTC
In case if all existing file handles are deferred handles and if all of
them gets closed due to handle lease break then we dont need to send
lease break acknowledgment to server, because last handle close will be
considered as lease break ack.
After closing deferred handels, we check for openfile list of inode,
if its empty then we skip sending lease break ack.

Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
Signed-off-by: Bharath SM <bharathsm@microsoft.com>
---
 fs/smb/client/file.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Steve French June 19, 2023, 4:54 a.m. UTC | #1
tentatively merged into cifs-2.6.git for-next pending more testing

On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>
> In case if all existing file handles are deferred handles and if all of
> them gets closed due to handle lease break then we dont need to send
> lease break acknowledgment to server, because last handle close will be
> considered as lease break ack.
> After closing deferred handels, we check for openfile list of inode,
> if its empty then we skip sending lease break ack.
>
> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> ---
>  fs/smb/client/file.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index 051283386e22..b8a3d60e7ac4 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
>          * not bother sending an oplock release if session to server still is
>          * disconnected since oplock already released by the server
>          */
> -       if (!oplock_break_cancelled) {
> +       spin_lock(&cinode->open_file_lock);
> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
> +               spin_unlock(&cinode->open_file_lock);
>                 /* check for server null since can race with kill_sb calling tree disconnect */
>                 if (tcon->ses && tcon->ses->server) {
>                         rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
>                         cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>                 } else
>                         pr_warn_once("lease break not sent for unmounted share\n");
> -       }
> +       } else
> +               spin_unlock(&cinode->open_file_lock);
>
>         cifs_done_oplock_break(cinode);
>  }
> --
> 2.34.1
>
Tom Talpey June 19, 2023, 12:10 p.m. UTC | #2
On 6/19/2023 12:54 AM, Steve French wrote:
> tentatively merged into cifs-2.6.git for-next pending more testing
> 
> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>>
>> In case if all existing file handles are deferred handles and if all of
>> them gets closed due to handle lease break then we dont need to send
>> lease break acknowledgment to server, because last handle close will be
>> considered as lease break ack.
>> After closing deferred handels, we check for openfile list of inode,
>> if its empty then we skip sending lease break ack.
>>
>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>> ---
>>   fs/smb/client/file.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>> index 051283386e22..b8a3d60e7ac4 100644
>> --- a/fs/smb/client/file.c
>> +++ b/fs/smb/client/file.c
>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
>>           * not bother sending an oplock release if session to server still is
>>           * disconnected since oplock already released by the server
>>           */

The comment just above is a woefully incorrect SMB1 artifact, and
it's even worse now.

Here's what it currently says:

> 	/*
> 	 * releasing stale oplock after recent reconnect of smb session using
> 	 * a now incorrect file handle is not a data integrity issue but do
> 	 * not bother sending an oplock release if session to server still is
> 	 * disconnected since oplock already released by the server
> 	 */

One option is deleting it entirely, but I suggest:

"MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
  an acknowledgement to be sent when the file has already been closed."

>> -       if (!oplock_break_cancelled) {
>> +       spin_lock(&cinode->open_file_lock);
>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
>> +               spin_unlock(&cinode->open_file_lock);
>>                  /* check for server null since can race with kill_sb calling tree disconnect */
>>                  if (tcon->ses && tcon->ses->server) {
>>                          rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
>>                          cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>>                  } else
>>                          pr_warn_once("lease break not sent for unmounted share\n");

Also, I think this warning is entirely pointless, in addition to
being similarly incorrect post-SMB1. Delete it. You will be able
to refactor the if/else branches more clearly in this case too.

With those changes considered,
Reviewed-by: Tom Talpey <tom@talpey.com>

Tom.

>> -       }
>> +       } else
>> +               spin_unlock(&cinode->open_file_lock);
>>
>>          cifs_done_oplock_break(cinode);
>>   }
>> --
>> 2.34.1
>>
> 
>
Bharath SM June 19, 2023, 5:27 p.m. UTC | #3
Please find the attached patch with suggested changes.

On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/19/2023 12:54 AM, Steve French wrote:
> > tentatively merged into cifs-2.6.git for-next pending more testing
> >
> > On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> >>
> >> In case if all existing file handles are deferred handles and if all of
> >> them gets closed due to handle lease break then we dont need to send
> >> lease break acknowledgment to server, because last handle close will be
> >> considered as lease break ack.
> >> After closing deferred handels, we check for openfile list of inode,
> >> if its empty then we skip sending lease break ack.
> >>
> >> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
> >> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >> ---
> >>   fs/smb/client/file.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> >> index 051283386e22..b8a3d60e7ac4 100644
> >> --- a/fs/smb/client/file.c
> >> +++ b/fs/smb/client/file.c
> >> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
> >>           * not bother sending an oplock release if session to server still is
> >>           * disconnected since oplock already released by the server
> >>           */
>
> The comment just above is a woefully incorrect SMB1 artifact, and
> it's even worse now.
>
> Here's what it currently says:
>
> >       /*
> >        * releasing stale oplock after recent reconnect of smb session using
> >        * a now incorrect file handle is not a data integrity issue but do
> >        * not bother sending an oplock release if session to server still is
> >        * disconnected since oplock already released by the server
> >        */
>
> One option is deleting it entirely, but I suggest:
>
> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
>   an acknowledgement to be sent when the file has already been closed."
>
> >> -       if (!oplock_break_cancelled) {
> >> +       spin_lock(&cinode->open_file_lock);
> >> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
> >> +               spin_unlock(&cinode->open_file_lock);
> >>                  /* check for server null since can race with kill_sb calling tree disconnect */
> >>                  if (tcon->ses && tcon->ses->server) {
> >>                          rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
> >> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
> >>                          cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >>                  } else
> >>                          pr_warn_once("lease break not sent for unmounted share\n");
>
> Also, I think this warning is entirely pointless, in addition to
> being similarly incorrect post-SMB1. Delete it. You will be able
> to refactor the if/else branches more clearly in this case too.
>
> With those changes considered,
> Reviewed-by: Tom Talpey <tom@talpey.com>
>
> Tom.
>
> >> -       }
> >> +       } else
> >> +               spin_unlock(&cinode->open_file_lock);
> >>
> >>          cifs_done_oplock_break(cinode);
> >>   }
> >> --
> >> 2.34.1
> >>
> >
> >
Tom Talpey June 19, 2023, 5:41 p.m. UTC | #4
On 6/19/2023 1:27 PM, Bharath SM wrote:
> Please find the attached patch with suggested changes.

LGTM, feel free to add my previous R-B.

Tom.

> On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 6/19/2023 12:54 AM, Steve French wrote:
>>> tentatively merged into cifs-2.6.git for-next pending more testing
>>>
>>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>>>>
>>>> In case if all existing file handles are deferred handles and if all of
>>>> them gets closed due to handle lease break then we dont need to send
>>>> lease break acknowledgment to server, because last handle close will be
>>>> considered as lease break ack.
>>>> After closing deferred handels, we check for openfile list of inode,
>>>> if its empty then we skip sending lease break ack.
>>>>
>>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
>>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>>>> ---
>>>>    fs/smb/client/file.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>>>> index 051283386e22..b8a3d60e7ac4 100644
>>>> --- a/fs/smb/client/file.c
>>>> +++ b/fs/smb/client/file.c
>>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
>>>>            * not bother sending an oplock release if session to server still is
>>>>            * disconnected since oplock already released by the server
>>>>            */
>>
>> The comment just above is a woefully incorrect SMB1 artifact, and
>> it's even worse now.
>>
>> Here's what it currently says:
>>
>>>        /*
>>>         * releasing stale oplock after recent reconnect of smb session using
>>>         * a now incorrect file handle is not a data integrity issue but do
>>>         * not bother sending an oplock release if session to server still is
>>>         * disconnected since oplock already released by the server
>>>         */
>>
>> One option is deleting it entirely, but I suggest:
>>
>> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
>>    an acknowledgement to be sent when the file has already been closed."
>>
>>>> -       if (!oplock_break_cancelled) {
>>>> +       spin_lock(&cinode->open_file_lock);
>>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>                   /* check for server null since can race with kill_sb calling tree disconnect */
>>>>                   if (tcon->ses && tcon->ses->server) {
>>>>                           rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
>>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
>>>>                           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>>>>                   } else
>>>>                           pr_warn_once("lease break not sent for unmounted share\n");
>>
>> Also, I think this warning is entirely pointless, in addition to
>> being similarly incorrect post-SMB1. Delete it. You will be able
>> to refactor the if/else branches more clearly in this case too.
>>
>> With those changes considered,
>> Reviewed-by: Tom Talpey <tom@talpey.com>
>>
>> Tom.
>>
>>>> -       }
>>>> +       } else
>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>
>>>>           cifs_done_oplock_break(cinode);
>>>>    }
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
Shyam Prasad N June 20, 2023, 7:43 a.m. UTC | #5
On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/19/2023 1:27 PM, Bharath SM wrote:
> > Please find the attached patch with suggested changes.
>
> LGTM, feel free to add my previous R-B.
>
> Tom.
>
> > On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 6/19/2023 12:54 AM, Steve French wrote:
> >>> tentatively merged into cifs-2.6.git for-next pending more testing
> >>>
> >>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> >>>>
> >>>> In case if all existing file handles are deferred handles and if all of
> >>>> them gets closed due to handle lease break then we dont need to send
> >>>> lease break acknowledgment to server, because last handle close will be
> >>>> considered as lease break ack.
> >>>> After closing deferred handels, we check for openfile list of inode,
> >>>> if its empty then we skip sending lease break ack.
> >>>>
> >>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
> >>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >>>> ---
> >>>>    fs/smb/client/file.c | 7 +++++--
> >>>>    1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> >>>> index 051283386e22..b8a3d60e7ac4 100644
> >>>> --- a/fs/smb/client/file.c
> >>>> +++ b/fs/smb/client/file.c
> >>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>            * not bother sending an oplock release if session to server still is
> >>>>            * disconnected since oplock already released by the server
> >>>>            */
> >>
> >> The comment just above is a woefully incorrect SMB1 artifact, and
> >> it's even worse now.
> >>
> >> Here's what it currently says:
> >>
> >>>        /*
> >>>         * releasing stale oplock after recent reconnect of smb session using
> >>>         * a now incorrect file handle is not a data integrity issue but do
> >>>         * not bother sending an oplock release if session to server still is
> >>>         * disconnected since oplock already released by the server
> >>>         */
> >>
> >> One option is deleting it entirely, but I suggest:
> >>
> >> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
> >>    an acknowledgement to be sent when the file has already been closed."
> >>
> >>>> -       if (!oplock_break_cancelled) {
> >>>> +       spin_lock(&cinode->open_file_lock);
> >>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
> >>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>                   /* check for server null since can race with kill_sb calling tree disconnect */
> >>>>                   if (tcon->ses && tcon->ses->server) {
> >>>>                           rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
> >>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>                           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >>>>                   } else
> >>>>                           pr_warn_once("lease break not sent for unmounted share\n");
> >>
> >> Also, I think this warning is entirely pointless, in addition to
> >> being similarly incorrect post-SMB1. Delete it. You will be able
> >> to refactor the if/else branches more clearly in this case too.
> >>
> >> With those changes considered,
> >> Reviewed-by: Tom Talpey <tom@talpey.com>
> >>

Hi Tom,

I'm leaning towards having the warning statement here. Although with
more useful details about the inode/lease etc.
If this condition is reached, it means that the cifs_inode still has
at least one handle open.
If that is the case, can the tcon/ses/server ever be NULL?

Regards,
Shyam

> >> Tom.
> >>
> >>>> -       }
> >>>> +       } else
> >>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>
> >>>>           cifs_done_oplock_break(cinode);
> >>>>    }
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>
> >>>
Tom Talpey June 20, 2023, 2:02 p.m. UTC | #6
On 6/20/2023 3:43 AM, Shyam Prasad N wrote:
> On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 6/19/2023 1:27 PM, Bharath SM wrote:
>>> Please find the attached patch with suggested changes.
>>
>> LGTM, feel free to add my previous R-B.
>>
>> Tom.
>>
>>> On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>> On 6/19/2023 12:54 AM, Steve French wrote:
>>>>> tentatively merged into cifs-2.6.git for-next pending more testing
>>>>>
>>>>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>>>>>>
>>>>>> In case if all existing file handles are deferred handles and if all of
>>>>>> them gets closed due to handle lease break then we dont need to send
>>>>>> lease break acknowledgment to server, because last handle close will be
>>>>>> considered as lease break ack.
>>>>>> After closing deferred handels, we check for openfile list of inode,
>>>>>> if its empty then we skip sending lease break ack.
>>>>>>
>>>>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
>>>>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>>>>>> ---
>>>>>>     fs/smb/client/file.c | 7 +++++--
>>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>>>>>> index 051283386e22..b8a3d60e7ac4 100644
>>>>>> --- a/fs/smb/client/file.c
>>>>>> +++ b/fs/smb/client/file.c
>>>>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
>>>>>>             * not bother sending an oplock release if session to server still is
>>>>>>             * disconnected since oplock already released by the server
>>>>>>             */
>>>>
>>>> The comment just above is a woefully incorrect SMB1 artifact, and
>>>> it's even worse now.
>>>>
>>>> Here's what it currently says:
>>>>
>>>>>         /*
>>>>>          * releasing stale oplock after recent reconnect of smb session using
>>>>>          * a now incorrect file handle is not a data integrity issue but do
>>>>>          * not bother sending an oplock release if session to server still is
>>>>>          * disconnected since oplock already released by the server
>>>>>          */
>>>>
>>>> One option is deleting it entirely, but I suggest:
>>>>
>>>> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
>>>>     an acknowledgement to be sent when the file has already been closed."
>>>>
>>>>>> -       if (!oplock_break_cancelled) {
>>>>>> +       spin_lock(&cinode->open_file_lock);
>>>>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
>>>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>>>                    /* check for server null since can race with kill_sb calling tree disconnect */
>>>>>>                    if (tcon->ses && tcon->ses->server) {
>>>>>>                            rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
>>>>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
>>>>>>                            cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>>>>>>                    } else
>>>>>>                            pr_warn_once("lease break not sent for unmounted share\n");
>>>>
>>>> Also, I think this warning is entirely pointless, in addition to
>>>> being similarly incorrect post-SMB1. Delete it. You will be able
>>>> to refactor the if/else branches more clearly in this case too.
>>>>
>>>> With those changes considered,
>>>> Reviewed-by: Tom Talpey <tom@talpey.com>
>>>>
> 
> Hi Tom,
> 
> I'm leaning towards having the warning statement here. Although with
> more useful details about the inode/lease etc.
> If this condition is reached, it means that the cifs_inode still has
> at least one handle open.
> If that is the case, can the tcon/ses/server ever be NULL?

I don't agree, my reading is that this is a race condition with
an unmount, and the tree connect and/or session is being torn
down. So I don't see the point in whining to the syslog.

Besides, there's nothing the client can do to recover, or prevent
the situation. Why alarm the admin? What "useful" details would
impact this?

Tom.

> 
> Regards,
> Shyam
> 
>>>> Tom.
>>>>
>>>>>> -       }
>>>>>> +       } else
>>>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>>>
>>>>>>            cifs_done_oplock_break(cinode);
>>>>>>     }
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>>>
> 
> 
>
Steve French June 21, 2023, 3:34 a.m. UTC | #7
We could make the unlikely error condition (lease break race with
umount) log as cifsFYI so no one would see it by default?

On Tue, Jun 20, 2023 at 9:02 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/20/2023 3:43 AM, Shyam Prasad N wrote:
> > On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 6/19/2023 1:27 PM, Bharath SM wrote:
> >>> Please find the attached patch with suggested changes.
> >>
> >> LGTM, feel free to add my previous R-B.
> >>
> >> Tom.
> >>
> >>> On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
> >>>>
> >>>> On 6/19/2023 12:54 AM, Steve French wrote:
> >>>>> tentatively merged into cifs-2.6.git for-next pending more testing
> >>>>>
> >>>>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> >>>>>>
> >>>>>> In case if all existing file handles are deferred handles and if all of
> >>>>>> them gets closed due to handle lease break then we dont need to send
> >>>>>> lease break acknowledgment to server, because last handle close will be
> >>>>>> considered as lease break ack.
> >>>>>> After closing deferred handels, we check for openfile list of inode,
> >>>>>> if its empty then we skip sending lease break ack.
> >>>>>>
> >>>>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
> >>>>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >>>>>> ---
> >>>>>>     fs/smb/client/file.c | 7 +++++--
> >>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> >>>>>> index 051283386e22..b8a3d60e7ac4 100644
> >>>>>> --- a/fs/smb/client/file.c
> >>>>>> +++ b/fs/smb/client/file.c
> >>>>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>>>             * not bother sending an oplock release if session to server still is
> >>>>>>             * disconnected since oplock already released by the server
> >>>>>>             */
> >>>>
> >>>> The comment just above is a woefully incorrect SMB1 artifact, and
> >>>> it's even worse now.
> >>>>
> >>>> Here's what it currently says:
> >>>>
> >>>>>         /*
> >>>>>          * releasing stale oplock after recent reconnect of smb session using
> >>>>>          * a now incorrect file handle is not a data integrity issue but do
> >>>>>          * not bother sending an oplock release if session to server still is
> >>>>>          * disconnected since oplock already released by the server
> >>>>>          */
> >>>>
> >>>> One option is deleting it entirely, but I suggest:
> >>>>
> >>>> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
> >>>>     an acknowledgement to be sent when the file has already been closed."
> >>>>
> >>>>>> -       if (!oplock_break_cancelled) {
> >>>>>> +       spin_lock(&cinode->open_file_lock);
> >>>>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
> >>>>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>>>                    /* check for server null since can race with kill_sb calling tree disconnect */
> >>>>>>                    if (tcon->ses && tcon->ses->server) {
> >>>>>>                            rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
> >>>>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>>>                            cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >>>>>>                    } else
> >>>>>>                            pr_warn_once("lease break not sent for unmounted share\n");
> >>>>
> >>>> Also, I think this warning is entirely pointless, in addition to
> >>>> being similarly incorrect post-SMB1. Delete it. You will be able
> >>>> to refactor the if/else branches more clearly in this case too.
> >>>>
> >>>> With those changes considered,
> >>>> Reviewed-by: Tom Talpey <tom@talpey.com>
> >>>>
> >
> > Hi Tom,
> >
> > I'm leaning towards having the warning statement here. Although with
> > more useful details about the inode/lease etc.
> > If this condition is reached, it means that the cifs_inode still has
> > at least one handle open.
> > If that is the case, can the tcon/ses/server ever be NULL?
>
> I don't agree, my reading is that this is a race condition with
> an unmount, and the tree connect and/or session is being torn
> down. So I don't see the point in whining to the syslog.
>
> Besides, there's nothing the client can do to recover, or prevent
> the situation. Why alarm the admin? What "useful" details would
> impact this?
>
> Tom.
>
> >
> > Regards,
> > Shyam
> >
> >>>> Tom.
> >>>>
> >>>>>> -       }
> >>>>>> +       } else
> >>>>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>>>
> >>>>>>            cifs_done_oplock_break(cinode);
> >>>>>>     }
> >>>>>> --
> >>>>>> 2.34.1
> >>>>>>
> >>>>>
> >>>>>
> >
> >
> >
Tom Talpey June 21, 2023, 4:08 p.m. UTC | #8
On 6/20/2023 11:34 PM, Steve French wrote:
> We could make the unlikely error condition (lease break race with
> umount) log as cifsFYI so no one would see it by default?

You guys obviously want the noise to stay. So, yes I'd agree that
a cifsFYI is better than a pr_warn_once. The FYI is silenced by
default, and it will appear every time if wanted.

Tom.

> On Tue, Jun 20, 2023 at 9:02 AM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 6/20/2023 3:43 AM, Shyam Prasad N wrote:
>>> On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>> On 6/19/2023 1:27 PM, Bharath SM wrote:
>>>>> Please find the attached patch with suggested changes.
>>>>
>>>> LGTM, feel free to add my previous R-B.
>>>>
>>>> Tom.
>>>>
>>>>> On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
>>>>>>
>>>>>> On 6/19/2023 12:54 AM, Steve French wrote:
>>>>>>> tentatively merged into cifs-2.6.git for-next pending more testing
>>>>>>>
>>>>>>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>>>>>>>>
>>>>>>>> In case if all existing file handles are deferred handles and if all of
>>>>>>>> them gets closed due to handle lease break then we dont need to send
>>>>>>>> lease break acknowledgment to server, because last handle close will be
>>>>>>>> considered as lease break ack.
>>>>>>>> After closing deferred handels, we check for openfile list of inode,
>>>>>>>> if its empty then we skip sending lease break ack.
>>>>>>>>
>>>>>>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
>>>>>>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>>>>>>>> ---
>>>>>>>>      fs/smb/client/file.c | 7 +++++--
>>>>>>>>      1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>>>>>>>> index 051283386e22..b8a3d60e7ac4 100644
>>>>>>>> --- a/fs/smb/client/file.c
>>>>>>>> +++ b/fs/smb/client/file.c
>>>>>>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
>>>>>>>>              * not bother sending an oplock release if session to server still is
>>>>>>>>              * disconnected since oplock already released by the server
>>>>>>>>              */
>>>>>>
>>>>>> The comment just above is a woefully incorrect SMB1 artifact, and
>>>>>> it's even worse now.
>>>>>>
>>>>>> Here's what it currently says:
>>>>>>
>>>>>>>          /*
>>>>>>>           * releasing stale oplock after recent reconnect of smb session using
>>>>>>>           * a now incorrect file handle is not a data integrity issue but do
>>>>>>>           * not bother sending an oplock release if session to server still is
>>>>>>>           * disconnected since oplock already released by the server
>>>>>>>           */
>>>>>>
>>>>>> One option is deleting it entirely, but I suggest:
>>>>>>
>>>>>> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
>>>>>>      an acknowledgement to be sent when the file has already been closed."
>>>>>>
>>>>>>>> -       if (!oplock_break_cancelled) {
>>>>>>>> +       spin_lock(&cinode->open_file_lock);
>>>>>>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
>>>>>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>>>>>                     /* check for server null since can race with kill_sb calling tree disconnect */
>>>>>>>>                     if (tcon->ses && tcon->ses->server) {
>>>>>>>>                             rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
>>>>>>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
>>>>>>>>                             cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>>>>>>>>                     } else
>>>>>>>>                             pr_warn_once("lease break not sent for unmounted share\n");
>>>>>>
>>>>>> Also, I think this warning is entirely pointless, in addition to
>>>>>> being similarly incorrect post-SMB1. Delete it. You will be able
>>>>>> to refactor the if/else branches more clearly in this case too.
>>>>>>
>>>>>> With those changes considered,
>>>>>> Reviewed-by: Tom Talpey <tom@talpey.com>
>>>>>>
>>>
>>> Hi Tom,
>>>
>>> I'm leaning towards having the warning statement here. Although with
>>> more useful details about the inode/lease etc.
>>> If this condition is reached, it means that the cifs_inode still has
>>> at least one handle open.
>>> If that is the case, can the tcon/ses/server ever be NULL?
>>
>> I don't agree, my reading is that this is a race condition with
>> an unmount, and the tree connect and/or session is being torn
>> down. So I don't see the point in whining to the syslog.
>>
>> Besides, there's nothing the client can do to recover, or prevent
>> the situation. Why alarm the admin? What "useful" details would
>> impact this?
>>
>> Tom.
>>
>>>
>>> Regards,
>>> Shyam
>>>
>>>>>> Tom.
>>>>>>
>>>>>>>> -       }
>>>>>>>> +       } else
>>>>>>>> +               spin_unlock(&cinode->open_file_lock);
>>>>>>>>
>>>>>>>>             cifs_done_oplock_break(cinode);
>>>>>>>>      }
>>>>>>>> --
>>>>>>>> 2.34.1
>>>>>>>>
>>>>>>>
>>>>>>>
>>>
>>>
>>>
> 
> 
>
Steve French June 21, 2023, 4:21 p.m. UTC | #9
In discussing with Bharath today he reminded me that the message would
be noisier than for unmount races - so I lean toward leaving it as is
(ie no extra debug message for this path - ie the current version of
the patch in for-next, which matches Tom's suggestion)

On Wed, Jun 21, 2023 at 11:08 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/20/2023 11:34 PM, Steve French wrote:
> > We could make the unlikely error condition (lease break race with
> > umount) log as cifsFYI so no one would see it by default?
>
> You guys obviously want the noise to stay. So, yes I'd agree that
> a cifsFYI is better than a pr_warn_once. The FYI is silenced by
> default, and it will appear every time if wanted.
>
> Tom.
>
> > On Tue, Jun 20, 2023 at 9:02 AM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 6/20/2023 3:43 AM, Shyam Prasad N wrote:
> >>> On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@talpey.com> wrote:
> >>>>
> >>>> On 6/19/2023 1:27 PM, Bharath SM wrote:
> >>>>> Please find the attached patch with suggested changes.
> >>>>
> >>>> LGTM, feel free to add my previous R-B.
> >>>>
> >>>> Tom.
> >>>>
> >>>>> On Mon, Jun 19, 2023 at 5:40 PM Tom Talpey <tom@talpey.com> wrote:
> >>>>>>
> >>>>>> On 6/19/2023 12:54 AM, Steve French wrote:
> >>>>>>> tentatively merged into cifs-2.6.git for-next pending more testing
> >>>>>>>
> >>>>>>> On Sun, Jun 18, 2023 at 10:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> In case if all existing file handles are deferred handles and if all of
> >>>>>>>> them gets closed due to handle lease break then we dont need to send
> >>>>>>>> lease break acknowledgment to server, because last handle close will be
> >>>>>>>> considered as lease break ack.
> >>>>>>>> After closing deferred handels, we check for openfile list of inode,
> >>>>>>>> if its empty then we skip sending lease break ack.
> >>>>>>>>
> >>>>>>>> Fixes: 59a556aebc43 ("SMB3: drop reference to cfile before sending oplock break")
> >>>>>>>> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >>>>>>>> ---
> >>>>>>>>      fs/smb/client/file.c | 7 +++++--
> >>>>>>>>      1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> >>>>>>>> index 051283386e22..b8a3d60e7ac4 100644
> >>>>>>>> --- a/fs/smb/client/file.c
> >>>>>>>> +++ b/fs/smb/client/file.c
> >>>>>>>> @@ -4941,7 +4941,9 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>>>>>              * not bother sending an oplock release if session to server still is
> >>>>>>>>              * disconnected since oplock already released by the server
> >>>>>>>>              */
> >>>>>>
> >>>>>> The comment just above is a woefully incorrect SMB1 artifact, and
> >>>>>> it's even worse now.
> >>>>>>
> >>>>>> Here's what it currently says:
> >>>>>>
> >>>>>>>          /*
> >>>>>>>           * releasing stale oplock after recent reconnect of smb session using
> >>>>>>>           * a now incorrect file handle is not a data integrity issue but do
> >>>>>>>           * not bother sending an oplock release if session to server still is
> >>>>>>>           * disconnected since oplock already released by the server
> >>>>>>>           */
> >>>>>>
> >>>>>> One option is deleting it entirely, but I suggest:
> >>>>>>
> >>>>>> "MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
> >>>>>>      an acknowledgement to be sent when the file has already been closed."
> >>>>>>
> >>>>>>>> -       if (!oplock_break_cancelled) {
> >>>>>>>> +       spin_lock(&cinode->open_file_lock);
> >>>>>>>> +       if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
> >>>>>>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>>>>>                     /* check for server null since can race with kill_sb calling tree disconnect */
> >>>>>>>>                     if (tcon->ses && tcon->ses->server) {
> >>>>>>>>                             rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
> >>>>>>>> @@ -4949,7 +4951,8 @@ void cifs_oplock_break(struct work_struct *work)
> >>>>>>>>                             cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >>>>>>>>                     } else
> >>>>>>>>                             pr_warn_once("lease break not sent for unmounted share\n");
> >>>>>>
> >>>>>> Also, I think this warning is entirely pointless, in addition to
> >>>>>> being similarly incorrect post-SMB1. Delete it. You will be able
> >>>>>> to refactor the if/else branches more clearly in this case too.
> >>>>>>
> >>>>>> With those changes considered,
> >>>>>> Reviewed-by: Tom Talpey <tom@talpey.com>
> >>>>>>
> >>>
> >>> Hi Tom,
> >>>
> >>> I'm leaning towards having the warning statement here. Although with
> >>> more useful details about the inode/lease etc.
> >>> If this condition is reached, it means that the cifs_inode still has
> >>> at least one handle open.
> >>> If that is the case, can the tcon/ses/server ever be NULL?
> >>
> >> I don't agree, my reading is that this is a race condition with
> >> an unmount, and the tree connect and/or session is being torn
> >> down. So I don't see the point in whining to the syslog.
> >>
> >> Besides, there's nothing the client can do to recover, or prevent
> >> the situation. Why alarm the admin? What "useful" details would
> >> impact this?
> >>
> >> Tom.
> >>
> >>>
> >>> Regards,
> >>> Shyam
> >>>
> >>>>>> Tom.
> >>>>>>
> >>>>>>>> -       }
> >>>>>>>> +       } else
> >>>>>>>> +               spin_unlock(&cinode->open_file_lock);
> >>>>>>>>
> >>>>>>>>             cifs_done_oplock_break(cinode);
> >>>>>>>>      }
> >>>>>>>> --
> >>>>>>>> 2.34.1
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>
> >>>
> >>>
> >
> >
> >
diff mbox series

Patch

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 051283386e22..b8a3d60e7ac4 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -4941,7 +4941,9 @@  void cifs_oplock_break(struct work_struct *work)
 	 * not bother sending an oplock release if session to server still is
 	 * disconnected since oplock already released by the server
 	 */
-	if (!oplock_break_cancelled) {
+	spin_lock(&cinode->open_file_lock);
+	if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
+		spin_unlock(&cinode->open_file_lock);
 		/* check for server null since can race with kill_sb calling tree disconnect */
 		if (tcon->ses && tcon->ses->server) {
 			rc = tcon->ses->server->ops->oplock_response(tcon, persistent_fid,
@@ -4949,7 +4951,8 @@  void cifs_oplock_break(struct work_struct *work)
 			cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
 		} else
 			pr_warn_once("lease break not sent for unmounted share\n");
-	}
+	} else
+		spin_unlock(&cinode->open_file_lock);
 
 	cifs_done_oplock_break(cinode);
 }