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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Patch 1 of this patch series
https://lore.kernel.org/all/20240306034353.190039-1-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 1
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 1
fix the original issue. Fix(es) for the hardlink-leasekey problem can
come in the next batch.

On Wed, Mar 6, 2024 at 9:14 AM <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 429d83d31280..f697c14cd8c6 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) {
> @@ -874,11 +885,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,
> @@ -919,8 +939,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,
> @@ -949,11 +975,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
>





[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux