As discussed in the other email thread that Satadru started, I feel that these two (multiuser regression and the sleep/resume issue) are different issues. Let's continue the discussion on that one in the dedicated email thread. As for the multiuser regression, here's a slightly updated version of my last patch here. https://github.com/sprasad-microsoft/smb3-kernel-client/commit/9bc6cde4609c98cf53a5f3e0462c68708dbb54b7.patch Steve, I leave it upto you whether you want to take for 5.17 or 5.18-rc1. For 5.18-rc1, I'll have another patch on top of this one that should fix parallel reconnects for multichannel for good. (I'm planning to split up session status to individual channels there). Regards, Shyam On Thu, Mar 17, 2022 at 10:59 PM Satadru Pramanik <satadru@xxxxxxxxx> wrote: > > Neither the one line patch nor the longer patch solve the issue. > > Dmesg is attached from booting up with a kernel built with the longer > patch. It covers sleeping, rebooting server, noticing cifs mount is > broken, unmounting and remounting the cifs mount, putting the laptop > to sleep again, rebooting the server, waking the laptop, and noticing > the mount is broken again. > > > On Thu, Mar 17, 2022 at 1:01 PM Steve French <smfrench@xxxxxxxxx> wrote: > > > > > And if it does, out of curiosity: Is the patch considered to be too > > invasive for 5.17 > > > > Good question - I am testing it right now and weighing the option of > > the one line patch which fixes at least one of the problems, vs. the > > longer patch (about 50 lines of code IIRC) of Shyam's that I am > > testing now. We will defer the larger changes Ronnie, Shyam and I > > have discussed to make the state machine more readable/understandable > > (and less likely to run into this in the future by changing the state > > names in the enum) for the three structures (server socket we are > > connected to, authenticated user session, tree connect for the share > > we have mounted) > > > > On Thu, Mar 17, 2022 at 11:57 AM Thorsten Leemhuis > > <regressions@xxxxxxxxxxxxx> wrote: > > > > > > On 17.03.22 16:47, Shyam Prasad N wrote: > > > > I looked at this problem in more detail. > > > > > > Thx. Is that patch something Satadru should test to see if it fixes his > > > regression? > > > https://lore.kernel.org/linux-cifs/CAFrh3J9soC36%2BBVuwHB=g9z_KB5Og2%2Bp2_W%2BBBoBOZveErz14w@xxxxxxxxxxxxxx/ > > > > > > And if it does, out of curiosity: Is the patch considered to be too > > > invasive for 5.17 as the final is just a few days away? > > > > > > Ciao, Thorsten > > > > > > > Here's a summary of what happened: > > > > Before my recent changes, when mchan was used, and a > > > > negotiate/sess-setup happened, all other channels were paused. So > > > > things were a lot simpler during a connect/reconnect. > > > > What I did with my recent changes is to allow I/O to flow in other > > > > channels when connect/reconnect happened on one of the channels. This > > > > meant that there could be multiple channels that do negotiate/session > > > > setup for the same session in parallel. i.e. first channel would > > > > create a new session. Other channels would bind to the same session. > > > > This meant that the server->tcpStatus needed to indicate an ongoing > > > > sess setup. So that multiple channels could do session setup in > > > > parallel. > > > > Unfortunately, I did not account for multiuser scenario, which does > > > > the reverse. i.e. uses the same server for different tcp sessions. > > > > > > > > Here's a patch I propose to fix this issue. Please review and let me > > > > know what you think. > > > > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/34333e9de1526c46e9ae5ff9a54f0199b827fd0e.patch > > > > > > > > Essentially, I'm doing 3 changes in this patch: > > > > 1. Making sure that tcpStatus is only till the end of tcp connection > > > > establishment. This means that tcpStatus reaches CifsGood when the tcp > > > > connection is good > > > > 2. Once cifs_negotiate_protocol starts, anything done will affect the > > > > session state, and should not modify tcpStatus. > > > > 3. To detect the small window between tcp connection setup and before > > > > negotiate, use need_neg() > > > > > > > > On Thu, Mar 17, 2022 at 9:14 AM ronnie sahlberg > > > > <ronniesahlberg@xxxxxxxxx> wrote: > > > >> > > > >> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@xxxxxxxxx> wrote: > > > >>> > > > >>> cifssmb3: fix incorrect session setup check for multiuser mounts > > > >> > > > >> If it fixes multiuser then Acked-by me. > > > >> We are so close to rc8 that we can not do intrusive changes, so if > > > >> it fixes it short term. > > > >> For longer term, post rc8 we need to rewrite the statemaching completely > > > >> and separate out "what happens in server->tcpStatus" as one statemachine and > > > >> "what happens in server->status" as a separate one. Right now it is a mess. > > > >> > > > >> > > > >>> > > > >>> A recent change to how the SMB3 server (socket) and session status > > > >>> is managed regressed multiuser mounts by changing the check > > > >>> for whether session setup is needed to the socket (TCP_Server_info) > > > >>> structure instead of the session struct (cifs_ses). Add additional > > > >>> check in cifs_setup_sesion to fix this. > > > >>> > > > >>> Fixes: 73f9bfbe3d81 ("cifs: maintain a state machine for tcp/smb/tcon sessions") > > > >>> Reported-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> > > > >>> Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx> > > > >>> --- > > > >>> fs/cifs/connect.c | 3 ++- > > > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > > >>> index 053cb449eb16..d3020abfe404 100644 > > > >>> --- a/fs/cifs/connect.c > > > >>> +++ b/fs/cifs/connect.c > > > >>> @@ -3924,7 +3924,8 @@ cifs_setup_session(const unsigned int xid, > > > >>> struct cifs_ses *ses, > > > >>> > > > >>> /* only send once per connect */ > > > >>> spin_lock(&cifs_tcp_ses_lock); > > > >>> - if (server->tcpStatus != CifsNeedSessSetup) { > > > >>> + if ((server->tcpStatus != CifsNeedSessSetup) && > > > >>> + (ses->status == CifsGood)) { > > > >>> spin_unlock(&cifs_tcp_ses_lock); > > > >>> return 0; > > > >>> } > > > >>> > > > >>> -- > > > >>> Thanks, > > > >>> > > > >>> Steve > > > > > > > > > > > > > > > > > > > > -- > > Thanks, > > > > Steve -- Regards, Shyam
