On Mon, Jun 19, 2023 at 11:11 PM Tom Talpey <tom@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxx>
> >>
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
> >>>>
> >>>
> >>>
--
Regards,
Shyam
