On 12/8/2023 5:37 AM, Meetakshi Setiya wrote:
Heeding the conversation above, I have updated the patch and added it as
an attachment here. The problem with failing xfstests with this patch was
reusing the lease key for a file to do operations on its hardlink. As per some
investigations I performed on the windows smb server and, in the MS-SMB2 docs,
leases are associated with file (full) path like Tom mentioned. Since
we maintain
lease key with the inode on the client, as a temporary fix, I have added a
check to avoid sending the lease key when a file has hardlinks.
+ /* If there is an existing lease, reuse it */
+
+ /*
+ * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2,
+ * lease keys are associated with the file name. We are maintaining lease keys
+ * with the inode on the client. If the file has hardlinks associated with it,
+ * it could be possible that the lease for a file be reused for an operation
+ * on the hardlink or vice versa. As a temporary fix, skip reusing the
+ * lease if the file has hardlinks.
+ */
+ if (dentry) {
+ inode = d_inode(dentry);
+ cinode = CIFS_I(inode);
+ if (cinode->lease_granted && inode->i_nlink == 1) {
+ oplock = SMB2_OPLOCK_LEVEL_LEASE;
+ memcpy(fid.lease_key, cinode->lease_key, SMB2_LEASE_KEY_SIZE);
+ }
+ }
+
This patch completely removes the fix for any hard linked file! How can
you justify proposing a "temporary fix" for upstream? It will simply
keep the old, problematic, behavior.
Also, the patch description hasn't been changed, and is therefore no
longer accurate.
Nak, in its present form.
Tom.
Thanks
Meetakshi
On Wed, Dec 6, 2023 at 7:38 PM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
Shyam Prasad N <nspmangalore@xxxxxxxxx> writes:
That's interesting. I'm assuming that the STATUS_INVALID_PARAMETER is
due to the server not recognizing the lease id for the file bar.
I'm not sure that this is a client bug.
If the server supports hard links, then should it not be aware that
foo and bar are the same files? AFAIK, file lease is associated with a
file, and not the dentry.
The patch is doing
+ //if there is an existing lease, reuse it
+ if (dentry) {
+ inode = d_inode(dentry);
+ cinode = CIFS_I(inode);
+ if (cinode->lease_granted) {
+ oplock = SMB2_OPLOCK_LEVEL_LEASE;
+ memcpy(fid.lease_key, cinode->lease_key, SMB2_LEASE_KEY_SIZE);
+ }
+ }
and @inode ends up being the same for foo and bar from reproducer. So,
the client is trying to close bar file by using lease key from foo. The
server then fails to match @cinode->lease_key for bar file.
