| Message ID | 20240209131552.471765-1-meetakshisetiyaoss@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] smb: client: do not defer close open handles to deleted files | expand |
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 <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. 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 <msetiya@microsoft.com> > --- > 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 > >
On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench@gmail.com> 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 <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. 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 <msetiya@microsoft.com> > > --- > > 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
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 <smfrench@gmail.com> 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 <nspmangalore@gmail.com> wrote: >> >> On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench@gmail.com> 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 <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. 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 <msetiya@microsoft.com> >> > > --- >> > > 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 <meetakshisetiyaoss@gmail.com> 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 <smfrench@gmail.com> 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 <nspmangalore@gmail.com> wrote: > >> > >> On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench@gmail.com> 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 <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. 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 <msetiya@microsoft.com> > >> > > --- > >> > > 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
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: