SMB3: Close deferred file handles if we get handle lease break

From: Bharath SM <bharathsm@microsoft.com>

We should not cache deferred file handles if we dont have
handle lease on a file. And we should immediately close all
deferred handles in case of handle lease break.

Signed-off-by: Bharath SM <bharathsm@microsoft.com>
Fixes: 9e31678fb403 ("SMB3: fix lease break timeout when multiple deferred close handles for the same file.")
---
 fs/cifs/cifsproto.h |  3 ++-
 fs/cifs/file.c      | 21 +++++++++++++++++++++
 fs/cifs/misc.c      |  4 ++--
 3 files changed, 25 insertions(+), 3 deletions(-)

This looks like a very important patch (I could repro some of the problems
he mentioned in other threads) - tentatively merged into cifs-2.6.git for-next

Am testing now - but let me know if anyone sees any problems with this.

On Thu, Mar 23, 2023 at 2:07 PM <bharathsm.hsk@gmail.com> wrote:
>
> From: Bharath SM <bharathsm@microsoft.com>
>
> We should not cache deferred file handles if we dont have
> handle lease on a file. And we should immediately close all
> deferred handles in case of handle lease break.
>
> Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> Fixes: 9e31678fb403 ("SMB3: fix lease break timeout when multiple deferred close handles for the same file.")
> ---
>  fs/cifs/cifsproto.h |  3 ++-
>  fs/cifs/file.c      | 21 +++++++++++++++++++++
>  fs/cifs/misc.c      |  4 ++--
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index e2eff66eefab..d2819d449f05 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -278,7 +278,8 @@ extern void cifs_add_deferred_close(struct cifsFileInfo *cfile,
>
>  extern void cifs_del_deferred_close(struct cifsFileInfo *cfile);
>
> -extern void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode);
> +extern void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode,
> +                                    bool wait_oplock_handler);
>
>  extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 4d4a2d82636d..ce75d8c1e3fe 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -4884,6 +4884,9 @@ void cifs_oplock_break(struct work_struct *work)
>         struct cifsInodeInfo *cinode = CIFS_I(inode);
>         struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>         struct TCP_Server_Info *server = tcon->ses->server;
> +       bool is_deferred = false;
> +       struct cifs_deferred_close *dclose;
> +
>         int rc = 0;
>         bool purge_cache = false;
>
> @@ -4921,6 +4924,23 @@ void cifs_oplock_break(struct work_struct *work)
>                 cifs_dbg(VFS, "Push locks rc = %d\n", rc);
>
>  oplock_break_ack:
> +       /*
> +        * When oplock break is received and there are no active file handles
> +        * but cached file handles, then schedule deferred close immediately.
> +        * So, new open will not use cached handle.
> +        */
> +       spin_lock(&CIFS_I(inode)->deferred_lock);
> +       is_deferred = cifs_is_deferred_close(cfile, &dclose);
> +       spin_unlock(&CIFS_I(inode)->deferred_lock);
> +       if (!CIFS_CACHE_HANDLE(cinode) && is_deferred &&
> +                       cfile->deferred_close_scheduled && delayed_work_pending(&cfile->deferred)) {
> +               if (cancel_delayed_work(&cfile->deferred)) {
> +                       _cifsFileInfo_put(cfile, false, false);
> +                       cifs_close_deferred_file(cinode, false);
> +                       goto oplock_break_done;
> +               }
> +       }
> +
>         /*
>          * releasing stale oplock after recent reconnect of smb session using
>          * a now incorrect file handle is not a data integrity issue but do
> @@ -4933,6 +4953,7 @@ void cifs_oplock_break(struct work_struct *work)
>                 cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>         }
>
> +oplock_break_done:
>         _cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
>         cifs_done_oplock_break(cinode);
>  }
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index a0d286ee723d..fd9d6b1ee1e2 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -728,7 +728,7 @@ cifs_del_deferred_close(struct cifsFileInfo *cfile)
>  }
>
>  void
> -cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
> +cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode, bool wait_oplock_handler)
>  {
>         struct cifsFileInfo *cfile = NULL;
>         struct file_list *tmp_list, *tmp_next_list;
> @@ -755,7 +755,7 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
>         spin_unlock(&cifs_inode->open_file_lock);
>
>         list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
> -               _cifsFileInfo_put(tmp_list->cfile, true, false);
> +               _cifsFileInfo_put(tmp_list->cfile, wait_oplock_handler, false);
>                 list_del(&tmp_list->list);
>                 kfree(tmp_list);
>         }
> --
> 2.25.1
>

paulo spotted some problems with it (missing spinlocks around calls to
cifs_del_deferred_close() e.g.)  which I have fixed up - updated
version attached.

There was one additional question in fs/cifs/file.c change.  Is this
patch now doing a double call to cifsFileInfo_put()  (see snippet
below)

@@ -4921,6 +4924,24 @@ void cifs_oplock_break(struct work_struct *work)
                cifs_dbg(VFS, "Push locks rc = %d\n", rc);

 oplock_break_ack:
+       /*
+        * When oplock break is received and there are no active file handles
+        * but cached file handles, then schedule deferred close immediately.
+        * So, new open will not use cached handle.
+        */
+       spin_lock(&CIFS_I(inode)->deferred_lock);
+       is_deferred = cifs_is_deferred_close(cfile, &dclose);
+       if (!CIFS_CACHE_HANDLE(cinode) && is_deferred &&
+                       cfile->deferred_close_scheduled &&
delayed_work_pending(&cfile->deferred)) {
+               spin_unlock(&CIFS_I(inode)->deferred_lock);
+               if (cancel_delayed_work(&cfile->deferred)) {
+                       _cifsFileInfo_put(cfile, false, false);
+                       cifs_close_deferred_file(cinode, false);
+                       goto oplock_break_done;
+               }
+       } else
+               spin_unlock(&CIFS_I(inode)->deferred_lock);
+
        /*
         * releasing stale oplock after recent reconnect of smb session using
         * a now incorrect file handle is not a data integrity issue but do
@@ -4933,6 +4954,7 @@ void cifs_oplock_break(struct work_struct *work)
                cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
        }

+oplock_break_done:
        _cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
        cifs_done_oplock_break(cinode);
 }

On Thu, Mar 23, 2023 at 2:48 PM Steve French <smfrench@gmail.com> wrote:
>
> This looks like a very important patch (I could repro some of the problems
> he mentioned in other threads) - tentatively merged into cifs-2.6.git for-next
>
> Am testing now - but let me know if anyone sees any problems with this.
>
> On Thu, Mar 23, 2023 at 2:07 PM <bharathsm.hsk@gmail.com> wrote:
> >
> > From: Bharath SM <bharathsm@microsoft.com>
> >
> > We should not cache deferred file handles if we dont have
> > handle lease on a file. And we should immediately close all
> > deferred handles in case of handle lease break.
> >
> > Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> > Fixes: 9e31678fb403 ("SMB3: fix lease break timeout when multiple deferred close handles for the same file.")
> > ---
> >  fs/cifs/cifsproto.h |  3 ++-
> >  fs/cifs/file.c      | 21 +++++++++++++++++++++
> >  fs/cifs/misc.c      |  4 ++--
> >  3 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index e2eff66eefab..d2819d449f05 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -278,7 +278,8 @@ extern void cifs_add_deferred_close(struct cifsFileInfo *cfile,
> >
> >  extern void cifs_del_deferred_close(struct cifsFileInfo *cfile);
> >
> > -extern void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode);
> > +extern void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode,
> > +                                    bool wait_oplock_handler);
> >
> >  extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 4d4a2d82636d..ce75d8c1e3fe 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -4884,6 +4884,9 @@ void cifs_oplock_break(struct work_struct *work)
> >         struct cifsInodeInfo *cinode = CIFS_I(inode);
> >         struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >         struct TCP_Server_Info *server = tcon->ses->server;
> > +       bool is_deferred = false;
> > +       struct cifs_deferred_close *dclose;
> > +
> >         int rc = 0;
> >         bool purge_cache = false;
> >
> > @@ -4921,6 +4924,23 @@ void cifs_oplock_break(struct work_struct *work)
> >                 cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> >
> >  oplock_break_ack:
> > +       /*
> > +        * When oplock break is received and there are no active file handles
> > +        * but cached file handles, then schedule deferred close immediately.
> > +        * So, new open will not use cached handle.
> > +        */
> > +       spin_lock(&CIFS_I(inode)->deferred_lock);
> > +       is_deferred = cifs_is_deferred_close(cfile, &dclose);
> > +       spin_unlock(&CIFS_I(inode)->deferred_lock);
> > +       if (!CIFS_CACHE_HANDLE(cinode) && is_deferred &&
> > +                       cfile->deferred_close_scheduled && delayed_work_pending(&cfile->deferred)) {
> > +               if (cancel_delayed_work(&cfile->deferred)) {
> > +                       _cifsFileInfo_put(cfile, false, false);
> > +                       cifs_close_deferred_file(cinode, false);
> > +                       goto oplock_break_done;
> > +               }
> > +       }
> > +
> >         /*
> >          * releasing stale oplock after recent reconnect of smb session using
> >          * a now incorrect file handle is not a data integrity issue but do
> > @@ -4933,6 +4953,7 @@ void cifs_oplock_break(struct work_struct *work)
> >                 cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >         }
> >
> > +oplock_break_done:
> >         _cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
> >         cifs_done_oplock_break(cinode);
> >  }
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index a0d286ee723d..fd9d6b1ee1e2 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -728,7 +728,7 @@ cifs_del_deferred_close(struct cifsFileInfo *cfile)
> >  }
> >
> >  void
> > -cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
> > +cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode, bool wait_oplock_handler)
> >  {
> >         struct cifsFileInfo *cfile = NULL;
> >         struct file_list *tmp_list, *tmp_next_list;
> > @@ -755,7 +755,7 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
> >         spin_unlock(&cifs_inode->open_file_lock);
> >
> >         list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
> > -               _cifsFileInfo_put(tmp_list->cfile, true, false);
> > +               _cifsFileInfo_put(tmp_list->cfile, wait_oplock_handler, false);
> >                 list_del(&tmp_list->list);
> >                 kfree(tmp_list);
> >         }
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
>
> Steve