| Message ID | 20240202170537.409508-1-meetakshisetiyaoss@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: client: do not defer close open handles to deleted files | expand |
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 > >
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.
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: