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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> >
> > From: Bharath SM <bharathsm@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > 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
--
Thanks,
Steve
From 78230defdbb78024785d5d976a2dc14152cda6ab Mon Sep 17 00:00:00 2001
From: Bharath SM <bharathsm@xxxxxxxxxxxxx>
Date: Thu, 23 Mar 2023 19:05:00 +0000
Subject: [PATCH] SMB3: Close deferred file handles if we get handle lease break
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. This is
very important e.g. to prevent access denied errors
in writing to the same file from a different client (during
this interval while the close is deferred).
Signed-off-by: Bharath SM <bharathsm@xxxxxxxxxxxxx>
Fixes: 9e31678fb403 ("SMB3: fix lease break timeout when multiple deferred close handles for the same file.")
Cc: stable@xxxxxxxxxxxxxxx # 6.0+
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/cifs/cifsproto.h | 3 ++-
fs/cifs/file.c | 22 ++++++++++++++++++++++
fs/cifs/misc.c | 19 ++++++++++++++-----
3 files changed, 38 insertions(+), 6 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 6831a9949c43..8e420fd97faf 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,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);
}
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index b44fb51968bf..2eab9d5c74fc 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -735,7 +735,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;
@@ -747,22 +747,26 @@ cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
INIT_LIST_HEAD(&file_head);
spin_lock(&cifs_inode->open_file_lock);
list_for_each_entry(cfile, &cifs_inode->openFileList, flist) {
+ spin_lock(&cifs_inode->deferred_lock);
if (delayed_work_pending(&cfile->deferred)) {
if (cancel_delayed_work(&cfile->deferred)) {
cifs_del_deferred_close(cfile);
tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
- if (tmp_list == NULL)
+ if (tmp_list == NULL) {
+ spin_unlock(&cifs_inode->deferred_lock);
break;
+ }
tmp_list->cfile = cfile;
list_add_tail(&tmp_list->list, &file_head);
}
}
+ spin_unlock(&cifs_inode->deferred_lock);
}
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);
}
@@ -780,8 +784,9 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
list_for_each_entry(cfile, &tcon->openFileList, tlist) {
if (delayed_work_pending(&cfile->deferred)) {
if (cancel_delayed_work(&cfile->deferred)) {
+ spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
cifs_del_deferred_close(cfile);
-
+ spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
if (tmp_list == NULL)
break;
@@ -813,17 +818,21 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
list_for_each_entry(cfile, &tcon->openFileList, tlist) {
full_path = build_path_from_dentry(cfile->dentry, page);
if (strstr(full_path, path)) {
+ spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
if (delayed_work_pending(&cfile->deferred)) {
if (cancel_delayed_work(&cfile->deferred)) {
cifs_del_deferred_close(cfile);
tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
- if (tmp_list == NULL)
+ if (tmp_list == NULL) {
+ spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
break;
+ }
tmp_list->cfile = cfile;
list_add_tail(&tmp_list->list, &file_head);
}
}
+ spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
}
}
spin_unlock(&tcon->open_file_lock);
--
2.34.1
