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@xxxxxxxxxx> 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@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
