Received-SPF: pass (google.com: domain of linux-kernel+bounces-59378-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240209131552.471765-1-meetakshisetiyaoss@gmail.com> <20240209131552.471765-3-meetakshisetiyaoss@gmail.com>
In-Reply-To: <20240209131552.471765-3-meetakshisetiyaoss@gmail.com>
From: Meetakshi Setiya <meetakshisetiyaoss@gmail.com>
Date: Fri, 9 Feb 2024 19:07:13 +0530
Message-ID: <CAFTVevVeRb-dKkQK2gVEhoDQH1QhJXh-xHZXz-GW35d26y--uA@mail.gmail.com>
Subject: Re: [PATCH 3/3] smb: client: retry compound request without reusing lease
To: sfrench@samba.org, pc@manguebit.com, ronniesahlberg@gmail.com, 
	sprasad@microsoft.com, nspmangalore@gmail.com, tom@talpey.com, 
	linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org, 
	samba-technical@lists.samba.org, bharathsm.hsk@gmail.com
Cc: Meetakshi Setiya <msetiya@microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Patch 2 of this patch series
https://lore.kernel.org/linux-cifs/20240209131552.471765-2-meetakshisetiyao=
ss@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=E2=80=AFPM <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 maintainin=
g 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 ha=
rdlink 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 t=
he request
> +        * again without the lease.
> +        */
>         if (dentry) {
>                 inode =3D d_inode(dentry);
>                 if (CIFS_I(inode)->lease_granted && server->ops->get_leas=
e_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_OP=
EN,
> +       int rc =3D smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FIL=
E_OPEN,
>                                 CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POI=
NT,
>                                 ACL_NO_MODE, NULL,
>                                 &(int){SMB2_OP_DELETE}, 1,
>                                 NULL, NULL, NULL, dentry);
> +       if (rc =3D=3D -EINVAL) {
> +               cifs_dbg(FYI, "invalid lease key, resending request witho=
ut lease");
> +               rc =3D smb2_compound_op(xid, tcon, cifs_sb, name, DELETE,=
 FILE_OPEN,
> +                               CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POI=
NT,
> +                               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, &cfi=
le);
>
> -       return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> +       int rc =3D smb2_set_path_attr(xid, tcon, from_name, to_name, cifs=
_sb,
>                                   co, DELETE, SMB2_OP_RENAME, cfile, sour=
ce_dentry);
> +       if (rc =3D=3D -EINVAL) {
> +               cifs_dbg(FYI, "invalid lease key, resending request witho=
ut lease");
> +               rc =3D 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 c=
ifs_tcon *tcon,
>         in_iov.iov_base =3D &eof;
>         in_iov.iov_len =3D 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 =3D 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 =3D=3D -EINVAL) {
> +               cifs_dbg(FYI, "invalid lease key, resending request witho=
ut lease");
> +               rc =3D 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
>