[PATCH 1/2] smb: client: reuse file lease key in compound operations

[PATCH 1/2] smb: client: reuse file lease key in compound operations

[meetakshisetiyaoss]

From: Meetakshi Setiya <msetiya@microsoft.com>

Currently, when a rename, unlink or set path size compound operation
is requested on a file that has a lot of dirty pages to be written
to the server, we do not send the lease key for these requests. As a
result, the server can assume that this request is from a new client, and
send a lease break notification to the same client, on the same
connection. As a response to the lease break, the client can consume
several credits to write the dirty pages to the server. Depending on the
server's credit grant implementation, the server can stop granting more
credits to this connection, and this can cause a deadlock (which can only
be resolved when the lease timer on the server expires).
One of the problems here is that the client is sending no lease key,
even if it has a lease for the file. This patch fixes the problem by
reusing the existing lease key on the file for rename, unlink and set path
size compound operations so that the client does not break its own lease.

A very trivial example could be a set of commands by a client that
maintains open handle (for write) to a file and then tries to rename
the file, eg.,

tail -f /dev/null > myfile &
mv myfile myfile2

Presently, the network capture on the client shows that the move (or
rename) would trigger a lease break on the same client, for the same file.
With the lease key reused, the lease break request-response overhead is
eliminated, thereby reducing the roundtrips performed for this set of
operations.

This patch provides perf benefit by reusing the lease key for rename,
unlink, and set path size operations. It avoids self-inflicted lease
breaks, reduces roundtrips and prevents potential deadlocks. It
leads to more efficient operations, especially when writing many dirty
pages to the server.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/cifsglob.h  |  4 +--
 fs/smb/client/cifsproto.h |  4 +--
 fs/smb/client/cifssmb.c   |  4 +--
 fs/smb/client/inode.c     | 10 ++++----
 fs/smb/client/smb2inode.c | 52 ++++++++++++++++++++++++---------------
 fs/smb/client/smb2proto.h |  4 +--
 6 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 54bc44a5f4c6..8ccfb83ffb53 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -364,7 +364,7 @@ struct smb_version_operations {
 			    struct cifs_open_info_data *data);
 	/* set size by path */
 	int (*set_path_size)(const unsigned int, struct cifs_tcon *,
-			     const char *, __u64, struct cifs_sb_info *, bool);
+			     const char *, __u64, struct cifs_sb_info *, bool, struct dentry *);
 	/* set size by file handle */
 	int (*set_file_size)(const unsigned int, struct cifs_tcon *,
 			     struct cifsFileInfo *, __u64, bool);
@@ -394,7 +394,7 @@ struct smb_version_operations {
 		     struct cifs_sb_info *);
 	/* unlink file */
 	int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
-		      struct cifs_sb_info *);
+		      struct cifs_sb_info *, struct dentry *);
 	/* open, rename and delete file */
 	int (*rename_pending_delete)(const char *, struct dentry *,
 				     const unsigned int);
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index afbab86331a1..7bf1d04857b0 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -402,7 +402,7 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
 				     __u32 pid_of_opener);
 extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
 			 const char *file_name, __u64 size,
-			 struct cifs_sb_info *cifs_sb, bool set_allocation);
+			 struct cifs_sb_info *cifs_sb, bool set_allocation, struct dentry *dentry);
 extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
 			      struct cifsFileInfo *cfile, __u64 size,
 			      bool set_allocation);
@@ -438,7 +438,7 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
 			const struct nls_table *nls_codepage,
 			int remap_special_chars);
 extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
-			  const char *name, struct cifs_sb_info *cifs_sb);
+			  const char *name, struct cifs_sb_info *cifs_sb, struct dentry *dentry);
 int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
 		  struct dentry *source_dentry,
 		  const char *from_name, const char *to_name,
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index e9e33b0b3ac4..1941ceb48fb7 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
 
 int
 CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
-	       struct cifs_sb_info *cifs_sb)
+	       struct cifs_sb_info *cifs_sb, struct dentry *dentry)
 {
 	DELETE_FILE_REQ *pSMB = NULL;
 	DELETE_FILE_RSP *pSMBr = NULL;
@@ -4983,7 +4983,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
 int
 CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
 	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
-	      bool set_allocation)
+	      bool set_allocation, struct dentry *dentry)
 {
 	struct smb_com_transaction2_spi_req *pSMB = NULL;
 	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index c532aa63a658..a71068bda2b4 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1851,7 +1851,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
 		goto psx_del_no_retry;
 	}
 
-	rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
+	rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
 
 psx_del_no_retry:
 	if (!rc) {
@@ -2802,7 +2802,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
 
 static int
 cifs_set_file_size(struct inode *inode, struct iattr *attrs,
-		   unsigned int xid, const char *full_path)
+		   unsigned int xid, const char *full_path, struct dentry *dentry)
 {
 	int rc;
 	struct cifsFileInfo *open_file;
@@ -2853,7 +2853,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	 */
 	if (server->ops->set_path_size)
 		rc = server->ops->set_path_size(xid, tcon, full_path,
-						attrs->ia_size, cifs_sb, false);
+						attrs->ia_size, cifs_sb, false, dentry);
 	else
 		rc = -ENOSYS;
 	cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
@@ -2943,7 +2943,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
-		rc = cifs_set_file_size(inode, attrs, xid, full_path);
+		rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
 		if (rc != 0)
 			goto out;
 	}
@@ -3109,7 +3109,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	}
 
 	if (attrs->ia_valid & ATTR_SIZE) {
-		rc = cifs_set_file_size(inode, attrs, xid, full_path);
+		rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
 		if (rc != 0)
 			goto cifs_setattr_exit;
 	}
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 5053a5550abe..446433606a04 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -70,7 +70,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 			    __u32 create_options, umode_t mode, struct kvec *in_iov,
 			    int *cmds, int num_cmds, struct cifsFileInfo *cfile,
 			    __u8 **extbuf, size_t *extbuflen,
-			    struct kvec *out_iov, int *out_buftype)
+			    struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
 {
 
 	struct reparse_data_buffer *rbuf;
@@ -87,6 +87,8 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 	int resp_buftype[MAX_COMPOUND];
 	struct smb2_query_info_rsp *qi_rsp = NULL;
 	struct cifs_open_info_data *idata;
+	struct inode *inode = NULL;
+	struct cifsInodeInfo *cinode = NULL;
 	int flags = 0;
 	__u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
 	unsigned int size[2];
@@ -118,6 +120,16 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		goto finished;
 	}
 
+	/* 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);
+		}
+	}
+
 	vars->oparms = (struct cifs_open_parms) {
 		.tcon = tcon,
 		.path = full_path,
@@ -695,7 +707,7 @@ int smb2_query_path_info(const unsigned int xid,
 			      FILE_READ_ATTRIBUTES, FILE_OPEN,
 			      create_options, ACL_NO_MODE,
 			      in_iov, cmds, 1, cfile,
-			      NULL, NULL, out_iov, out_buftype);
+			      NULL, NULL, out_iov, out_buftype, NULL);
 	hdr = out_iov[0].iov_base;
 	/*
 	 * If first iov is unset, then SMB session was dropped or we've got a
@@ -723,7 +735,7 @@ int smb2_query_path_info(const unsigned int xid,
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				      FILE_READ_ATTRIBUTES, FILE_OPEN,
 				      create_options, ACL_NO_MODE, in_iov, cmds,
-				      num_cmds, cfile, NULL, NULL, NULL, NULL);
+				      num_cmds, cfile, NULL, NULL, NULL, NULL, NULL);
 		break;
 	case -EREMOTE:
 		break;
@@ -783,7 +795,7 @@ int smb311_posix_query_path_info(const unsigned int xid,
 	rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 			      FILE_READ_ATTRIBUTES, FILE_OPEN,
 			      create_options, ACL_NO_MODE, in_iov, cmds, 1,
-			      cfile, &sidsbuf, &sidsbuflen, out_iov, out_buftype);
+			      cfile, &sidsbuf, &sidsbuflen, out_iov, out_buftype, NULL);
 	/*
 	 * If first iov is unset, then SMB session was dropped or we've got a
 	 * cached open file (@cfile).
@@ -811,7 +823,7 @@ int smb311_posix_query_path_info(const unsigned int xid,
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				      FILE_READ_ATTRIBUTES, FILE_OPEN,
 				      create_options, ACL_NO_MODE, in_iov, cmds,
-				      num_cmds, cfile, &sidsbuf, &sidsbuflen, NULL, NULL);
+				      num_cmds, cfile, &sidsbuf, &sidsbuflen, NULL, NULL, NULL);
 		break;
 	}
 
@@ -850,7 +862,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
 				FILE_WRITE_ATTRIBUTES, FILE_CREATE,
 				CREATE_NOT_FILE, mode, NULL,
 				&(int){SMB2_OP_MKDIR}, 1,
-				NULL, NULL, NULL, NULL, NULL);
+				NULL, NULL, NULL, NULL, NULL, NULL);
 }
 
 void
@@ -875,7 +887,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
 				 FILE_WRITE_ATTRIBUTES, FILE_CREATE,
 				 CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
 				 &(int){SMB2_OP_SET_INFO}, 1,
-				 cfile, NULL, NULL, NULL, NULL);
+				 cfile, NULL, NULL, NULL, NULL, NULL);
 	if (tmprc == 0)
 		cifs_i->cifsAttrs = dosattrs;
 }
@@ -888,24 +900,24 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
 	return smb2_compound_op(xid, tcon, cifs_sb, name,
 				DELETE, FILE_OPEN, CREATE_NOT_FILE,
 				ACL_NO_MODE, NULL, &(int){SMB2_OP_RMDIR}, 1,
-				NULL, NULL, NULL, NULL, NULL);
+				NULL, NULL, NULL, NULL, NULL, NULL);
 }
 
 int
 smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
-	    struct cifs_sb_info *cifs_sb)
+	    struct cifs_sb_info *cifs_sb, struct dentry *dentry)
 {
 	return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
 				CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
 				ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1,
-				NULL, NULL, NULL, NULL, NULL);
+				NULL, NULL, NULL, NULL, NULL, dentry);
 }
 
 static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
 			      const char *from_name, const char *to_name,
 			      struct cifs_sb_info *cifs_sb,
 			      __u32 create_options, __u32 access,
-			      int command, struct cifsFileInfo *cfile)
+			      int command, struct cifsFileInfo *cfile, struct dentry *dentry)
 {
 	struct kvec in_iov;
 	__le16 *smb2_to_name = NULL;
@@ -920,7 +932,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
 	in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
 	rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
 			      FILE_OPEN, create_options, ACL_NO_MODE, &in_iov,
-			      &command, 1, cfile, NULL, NULL, NULL, NULL);
+			      &command, 1, cfile, NULL, NULL, NULL, NULL, dentry);
 smb2_rename_path:
 	kfree(smb2_to_name);
 	return rc;
@@ -939,7 +951,7 @@ int smb2_rename_path(const unsigned int xid,
 	cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
 
 	return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
-				  co, DELETE, SMB2_OP_RENAME, cfile);
+				  co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
 }
 
 int smb2_create_hardlink(const unsigned int xid,
@@ -952,13 +964,13 @@ int smb2_create_hardlink(const unsigned int xid,
 
 	return smb2_set_path_attr(xid, tcon, from_name, to_name,
 				  cifs_sb, co, FILE_READ_ATTRIBUTES,
-				  SMB2_OP_HARDLINK, NULL);
+				  SMB2_OP_HARDLINK, NULL, NULL);
 }
 
 int
 smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
 		   const char *full_path, __u64 size,
-		   struct cifs_sb_info *cifs_sb, bool set_alloc)
+		   struct cifs_sb_info *cifs_sb, bool set_alloc, struct dentry *dentry)
 {
 	struct cifsFileInfo *cfile;
 	struct kvec in_iov;
@@ -971,7 +983,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
 				FILE_WRITE_DATA, FILE_OPEN,
 				0, ACL_NO_MODE, &in_iov,
 				&(int){SMB2_OP_SET_EOF}, 1,
-				cfile, NULL, NULL, NULL, NULL);
+				cfile, NULL, NULL, NULL, NULL, dentry);
 }
 
 int
@@ -1000,7 +1012,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
 			      FILE_WRITE_ATTRIBUTES, FILE_OPEN,
 			      0, ACL_NO_MODE, &in_iov,
 			      &(int){SMB2_OP_SET_INFO}, 1, cfile,
-			      NULL, NULL, NULL, NULL);
+			      NULL, NULL, NULL, NULL, NULL);
 	cifs_put_tlink(tlink);
 	return rc;
 }
@@ -1035,7 +1047,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				      da, cd, co, ACL_NO_MODE, in_iov,
-				      cmds, 2, cfile, NULL, NULL, NULL, NULL);
+				      cmds, 2, cfile, NULL, NULL, NULL, NULL, NULL);
 		if (!rc) {
 			rc = smb311_posix_get_inode_info(&new, full_path,
 							 data, sb, xid);
@@ -1045,7 +1057,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				      da, cd, co, ACL_NO_MODE, in_iov,
-				      cmds, 2, cfile, NULL, NULL, NULL, NULL);
+				      cmds, 2, cfile, NULL, NULL, NULL, NULL, NULL);
 		if (!rc) {
 			rc = cifs_get_inode_info(&new, full_path,
 						 data, sb, xid, NULL);
@@ -1073,7 +1085,7 @@ int smb2_query_reparse_point(const unsigned int xid,
 			      FILE_READ_ATTRIBUTES, FILE_OPEN,
 			      OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
 			      &(int){SMB2_OP_GET_REPARSE}, 1, cfile,
-			      NULL, NULL, NULL, NULL);
+			      NULL, NULL, NULL, NULL, NULL);
 	if (rc)
 		goto out;
 
diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
index 1e20f87a5f58..c18a262b42f0 100644
--- a/fs/smb/client/smb2proto.h
+++ b/fs/smb/client/smb2proto.h
@@ -75,7 +75,7 @@ int smb2_query_path_info(const unsigned int xid,
 			 struct cifs_open_info_data *data);
 extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
 			      const char *full_path, __u64 size,
-			      struct cifs_sb_info *cifs_sb, bool set_alloc);
+			      struct cifs_sb_info *cifs_sb, bool set_alloc, struct dentry *dentry);
 extern int smb2_set_file_info(struct inode *inode, const char *full_path,
 			      FILE_BASIC_INFO *buf, const unsigned int xid);
 extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
@@ -91,7 +91,7 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
 extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
 		      const char *name, struct cifs_sb_info *cifs_sb);
 extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
-		       const char *name, struct cifs_sb_info *cifs_sb);
+		       const char *name, struct cifs_sb_info *cifs_sb, struct dentry *dentry);
 int smb2_rename_path(const unsigned int xid,
 		     struct cifs_tcon *tcon,
 		     struct dentry *source_dentry,
-- 
2.39.2

[PATCH 2/2] smb: client: retry compound request without reusing lease

[meetakshisetiyaoss]

From: Meetakshi Setiya <msetiya@microsoft.com>

There is a shortcoming in the current implementation of the file
lease mechanism exposed when the lease keys were attempted to be reused
for unlink, rename and set_path_size operations for a client. As per
MS-SMB2, lease keys are associated with the file name. Linux cifs client
maintains lease keys with the inode. If the file has any hardlinks,
it is possible that the lease for a file be wrongly reused for an
operation on the hardlink or vice versa. In these cases, the mentioned
compound operations fail with STATUS_INVALID_PARAMETER.
This patch adds a fallback to the old mechanism of not sending any
lease with these compound operations if the request with lease key fails
with STATUS_INVALID_PARAMETER. Resending the same request without lease
key should not hurt any functionality, but might impact performance
especially in cases where the error is not because of the usage of wrong
lease key and we might end up doing an extra roundtrip.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/smb2inode.c | 40 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 446433606a04..d3d7a4652a5b 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -121,6 +121,17 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 	}
 
 	/* 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 workaround, send request using an existing lease key and if the server
+	 * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request
+	 * again without the lease.
+	 */
 	if (dentry) {
 		inode = d_inode(dentry);
 		cinode = CIFS_I(inode);
@@ -907,10 +918,18 @@ int
 smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
 	    struct cifs_sb_info *cifs_sb, struct dentry *dentry)
 {
-	return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+	int rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
 				CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
 				ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1,
 				NULL, NULL, NULL, NULL, NULL, dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+				CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
+				ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1,
+				NULL, NULL, NULL, NULL, NULL, NULL);
+	}
+	return rc;
 }
 
 static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
@@ -950,8 +969,14 @@ int smb2_rename_path(const unsigned int xid,
 	drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb);
 	cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
 
-	return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+	int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
 				  co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+				  co, DELETE, SMB2_OP_RENAME, cfile, NULL);
+	}
+	return rc;
 }
 
 int smb2_create_hardlink(const unsigned int xid,
@@ -979,11 +1004,20 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
 	in_iov.iov_base = &eof;
 	in_iov.iov_len = sizeof(eof);
 	cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
-	return smb2_compound_op(xid, tcon, cifs_sb, full_path,
+	int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
 				FILE_WRITE_DATA, FILE_OPEN,
 				0, ACL_NO_MODE, &in_iov,
 				&(int){SMB2_OP_SET_EOF}, 1,
 				cfile, NULL, NULL, NULL, NULL, dentry);
+	if (rc == -EINVAL) {
+		cifs_dbg(FYI, "invalid lease key, resending request without lease");
+		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
+				FILE_WRITE_DATA, FILE_OPEN,
+				0, ACL_NO_MODE, &in_iov,
+				&(int){SMB2_OP_SET_EOF}, 1,
+				cfile, NULL, NULL, NULL, NULL, NULL);
+	}
+	return rc;
 }
 
 int
-- 
2.39.2

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Paulo Alcantara]

meetakshisetiyaoss@gmail.com writes:

> From: Meetakshi Setiya <msetiya@microsoft.com>
>
> There is a shortcoming in the current implementation of the file
> lease mechanism exposed when the lease keys were attempted to be reused
> for unlink, rename and set_path_size operations for a client. As per
> MS-SMB2, lease keys are associated with the file name. Linux cifs client
> maintains lease keys with the inode. If the file has any hardlinks,
> it is possible that the lease for a file be wrongly reused for an
> operation on the hardlink or vice versa. In these cases, the mentioned
> compound operations fail with STATUS_INVALID_PARAMETER.
> This patch adds a fallback to the old mechanism of not sending any
> lease with these compound operations if the request with lease key fails
> with STATUS_INVALID_PARAMETER. Resending the same request without lease
> key should not hurt any functionality, but might impact performance
> especially in cases where the error is not because of the usage of wrong
> lease key and we might end up doing an extra roundtrip.

What's the problem with checking ->i_nlink to decide whether reusing
lease key?

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Meetakshi Setiya]

On Fri, Dec 29, 2023 at 9:13 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> meetakshisetiyaoss@gmail.com writes:
>
> > From: Meetakshi Setiya <msetiya@microsoft.com>
> >
> > There is a shortcoming in the current implementation of the file
> > lease mechanism exposed when the lease keys were attempted to be reused
> > for unlink, rename and set_path_size operations for a client. As per
> > MS-SMB2, lease keys are associated with the file name. Linux cifs client
> > maintains lease keys with the inode. If the file has any hardlinks,
> > it is possible that the lease for a file be wrongly reused for an
> > operation on the hardlink or vice versa. In these cases, the mentioned
> > compound operations fail with STATUS_INVALID_PARAMETER.
> > This patch adds a fallback to the old mechanism of not sending any
> > lease with these compound operations if the request with lease key fails
> > with STATUS_INVALID_PARAMETER. Resending the same request without lease
> > key should not hurt any functionality, but might impact performance
> > especially in cases where the error is not because of the usage of wrong
> > lease key and we might end up doing an extra roundtrip.
>
> What's the problem with checking ->i_nlink to decide whether reusing
> lease key?

As per the discussion with Tom on the previous version of the changes, I
conferred with Shyam and Steve about possible workarounds and this seemed like a
choice which did the job without much perf drawbacks and code changes. One
highlighted difference between the two could be that in the previous
version, lease
would not be reused for any file with hardlinks at all, even though the inode
may hold the correct lease for that particular file. The current changes
would take care of this by sending the lease at least once, irrespective of the
number of hardlinks.

Thanks
Meetakshi

Re: [PATCH 1/2] smb: client: reuse file lease key in compound operations

[kernel test robot]

Hello,

kernel test robot noticed "xfstests.generic.591.fail" on:

commit: 3334196d7efd8db2a7a850331d01c3c4627879ff ("[PATCH 1/2] smb: client: reuse file lease key in compound operations")
url: https://github.com/intel-lab-lkp/linux/commits/meetakshisetiyaoss-gmail-com/smb-client-retry-compound-request-without-reusing-lease/20231229-223806
base: git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link: https://lore.kernel.org/all/20231229143521.44880-1-meetakshisetiyaoss@gmail.com/
patch subject: [PATCH 1/2] smb: client: reuse file lease key in compound operations

in testcase: xfstests
version: xfstests-x86_64-f814a0d8-1_20231225
with following parameters:

	disk: 4HDD
	fs: ext4
	fs2: smbv3
	test: generic-591



compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202401031449.d24cef1f-oliver.sang@intel.com

2023-12-31 06:57:39 mount /dev/sdb1 /fs/sdb1
2023-12-31 06:57:40 mkdir -p /smbv3//cifs/sdb1
2023-12-31 06:57:40 export FSTYP=cifs
2023-12-31 06:57:40 export TEST_DEV=//localhost/fs/sdb1
2023-12-31 06:57:40 export TEST_DIR=/smbv3//cifs/sdb1
2023-12-31 06:57:40 export CIFS_MOUNT_OPTIONS=-ousername=root,password=pass,noperm,vers=3.0,mfsymlinks,actimeo=0
2023-12-31 06:57:40 echo generic/591
2023-12-31 06:57:40 ./check -E tests/cifs/exclude.incompatible-smb3.txt -E tests/cifs/exclude.very-slow.txt generic/591
FSTYP         -- cifs
PLATFORM      -- Linux/x86_64 lkp-skl-d05 6.7.0-rc7-00010-g3334196d7efd #1 SMP PREEMPT_DYNAMIC Sun Dec 31 14:28:02 CST 2023

generic/591       - output mismatch (see /lkp/benchmarks/xfstests/results//generic/591.out.bad)
    --- tests/generic/591.out	2023-12-25 18:24:02.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//generic/591.out.bad	2023-12-31 07:00:17.990085717 +0000
    @@ -2,4 +2,6 @@
     concurrent reader with O_DIRECT
     concurrent reader without O_DIRECT
     sequential reader with O_DIRECT
    +splice-test: open: /smbv3/cifs/sdb1/a: No such file or directory
     sequential reader without O_DIRECT
    +splice-test: open: /smbv3/cifs/sdb1/a: No such file or directory
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/591.out /lkp/benchmarks/xfstests/results//generic/591.out.bad'  to see the entire diff)
Ran: generic/591
Failures: generic/591
Failed 1 of 1 tests




The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240103/202401031449.d24cef1f-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Paulo Alcantara]

Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:

> As per the discussion with Tom on the previous version of the changes, I
> conferred with Shyam and Steve about possible workarounds and this seemed like a
> choice which did the job without much perf drawbacks and code changes. One
> highlighted difference between the two could be that in the previous
> version, lease
> would not be reused for any file with hardlinks at all, even though the inode
> may hold the correct lease for that particular file. The current changes
> would take care of this by sending the lease at least once, irrespective of the
> number of hardlinks.

Thanks for the explanation.  However, the code change size is no excuse
for providing workarounds rather than the actual fix.

A possible way to handle such case would be keeping a list of
pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
could look up the lease key by using @dentry.  I'm not sure if there's a
better way to handle it as I haven't looked into it further.

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Tom Talpey]

On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:
> 
>> As per the discussion with Tom on the previous version of the changes, I
>> conferred with Shyam and Steve about possible workarounds and this seemed like a
>> choice which did the job without much perf drawbacks and code changes. One
>> highlighted difference between the two could be that in the previous
>> version, lease
>> would not be reused for any file with hardlinks at all, even though the inode
>> may hold the correct lease for that particular file. The current changes
>> would take care of this by sending the lease at least once, irrespective of the
>> number of hardlinks.
> 
> Thanks for the explanation.  However, the code change size is no excuse
> for providing workarounds rather than the actual fix.

I have to agree. And it really isn't much of a workaround either.

> A possible way to handle such case would be keeping a list of
> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> could look up the lease key by using @dentry.  I'm not sure if there's a
> better way to handle it as I haven't looked into it further.

A list would also allow for better handling of lease revocation.
It seems to me this approach basically discards the original lease,
putting the client's cached data at risk, no? And what happens if
EINVAL comes back again, or the connection breaks? Is this a
recoverable situation?

Also, what's up with the xfstest the robot mailed about?

Tom.

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Paulo Alcantara]

Tom Talpey <tom@talpey.com> writes:

> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
>> A possible way to handle such case would be keeping a list of
>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
>> could look up the lease key by using @dentry.  I'm not sure if there's a
>> better way to handle it as I haven't looked into it further.
>
> A list would also allow for better handling of lease revocation.

It's also worth mentioning that we could also map the dentry directly to
lease key, so no need to store pathnames inside the inode.

> It seems to me this approach basically discards the original lease,
> putting the client's cached data at risk, no? And what happens if
> EINVAL comes back again, or the connection breaks? Is this a
> recoverable situation?

These are really good points and would need to be investigated before
coming up with an implementation.

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Shyam Prasad N]

On Fri, Jan 5, 2024 at 4:39 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Tom Talpey <tom@talpey.com> writes:
>
> > On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >> A possible way to handle such case would be keeping a list of
> >> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >> could look up the lease key by using @dentry.  I'm not sure if there's a
> >> better way to handle it as I haven't looked into it further.
> >
> > A list would also allow for better handling of lease revocation.
>
> It's also worth mentioning that we could also map the dentry directly to
> lease key, so no need to store pathnames inside the inode.

We were discussing just this yesterday. That we don't really need path
names as the key. It could be the dentry.

>
> > It seems to me this approach basically discards the original lease,
> > putting the client's cached data at risk, no? And what happens if
> > EINVAL comes back again, or the connection breaks? Is this a
> > recoverable situation?
>
> These are really good points and would need to be investigated before
> coming up with an implementation.



-- 
Regards,
Shyam

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Shyam Prasad N]

On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> > Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:
> >
> >> As per the discussion with Tom on the previous version of the changes, I
> >> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >> choice which did the job without much perf drawbacks and code changes. One
> >> highlighted difference between the two could be that in the previous
> >> version, lease
> >> would not be reused for any file with hardlinks at all, even though the inode
> >> may hold the correct lease for that particular file. The current changes
> >> would take care of this by sending the lease at least once, irrespective of the
> >> number of hardlinks.
> >
> > Thanks for the explanation.  However, the code change size is no excuse
> > for providing workarounds rather than the actual fix.
>
> I have to agree. And it really isn't much of a workaround either.
>

The original problem, i.e. compound operations like
unlink/rename/setsize not sending a lease key is very prevalent on the
field.
Unfortunately, fixing that exposed this problem with hard links.
So Steve suggested getting this fix to a shape where it's fixing the
original problem, even if it means that it does not fix it for the
case of where there are open handles to multiple hard links to the
same file.
Only thing we need to be careful of is that it does not make things
worse for other workloads.

> > A possible way to handle such case would be keeping a list of
> > pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> > could look up the lease key by using @dentry.  I'm not sure if there's a
> > better way to handle it as I haven't looked into it further.
>

This seems like a reasonable change to make. That will make sure that
we stick to what the protocol recommends.
I'm not sure that this change will be a simple one. There could be
several places where we make an assumption that the lease is
associated with an inode, and not a link.

And I'm not yet fully convinced that the spec itself is doing the
right thing by tying the lease with the link, rather than the file.
Shouldn't the lease protect the data of the file, and not the link
itself? If opening two links to the same file with two different lease
keys end up breaking each other's leases, what's the point?

> A list would also allow for better handling of lease revocation.
> It seems to me this approach basically discards the original lease,
> putting the client's cached data at risk, no? And what happens if
> EINVAL comes back again, or the connection breaks? Is this a
> recoverable situation?
>
> Also, what's up with the xfstest the robot mailed about?
Meetakshi is investigating this one.
Initial investigations led us to believe that this is related to
deferred closes. The failing tests do show that the close is getting
delayed, and that setting closetimeo=0 causes the test to pass.
However, it is not clear why the test started failing only now. We'll
have the answers soon.

>
> Tom.



-- 
Regards,
Shyam

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Stefan Metzmacher]

Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
>>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:
>>>
>>>> As per the discussion with Tom on the previous version of the changes, I
>>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
>>>> choice which did the job without much perf drawbacks and code changes. One
>>>> highlighted difference between the two could be that in the previous
>>>> version, lease
>>>> would not be reused for any file with hardlinks at all, even though the inode
>>>> may hold the correct lease for that particular file. The current changes
>>>> would take care of this by sending the lease at least once, irrespective of the
>>>> number of hardlinks.
>>>
>>> Thanks for the explanation.  However, the code change size is no excuse
>>> for providing workarounds rather than the actual fix.
>>
>> I have to agree. And it really isn't much of a workaround either.
>>
> 
> The original problem, i.e. compound operations like
> unlink/rename/setsize not sending a lease key is very prevalent on the
> field.
> Unfortunately, fixing that exposed this problem with hard links.
> So Steve suggested getting this fix to a shape where it's fixing the
> original problem, even if it means that it does not fix it for the
> case of where there are open handles to multiple hard links to the
> same file.
> Only thing we need to be careful of is that it does not make things
> worse for other workloads.
> 
>>> A possible way to handle such case would be keeping a list of
>>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
>>> could look up the lease key by using @dentry.  I'm not sure if there's a
>>> better way to handle it as I haven't looked into it further.
>>
> 
> This seems like a reasonable change to make. That will make sure that
> we stick to what the protocol recommends.
> I'm not sure that this change will be a simple one. There could be
> several places where we make an assumption that the lease is
> associated with an inode, and not a link.
> 
> And I'm not yet fully convinced that the spec itself is doing the
> right thing by tying the lease with the link, rather than the file.
> Shouldn't the lease protect the data of the file, and not the link
> itself? If opening two links to the same file with two different lease
> keys end up breaking each other's leases, what's the point?

I guess the reason for making the lease key per path on
the client is that the client can't know about possible hardlinks
before opening the file, but that open wants to use a leasekey...
Or a "stat" open that won't any lease needs to be done first,
which doubles the roundtrip for every open.

And hard links are not that common...

Maybe choosing und using a new leasekey would be the
way to start with and when a hardlink is detected
the open on the hardlink is closed again and retried
with the former lease key, which would also upgrade it again.

But sharing the handle lease for two pathnames seems wrong,
as the idea of the handle lease is to cache the patchname on the client.

While sharing the RW lease between two hardlinks would be desired.

metze

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Shyam Prasad N]

Hi Metze,

On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:
> >>>
> >>>> As per the discussion with Tom on the previous version of the changes, I
> >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >>>> choice which did the job without much perf drawbacks and code changes. One
> >>>> highlighted difference between the two could be that in the previous
> >>>> version, lease
> >>>> would not be reused for any file with hardlinks at all, even though the inode
> >>>> may hold the correct lease for that particular file. The current changes
> >>>> would take care of this by sending the lease at least once, irrespective of the
> >>>> number of hardlinks.
> >>>
> >>> Thanks for the explanation.  However, the code change size is no excuse
> >>> for providing workarounds rather than the actual fix.
> >>
> >> I have to agree. And it really isn't much of a workaround either.
> >>
> >
> > The original problem, i.e. compound operations like
> > unlink/rename/setsize not sending a lease key is very prevalent on the
> > field.
> > Unfortunately, fixing that exposed this problem with hard links.
> > So Steve suggested getting this fix to a shape where it's fixing the
> > original problem, even if it means that it does not fix it for the
> > case of where there are open handles to multiple hard links to the
> > same file.
> > Only thing we need to be careful of is that it does not make things
> > worse for other workloads.
> >
> >>> A possible way to handle such case would be keeping a list of
> >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >>> could look up the lease key by using @dentry.  I'm not sure if there's a
> >>> better way to handle it as I haven't looked into it further.
> >>
> >
> > This seems like a reasonable change to make. That will make sure that
> > we stick to what the protocol recommends.
> > I'm not sure that this change will be a simple one. There could be
> > several places where we make an assumption that the lease is
> > associated with an inode, and not a link.
> >
> > And I'm not yet fully convinced that the spec itself is doing the
> > right thing by tying the lease with the link, rather than the file.
> > Shouldn't the lease protect the data of the file, and not the link
> > itself? If opening two links to the same file with two different lease
> > keys end up breaking each other's leases, what's the point?
>
> I guess the reason for making the lease key per path on
> the client is that the client can't know about possible hardlinks
> before opening the file, but that open wants to use a leasekey...
> Or a "stat" open that won't any lease needs to be done first,
> which doubles the roundtrip for every open.
>
> And hard links are not that common...
>

That does makes sense.

> Maybe choosing und using a new leasekey would be the
> way to start with and when a hardlink is detected
> the open on the hardlink is closed again and retried
> with the former lease key, which would also upgrade it again.

That would not work today, as the former lease key would be associated
with the other hardlink. And would result in the server returning
STATUS_INVALID_PARAMETER.

>
> But sharing the handle lease for two pathnames seems wrong,
> as the idea of the handle lease is to cache the patchname on the client.
>
> While sharing the RW lease between two hardlinks would be desired.
>
> metze



-- 
Regards,
Shyam

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Stefan Metzmacher]

Hi Shyam,

>> Maybe choosing und using a new leasekey would be the
>> way to start with and when a hardlink is detected
>> the open on the hardlink is closed again and retried
>> with the former lease key, which would also upgrade it again.
> 
> That would not work today, as the former lease key would be associated
> with the other hardlink. And would result in the server returning
> STATUS_INVALID_PARAMETER.

And that's the original problem you try to solve, correct?

Then there's nothing we can do expect for using a dentry pased
lease key and live with the fact that they don't allow write caching
anymore. The RH state should still be granted to both lease keys...

metze

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Shyam Prasad N]

On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Shyam,
>
> >> Maybe choosing und using a new leasekey would be the
> >> way to start with and when a hardlink is detected
> >> the open on the hardlink is closed again and retried
> >> with the former lease key, which would also upgrade it again.
> >
> > That would not work today, as the former lease key would be associated
> > with the other hardlink. And would result in the server returning
> > STATUS_INVALID_PARAMETER.
>
> And that's the original problem you try to solve, correct?
Correct. I thought you were proposing this as a solution.
>
> Then there's nothing we can do expect for using a dentry pased
> lease key and live with the fact that they don't allow write caching
> anymore. The RH state should still be granted to both lease keys...

Yes. It's not ideal. But I guess we need to live with it.
Thanks for the inputs.

Steve/Paulo/Tom: What do you feel about fixing this in two phases?

First, take Meetakshi's earlier patch, which would fix the problem of
unnecessary lease breaks (and possible deadlock situation with the
server) due to unlink/rename/setfilesize for files that do not have
multiple hard links. i
.e. during these operations, check if link count for the file is 1.
Only if that is the case, send the lease key for the file. This would
mean that the problem remains for files that have multiple hard links.
But at least the hard link xfstest would pass.

As a following patch, work on the full fix. i.e. maintain a list of
lease keys for the file, keyed by the dentry.
This patch would replace the cinode->lease_key with a map/list, lookup
the correct lease from the list on usage.
This would obviously remove the check for the link count done by the
above patch.

Reason being that we already have the first patch, and I'm not sure
we'll be able to work on the second one soon enough.

>
> metze
>


-- 
Regards,
Shyam

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Steve French]

On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote:
> >
> > Hi Shyam,
> >
> > >> Maybe choosing und using a new leasekey would be the
> > >> way to start with and when a hardlink is detected
> > >> the open on the hardlink is closed again and retried
> > >> with the former lease key, which would also upgrade it again.
> > >
> > > That would not work today, as the former lease key would be associated
> > > with the other hardlink. And would result in the server returning
> > > STATUS_INVALID_PARAMETER.
> >
> > And that's the original problem you try to solve, correct?
> Correct. I thought you were proposing this as a solution.
> >
> > Then there's nothing we can do expect for using a dentry pased
> > lease key and live with the fact that they don't allow write caching
> > anymore. The RH state should still be granted to both lease keys...
>
> Yes. It's not ideal. But I guess we need to live with it.
> Thanks for the inputs.
>
> Steve/Paulo/Tom: What do you feel about fixing this in two phases?
>
> First, take Meetakshi's earlier patch, which would fix the problem of
> unnecessary lease breaks (and possible deadlock situation with the
> server) due to unlink/rename/setfilesize for files that do not have
> multiple hard links. i
> .e. during these operations, check if link count for the file is 1.
> Only if that is the case, send the lease key for the file. This would
> mean that the problem remains for files that have multiple hard links.
> But at least the hard link xfstest would pass.

Since this approach could be a huge performance degradation for some
(albeit rare)
workloads (e.g. if hardlinks exist for files, but such files are not opened by
different names at the same time), I prefer the two phase approach
that simply retries when we get invalid argument without the lease key
(which has no risk since the current code just fails, and retry will "fix" the
issue albeit not as good as being able to cache the second open)

> As a following patch, work on the full fix. i.e. maintain a list of
> lease keys for the file, keyed by the dentry.
> This patch would replace the cinode->lease_key with a map/list, lookup
> the correct lease from the list on usage.
> This would obviously remove the check for the link count done by the
> above patch.

I don't like the idea of hurting caching for all hardlinked files as a hack,
so for the longer term solution I prefer a solution that caches the
dentry pointer with the lease key, although that brings up the obvious
question of whether the dentry could be freed and reallocated
in some deferred close cases and cause the lease key to be valid
but us not to match it due to new dentry).

I lean toward something like:
1) store the dentry for the lease key, not just the lease key in the
cifs inode info (today we only store the lease key).
We could store an array of lease key/dentry pairs but I worry that
this would run the risk of use after free and/or lock contention bugs
(and additional memory usage if a malicious app tried to open all
hardlinked files)
2) if link count is greater than 1, check that the dentry matches when
deciding whether to use the lease key (presumably
we don't have to worry about it link count is 1)

-- 
Thanks,

Steve

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Meetakshi Setiya]

Hi

Here is why xfstest 591 must be failing when the lease key is being
reused for the
unlink compound operation and closetimeo is set to any value other than 0
(deferred close is supported).

The splice_test program xfstest 591 uses calls unlink() at the end of the
program to delete the file A without closing the file handle first. This
file is thus marked for delete and the server returns STATUS_DELETE_PENDING.
If the mount options have specified anything other than closetimeo=0 i.e. if
we have enabled deferred closes, the open handle to this file (which was not
closed by the application), would have deferred closed once the application
ends. When the shell script runs the splice_test program again, the same
handle from the previous run of the splice_test program - that was supposed
to be closed because the file is deleted- is used by the next open to
open file A
since handle caching is still allowed for its lease.
This leads to the error: No such file or directory which we see in
the file 591.out.bad.
This error was not observed when unlink operation broke leases because when
the server issued a lease break to the client, it made the client flush its
remaining writes/locks to the server and downgrade its lease from RWH to R.
Since handle caching is not involved here anymore, the handle was also not
reused anymore by the next open.
Now that the patch has removed the lease break communication with the
server, something similar to what happens when a client gets lease break
notification might need to be done. One solution could be to flush all
cached writes to the server and downgrade all leases for open handles to
file A to R or 0 as soon as unlink is issued for the file A. In this case, even
if some file handles are deferred closed, they would not be reused.
A simple repro for this bug when the above patch 1/2 is applied (courtesy of
@bharath):
1.  Mount share with closetimeo=30
2.  (shell1): tail -f /dev/null > myfile
3.  (shell2): rm myfile
4.  (shell1): ctrl+c to close myfile handle. This would be deferred closed
5.  Touch myfile will fail for a period  = closetimeo value

Thanks
Meetakshi

On Sat, Jan 6, 2024 at 12:12 AM Steve French <smfrench@gmail.com> wrote:
>
> On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote:
> > >
> > > Hi Shyam,
> > >
> > > >> Maybe choosing und using a new leasekey would be the
> > > >> way to start with and when a hardlink is detected
> > > >> the open on the hardlink is closed again and retried
> > > >> with the former lease key, which would also upgrade it again.
> > > >
> > > > That would not work today, as the former lease key would be associated
> > > > with the other hardlink. And would result in the server returning
> > > > STATUS_INVALID_PARAMETER.
> > >
> > > And that's the original problem you try to solve, correct?
> > Correct. I thought you were proposing this as a solution.
> > >
> > > Then there's nothing we can do expect for using a dentry pased
> > > lease key and live with the fact that they don't allow write caching
> > > anymore. The RH state should still be granted to both lease keys...
> >
> > Yes. It's not ideal. But I guess we need to live with it.
> > Thanks for the inputs.
> >
> > Steve/Paulo/Tom: What do you feel about fixing this in two phases?
> >
> > First, take Meetakshi's earlier patch, which would fix the problem of
> > unnecessary lease breaks (and possible deadlock situation with the
> > server) due to unlink/rename/setfilesize for files that do not have
> > multiple hard links. i
> > .e. during these operations, check if link count for the file is 1.
> > Only if that is the case, send the lease key for the file. This would
> > mean that the problem remains for files that have multiple hard links.
> > But at least the hard link xfstest would pass.
>
> Since this approach could be a huge performance degradation for some
> (albeit rare)
> workloads (e.g. if hardlinks exist for files, but such files are not opened by
> different names at the same time), I prefer the two phase approach
> that simply retries when we get invalid argument without the lease key
> (which has no risk since the current code just fails, and retry will "fix" the
> issue albeit not as good as being able to cache the second open)
>
> > As a following patch, work on the full fix. i.e. maintain a list of
> > lease keys for the file, keyed by the dentry.
> > This patch would replace the cinode->lease_key with a map/list, lookup
> > the correct lease from the list on usage.
> > This would obviously remove the check for the link count done by the
> > above patch.
>
> I don't like the idea of hurting caching for all hardlinked files as a hack,
> so for the longer term solution I prefer a solution that caches the
> dentry pointer with the lease key, although that brings up the obvious
> question of whether the dentry could be freed and reallocated
> in some deferred close cases and cause the lease key to be valid
> but us not to match it due to new dentry).
>
> I lean toward something like:
> 1) store the dentry for the lease key, not just the lease key in the
> cifs inode info (today we only store the lease key).
> We could store an array of lease key/dentry pairs but I worry that
> this would run the risk of use after free and/or lock contention bugs
> (and additional memory usage if a malicious app tried to open all
> hardlinked files)
> 2) if link count is greater than 1, check that the dentry matches when
> deciding whether to use the lease key (presumably
> we don't have to worry about it link count is 1)
>
> --
> Thanks,
>
> Steve

Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Meetakshi Setiya]

Hi

There was a pending fix to reuse lease key in compound operations on
the smb client, essentially proposed to resolve a customer-reported
bug. A scenario similar to what the client faced would be this:
Imagine a file that has a lot of dirty pages to be written to the
server. If rename/unlink/set path size compound operations are
performed on this file, with the current implementation, we do not
send the lease key. Resultantly, this would lead to the server
assuming that the request came from a new client and it would send a
lease break notification to the same client, on the same connection.
This lease break can lead the client to consume several credits while
writing its dirty pages to the server. A deadlock may arise when the
server stops granting more credits to this connection and the deadlock
would be lifted only when the lease timer on the server expires.

The fix initially proposed just copied the lease key from the inode
and passed it along to SMB2_open_init() from unlink, rename and set
path size compound operations. Incidentally, that exposed a
shortcoming in the smb client-side code. As per MS-SMB2, lease keys
are associated with the file name. Linux cifs client maintains lease
keys with the inode. If the file has any hardlinks, it is possible
that the lease for a file be wrongly reused for an operation on the
hardlink or vice versa. In these cases, the mentioned compound
operations fail with STATUS_INVALID_PARAMETER.

A simple, but hacky fix for the above, as per my discussion with Steve
would be to have a two-phased approach and resend the compound op
request again without the lease key if STATUS_INVALID_PARAMETER is
received. This fix could be easily backported to older kernels since
it would be pretty straightforward and does not involve a lot of code
changes. Such a fallback mechanism should not hurt any functionality
but might impact performance especially in cases where the error is
not because of the usage of wrong lease key and we might end up doing
an extra roundtrip. From what I know, use cases where two or more
file+hardlinks are being used together at the same time are kind of
rare, so we might not run into cases where resending requests in this
fashion has to be performed often.

I understand this is not a perfect or correct fix to the problem, but
for the time being, it might help solve the customer reported issue I
mentioned at the start and could also be backported readily. Since
hurting caching for all hardlinked files is not ideal, I am already
working on another fix that stores the dentry pointer with the lease
key in the cinode object.

From my discussion with Steve and Shyam, the fix proposed was this:
Instead of having a list of dentries/key dentry pairs, we can store a
dentry pointer in addition to the lease key in the cinode object. The
dentry (as a proxy for filename/path) would be the one the lease key
is associated with. There would be a reference counter to maintain the
number of open handles for that file(path)/dentry. When a new file is
created, not only its lease key, but its dentry too be set in the
cinode object. Whenever there is a new handle opened to this dentry,
the reference count to the leasekey/dentry in the cinode object would
be increased. While reusing the lease key, check if dentry (from the
request) matches the dentry stored in the cinode. If it does, copy the
lease key, if it does not, do not use that lease key. When all file
handles to a dentry have been closed, clear the dentry and lease key
from the cinode object. This has the potential to fix the hardlink
issue since dentries for different hardlinks would be different and
one would not end up reusing lease from another. Some implementation
details I am unsure about is should the open request for a file with
mismatched dentry (which would not get to reuse the lease key) send no
lease context at all, or create a new lease key and use that, or
something else?

I am writing this mail to seek advice/feedback on the situation. Any
suggestions or better solutions to any of the issues mentioned in this
mail are very much appreciated.

Thanks
Meetakshi




On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes:
> >>>
> >>>> As per the discussion with Tom on the previous version of the changes, I
> >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >>>> choice which did the job without much perf drawbacks and code changes. One
> >>>> highlighted difference between the two could be that in the previous
> >>>> version, lease
> >>>> would not be reused for any file with hardlinks at all, even though the inode
> >>>> may hold the correct lease for that particular file. The current changes
> >>>> would take care of this by sending the lease at least once, irrespective of the
> >>>> number of hardlinks.
> >>>
> >>> Thanks for the explanation.  However, the code change size is no excuse
> >>> for providing workarounds rather than the actual fix.
> >>
> >> I have to agree. And it really isn't much of a workaround either.
> >>
> >
> > The original problem, i.e. compound operations like
> > unlink/rename/setsize not sending a lease key is very prevalent on the
> > field.
> > Unfortunately, fixing that exposed this problem with hard links.
> > So Steve suggested getting this fix to a shape where it's fixing the
> > original problem, even if it means that it does not fix it for the
> > case of where there are open handles to multiple hard links to the
> > same file.
> > Only thing we need to be careful of is that it does not make things
> > worse for other workloads.
> >
> >>> A possible way to handle such case would be keeping a list of
> >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >>> could look up the lease key by using @dentry.  I'm not sure if there's a
> >>> better way to handle it as I haven't looked into it further.
> >>
> >
> > This seems like a reasonable change to make. That will make sure that
> > we stick to what the protocol recommends.
> > I'm not sure that this change will be a simple one. There could be
> > several places where we make an assumption that the lease is
> > associated with an inode, and not a link.
> >
> > And I'm not yet fully convinced that the spec itself is doing the
> > right thing by tying the lease with the link, rather than the file.
> > Shouldn't the lease protect the data of the file, and not the link
> > itself? If opening two links to the same file with two different lease
> > keys end up breaking each other's leases, what's the point?
>
> I guess the reason for making the lease key per path on
> the client is that the client can't know about possible hardlinks
> before opening the file, but that open wants to use a leasekey...
> Or a "stat" open that won't any lease needs to be done first,
> which doubles the roundtrip for every open.
>
> And hard links are not that common...
>
> Maybe choosing und using a new leasekey would be the
> way to start with and when a hardlink is detected
> the open on the hardlink is closed again and retried
> with the former lease key, which would also upgrade it again.
>
> But sharing the handle lease for two pathnames seems wrong,
> as the idea of the handle lease is to cache the patchname on the client.
>
> While sharing the RW lease between two hardlinks would be desired.
>
> metze