* [PATCH] smb: client: do not defer close open handles to deleted files
@ 2024-02-02 17:05 meetakshisetiyaoss
2024-02-04 21:57 ` Steve French
0 siblings, 1 reply; 3+ messages in thread
From: meetakshisetiyaoss @ 2024-02-02 17:05 UTC (permalink / raw)
To: sfrench, pc, ronniesahlberg, sprasad, nspmangalore, tom,
linux-cifs, linux-kernel, samba-technical, bharathsm.hsk,
msetiya
From: Meetakshi Setiya <msetiya@microsoft.com>
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. When doing close in cifs_close for each of
these handles, check the flag and do not defer close these handles if
the corresponding filepath has been deleted.
Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
fs/smb/client/cifsglob.h | 1 +
fs/smb/client/cifsproto.h | 4 ++++
fs/smb/client/file.c | 2 +-
fs/smb/client/inode.c | 2 ++
fs/smb/client/misc.c | 22 ++++++++++++++++++++++
5 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index decf80131bbe..826da216a9e1 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1399,6 +1399,7 @@ struct cifs_fid_locks {
struct list_head locks; /* locks held by fid above */
};
+#define FILE_DELETED 0x550
struct cifsFileInfo {
/* following two lists are protected by tcon->open_file_lock */
struct list_head tlist; /* pointer to next fid owned by tcon */
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 770db9026850..67f75497db02 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -292,6 +292,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..4c87ddbe8a5a 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -1085,7 +1085,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->f_flags & 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 24489e1e238a..be5bc01319d8 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1822,6 +1822,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..ebc867bfc034 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 the
+ * same, 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->f_flags |= FILE_DELETED;
+ }
+ spin_unlock(&tcon->open_file_lock);
+}
+
/* parses DFS referral V3 structure
* caller is responsible for freeing target_nodes
* returns:
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] smb: client: do not defer close open handles to deleted files
2024-02-02 17:05 [PATCH] smb: client: do not defer close open handles to deleted files meetakshisetiyaoss
@ 2024-02-04 21:57 ` Steve French
2024-02-08 16:40 ` Shyam Prasad N
0 siblings, 1 reply; 3+ messages in thread
From: Steve French @ 2024-02-04 21:57 UTC (permalink / raw)
To: meetakshisetiyaoss
Cc: pc, ronniesahlberg, sprasad, nspmangalore, tom, linux-cifs,
linux-kernel, samba-technical, bharathsm.hsk, msetiya
This was puzzling because I tried some experiments with deleting files
that were deferred close and noticed that we already making sure that
they weren't deferred close in cifs_unlink by calling
cifs_close_deferred_file_under_dentry(tcon, full_path) even in
Rohith's original code from a few years ago - so I didn't see cases
where we had a file marked for delete that was deferred close. How
did you reproduce that problem?
On Fri, Feb 2, 2024 at 11:39 AM <meetakshisetiyaoss@gmail.com> wrote:
>
> From: Meetakshi Setiya <msetiya@microsoft.com>
>
> 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. When doing close in cifs_close for each of
> these handles, check the flag and do not defer close these handles if
> the corresponding filepath has been deleted.
>
> Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
> ---
> fs/smb/client/cifsglob.h | 1 +
> fs/smb/client/cifsproto.h | 4 ++++
> fs/smb/client/file.c | 2 +-
> fs/smb/client/inode.c | 2 ++
> fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> 5 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index decf80131bbe..826da216a9e1 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1399,6 +1399,7 @@ struct cifs_fid_locks {
> struct list_head locks; /* locks held by fid above */
> };
>
> +#define FILE_DELETED 0x550
> struct cifsFileInfo {
> /* following two lists are protected by tcon->open_file_lock */
> struct list_head tlist; /* pointer to next fid owned by tcon */
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 770db9026850..67f75497db02 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -292,6 +292,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..4c87ddbe8a5a 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -1085,7 +1085,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->f_flags & 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 24489e1e238a..be5bc01319d8 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1822,6 +1822,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..ebc867bfc034 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 the
> + * same, 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->f_flags |= FILE_DELETED;
> + }
> + spin_unlock(&tcon->open_file_lock);
> +}
> +
> /* parses DFS referral V3 structure
> * caller is responsible for freeing target_nodes
> * returns:
> --
> 2.39.2
>
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] smb: client: do not defer close open handles to deleted files
2024-02-04 21:57 ` Steve French
@ 2024-02-08 16:40 ` Shyam Prasad N
0 siblings, 0 replies; 3+ messages in thread
From: Shyam Prasad N @ 2024-02-08 16:40 UTC (permalink / raw)
To: Steve French
Cc: meetakshisetiyaoss, pc, ronniesahlberg, sprasad, tom, linux-cifs,
linux-kernel, samba-technical, bharathsm.hsk, msetiya
On Mon, Feb 5, 2024 at 3:27 AM Steve French <smfrench@gmail.com> wrote:
>
> This was puzzling because I tried some experiments with deleting files
> that were deferred close and noticed that we already making sure that
> they weren't deferred close in cifs_unlink by calling
> cifs_close_deferred_file_under_dentry(tcon, full_path) even in
> Rohith's original code from a few years ago - so I didn't see cases
> where we had a file marked for delete that was deferred close. How
> did you reproduce that problem?
>
The issue here is that other bugs are hiding this bug.
When Meetakshi fixed the missing lease key bug for deletes/renames/set
size, the tests started breaking.
But the bug definitely exists.
> On Fri, Feb 2, 2024 at 11:39 AM <meetakshisetiyaoss@gmail.com> wrote:
> >
> > From: Meetakshi Setiya <msetiya@microsoft.com>
> >
> > 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. When doing close in cifs_close for each of
> > these handles, check the flag and do not defer close these handles if
> > the corresponding filepath has been deleted.
> >
> > Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
> > ---
> > fs/smb/client/cifsglob.h | 1 +
> > fs/smb/client/cifsproto.h | 4 ++++
> > fs/smb/client/file.c | 2 +-
> > fs/smb/client/inode.c | 2 ++
> > fs/smb/client/misc.c | 22 ++++++++++++++++++++++
> > 5 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > index decf80131bbe..826da216a9e1 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -1399,6 +1399,7 @@ struct cifs_fid_locks {
> > struct list_head locks; /* locks held by fid above */
> > };
> >
> > +#define FILE_DELETED 0x550
> > struct cifsFileInfo {
> > /* following two lists are protected by tcon->open_file_lock */
> > struct list_head tlist; /* pointer to next fid owned by tcon */
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index 770db9026850..67f75497db02 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -292,6 +292,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..4c87ddbe8a5a 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -1085,7 +1085,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->f_flags & 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 24489e1e238a..be5bc01319d8 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -1822,6 +1822,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..ebc867bfc034 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 the
> > + * same, 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->f_flags |= FILE_DELETED;
> > + }
> > + spin_unlock(&tcon->open_file_lock);
> > +}
> > +
> > /* parses DFS referral V3 structure
> > * caller is responsible for freeing target_nodes
> > * returns:
> > --
> > 2.39.2
> >
> >
>
>
> --
> Thanks,
>
> Steve
Hi Meetakshi,
Please fix the issue that Steve indicated in our call.
i.e. use new "flags" field for cfile, rather than f_flags. And use a
power of 2 value for FILE_DELETED.
Other than that, it looks good to me.
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-08 16:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-02 17:05 [PATCH] smb: client: do not defer close open handles to deleted files meetakshisetiyaoss
2024-02-04 21:57 ` Steve French
2024-02-08 16:40 ` Shyam Prasad N
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).