Message-ID: <CAFTVevU94O1NEJ1XzrUSDoC7b4=W=wciqRWX8LoB1X1=AUNZfA@mail.gmail.com>
Date: Wed, 21 Feb 2024 10:14:34 +0530
From: Meetakshi Setiya <meetakshisetiyaoss@...il.com>
To: Steve French <smfrench@...il.com>
Cc: Shyam Prasad N <nspmangalore@...il.com>, Steve French <sfrench@...ba.org>, 
	Paulo Alcantara <pc@...guebit.com>, ronnie sahlberg <ronniesahlberg@...il.com>, 
	Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>, 
	CIFS <linux-cifs@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	samba-technical <samba-technical@...ts.samba.org>, Bharath SM <bharathsm.hsk@...il.com>, 
	Meetakshi Setiya <msetiya@...rosoft.com>
Subject: Re: [PATCH 1/3] smb: client: do not defer close open handles to
 deleted files

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@...il.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@...il.com> wrote:
>>
>> On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench@...il.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@...il.com> wrote:
>> > >
>> > > From: Meetakshi Setiya <msetiya@...rosoft.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@...rosoft.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

Download attachment "0001-smb-client-do-not-defer-close-open-handles-to-delete.patch" of type "application/octet-stream" (8053 bytes)

Powered by blists - more mailing lists