linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thorsten Leemhuis <regressions@leemhuis.info>
To: Shyam Prasad N <nspmangalore@gmail.com>,
	ronnie sahlberg <ronniesahlberg@gmail.com>
Cc: Steve French <smfrench@gmail.com>,
	CIFS <linux-cifs@vger.kernel.org>,
	Satadru Pramanik <satadru@gmail.com>
Subject: Re: [PATCH][SMB3] fix multiuser mount regression
Date: Thu, 17 Mar 2022 17:57:25 +0100	[thread overview]
Message-ID: <5a951b61-41b7-91b5-b774-75314df190c8@leemhuis.info> (raw)
In-Reply-To: <CANT5p=oKgNJn7Dn0BDCbF28V+zKE9w8dgL0-Ra1fafKdjKHYaA@mail.gmail.com>

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@mail.gmail.com/

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@gmail.com> wrote:
>>
>> On Thu, Mar 17, 2022 at 1:20 PM Steve French <smfrench@gmail.com> 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@redhat.com>
>>> Signed-off-by: Steve French <stfrench@microsoft.com>
>>> ---
>>>  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
> 
> 
> 

  reply	other threads:[~2022-03-17 16:57 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-17  3:20 [PATCH][SMB3] fix multiuser mount regression Steve French
2022-03-17  3:44 ` ronnie sahlberg
2022-03-17 12:16   ` Satadru Pramanik
2022-03-17 15:47   ` Shyam Prasad N
2022-03-17 16:57     ` Thorsten Leemhuis [this message]
2022-03-17 17:00       ` Steve French
2022-03-17 17:29         ` Satadru Pramanik
2022-03-18 14:19           ` Shyam Prasad N

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a951b61-41b7-91b5-b774-75314df190c8@leemhuis.info \
    --to=regressions@leemhuis.info \
    --cc=linux-cifs@vger.kernel.org \
    --cc=nspmangalore@gmail.com \
    --cc=ronniesahlberg@gmail.com \
    --cc=satadru@gmail.com \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).