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@xxxxxxxxxx> wrote:
>
> On 6/20/2023 3:43 AM, Shyam Prasad N wrote:
> > 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?
>
> 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
> >>>>>>
> >>>>>
> >>>>>
> >
> >
> >
--
Thanks,
Steve
