Re: [PATCH] smb: client: do not defer close open handles to deleted files

From: Shyam Prasad N <nspmangalore@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: meetakshisetiyaoss@gmail.com, pc@manguebit.com,
	ronniesahlberg@gmail.com,  sprasad@microsoft.com, tom@talpey.com,
	linux-cifs@vger.kernel.org,  linux-kernel@vger.kernel.org,
	samba-technical@lists.samba.org,  bharathsm.hsk@gmail.com,
	msetiya@microsoft.com
Subject: Re: [PATCH] smb: client: do not defer close open handles to deleted files
Date: Thu, 8 Feb 2024 22:10:59 +0530	[thread overview]
Message-ID: <CANT5p=p8BxhPANdU=YsJiXg_s=j8COG3-VgPK0MopFeMzDUfhA@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5msEtpJGTvK2F2TY0jR9hU0e7LYJekiyV1zD7MM+i9Oxgg@mail.gmail.com>

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