| Message ID | CAH2r5mvqK0CkcR_DOwUfDVzsWco-SXb4rTt14b-YF5CpqCNrZQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMB3: force unmount was failing to close deferred close files | expand |
Ack by me. On Tue, May 9, 2023 at 12:05 PM Steve French <smfrench@gmail.com> wrote: > > In investigating a failure with xfstest generic/392 it > was noticed that mounts were reusing a superblock that should > already have been freed. This turned out to be related to > deferred close files keeping a reference count until the > closetimeo expired. > > Currently the only way an fs knows that mount is beginning is > when force unmount is called, but when this, ie umount_begin(), > is called all deferred close files on the share (tree > connection) should be closed immediately (unless shared by > another mount) to avoid using excess resources on the server > and to avoid reusing a superblock which should already be freed. > > In umount_begin, close all deferred close handles for that > share if this is the last mount using that share on this > client (ie send the SMB3 close request over the wire for those > that have been already closed by the app but that we have > kept a handle lease open for and have not sent closes to the > server for yet). > > Reported-by: David Howells <dhowells@redhat.com> > Cc: <stable@vger.kernel.org> > Fixes: 78c09634f7dc ("Cifs: Fix kernel oops caused by deferred close > for files.") > > See attached > -- > Thanks, > > Steve
On Tue, May 9, 2023 at 1:03 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > Ack by me. > > On Tue, May 9, 2023 at 12:05 PM Steve French <smfrench@gmail.com> wrote: > > > > In investigating a failure with xfstest generic/392 it > > was noticed that mounts were reusing a superblock that should > > already have been freed. This turned out to be related to > > deferred close files keeping a reference count until the > > closetimeo expired. > > > > Currently the only way an fs knows that mount is beginning is > > when force unmount is called, but when this, ie umount_begin(), > > is called all deferred close files on the share (tree > > connection) should be closed immediately (unless shared by > > another mount) to avoid using excess resources on the server > > and to avoid reusing a superblock which should already be freed. > > > > In umount_begin, close all deferred close handles for that > > share if this is the last mount using that share on this > > client (ie send the SMB3 close request over the wire for those > > that have been already closed by the app but that we have > > kept a handle lease open for and have not sent closes to the > > server for yet). > > > > Reported-by: David Howells <dhowells@redhat.com> > > Cc: <stable@vger.kernel.org> > > Fixes: 78c09634f7dc ("Cifs: Fix kernel oops caused by deferred close > > for files.") > > > > See attached > > -- > > Thanks, > > > > Steve cifs_put_tcon sounds like a more logical place to do this. When the ref count has dropped to 0.
On Tue, May 16, 2023 at 1:51 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Tue, May 9, 2023 at 1:03 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > > > Ack by me. > > > > On Tue, May 9, 2023 at 12:05 PM Steve French <smfrench@gmail.com> wrote: > > > > > > In investigating a failure with xfstest generic/392 it > > > was noticed that mounts were reusing a superblock that should > > > already have been freed. This turned out to be related to > > > deferred close files keeping a reference count until the > > > closetimeo expired. > > > > > > Currently the only way an fs knows that mount is beginning is > > > when force unmount is called, but when this, ie umount_begin(), > > > is called all deferred close files on the share (tree > > > connection) should be closed immediately (unless shared by > > > another mount) to avoid using excess resources on the server > > > and to avoid reusing a superblock which should already be freed. > > > > > > In umount_begin, close all deferred close handles for that > > > share if this is the last mount using that share on this > > > client (ie send the SMB3 close request over the wire for those > > > that have been already closed by the app but that we have > > > kept a handle lease open for and have not sent closes to the > > > server for yet). > > > > > > Reported-by: David Howells <dhowells@redhat.com> > > > Cc: <stable@vger.kernel.org> > > > Fixes: 78c09634f7dc ("Cifs: Fix kernel oops caused by deferred close > > > for files.") > cifs_put_tcon sounds like a more logical place to do this. When the > ref count has dropped to 0. > > -- > Regards, > Shyam The problem with waiting till cifs_put_tcon (although it wouldn't hurt to also call close_all_deferred_files() there or in cifs_kill_sb) is that that is too late. We don't get notified when umount begins except on force umount ("umount_begin") so a simple scenario like: echo "data in new file" >> /mnt2/newfile ; umount /mnt2 will wait until the deferred close timeout ("closetimeo") before closing these files and then sending the tree disconnect, so in that example tree discconnect was unnecessarily delayed (although "umount" completes immediately, we can't send the tree disconnect until all the closetimeouts expire since we will not receive the "kill_sb()" call until the deferred close references are released). Closing deferred files in various scenarios ("umount -f" and calls to "shutdown") can help us send the tree disconnect faster and not tie up resources on the server longer for files we won't be using
From 6c840c704d9dc3f02970c46ebebb359e423c0548 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 9 May 2023 01:00:42 -0500
Subject: [PATCH] SMB3: force unmount was failing to close deferred close files
In investigating a failure with xfstest generic/392 it
was noticed that mounts were reusing a superblock that should
already have been freed. This turned out to be related to
deferred close files keeping a reference count until the
closetimeo expired.
Currently the only way an fs knows that mount is beginning is
when force unmount is called, but when this, ie umount_begin(),
is called all deferred close files on the share (tree
connection) should be closed immediately (unless shared by
another mount) to avoid using excess resources on the server
and to avoid reusing a superblock which should already be freed.
In umount_begin, close all deferred close handles for that
share if this is the last mount using that share on this
client (ie send the SMB3 close request over the wire for those
that have been already closed by the app but that we have
kept a handle lease open for and have not sent closes to the
server for yet).
Reported-by: David Howells <dhowells@redhat.com>
Cc: <stable@vger.kernel.org>
Fixes: 78c09634f7dc ("Cifs: Fix kernel oops caused by deferred close for files.")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/cifsfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 81430abacf93..8b6b3b6985f3 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -744,6 +744,7 @@ static void cifs_umount_begin(struct super_block *sb)
spin_unlock(&tcon->tc_lock);
spin_unlock(&cifs_tcp_ses_lock);
+ cifs_close_all_deferred_files(tcon);
/* cancel_brl_requests(tcon); */ /* BB mark all brl mids as exiting */
/* cancel_notify_requests(tcon); */
if (tcon->ses && tcon->ses->server) {
--
2.34.1
In investigating a failure with xfstest generic/392 it was noticed that mounts were reusing a superblock that should already have been freed. This turned out to be related to deferred close files keeping a reference count until the closetimeo expired. Currently the only way an fs knows that mount is beginning is when force unmount is called, but when this, ie umount_begin(), is called all deferred close files on the share (tree connection) should be closed immediately (unless shared by another mount) to avoid using excess resources on the server and to avoid reusing a superblock which should already be freed. In umount_begin, close all deferred close handles for that share if this is the last mount using that share on this client (ie send the SMB3 close request over the wire for those that have been already closed by the app but that we have kept a handle lease open for and have not sent closes to the server for yet). Reported-by: David Howells <dhowells@redhat.com> Cc: <stable@vger.kernel.org> Fixes: 78c09634f7dc ("Cifs: Fix kernel oops caused by deferred close for files.") See attached