[PATCH 1/3] smb: client: do not defer close open handles to deleted files
Steve French
smfrench at gmail.com
Sat Feb 10 21:04:15 UTC 2024
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 at gmail.com> wrote:
> On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench at 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 at gmail.com> wrote:
> > >
> > > From: Meetakshi Setiya <msetiya at 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 at 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
>
More information about the samba-technical
mailing list