From: Meetakshi Setiya <[email protected]>
When a file/dentry has been deleted before closing all its open handles,
currently, closing them can add them to the deferred close list. This can
lead to problems in creating file with the same name when the file is
re-created before the deferred close completes. This issue was seen while
reusing a client's already existing lease on a file for compound operations
and xfstest 591 failed because of the deferred close handle that remained
valid even after the file was deleted and was being reused to create a file
with the same name. The server in this case returns an error on open with
STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
handles are closed (duration specified in closetimeo).
This patch fixes the issue by flagging all open handles for the deleted
file (file path to be precise) with FILE_DELETED at the end of the unlink
operation. A new field cflags has been introduced in the cifsFileInfo
structure to set the FILE_DELETED flag without interfering with the f_flags
field. This cflags field could be useful in the future for setting more
flags specific to cfile.
When doing close in cifs_close for each of these handles, check the
FILE_DELETED flag and do not defer close these handles if the corresponding
filepath has been deleted.
Signed-off-by: Meetakshi Setiya <[email protected]>
---
fs/smb/client/cifsglob.h | 3 +++
fs/smb/client/cifsproto.h | 4 ++++
fs/smb/client/file.c | 3 ++-
fs/smb/client/inode.c | 2 ++
fs/smb/client/misc.c | 22 ++++++++++++++++++++++
5 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 16befff4cbb4..f6b4acdcdeb3 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
struct list_head locks; /* locks held by fid above */
};
+#define FILE_DELETED 00000001
+
struct cifsFileInfo {
/* following two lists are protected by tcon->open_file_lock */
struct list_head tlist; /* pointer to next fid owned by tcon */
@@ -1413,6 +1415,7 @@ struct cifsFileInfo {
struct dentry *dentry;
struct tcon_link *tlink;
unsigned int f_flags;
+ unsigned int cflags; /* flags for this file */
bool invalidHandle:1; /* file closed via session abend */
bool swapfile:1;
bool oplock_break_cancelled:1;
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index a841bf4967fa..f995b766b177 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
const char *path);
+
+extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
+ *cifs_tcon, const char *path);
+
extern struct TCP_Server_Info *
cifs_get_tcp_session(struct smb3_fs_context *ctx,
struct TCP_Server_Info *primary_server);
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index b75282c204da..91cf4d2df4de 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
cfile->uid = current_fsuid();
cfile->dentry = dget(dentry);
cfile->f_flags = file->f_flags;
+ cfile->cflags = 0;
cfile->invalidHandle = false;
cfile->deferred_close_scheduled = false;
cfile->tlink = cifs_get_tlink(tlink);
@@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
&& cinode->lease_granted &&
!test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
- dclose) {
+ dclose && !(cfile->cflags & FILE_DELETED)) {
if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
inode_set_mtime_to_ts(inode,
inode_set_ctime_current(inode));
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index d02f8ba29cb5..8121b5b1aa22 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
cifs_inode = CIFS_I(dir);
CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
unlink_out:
+ if (rc == 0)
+ cifs_mark_open_handles_for_deleted_file(tcon, full_path);
free_dentry_path(page);
kfree(attrs);
free_xid(xid);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 0748d7b757b9..8e46dee1a36c 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
free_dentry_path(page);
}
+/*
+ * If a dentry has been deleted, all corresponding open handles should know that
+ * so that we do not defer close them.
+ */
+void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
+ const char *path)
+{
+ struct cifsFileInfo *cfile;
+ void *page;
+ const char *full_path;
+
+ page = alloc_dentry_path();
+ spin_lock(&tcon->open_file_lock);
+ list_for_each_entry(cfile, &tcon->openFileList, tlist) {
+ full_path = build_path_from_dentry(cfile->dentry, page);
+ if (strcmp(full_path, path) == 0)
+ cfile->cflags |= FILE_DELETED;
+ }
+ spin_unlock(&tcon->open_file_lock);
+ free_dentry_path(page);
+}
+
/* parses DFS referral V3 structure
* caller is responsible for freeing target_nodes
* returns:
--
2.39.2
From: Meetakshi Setiya <[email protected]>
Currently, when a rename, unlink or set path size compound operation
is requested on a file that has a lot of dirty pages to be written
to the server, we do not send the lease key for these requests. As a
result, the server can assume that this request is from a new client, and
send a lease break notification to the same client, on the same
connection. As a response to the lease break, the client can consume
several credits to write the dirty pages to the server. Depending on the
server's credit grant implementation, the server can stop granting more
credits to this connection, and this can cause a deadlock (which can only
be resolved when the lease timer on the server expires).
One of the problems here is that the client is sending no lease key,
even if it has a lease for the file. This patch fixes the problem by
reusing the existing lease key on the file for rename, unlink and set path
size compound operations so that the client does not break its own lease.
A very trivial example could be a set of commands by a client that
maintains open handle (for write) to a file and then tries to copy the
contents of that file to another one, eg.,
tail -f /dev/null > myfile &
mv myfile myfile2
Presently, the network capture on the client shows that the move (or
rename) would trigger a lease break on the same client, for the same file.
With the lease key reused, the lease break request-response overhead is
eliminated, thereby reducing the roundtrips performed for this set of
operations.
The patch fixes the bug described above and also provides perf benefit.
Signed-off-by: Meetakshi Setiya <[email protected]>
---
fs/smb/client/cifsglob.h | 5 ++--
fs/smb/client/cifsproto.h | 6 +++--
fs/smb/client/cifssmb.c | 4 ++--
fs/smb/client/inode.c | 10 ++++----
fs/smb/client/smb2inode.c | 48 ++++++++++++++++++++++++---------------
fs/smb/client/smb2proto.h | 6 +++--
6 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index f6b4acdcdeb3..baeed01d356b 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -371,7 +371,8 @@ struct smb_version_operations {
struct cifs_open_info_data *data);
/* set size by path */
int (*set_path_size)(const unsigned int, struct cifs_tcon *,
- const char *, __u64, struct cifs_sb_info *, bool);
+ const char *, __u64, struct cifs_sb_info *, bool,
+ struct dentry *);
/* set size by file handle */
int (*set_file_size)(const unsigned int, struct cifs_tcon *,
struct cifsFileInfo *, __u64, bool);
@@ -401,7 +402,7 @@ struct smb_version_operations {
struct cifs_sb_info *);
/* unlink file */
int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
- struct cifs_sb_info *);
+ struct cifs_sb_info *, struct dentry *);
/* open, rename and delete file */
int (*rename_pending_delete)(const char *, struct dentry *,
const unsigned int);
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index f995b766b177..e108964950b5 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -406,7 +406,8 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
__u32 pid_of_opener);
extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
const char *file_name, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_allocation);
+ struct cifs_sb_info *cifs_sb, bool set_allocation,
+ struct dentry *dentry);
extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
struct cifsFileInfo *cfile, __u64 size,
bool set_allocation);
@@ -442,7 +443,8 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
const struct nls_table *nls_codepage,
int remap_special_chars);
extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
- const char *name, struct cifs_sb_info *cifs_sb);
+ const char *name, struct cifs_sb_info *cifs_sb,
+ struct dentry *dentry);
int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
struct dentry *source_dentry,
const char *from_name, const char *to_name,
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 01e89070df5a..301189ee1335 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
int
CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
- struct cifs_sb_info *cifs_sb)
+ struct cifs_sb_info *cifs_sb, struct dentry *dentry)
{
DELETE_FILE_REQ *pSMB = NULL;
DELETE_FILE_RSP *pSMBr = NULL;
@@ -4993,7 +4993,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
int
CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
- bool set_allocation)
+ bool set_allocation, struct dentry *dentry)
{
struct smb_com_transaction2_spi_req *pSMB = NULL;
struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 8121b5b1aa22..c3e86876a2a8 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1846,7 +1846,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
goto psx_del_no_retry;
}
- rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
+ rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
psx_del_no_retry:
if (!rc) {
@@ -2799,7 +2799,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
static int
cifs_set_file_size(struct inode *inode, struct iattr *attrs,
- unsigned int xid, const char *full_path)
+ unsigned int xid, const char *full_path, struct dentry *dentry)
{
int rc;
struct cifsFileInfo *open_file;
@@ -2850,7 +2850,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
*/
if (server->ops->set_path_size)
rc = server->ops->set_path_size(xid, tcon, full_path,
- attrs->ia_size, cifs_sb, false);
+ attrs->ia_size, cifs_sb, false, dentry);
else
rc = -ENOSYS;
cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
@@ -2940,7 +2940,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
rc = 0;
if (attrs->ia_valid & ATTR_SIZE) {
- rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
if (rc != 0)
goto out;
}
@@ -3107,7 +3107,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
}
if (attrs->ia_valid & ATTR_SIZE) {
- rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
if (rc != 0)
goto cifs_setattr_exit;
}
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 05818cd6d932..69f3442c5b96 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -98,7 +98,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
__u32 desired_access, __u32 create_disposition,
__u32 create_options, umode_t mode, struct kvec *in_iov,
int *cmds, int num_cmds, struct cifsFileInfo *cfile,
- struct kvec *out_iov, int *out_buftype)
+ struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
{
struct reparse_data_buffer *rbuf;
@@ -115,6 +115,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
int resp_buftype[MAX_COMPOUND];
struct smb2_query_info_rsp *qi_rsp = NULL;
struct cifs_open_info_data *idata;
+ struct inode *inode = NULL;
int flags = 0;
__u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
unsigned int size[2];
@@ -152,6 +153,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
goto finished;
}
+ /* if there is an existing lease, reuse it */
+ if (dentry) {
+ inode = d_inode(dentry);
+ if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
+ oplock = SMB2_OPLOCK_LEVEL_LEASE;
+ server->ops->get_lease_key(inode, &fid);
+ }
+ }
+
vars->oparms = (struct cifs_open_parms) {
.tcon = tcon,
.path = full_path,
@@ -747,7 +757,7 @@ int smb2_query_path_info(const unsigned int xid,
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
FILE_READ_ATTRIBUTES, FILE_OPEN,
create_options, ACL_NO_MODE, in_iov,
- cmds, 1, cfile, out_iov, out_buftype);
+ cmds, 1, cfile, out_iov, out_buftype, NULL);
hdr = out_iov[0].iov_base;
/*
* If first iov is unset, then SMB session was dropped or we've got a
@@ -779,7 +789,7 @@ int smb2_query_path_info(const unsigned int xid,
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
FILE_READ_ATTRIBUTES, FILE_OPEN,
create_options, ACL_NO_MODE, in_iov,
- cmds, num_cmds, cfile, NULL, NULL);
+ cmds, num_cmds, cfile, NULL, NULL, NULL);
break;
case -EREMOTE:
break;
@@ -811,7 +821,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
FILE_WRITE_ATTRIBUTES, FILE_CREATE,
CREATE_NOT_FILE, mode,
NULL, &(int){SMB2_OP_MKDIR}, 1,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
void
@@ -836,7 +846,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
FILE_WRITE_ATTRIBUTES, FILE_CREATE,
CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_INFO}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
if (tmprc == 0)
cifs_i->cifsAttrs = dosattrs;
}
@@ -850,25 +860,26 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
DELETE, FILE_OPEN, CREATE_NOT_FILE,
ACL_NO_MODE, NULL,
&(int){SMB2_OP_RMDIR}, 1,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
int
smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
- struct cifs_sb_info *cifs_sb)
+ struct cifs_sb_info *cifs_sb, struct dentry *dentry)
{
return 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, NULL, NULL, dentry);
}
static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
const char *from_name, const char *to_name,
struct cifs_sb_info *cifs_sb,
__u32 create_options, __u32 access,
- int command, struct cifsFileInfo *cfile)
+ int command, struct cifsFileInfo *cfile,
+ struct dentry *dentry)
{
struct kvec in_iov;
__le16 *smb2_to_name = NULL;
@@ -883,7 +894,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
FILE_OPEN, create_options, ACL_NO_MODE,
- &in_iov, &command, 1, cfile, NULL, NULL);
+ &in_iov, &command, 1, cfile, NULL, NULL, dentry);
smb2_rename_path:
kfree(smb2_to_name);
return rc;
@@ -902,7 +913,7 @@ int smb2_rename_path(const unsigned int xid,
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,
- co, DELETE, SMB2_OP_RENAME, cfile);
+ co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
}
int smb2_create_hardlink(const unsigned int xid,
@@ -915,13 +926,14 @@ int smb2_create_hardlink(const unsigned int xid,
return smb2_set_path_attr(xid, tcon, from_name, to_name,
cifs_sb, co, FILE_READ_ATTRIBUTES,
- SMB2_OP_HARDLINK, NULL);
+ SMB2_OP_HARDLINK, NULL, NULL);
}
int
smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
const char *full_path, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_alloc)
+ struct cifs_sb_info *cifs_sb, bool set_alloc,
+ struct dentry *dentry)
{
struct cifsFileInfo *cfile;
struct kvec in_iov;
@@ -934,7 +946,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
FILE_WRITE_DATA, FILE_OPEN,
0, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_EOF}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, dentry);
}
int
@@ -963,7 +975,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
FILE_WRITE_ATTRIBUTES, FILE_OPEN,
0, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_INFO}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
cifs_put_tlink(tlink);
return rc;
}
@@ -998,7 +1010,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
da, cd, co, ACL_NO_MODE, in_iov,
- cmds, 2, cfile, NULL, NULL);
+ cmds, 2, cfile, NULL, NULL, NULL);
if (!rc) {
rc = smb311_posix_get_inode_info(&new, full_path,
data, sb, xid);
@@ -1008,7 +1020,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
da, cd, co, ACL_NO_MODE, in_iov,
- cmds, 2, cfile, NULL, NULL);
+ cmds, 2, cfile, NULL, NULL, NULL);
if (!rc) {
rc = cifs_get_inode_info(&new, full_path,
data, sb, xid, NULL);
@@ -1036,7 +1048,7 @@ int smb2_query_reparse_point(const unsigned int xid,
FILE_READ_ATTRIBUTES, FILE_OPEN,
OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_GET_REPARSE}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
if (rc)
goto out;
diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
index b3069911e9dd..221143788a1c 100644
--- a/fs/smb/client/smb2proto.h
+++ b/fs/smb/client/smb2proto.h
@@ -75,7 +75,8 @@ int smb2_query_path_info(const unsigned int xid,
struct cifs_open_info_data *data);
extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
const char *full_path, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_alloc);
+ struct cifs_sb_info *cifs_sb, bool set_alloc,
+ struct dentry *dentry);
extern int smb2_set_file_info(struct inode *inode, const char *full_path,
FILE_BASIC_INFO *buf, const unsigned int xid);
extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
@@ -91,7 +92,8 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
const char *name, struct cifs_sb_info *cifs_sb);
extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
- const char *name, struct cifs_sb_info *cifs_sb);
+ const char *name, struct cifs_sb_info *cifs_sb,
+ struct dentry *dentry);
int smb2_rename_path(const unsigned int xid,
struct cifs_tcon *tcon,
struct dentry *source_dentry,
--
2.39.2
From: Meetakshi Setiya <[email protected]>
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 <[email protected]>
---
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
On its own, the patch is expected to regress xfstests generic 013,
002 and 591.
The fix for 591 is in part 1 of this patch series
https://lore.kernel.org/linux-cifs/[email protected]/
The fix for 002 and 013 is in part 3 of this patch series
https://lore.kernel.org/linux-cifs/[email protected]/
Thanks
Meetakshi
On Fri, Feb 9, 2024 at 6:46 PM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> Currently, when a rename, unlink or set path size compound operation
> is requested on a file that has a lot of dirty pages to be written
> to the server, we do not send the lease key for these requests. As a
> result, the server can assume that this request is from a new client, and
> send a lease break notification to the same client, on the same
> connection. As a response to the lease break, the client can consume
> several credits to write the dirty pages to the server. Depending on the
> server's credit grant implementation, the server can stop granting more
> credits to this connection, and this can cause a deadlock (which can only
> be resolved when the lease timer on the server expires).
> One of the problems here is that the client is sending no lease key,
> even if it has a lease for the file. This patch fixes the problem by
> reusing the existing lease key on the file for rename, unlink and set path
> size compound operations so that the client does not break its own lease.
>
> A very trivial example could be a set of commands by a client that
> maintains open handle (for write) to a file and then tries to copy the
> contents of that file to another one, eg.,
>
> tail -f /dev/null > myfile &
> mv myfile myfile2
>
> Presently, the network capture on the client shows that the move (or
> rename) would trigger a lease break on the same client, for the same file.
> With the lease key reused, the lease break request-response overhead is
> eliminated, thereby reducing the roundtrips performed for this set of
> operations.
>
> The patch fixes the bug described above and also provides perf benefit.
>
> Signed-off-by: Meetakshi Setiya <[email protected]>
> ---
> fs/smb/client/cifsglob.h | 5 ++--
> fs/smb/client/cifsproto.h | 6 +++--
> fs/smb/client/cifssmb.c | 4 ++--
> fs/smb/client/inode.c | 10 ++++----
> fs/smb/client/smb2inode.c | 48 ++++++++++++++++++++++++---------------
> fs/smb/client/smb2proto.h | 6 +++--
> 6 files changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index f6b4acdcdeb3..baeed01d356b 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -371,7 +371,8 @@ struct smb_version_operations {
> struct cifs_open_info_data *data);
> /* set size by path */
> int (*set_path_size)(const unsigned int, struct cifs_tcon *,
> - const char *, __u64, struct cifs_sb_info *, bool);
> + const char *, __u64, struct cifs_sb_info *, bool,
> + struct dentry *);
> /* set size by file handle */
> int (*set_file_size)(const unsigned int, struct cifs_tcon *,
> struct cifsFileInfo *, __u64, bool);
> @@ -401,7 +402,7 @@ struct smb_version_operations {
> struct cifs_sb_info *);
> /* unlink file */
> int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
> - struct cifs_sb_info *);
> + struct cifs_sb_info *, struct dentry *);
> /* open, rename and delete file */
> int (*rename_pending_delete)(const char *, struct dentry *,
> const unsigned int);
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index f995b766b177..e108964950b5 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -406,7 +406,8 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
> __u32 pid_of_opener);
> extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> const char *file_name, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_allocation);
> + struct cifs_sb_info *cifs_sb, bool set_allocation,
> + struct dentry *dentry);
> extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifsFileInfo *cfile, __u64 size,
> bool set_allocation);
> @@ -442,7 +443,8 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
> const struct nls_table *nls_codepage,
> int remap_special_chars);
> extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
> - const char *name, struct cifs_sb_info *cifs_sb);
> + const char *name, struct cifs_sb_info *cifs_sb,
> + struct dentry *dentry);
> int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
> struct dentry *source_dentry,
> const char *from_name, const char *to_name,
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 01e89070df5a..301189ee1335 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
>
> int
> CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> - struct cifs_sb_info *cifs_sb)
> + struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> DELETE_FILE_REQ *pSMB = NULL;
> DELETE_FILE_RSP *pSMBr = NULL;
> @@ -4993,7 +4993,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
> int
> CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> - bool set_allocation)
> + bool set_allocation, struct dentry *dentry)
> {
> struct smb_com_transaction2_spi_req *pSMB = NULL;
> struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 8121b5b1aa22..c3e86876a2a8 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1846,7 +1846,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> goto psx_del_no_retry;
> }
>
> - rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
> + rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
>
> psx_del_no_retry:
> if (!rc) {
> @@ -2799,7 +2799,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
>
> static int
> cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> - unsigned int xid, const char *full_path)
> + unsigned int xid, const char *full_path, struct dentry *dentry)
> {
> int rc;
> struct cifsFileInfo *open_file;
> @@ -2850,7 +2850,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> */
> if (server->ops->set_path_size)
> rc = server->ops->set_path_size(xid, tcon, full_path,
> - attrs->ia_size, cifs_sb, false);
> + attrs->ia_size, cifs_sb, false, dentry);
> else
> rc = -ENOSYS;
> cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
> @@ -2940,7 +2940,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
> rc = 0;
>
> if (attrs->ia_valid & ATTR_SIZE) {
> - rc = cifs_set_file_size(inode, attrs, xid, full_path);
> + rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
> if (rc != 0)
> goto out;
> }
> @@ -3107,7 +3107,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
> }
>
> if (attrs->ia_valid & ATTR_SIZE) {
> - rc = cifs_set_file_size(inode, attrs, xid, full_path);
> + rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
> if (rc != 0)
> goto cifs_setattr_exit;
> }
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 05818cd6d932..69f3442c5b96 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -98,7 +98,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> __u32 desired_access, __u32 create_disposition,
> __u32 create_options, umode_t mode, struct kvec *in_iov,
> int *cmds, int num_cmds, struct cifsFileInfo *cfile,
> - struct kvec *out_iov, int *out_buftype)
> + struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
> {
>
> struct reparse_data_buffer *rbuf;
> @@ -115,6 +115,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> int resp_buftype[MAX_COMPOUND];
> struct smb2_query_info_rsp *qi_rsp = NULL;
> struct cifs_open_info_data *idata;
> + struct inode *inode = NULL;
> int flags = 0;
> __u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
> unsigned int size[2];
> @@ -152,6 +153,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> goto finished;
> }
>
> + /* if there is an existing lease, reuse it */
> + if (dentry) {
> + inode = d_inode(dentry);
> + if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> + oplock = SMB2_OPLOCK_LEVEL_LEASE;
> + server->ops->get_lease_key(inode, &fid);
> + }
> + }
> +
> vars->oparms = (struct cifs_open_parms) {
> .tcon = tcon,
> .path = full_path,
> @@ -747,7 +757,7 @@ int smb2_query_path_info(const unsigned int xid,
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> create_options, ACL_NO_MODE, in_iov,
> - cmds, 1, cfile, out_iov, out_buftype);
> + cmds, 1, cfile, out_iov, out_buftype, NULL);
> hdr = out_iov[0].iov_base;
> /*
> * If first iov is unset, then SMB session was dropped or we've got a
> @@ -779,7 +789,7 @@ int smb2_query_path_info(const unsigned int xid,
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> create_options, ACL_NO_MODE, in_iov,
> - cmds, num_cmds, cfile, NULL, NULL);
> + cmds, num_cmds, cfile, NULL, NULL, NULL);
> break;
> case -EREMOTE:
> break;
> @@ -811,7 +821,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
> FILE_WRITE_ATTRIBUTES, FILE_CREATE,
> CREATE_NOT_FILE, mode,
> NULL, &(int){SMB2_OP_MKDIR}, 1,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> void
> @@ -836,7 +846,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
> FILE_WRITE_ATTRIBUTES, FILE_CREATE,
> CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_INFO}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> if (tmprc == 0)
> cifs_i->cifsAttrs = dosattrs;
> }
> @@ -850,25 +860,26 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> DELETE, FILE_OPEN, CREATE_NOT_FILE,
> ACL_NO_MODE, NULL,
> &(int){SMB2_OP_RMDIR}, 1,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> int
> smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> - struct cifs_sb_info *cifs_sb)
> + struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> return 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, NULL, NULL, dentry);
> }
>
> static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> const char *from_name, const char *to_name,
> struct cifs_sb_info *cifs_sb,
> __u32 create_options, __u32 access,
> - int command, struct cifsFileInfo *cfile)
> + int command, struct cifsFileInfo *cfile,
> + struct dentry *dentry)
> {
> struct kvec in_iov;
> __le16 *smb2_to_name = NULL;
> @@ -883,7 +894,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
> rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
> FILE_OPEN, create_options, ACL_NO_MODE,
> - &in_iov, &command, 1, cfile, NULL, NULL);
> + &in_iov, &command, 1, cfile, NULL, NULL, dentry);
> smb2_rename_path:
> kfree(smb2_to_name);
> return rc;
> @@ -902,7 +913,7 @@ int smb2_rename_path(const unsigned int xid,
> 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,
> - co, DELETE, SMB2_OP_RENAME, cfile);
> + co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
> }
>
> int smb2_create_hardlink(const unsigned int xid,
> @@ -915,13 +926,14 @@ int smb2_create_hardlink(const unsigned int xid,
>
> return smb2_set_path_attr(xid, tcon, from_name, to_name,
> cifs_sb, co, FILE_READ_ATTRIBUTES,
> - SMB2_OP_HARDLINK, NULL);
> + SMB2_OP_HARDLINK, NULL, NULL);
> }
>
> int
> smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_alloc)
> + struct cifs_sb_info *cifs_sb, bool set_alloc,
> + struct dentry *dentry)
> {
> struct cifsFileInfo *cfile;
> struct kvec in_iov;
> @@ -934,7 +946,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> FILE_WRITE_DATA, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_EOF}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, dentry);
> }
>
> int
> @@ -963,7 +975,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
> FILE_WRITE_ATTRIBUTES, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_INFO}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> cifs_put_tlink(tlink);
> return rc;
> }
> @@ -998,7 +1010,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> da, cd, co, ACL_NO_MODE, in_iov,
> - cmds, 2, cfile, NULL, NULL);
> + cmds, 2, cfile, NULL, NULL, NULL);
> if (!rc) {
> rc = smb311_posix_get_inode_info(&new, full_path,
> data, sb, xid);
> @@ -1008,7 +1020,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> da, cd, co, ACL_NO_MODE, in_iov,
> - cmds, 2, cfile, NULL, NULL);
> + cmds, 2, cfile, NULL, NULL, NULL);
> if (!rc) {
> rc = cifs_get_inode_info(&new, full_path,
> data, sb, xid, NULL);
> @@ -1036,7 +1048,7 @@ int smb2_query_reparse_point(const unsigned int xid,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_GET_REPARSE}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> if (rc)
> goto out;
>
> diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
> index b3069911e9dd..221143788a1c 100644
> --- a/fs/smb/client/smb2proto.h
> +++ b/fs/smb/client/smb2proto.h
> @@ -75,7 +75,8 @@ int smb2_query_path_info(const unsigned int xid,
> struct cifs_open_info_data *data);
> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_alloc);
> + struct cifs_sb_info *cifs_sb, bool set_alloc,
> + struct dentry *dentry);
> extern int smb2_set_file_info(struct inode *inode, const char *full_path,
> FILE_BASIC_INFO *buf, const unsigned int xid);
> extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
> @@ -91,7 +92,8 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
> extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
> const char *name, struct cifs_sb_info *cifs_sb);
> extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
> - const char *name, struct cifs_sb_info *cifs_sb);
> + const char *name, struct cifs_sb_info *cifs_sb,
> + struct dentry *dentry);
> int smb2_rename_path(const unsigned int xid,
> struct cifs_tcon *tcon,
> struct dentry *source_dentry,
> --
> 2.39.2
>
Patch 2 of this patch series
https://lore.kernel.org/linux-cifs/[email protected]/
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 <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
could we make the "file_is_deleted" a boolean instead of using up more
space making it an int?
Alternatively - would it be possible to simply look at the file
attributes to see if it was marked as deleted (ie we should already be
setting ATTR_DELETE_ON_CLOSE)
On Fri, Feb 9, 2024 at 7:17 AM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> When a file/dentry has been deleted before closing all its open handles,
> currently, closing them can add them to the deferred close list. This can
> lead to problems in creating file with the same name when the file is
> re-created before the deferred close completes. This issue was seen while
> reusing a client's already existing lease on a file for compound operations
> and xfstest 591 failed because of the deferred close handle that remained
> valid even after the file was deleted and was being reused to create a file
> with the same name. The server in this case returns an error on open with
> STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
> handles are closed (duration specified in closetimeo).
>
> This patch fixes the issue by flagging all open handles for the deleted
> file (file path to be precise) with FILE_DELETED at the end of the unlink
> operation. A new field cflags has been introduced in the cifsFileInfo
> structure to set the FILE_DELETED flag without interfering with the f_flags
> field. This cflags field could be useful in the future for setting more
> flags specific to cfile.
> When doing close in cifs_close for each of these handles, check the
> FILE_DELETED flag and do not defer close these handles if the corresponding
> filepath has been deleted.
>
> Signed-off-by: Meetakshi Setiya <[email protected]>
> ---
> fs/smb/client/cifsglob.h | 3 +++
> fs/smb/client/cifsproto.h | 4 ++++
> fs/smb/client/file.c | 3 ++-
> fs/smb/client/inode.c | 2 ++
> fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> 5 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 16befff4cbb4..f6b4acdcdeb3 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
> struct list_head locks; /* locks held by fid above */
> };
>
> +#define FILE_DELETED 00000001
> +
> struct cifsFileInfo {
> /* following two lists are protected by tcon->open_file_lock */
> struct list_head tlist; /* pointer to next fid owned by tcon */
> @@ -1413,6 +1415,7 @@ struct cifsFileInfo {
> struct dentry *dentry;
> struct tcon_link *tlink;
> unsigned int f_flags;
> + unsigned int cflags; /* flags for this file */
> bool invalidHandle:1; /* file closed via session abend */
> bool swapfile:1;
> bool oplock_break_cancelled:1;
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index a841bf4967fa..f995b766b177 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
>
> extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
> const char *path);
> +
> +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
> + *cifs_tcon, const char *path);
> +
> extern struct TCP_Server_Info *
> cifs_get_tcp_session(struct smb3_fs_context *ctx,
> struct TCP_Server_Info *primary_server);
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index b75282c204da..91cf4d2df4de 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> cfile->uid = current_fsuid();
> cfile->dentry = dget(dentry);
> cfile->f_flags = file->f_flags;
> + cfile->cflags = 0;
> cfile->invalidHandle = false;
> cfile->deferred_close_scheduled = false;
> cfile->tlink = cifs_get_tlink(tlink);
> @@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
> if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
> && cinode->lease_granted &&
> !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
> - dclose) {
> + dclose && !(cfile->cflags & FILE_DELETED)) {
> if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
> inode_set_mtime_to_ts(inode,
> inode_set_ctime_current(inode));
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index d02f8ba29cb5..8121b5b1aa22 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> cifs_inode = CIFS_I(dir);
> CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
> unlink_out:
> + if (rc == 0)
> + cifs_mark_open_handles_for_deleted_file(tcon, full_path);
> free_dentry_path(page);
> kfree(attrs);
> free_xid(xid);
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index 0748d7b757b9..8e46dee1a36c 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
> free_dentry_path(page);
> }
>
> +/*
> + * If a dentry has been deleted, all corresponding open handles should know that
> + * so that we do not defer close them.
> + */
> +void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
> + const char *path)
> +{
> + struct cifsFileInfo *cfile;
> + void *page;
> + const char *full_path;
> +
> + page = alloc_dentry_path();
> + spin_lock(&tcon->open_file_lock);
> + list_for_each_entry(cfile, &tcon->openFileList, tlist) {
> + full_path = build_path_from_dentry(cfile->dentry, page);
> + if (strcmp(full_path, path) == 0)
> + cfile->cflags |= FILE_DELETED;
> + }
> + spin_unlock(&tcon->open_file_lock);
> + free_dentry_path(page);
> +}
> +
> /* parses DFS referral V3 structure
> * caller is responsible for freeing target_nodes
> * returns:
> --
> 2.39.2
>
>
--
Thanks,
Steve
On Sat, Feb 10, 2024 at 11:29 AM Steve French <[email protected]> wrote:
>
> could we make the "file_is_deleted" a boolean instead of using up more
> space making it an int?
Meetakshi initially had it as a bool. I asked her to make it a bitmap,
just in case there can be other such flags that are needed in the
future.
>
> Alternatively - would it be possible to simply look at the file
> attributes to see if it was marked as deleted (ie we should already be
> setting ATTR_DELETE_ON_CLOSE)
It does not look like we use this attr anywhere today. (Am I missing something?)
Also, it looks like a flag that SMB uses in requests and responses.
I feel that it's better to keep a different flag for this purpose.
>
> On Fri, Feb 9, 2024 at 7:17 AM <[email protected]> wrote:
> >
> > From: Meetakshi Setiya <[email protected]>
> >
> > When a file/dentry has been deleted before closing all its open handles,
> > currently, closing them can add them to the deferred close list. This can
> > lead to problems in creating file with the same name when the file is
> > re-created before the deferred close completes. This issue was seen while
> > reusing a client's already existing lease on a file for compound operations
> > and xfstest 591 failed because of the deferred close handle that remained
> > valid even after the file was deleted and was being reused to create a file
> > with the same name. The server in this case returns an error on open with
> > STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
> > handles are closed (duration specified in closetimeo).
> >
> > This patch fixes the issue by flagging all open handles for the deleted
> > file (file path to be precise) with FILE_DELETED at the end of the unlink
> > operation. A new field cflags has been introduced in the cifsFileInfo
> > structure to set the FILE_DELETED flag without interfering with the f_flags
> > field. This cflags field could be useful in the future for setting more
> > flags specific to cfile.
> > When doing close in cifs_close for each of these handles, check the
> > FILE_DELETED flag and do not defer close these handles if the corresponding
> > filepath has been deleted.
> >
> > Signed-off-by: Meetakshi Setiya <[email protected]>
> > ---
> > fs/smb/client/cifsglob.h | 3 +++
> > fs/smb/client/cifsproto.h | 4 ++++
> > fs/smb/client/file.c | 3 ++-
> > fs/smb/client/inode.c | 2 ++
> > fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> > 5 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > index 16befff4cbb4..f6b4acdcdeb3 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
> > struct list_head locks; /* locks held by fid above */
> > };
> >
> > +#define FILE_DELETED 00000001
> > +
> > struct cifsFileInfo {
> > /* following two lists are protected by tcon->open_file_lock */
> > struct list_head tlist; /* pointer to next fid owned by tcon */
> > @@ -1413,6 +1415,7 @@ struct cifsFileInfo {
> > struct dentry *dentry;
> > struct tcon_link *tlink;
> > unsigned int f_flags;
> > + unsigned int cflags; /* flags for this file */
> > bool invalidHandle:1; /* file closed via session abend */
> > bool swapfile:1;
> > bool oplock_break_cancelled:1;
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index a841bf4967fa..f995b766b177 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
> >
> > extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
> > const char *path);
> > +
> > +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
> > + *cifs_tcon, const char *path);
> > +
> > extern struct TCP_Server_Info *
> > cifs_get_tcp_session(struct smb3_fs_context *ctx,
> > struct TCP_Server_Info *primary_server);
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index b75282c204da..91cf4d2df4de 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> > cfile->uid = current_fsuid();
> > cfile->dentry = dget(dentry);
> > cfile->f_flags = file->f_flags;
> > + cfile->cflags = 0;
> > cfile->invalidHandle = false;
> > cfile->deferred_close_scheduled = false;
> > cfile->tlink = cifs_get_tlink(tlink);
> > @@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
> > if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
> > && cinode->lease_granted &&
> > !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
> > - dclose) {
> > + dclose && !(cfile->cflags & FILE_DELETED)) {
> > if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
> > inode_set_mtime_to_ts(inode,
> > inode_set_ctime_current(inode));
> > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> > index d02f8ba29cb5..8121b5b1aa22 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> > cifs_inode = CIFS_I(dir);
> > CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
> > unlink_out:
> > + if (rc == 0)
> > + cifs_mark_open_handles_for_deleted_file(tcon, full_path);
> > free_dentry_path(page);
> > kfree(attrs);
> > free_xid(xid);
> > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> > index 0748d7b757b9..8e46dee1a36c 100644
> > --- a/fs/smb/client/misc.c
> > +++ b/fs/smb/client/misc.c
> > @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
> > free_dentry_path(page);
> > }
> >
> > +/*
> > + * If a dentry has been deleted, all corresponding open handles should know that
> > + * so that we do not defer close them.
> > + */
> > +void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
> > + const char *path)
> > +{
> > + struct cifsFileInfo *cfile;
> > + void *page;
> > + const char *full_path;
> > +
> > + page = alloc_dentry_path();
> > + spin_lock(&tcon->open_file_lock);
> > + list_for_each_entry(cfile, &tcon->openFileList, tlist) {
> > + full_path = build_path_from_dentry(cfile->dentry, page);
> > + if (strcmp(full_path, path) == 0)
> > + cfile->cflags |= FILE_DELETED;
> > + }
> > + spin_unlock(&tcon->open_file_lock);
> > + free_dentry_path(page);
> > +}
> > +
> > /* parses DFS referral V3 structure
> > * caller is responsible for freeing target_nodes
> > * returns:
> > --
> > 2.39.2
> >
> >
>
>
> --
> Thanks,
>
> Steve
--
Regards,
Shyam
PFA the follow up patch that uses a boolean field status_file_deleted
instead of a bitmap to mark open handles for files that have been deleted.
Additionally, if SMB2 query info response is received with the DeletePending
flag set to true for a client, mark the corresponding open file handles
in this case too.
Thanks
Meetakshi
On Sun, Feb 11, 2024 at 2:34 AM Steve French <[email protected]> wrote:
>
> I lean toward using the existing attr field since it presumably could be set remotely and best to be safe and consistent and also uses less space
>
> ATTR_DELETE_ON_CLOSE
>
> On Sat, Feb 10, 2024, 11:50 Shyam Prasad N <[email protected]> wrote:
>>
>> On Sat, Feb 10, 2024 at 11:29 AM Steve French <[email protected]> wrote:
>> >
>> > could we make the "file_is_deleted" a boolean instead of using up more
>> > space making it an int?
>>
>> Meetakshi initially had it as a bool. I asked her to make it a bitmap,
>> just in case there can be other such flags that are needed in the
>> future.
>>
>> >
>> > Alternatively - would it be possible to simply look at the file
>> > attributes to see if it was marked as deleted (ie we should already be
>> > setting ATTR_DELETE_ON_CLOSE)
>>
>> It does not look like we use this attr anywhere today. (Am I missing something?)
>> Also, it looks like a flag that SMB uses in requests and responses.
>> I feel that it's better to keep a different flag for this purpose.
>>
>> >
>> > On Fri, Feb 9, 2024 at 7:17 AM <[email protected]> wrote:
>> > >
>> > > From: Meetakshi Setiya <[email protected]>
>> > >
>> > > When a file/dentry has been deleted before closing all its open handles,
>> > > currently, closing them can add them to the deferred close list. This can
>> > > lead to problems in creating file with the same name when the file is
>> > > re-created before the deferred close completes. This issue was seen while
>> > > reusing a client's already existing lease on a file for compound operations
>> > > and xfstest 591 failed because of the deferred close handle that remained
>> > > valid even after the file was deleted and was being reused to create a file
>> > > with the same name. The server in this case returns an error on open with
>> > > STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
>> > > handles are closed (duration specified in closetimeo).
>> > >
>> > > This patch fixes the issue by flagging all open handles for the deleted
>> > > file (file path to be precise) with FILE_DELETED at the end of the unlink
>> > > operation. A new field cflags has been introduced in the cifsFileInfo
>> > > structure to set the FILE_DELETED flag without interfering with the f_flags
>> > > field. This cflags field could be useful in the future for setting more
>> > > flags specific to cfile.
>> > > When doing close in cifs_close for each of these handles, check the
>> > > FILE_DELETED flag and do not defer close these handles if the corresponding
>> > > filepath has been deleted.
>> > >
>> > > Signed-off-by: Meetakshi Setiya <[email protected]>
>> > > ---
>> > > fs/smb/client/cifsglob.h | 3 +++
>> > > fs/smb/client/cifsproto.h | 4 ++++
>> > > fs/smb/client/file.c | 3 ++-
>> > > fs/smb/client/inode.c | 2 ++
>> > > fs/smb/client/misc.c | 22 ++++++++++++++++++++++
>> > > 5 files changed, 33 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> > > index 16befff4cbb4..f6b4acdcdeb3 100644
>> > > --- a/fs/smb/client/cifsglob.h
>> > > +++ b/fs/smb/client/cifsglob.h
>> > > @@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
>> > > struct list_head locks; /* locks held by fid above */
>> > > };
>> > >
>> > > +#define FILE_DELETED 00000001
>> > > +
>> > > struct cifsFileInfo {
>> > > /* following two lists are protected by tcon->open_file_lock */
>> > > struct list_head tlist; /* pointer to next fid owned by tcon */
>> > > @@ -1413,6 +1415,7 @@ struct cifsFileInfo {
>> > > struct dentry *dentry;
>> > > struct tcon_link *tlink;
>> > > unsigned int f_flags;
>> > > + unsigned int cflags; /* flags for this file */
>> > > bool invalidHandle:1; /* file closed via session abend */
>> > > bool swapfile:1;
>> > > bool oplock_break_cancelled:1;
>> > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> > > index a841bf4967fa..f995b766b177 100644
>> > > --- a/fs/smb/client/cifsproto.h
>> > > +++ b/fs/smb/client/cifsproto.h
>> > > @@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
>> > >
>> > > extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
>> > > const char *path);
>> > > +
>> > > +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
>> > > + *cifs_tcon, const char *path);
>> > > +
>> > > extern struct TCP_Server_Info *
>> > > cifs_get_tcp_session(struct smb3_fs_context *ctx,
>> > > struct TCP_Server_Info *primary_server);
>> > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>> > > index b75282c204da..91cf4d2df4de 100644
>> > > --- a/fs/smb/client/file.c
>> > > +++ b/fs/smb/client/file.c
>> > > @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>> > > cfile->uid = current_fsuid();
>> > > cfile->dentry = dget(dentry);
>> > > cfile->f_flags = file->f_flags;
>> > > + cfile->cflags = 0;
>> > > cfile->invalidHandle = false;
>> > > cfile->deferred_close_scheduled = false;
>> > > cfile->tlink = cifs_get_tlink(tlink);
>> > > @@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
>> > > if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
>> > > && cinode->lease_granted &&
>> > > !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
>> > > - dclose) {
>> > > + dclose && !(cfile->cflags & FILE_DELETED)) {
>> > > if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
>> > > inode_set_mtime_to_ts(inode,
>> > > inode_set_ctime_current(inode));
>> > > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
>> > > index d02f8ba29cb5..8121b5b1aa22 100644
>> > > --- a/fs/smb/client/inode.c
>> > > +++ b/fs/smb/client/inode.c
>> > > @@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
>> > > cifs_inode = CIFS_I(dir);
>> > > CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
>> > > unlink_out:
>> > > + if (rc == 0)
>> > > + cifs_mark_open_handles_for_deleted_file(tcon, full_path);
>> > > free_dentry_path(page);
>> > > kfree(attrs);
>> > > free_xid(xid);
>> > > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>> > > index 0748d7b757b9..8e46dee1a36c 100644
>> > > --- a/fs/smb/client/misc.c
>> > > +++ b/fs/smb/client/misc.c
>> > > @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
>> > > free_dentry_path(page);
>> > > }
>> > >
>> > > +/*
>> > > + * If a dentry has been deleted, all corresponding open handles should know that
>> > > + * so that we do not defer close them.
>> > > + */
>> > > +void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
>> > > + const char *path)
>> > > +{
>> > > + struct cifsFileInfo *cfile;
>> > > + void *page;
>> > > + const char *full_path;
>> > > +
>> > > + page = alloc_dentry_path();
>> > > + spin_lock(&tcon->open_file_lock);
>> > > + list_for_each_entry(cfile, &tcon->openFileList, tlist) {
>> > > + full_path = build_path_from_dentry(cfile->dentry, page);
>> > > + if (strcmp(full_path, path) == 0)
>> > > + cfile->cflags |= FILE_DELETED;
>> > > + }
>> > > + spin_unlock(&tcon->open_file_lock);
>> > > + free_dentry_path(page);
>> > > +}
>> > > +
>> > > /* parses DFS referral V3 structure
>> > > * caller is responsible for freeing target_nodes
>> > > * returns:
>> > > --
>> > > 2.39.2
>> > >
>> > >
>> >
>> >
>> > --
>> > Thanks,
>> >
>> > Steve
>>
>>
>>
>> --
>> Regards,
>> Shyam
On Wed, Feb 21, 2024 at 10:14 AM Meetakshi Setiya
<[email protected]> wrote:
>
> PFA the follow up patch that uses a boolean field status_file_deleted
> instead of a bitmap to mark open handles for files that have been deleted.
> Additionally, if SMB2 query info response is received with the DeletePending
> flag set to true for a client, mark the corresponding open file handles
> in this case too.
>
> Thanks
> Meetakshi
>
> On Sun, Feb 11, 2024 at 2:34 AM Steve French <[email protected]> wrote:
> >
> > I lean toward using the existing attr field since it presumably could be set remotely and best to be safe and consistent and also uses less space
> >
> > ATTR_DELETE_ON_CLOSE
> >
> > On Sat, Feb 10, 2024, 11:50 Shyam Prasad N <[email protected]> wrote:
> >>
> >> On Sat, Feb 10, 2024 at 11:29 AM Steve French <[email protected]> wrote:
> >> >
> >> > could we make the "file_is_deleted" a boolean instead of using up more
> >> > space making it an int?
> >>
> >> Meetakshi initially had it as a bool. I asked her to make it a bitmap,
> >> just in case there can be other such flags that are needed in the
> >> future.
> >>
> >> >
> >> > Alternatively - would it be possible to simply look at the file
> >> > attributes to see if it was marked as deleted (ie we should already be
> >> > setting ATTR_DELETE_ON_CLOSE)
> >>
> >> It does not look like we use this attr anywhere today. (Am I missing something?)
> >> Also, it looks like a flag that SMB uses in requests and responses.
> >> I feel that it's better to keep a different flag for this purpose.
> >>
> >> >
> >> > On Fri, Feb 9, 2024 at 7:17 AM <[email protected]> wrote:
> >> > >
> >> > > From: Meetakshi Setiya <[email protected]>
> >> > >
> >> > > When a file/dentry has been deleted before closing all its open handles,
> >> > > currently, closing them can add them to the deferred close list. This can
> >> > > lead to problems in creating file with the same name when the file is
> >> > > re-created before the deferred close completes. This issue was seen while
> >> > > reusing a client's already existing lease on a file for compound operations
> >> > > and xfstest 591 failed because of the deferred close handle that remained
> >> > > valid even after the file was deleted and was being reused to create a file
> >> > > with the same name. The server in this case returns an error on open with
> >> > > STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
> >> > > handles are closed (duration specified in closetimeo).
> >> > >
> >> > > This patch fixes the issue by flagging all open handles for the deleted
> >> > > file (file path to be precise) with FILE_DELETED at the end of the unlink
> >> > > operation. A new field cflags has been introduced in the cifsFileInfo
> >> > > structure to set the FILE_DELETED flag without interfering with the f_flags
> >> > > field. This cflags field could be useful in the future for setting more
> >> > > flags specific to cfile.
> >> > > When doing close in cifs_close for each of these handles, check the
> >> > > FILE_DELETED flag and do not defer close these handles if the corresponding
> >> > > filepath has been deleted.
> >> > >
> >> > > Signed-off-by: Meetakshi Setiya <[email protected]>
> >> > > ---
> >> > > fs/smb/client/cifsglob.h | 3 +++
> >> > > fs/smb/client/cifsproto.h | 4 ++++
> >> > > fs/smb/client/file.c | 3 ++-
> >> > > fs/smb/client/inode.c | 2 ++
> >> > > fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> >> > > 5 files changed, 33 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> >> > > index 16befff4cbb4..f6b4acdcdeb3 100644
> >> > > --- a/fs/smb/client/cifsglob.h
> >> > > +++ b/fs/smb/client/cifsglob.h
> >> > > @@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
> >> > > struct list_head locks; /* locks held by fid above */
> >> > > };
> >> > >
> >> > > +#define FILE_DELETED 00000001
> >> > > +
> >> > > struct cifsFileInfo {
> >> > > /* following two lists are protected by tcon->open_file_lock */
> >> > > struct list_head tlist; /* pointer to next fid owned by tcon */
> >> > > @@ -1413,6 +1415,7 @@ struct cifsFileInfo {
> >> > > struct dentry *dentry;
> >> > > struct tcon_link *tlink;
> >> > > unsigned int f_flags;
> >> > > + unsigned int cflags; /* flags for this file */
> >> > > bool invalidHandle:1; /* file closed via session abend */
> >> > > bool swapfile:1;
> >> > > bool oplock_break_cancelled:1;
> >> > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> >> > > index a841bf4967fa..f995b766b177 100644
> >> > > --- a/fs/smb/client/cifsproto.h
> >> > > +++ b/fs/smb/client/cifsproto.h
> >> > > @@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
> >> > >
> >> > > extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
> >> > > const char *path);
> >> > > +
> >> > > +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
> >> > > + *cifs_tcon, const char *path);
> >> > > +
> >> > > extern struct TCP_Server_Info *
> >> > > cifs_get_tcp_session(struct smb3_fs_context *ctx,
> >> > > struct TCP_Server_Info *primary_server);
> >> > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> >> > > index b75282c204da..91cf4d2df4de 100644
> >> > > --- a/fs/smb/client/file.c
> >> > > +++ b/fs/smb/client/file.c
> >> > > @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> >> > > cfile->uid = current_fsuid();
> >> > > cfile->dentry = dget(dentry);
> >> > > cfile->f_flags = file->f_flags;
> >> > > + cfile->cflags = 0;
> >> > > cfile->invalidHandle = false;
> >> > > cfile->deferred_close_scheduled = false;
> >> > > cfile->tlink = cifs_get_tlink(tlink);
> >> > > @@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
> >> > > if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
> >> > > && cinode->lease_granted &&
> >> > > !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
> >> > > - dclose) {
> >> > > + dclose && !(cfile->cflags & FILE_DELETED)) {
> >> > > if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
> >> > > inode_set_mtime_to_ts(inode,
> >> > > inode_set_ctime_current(inode));
> >> > > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> >> > > index d02f8ba29cb5..8121b5b1aa22 100644
> >> > > --- a/fs/smb/client/inode.c
> >> > > +++ b/fs/smb/client/inode.c
> >> > > @@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> >> > > cifs_inode = CIFS_I(dir);
> >> > > CIFS_I(dir)->time = 0; /* force revalidate of dir as well */
> >> > > unlink_out:
> >> > > + if (rc == 0)
> >> > > + cifs_mark_open_handles_for_deleted_file(tcon, full_path);
> >> > > free_dentry_path(page);
> >> > > kfree(attrs);
> >> > > free_xid(xid);
> >> > > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> >> > > index 0748d7b757b9..8e46dee1a36c 100644
> >> > > --- a/fs/smb/client/misc.c
> >> > > +++ b/fs/smb/client/misc.c
> >> > > @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
> >> > > free_dentry_path(page);
> >> > > }
> >> > >
> >> > > +/*
> >> > > + * If a dentry has been deleted, all corresponding open handles should know that
> >> > > + * so that we do not defer close them.
> >> > > + */
> >> > > +void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
> >> > > + const char *path)
> >> > > +{
> >> > > + struct cifsFileInfo *cfile;
> >> > > + void *page;
> >> > > + const char *full_path;
> >> > > +
> >> > > + page = alloc_dentry_path();
> >> > > + spin_lock(&tcon->open_file_lock);
> >> > > + list_for_each_entry(cfile, &tcon->openFileList, tlist) {
Is there a reason why we need to iterate over tcon->openFileList here?
There may be a large number of open files in the tcon.
Why not iterate over cinode->openFileList? That will be a much smaller
list and should cover all the open files that you need. Right?
> >> > > + full_path = build_path_from_dentry(cfile->dentry, page);
> >> > > + if (strcmp(full_path, path) == 0)
> >> > > + cfile->cflags |= FILE_DELETED;
> >> > > + }
> >> > > + spin_unlock(&tcon->open_file_lock);
> >> > > + free_dentry_path(page);
> >> > > +}
> >> > > +
> >> > > /* parses DFS referral V3 structure
> >> > > * caller is responsible for freeing target_nodes
> >> > > * returns:
> >> > > --
> >> > > 2.39.2
> >> > >
> >> > >
> >> >
> >> >
> >> > --
> >> > Thanks,
> >> >
> >> > Steve
> >>
> >>
> >>
> >> --
> >> Regards,
> >> Shyam
--
Regards,
Shyam