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 >
