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 > > >
