diff mbox series

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

Message ID 20240209131552.471765-3-meetakshisetiyaoss@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] smb: client: do not defer close open handles to deleted files | expand

Commit Message

Meetakshi Setiya Feb. 9, 2024, 1:15 p.m. UTC 
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 smb
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 | 41 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

Comments

Meetakshi Setiya Feb. 9, 2024, 1:37 p.m. UTC | #1 
Patch 2 of this patch series
https://lore.kernel.org/linux-cifs/20240209131552.471765-2-meetakshisetiyaoss@gmail.com/
aims to fix a customer reported bug by reusing lease key in unlink,
rename and set_path_size compound operations on the smb client. The
bug, its implications and reproduction has been described in the
commit message of the patch. In short, unlink, rename and
set_path_size operations can cause the server to send lease breaks to
the same client, on the same connection which hurts performance.

The aim is to have a fix in place for this problem without regressing
existing behaviour. Also, the proposed changes should go in smaller
batches so that they can be backported with relative ease. Patch 2
regressed a few operations on hardlinks (eg., xfstests generic 002,
013).  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 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 fix for the regressions 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 would help patch 2
fix the original issue. Fix(es) for the hardlink-leasekey problem can
come in the next batch.

Thanks
Meetakshi

On Fri, Feb 9, 2024 at 6:46 PM <meetakshisetiyaoss@gmail.com> wrote:
>
> 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 smb
> 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 | 41 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 69f3442c5b96..c0d099a9e1ee 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -154,6 +154,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 filepath. We are maintaining lease keys
> +        * with the inode on the client. If the file has hardlinks, it is possible
> +        * that the lease for a file be reused for an operation on its 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);
>                 if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> @@ -867,11 +878,20 @@ 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, 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);
> +       }
> +       return rc;
>  }
>
>  static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> @@ -912,8 +932,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");
> +               rc = 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,
> @@ -942,11 +968,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, 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);
> +       }
> +       return rc;
>  }
>
>  int
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 69f3442c5b96..c0d099a9e1ee 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -154,6 +154,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 filepath. We are maintaining lease keys
+	 * with the inode on the client. If the file has hardlinks, it is possible
+	 * that the lease for a file be reused for an operation on its 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);
 		if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
@@ -867,11 +878,20 @@  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, 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);
+	}
+	return rc;
 }
 
 static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
@@ -912,8 +932,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");
+		rc = 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,
@@ -942,11 +968,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, 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);
+	}
+	return rc;
 }
 
 int