Thanks for the review Paulo, here is the updated patch that uses
cifs_dbg (FYI) instead of cifs_info.
Best
Meetakshi
On Fri, Nov 8, 2024 at 8:50 PM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
>
> Shyam Prasad N <nspmangalore@xxxxxxxxx> writes:
>
> > On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
> >>
> >> meetakshisetiyaoss@xxxxxxxxx writes:
> >>
> >> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
> >> > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> > {
> >> > int rc = 0;
> >> > + int retries = 0;
> >> > unsigned int xid;
> >> > struct cifs_ses *ses;
> >> > struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> >> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> > cifs_dbg(FYI, "Session needs reconnect\n");
> >> >
> >> > mutex_lock(&ses->session_mutex);
> >> > +
> >> > +retry_old_session:
> >> > rc = cifs_negotiate_protocol(xid, ses, server);
> >> > if (rc) {
> >> > mutex_unlock(&ses->session_mutex);
> >> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> > rc = cifs_setup_session(xid, ses, server,
> >> > ctx->local_nls);
> >> > if (rc) {
> >> > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> >> > + (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> >> > + retries++;
> >> > + cifs_info("Session reconnect failed, retrying with alternate password\n");
> >>
> >> Please don't add more noisy messages over reconnect. Remember that if
> >> SMB session doesn't get re-established, there will be flood enough on
> >> dmesg with "Send error in SessSetup = ..." messages on every 2s that
> >> already pisses off users and customers.
> >>
> > Perhaps we could do a cifs_dbg instead of cifs_info.
>
> Yep, with FYI.
>
> > But Paulo, the problem here is that we retry every 2s. I think we
> > should address that instead.
> > One way is to do an exponential backoff every time we retry.
>
> Agreed, but that doesn't mean we should add more noisy messages.
>
> > I'd also want to understand why we need the reconnect work?
>
> I see it as an optimisation to allow next IOs to not take longer because
> it needs to reconnect SMB session. If there was no prior filesystem
> activity, why don't allow the client itself to reconnect the session?
>
> Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so
> reconnect worker would be required to reconnect the session and then
> allow SMB2_IOCTL to work. We'd need to change that because with recent
> special file support, we need to avoid failures when creating or parsing
> reparse points because the SMB session isn't re-established yet.
>
> > Why not always do smb2_reconnect when someone does filesystem calls on
> > the mount point?
>
> We already do for most operations. SMB2_IOCTL and SMB2_TREE_CONNECT,
> for instance, can't call smb2_reconnect() as they would deadlock.
Attachment:
0002-cifs-support-mounting-with-alternate-password-to-all.patch
Description: Binary data
