Re: Multichannel patches

[Steve French <smfrench@gmail.com>]

test run started with those 9 patches (8 multichannel, and Aurelien's
small patch)

http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/11/builds/68

On Sat, Jun 5, 2021 at 12:43 PM Steve French <smfrench@gmail.com> wrote:
>
> Had to fix two things in those recently updated patches to get them to
> build.  You may want to fold those in as well to make the patches
> clearer to read and bisectable
>
> smfrench@smfrench-ThinkPad-P52:~/cifs-2.6/fs/cifs$ git diff -a
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 6e4934052159..f340b7d389ef 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -260,7 +260,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>
>                 /* If all channels need reconnect, then tcon needs reconnect */
>                 if (!CIFS_ALL_CHANS_NEED_RECONNECT(ses))
> -                       goto skip_tcon_reconnect;
> +                       continue;
>
>                 list_for_each(tmp2, &ses->tcon_list) {
>                         tcon = list_entry(tmp2, struct cifs_tcon, tcon_list);
> @@ -268,8 +268,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                 }
>                 if (ses->tcon_ipc)
>                         ses->tcon_ipc->need_reconnect = true;
> -
> -skip_tcon_reconnect:
>         }
>         spin_unlock(&cifs_tcp_ses_lock);
>         mutex_unlock(&ses->session_mutex);
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index cdbd256be4e4..202d98d06606 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -92,10 +92,10 @@ unsigned long cifs_ses_get_chan_index(struct cifs_ses *ses,
>  void cifs_chan_set_need_reconnect(struct cifs_ses *ses,
>                                    struct TCP_Server_Info *server)
>  {
> -       size_t chan_index = cifs_ses_get_chan_index(ses, server);
> +       unsigned long chan_index = cifs_ses_get_chan_index(ses, server);
>
>         set_bit(chan_index, &ses->chans_need_reconnect);
> -       cifs_dbg(FYI, "Set reconnect bitmask for chan %u; now 0x%lx\n",
> +       cifs_dbg(FYI, "Set reconnect bitmask for chan %lu; now 0x%lx\n",
>                  chan_index, ses->chans_need_reconnect);
>
> On Sat, Jun 5, 2021 at 9:08 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > The buildbot testing once hit a deadlock when running with the above patches.
> > I found one possibility during cifs_reconnect, where a deadlock can occur.
> >
> > I've fixed that and some warnings that kernel bots have identified in
> > the below two patches:
> > https://github.com/sprasad-microsoft/smb-kernel-client/pull/5/commits/f3e65f72b03b03bc4b301e8e04e9babb0e9582cf.patch
> > https://github.com/sprasad-microsoft/smb-kernel-client/pull/5/commits/7b3e867e994a7cbc88efe85c95167ae49d4b7a9d.patch
> >
> > Regards,
> > Shyam
> >
> > On Fri, Jun 4, 2021 at 8:55 PM Paulo Alcantara <pc@cjr.nz> wrote:
> > >
> > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > >
> > > > @Paulo Alcantara That would be great if you can help testing my
> > > > changes. Please test with these new changes.
> > >
> > > OK.
> > >
> > > >> The super is only used for providing cifs_sb_info::origin_fullpath as key to find the corresponding failover targets in referral cache.
> > > > I'm wondering what would happen if there are multiple tcons to the
> > > > same origin_fullpath (possibly in different sessions)?
> > >
> > > That is certainly a problem, indeed.  I'm waiting for the DFS tests to
> > > finish and then send a series that contains a potential fix for that --
> > > e.g. not sharing TCP servers when mounting DFS shares.  We used to not
> > > share tcons with DFS mounts because they might contain different prefix
> > > paths but connected to same share, however that wasn't enough because
> > > multiple DFS mounts may connect to same target servers, although they
> > > might failover to completely different servers.
> > >
> > > > Also, doesn't failover targets apply to each channel under a session?
> > > > Shouldn't we switch targets on reconnect of secondary channels too?
> > >
> > > That's a interesting question.  I recall discussing this with Aurelien
> > > some time ago while running a few DFS + multichannel tests.
> > >
> > > So yes, I agree with you that when we successfully reconnect to failover
> > > target (primary channel), then we should also update all secondary
> > > channels with the new server's ip address and reconnect them.
> >
> >
> >
> > --
> > Regards,
> > Shyam
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve