Please find the attached patch with suggested changes.
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>
>
> Tom.
>
> >> - }
> >> + } else
> >> + spin_unlock(&cinode->open_file_lock);
> >>
> >> cifs_done_oplock_break(cinode);
> >> }
> >> --
> >> 2.34.1
> >>
> >
> >
Attachment:
0001-SMB3-Do-not-send-lease-break-acknowledgment-if-all-f.patch
Description: Binary data
