Re: [PATCH 3/3] smb: client: retry compound request without reusing lease
From: Meetakshi Setiya
Date: Fri Feb 09 2024 - 08:37:39 EST
Patch 2 of this patch series
https://lore.kernel.org/linux-cifs/20240209131552.471765-2-meetakshisetiyaoss@xxxxxxxxx/
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@xxxxxxxxx> wrote:
>
> From: Meetakshi Setiya <msetiya@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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
>