Re: [PATCH] smb: client: fix delay on concurrent opens

[Paulo Alcantara <pc@manguebit.com>]

Hi Bharath,

Bharath SM <bharathsm.hsk@gmail.com> writes:

> On Mon, May 5, 2025 at 6:42 PM Paulo Alcantara <pc@manguebit.com> wrote:
>>
>> Bharath SM <bharathsm.hsk@gmail.com> writes:
>>
>> > If the file has only deferred handles and a handle lease break occurs,
>> > closing all the handles triggers an implicit acknowledgment. After the
>> > last handle is closed, the server may release the structures
>> > associated with the file handle. If the acknowledgment is sent after
>> > closing all the handles, the server may ignore it or it may return an
>> > invalid file error, as it could have already freed the handle/lease
>> > key and related structures.
>>
>> I couldn't find anything in the specs related to the above.  Could you
>> please point me out to the correct specs or is this just theorical?
>
> Sorry for confusion, this is not from spec. I was trying to say, if we
> remove the check
>  "&& !list_empty(&cinode->openFileList" in code then client may get
> "STATUS_OBJECT_NAME_NOT_FOUND" error when client sends lease break ack after
> closing all file handles. Attached the pcap for ref.

Ah, thanks for the explanation!  Yes, that's indeed a problem.  We
should be calling _cifsFileInfo_put() right after sending the lease
break ack, as the old code did.

>> Have you been able to reproduce the above?  If so, please share the
>> details.
>
> Yes, attached the wireshark capture. please take a look.
> steps:
> 1) Build cifs.ko by removing  "&& !list_empty(&cinode->openFileList)"
> in cifs_oplock_break
> 2) Mount file share with "nosharesock,closetimeo=30" at /mnt/test1 and
> /mnt/test2 ...
> 3) cd  /mnt/test1; echo "aaa" >> testfile
> 4) On other shell, cd /mnt/test2; echo "aaa" >> testfile
>
>> If the server is returning an invalid file error for a lease break ack
>> sent by the client that should be a no-op, isn't that a server bug then?
>
> Windows Server returns STATUS_OBJECT_NAME_NOT_FOUND error code in such cases.
> But I am not sure if this is a server bug.

That's a legitimate error.  See above.

>> > Additionally, this would result in an extra command being sent to the
>> > server. This check was added to avoid this case to send lease break
>> > ack only when openfilelist is not empty.
>>
>> I understand.  The problem with attempting to save that extra roundtrip
>> has caused performance problems with our customers accessing their
>> Windows Server SMB shares.
>
> I agree that we need to fix the customer issue on priority, but just
> pointing out that when
> we remove the existing check we will end up with this behavior. But if
> we can reproduce
> the cx issue or scenario then may be able to find a better fix which
> can handle both cases.?
>
> Please let me know your comments.

Unfortunately I don't have any reproducers for the customer issue.

With these changes I no longer get the STATUS_OBJECT_NAME_NOT_FOUND
error with your reproducer.  Let me know.

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 851b74f557c1..5facc85b408a 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -3083,11 +3083,10 @@ void cifs_oplock_break(struct work_struct *work)
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
 	struct cifs_tcon *tcon;
 	struct TCP_Server_Info *server;
+	bool purge_cache = false;
 	struct tcon_link *tlink;
+	struct cifs_fid *fid;
 	int rc = 0;
-	bool purge_cache = false, oplock_break_cancelled;
-	__u64 persistent_fid, volatile_fid;
-	__u16 net_fid;
 
 	wait_on_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS,
 			TASK_UNINTERRUPTIBLE);
@@ -3134,32 +3133,21 @@ void cifs_oplock_break(struct work_struct *work)
 	 * file handles but cached, then schedule deferred close immediately.
 	 * So, new open will not use cached handle.
 	 */
-
 	if (!CIFS_CACHE_HANDLE(cinode) && !list_empty(&cinode->deferred_closes))
 		cifs_close_deferred_file(cinode);
 
-	persistent_fid = cfile->fid.persistent_fid;
-	volatile_fid = cfile->fid.volatile_fid;
-	net_fid = cfile->fid.netfid;
-	oplock_break_cancelled = cfile->oplock_break_cancelled;
-
-	_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
-	/*
-	 * MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) do not require
-	 * an acknowledgment to be sent when the file has already been closed.
-	 */
-	spin_lock(&cinode->open_file_lock);
-	/* check list empty since can race with kill_sb calling tree disconnect */
-	if (!oplock_break_cancelled && !list_empty(&cinode->openFileList)) {
-		spin_unlock(&cinode->open_file_lock);
-		rc = server->ops->oplock_response(tcon, persistent_fid,
-						  volatile_fid, net_fid, cinode);
+	fid = &cfile->fid;
+	/* MS-SMB2 3.2.5.19.1 and 3.2.5.19.2 (and MS-CIFS 3.2.5.42) */
+	if (!cfile->oplock_break_cancelled) {
+		rc = server->ops->oplock_response(tcon, fid->persistent_fid,
+						  fid->volatile_fid,
+						  fid->netfid, cinode);
 		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
-	} else
-		spin_unlock(&cinode->open_file_lock);
+	}
 
 	cifs_put_tlink(tlink);
 out:
+	_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
 	cifs_done_oplock_break(cinode);
 }