[PATCH 1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

[PATCH 1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

cifs_chan_is_iface_active checks the channels of a session to see
if the associated iface is active. This should always happen
with chan_lock held. However, these two callers of this function
were missing this locking.

This change makes sure the function calls are protected with
proper locking.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/connect.c | 7 +++++--
 fs/smb/client/smb2ops.c | 7 ++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 8b7cffba1ad5..3052a208c6ca 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
 		/* check if iface is still active */
-		if (!cifs_chan_is_iface_active(ses, server))
+		spin_lock(&ses->chan_lock);
+		if (!cifs_chan_is_iface_active(ses, server)) {
+			spin_unlock(&ses->chan_lock);
 			cifs_chan_update_iface(ses, server);
+			spin_lock(&ses->chan_lock);
+		}
 
-		spin_lock(&ses->chan_lock);
 		if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
 			spin_unlock(&ses->chan_lock);
 			continue;
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 441d144bd712..104c58df0368 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
 		goto out;
 
 	/* check if iface is still active */
+	spin_lock(&ses->chan_lock);
 	pserver = ses->chans[0].server;
-	if (pserver && !cifs_chan_is_iface_active(ses, pserver))
+	if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
+		spin_unlock(&ses->chan_lock);
 		cifs_chan_update_iface(ses, pserver);
+		spin_lock(&ses->chan_lock);
+	}
+	spin_unlock(&ses->chan_lock);
 
 out:
 	kfree(out_buf);
-- 
2.34.1

[PATCH 2/4] cifs: do not depend on release_iface for maintaining iface_list

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

parse_server_interfaces should be in complete charge of maintaining
the iface_list linked list. Today, iface entries are removed
from the list only when the last refcount is dropped.
i.e. in release_iface. However, this can result in undercounting
of refcount if the server stops advertising interfaces (which
Azure SMB server does).

This change puts parse_server_interfaces in full charge of
maintaining the iface_list. So if an empty list is returned
by the server, the entries in the list will immediately be
removed. This way, a following call to the same function will
not find entries in the list.

Fixes: aa45dadd34e4 ("cifs: change iface_list from array to sorted linked list")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cifsglob.h |  1 -
 fs/smb/client/smb2ops.c  | 27 +++++++++++++++++----------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index ba80c854c9ca..f840756e0169 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1014,7 +1014,6 @@ release_iface(struct kref *ref)
 	struct cifs_server_iface *iface = container_of(ref,
 						       struct cifs_server_iface,
 						       refcount);
-	list_del_init(&iface->iface_head);
 	kfree(iface);
 }
 
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 104c58df0368..b813485c0e86 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -595,16 +595,12 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 	}
 
 	/*
-	 * Go through iface_list and do kref_put to remove
-	 * any unused ifaces. ifaces in use will be removed
-	 * when the last user calls a kref_put on it
+	 * Go through iface_list and mark them as inactive
 	 */
 	list_for_each_entry_safe(iface, niface, &ses->iface_list,
-				 iface_head) {
+				 iface_head)
 		iface->is_active = 0;
-		kref_put(&iface->refcount, release_iface);
-		ses->iface_count--;
-	}
+
 	spin_unlock(&ses->iface_lock);
 
 	/*
@@ -678,10 +674,7 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 					 iface_head) {
 			ret = iface_cmp(iface, &tmp_iface);
 			if (!ret) {
-				/* just get a ref so that it doesn't get picked/freed */
 				iface->is_active = 1;
-				kref_get(&iface->refcount);
-				ses->iface_count++;
 				spin_unlock(&ses->iface_lock);
 				goto next_iface;
 			} else if (ret < 0) {
@@ -748,6 +741,20 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 	}
 
 out:
+	/*
+	 * Go through the list again and put the inactive entries
+	 */
+	spin_lock(&ses->iface_lock);
+	list_for_each_entry_safe(iface, niface, &ses->iface_list,
+				 iface_head) {
+		if (!iface->is_active) {
+			list_del(&iface->iface_head);
+			kref_put(&iface->refcount, release_iface);
+			ses->iface_count--;
+		}
+	}
+	spin_unlock(&ses->iface_lock);
+
 	return rc;
 }
 
-- 
2.34.1

[PATCH 3/4] cifs: cifs_pick_channel should skip unhealthy channels

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

cifs_pick_channel does not take into account the current
state of the channel today. As a result, if some channels
are unhealthy, they could still get picked, resulting
in unnecessary errors.

This change checks the channel transport status before
making the choice. If all channels are unhealthy, the
primary channel will be returned anyway.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/transport.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 4f717ad7c21b..f8e6636e90a3 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses)
 		if (!server || server->terminate)
 			continue;
 
+		/*
+		 * do not pick a channel that's not healthy.
+		 * if all channels are unhealthy, we'll use
+		 * the primary channel
+		 */
+		spin_lock(&server->srv_lock);
+		if (server->tcpStatus != CifsNew &&
+		    server->tcpStatus != CifsGood) {
+			spin_unlock(&server->srv_lock);
+			continue;
+		}
+		spin_unlock(&server->srv_lock);
+
 		/*
 		 * strictly speaking, we should pick up req_lock to read
 		 * server->in_flight. But it shouldn't matter much here if we
-- 
2.34.1

[PATCH 4/4] cifs: fix in logging in cifs_chan_update_iface

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

Recently, cifs_chan_update_iface was modified to not
remove an iface if a suitable replacement was not found.
With that, there were two conditionals that were exactly
the same. This change removes that extra condition check.

Also, fixed a logging in the same function to indicate
the correct message.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/sess.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index 2d3b332a79a1..a16e175731eb 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -440,8 +440,14 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 	}
 
 	if (!iface) {
-		cifs_dbg(FYI, "unable to get the interface matching: %pIS\n",
-			 &ss);
+		if (!chan_index)
+			cifs_dbg(FYI, "unable to get the interface matching: %pIS\n",
+				 &ss);
+		else {
+			cifs_dbg(FYI, "unable to find another interface to replace: %pIS\n",
+				 &old_iface->sockaddr);
+		}
+
 		spin_unlock(&ses->iface_lock);
 		return 0;
 	}
@@ -459,10 +465,6 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 		iface->weight_fulfilled++;
 
 		kref_put(&old_iface->refcount, release_iface);
-	} else if (old_iface) {
-		/* if a new candidate is not found, keep things as is */
-		cifs_dbg(FYI, "could not replace iface: %pIS\n",
-			 &old_iface->sockaddr);
 	} else if (!chan_index) {
 		/* special case: update interface for primary channel */
 		if (iface) {
-- 
2.34.1

Re: [PATCH 2/4] cifs: do not depend on release_iface for maintaining iface_list

[Shyam Prasad N]

On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> parse_server_interfaces should be in complete charge of maintaining
> the iface_list linked list. Today, iface entries are removed
> from the list only when the last refcount is dropped.
> i.e. in release_iface. However, this can result in undercounting
> of refcount if the server stops advertising interfaces (which
> Azure SMB server does).
>
> This change puts parse_server_interfaces in full charge of
> maintaining the iface_list. So if an empty list is returned
> by the server, the entries in the list will immediately be
> removed. This way, a following call to the same function will
> not find entries in the list.
>
> Fixes: aa45dadd34e4 ("cifs: change iface_list from array to sorted linked list")
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cifsglob.h |  1 -
>  fs/smb/client/smb2ops.c  | 27 +++++++++++++++++----------
>  2 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index ba80c854c9ca..f840756e0169 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1014,7 +1014,6 @@ release_iface(struct kref *ref)
>         struct cifs_server_iface *iface = container_of(ref,
>                                                        struct cifs_server_iface,
>                                                        refcount);
> -       list_del_init(&iface->iface_head);
>         kfree(iface);
>  }
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 104c58df0368..b813485c0e86 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -595,16 +595,12 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
>         }
>
>         /*
> -        * Go through iface_list and do kref_put to remove
> -        * any unused ifaces. ifaces in use will be removed
> -        * when the last user calls a kref_put on it
> +        * Go through iface_list and mark them as inactive
>          */
>         list_for_each_entry_safe(iface, niface, &ses->iface_list,
> -                                iface_head) {
> +                                iface_head)
>                 iface->is_active = 0;
> -               kref_put(&iface->refcount, release_iface);
> -               ses->iface_count--;
> -       }
> +
>         spin_unlock(&ses->iface_lock);
>
>         /*
> @@ -678,10 +674,7 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
>                                          iface_head) {
>                         ret = iface_cmp(iface, &tmp_iface);
>                         if (!ret) {
> -                               /* just get a ref so that it doesn't get picked/freed */
>                                 iface->is_active = 1;
> -                               kref_get(&iface->refcount);
> -                               ses->iface_count++;
>                                 spin_unlock(&ses->iface_lock);
>                                 goto next_iface;
>                         } else if (ret < 0) {
> @@ -748,6 +741,20 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
>         }
>
>  out:
> +       /*
> +        * Go through the list again and put the inactive entries
> +        */
> +       spin_lock(&ses->iface_lock);
> +       list_for_each_entry_safe(iface, niface, &ses->iface_list,
> +                                iface_head) {
> +               if (!iface->is_active) {
> +                       list_del(&iface->iface_head);
> +                       kref_put(&iface->refcount, release_iface);
> +                       ses->iface_count--;
> +               }
> +       }
> +       spin_unlock(&ses->iface_lock);
> +
>         return rc;
>  }
>
> --
> 2.34.1
>
Hi Steve..
This one should also be marked for stable. I missed tagging it before I sent.

-- 
Regards,
Shyam

Re: [PATCH 1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

[Shyam Prasad N]

On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> cifs_chan_is_iface_active checks the channels of a session to see
> if the associated iface is active. This should always happen
> with chan_lock held. However, these two callers of this function
> were missing this locking.
>
> This change makes sure the function calls are protected with
> proper locking.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/connect.c | 7 +++++--
>  fs/smb/client/smb2ops.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 8b7cffba1ad5..3052a208c6ca 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
>         spin_lock(&cifs_tcp_ses_lock);
>         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
>                 /* check if iface is still active */
> -               if (!cifs_chan_is_iface_active(ses, server))
> +               spin_lock(&ses->chan_lock);
> +               if (!cifs_chan_is_iface_active(ses, server)) {
> +                       spin_unlock(&ses->chan_lock);
>                         cifs_chan_update_iface(ses, server);
> +                       spin_lock(&ses->chan_lock);
> +               }
>
> -               spin_lock(&ses->chan_lock);
>                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
>                         spin_unlock(&ses->chan_lock);
>                         continue;
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 441d144bd712..104c58df0368 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
>                 goto out;
>
>         /* check if iface is still active */
> +       spin_lock(&ses->chan_lock);
>         pserver = ses->chans[0].server;
> -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> +               spin_unlock(&ses->chan_lock);
>                 cifs_chan_update_iface(ses, pserver);
> +               spin_lock(&ses->chan_lock);
> +       }
> +       spin_unlock(&ses->chan_lock);
>
>  out:
>         kfree(out_buf);
> --
> 2.34.1
>

This one fixes two changes. Not sure if it's valid to have two Fixes tag.
Fixes: b54034a73baf ("cifs: during reconnect, update interface if necessary")
Fixes: fa1d0508bdd4 ("cifs: account for primary channel in the interface list")

Should also CC stable.

-- 
Regards,
Shyam

Re: [PATCH 1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

updated patch attached (to fix minor merge conflict)

On Fri, Dec 29, 2023 at 5:25 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > cifs_chan_is_iface_active checks the channels of a session to see
> > if the associated iface is active. This should always happen
> > with chan_lock held. However, these two callers of this function
> > were missing this locking.
> >
> > This change makes sure the function calls are protected with
> > proper locking.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/connect.c | 7 +++++--
> >  fs/smb/client/smb2ops.c | 7 ++++++-
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 8b7cffba1ad5..3052a208c6ca 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
> >         spin_lock(&cifs_tcp_ses_lock);
> >         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
> >                 /* check if iface is still active */
> > -               if (!cifs_chan_is_iface_active(ses, server))
> > +               spin_lock(&ses->chan_lock);
> > +               if (!cifs_chan_is_iface_active(ses, server)) {
> > +                       spin_unlock(&ses->chan_lock);
> >                         cifs_chan_update_iface(ses, server);
> > +                       spin_lock(&ses->chan_lock);
> > +               }
> >
> > -               spin_lock(&ses->chan_lock);
> >                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
> >                         spin_unlock(&ses->chan_lock);
> >                         continue;
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 441d144bd712..104c58df0368 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
> >                 goto out;
> >
> >         /* check if iface is still active */
> > +       spin_lock(&ses->chan_lock);
> >         pserver = ses->chans[0].server;
> > -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> > +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> > +               spin_unlock(&ses->chan_lock);
> >                 cifs_chan_update_iface(ses, pserver);
> > +               spin_lock(&ses->chan_lock);
> > +       }
> > +       spin_unlock(&ses->chan_lock);
> >
> >  out:
> >         kfree(out_buf);
> > --
> > 2.34.1
> >
>
> This one fixes two changes. Not sure if it's valid to have two Fixes tag.

Yes - that is ok (two Fixes tags)
-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-cifs_chan_is_iface_active-should-be-called-with.patch --]
[-- Type: text/x-patch, Size: 2344 bytes --]

From 9ddf79e297171e34662d9bc79224035a594a0f13 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Fri, 29 Dec 2023 11:16:15 +0000
Subject: [PATCH] cifs: cifs_chan_is_iface_active should be called with
 chan_lock held

cifs_chan_is_iface_active checks the channels of a session to see
if the associated iface is active. This should always happen
with chan_lock held. However, these two callers of this function
were missing this locking.

This change makes sure the function calls are protected with
proper locking.

Fixes: b54034a73baf ("cifs: during reconnect, update interface if necessary")
Fixes: fa1d0508bdd4 ("cifs: account for primary channel in the interface list")
Cc: stable@vger.kernel.org
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/connect.c | 7 +++++--
 fs/smb/client/smb2ops.c | 7 ++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index dd2a1fb65e71..08c04a78303d 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -228,10 +228,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
 			break;
 
 		/* check if iface is still active */
-		if (!cifs_chan_is_iface_active(ses, server))
+		spin_lock(&ses->chan_lock);
+		if (!cifs_chan_is_iface_active(ses, server)) {
+			spin_unlock(&ses->chan_lock);
 			cifs_chan_update_iface(ses, server);
+			spin_lock(&ses->chan_lock);
+		}
 
-		spin_lock(&ses->chan_lock);
 		if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
 			spin_unlock(&ses->chan_lock);
 			continue;
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 9a9d9dec7081..14bc745de199 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -791,9 +791,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
 		goto out;
 
 	/* check if iface is still active */
+	spin_lock(&ses->chan_lock);
 	pserver = ses->chans[0].server;
-	if (pserver && !cifs_chan_is_iface_active(ses, pserver))
+	if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
+		spin_unlock(&ses->chan_lock);
 		cifs_chan_update_iface(ses, pserver);
+		spin_lock(&ses->chan_lock);
+	}
+	spin_unlock(&ses->chan_lock);
 
 out:
 	kfree(out_buf);
-- 
2.40.1

Re: [PATCH 1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

[Steve French]

ignore my rebased patch. I applied his series in wrong order

On Fri, Dec 29, 2023 at 8:57 AM Steve French <smfrench@gmail.com> wrote:
>
> updated patch attached (to fix minor merge conflict)
>
> On Fri, Dec 29, 2023 at 5:25 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
> > >
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > cifs_chan_is_iface_active checks the channels of a session to see
> > > if the associated iface is active. This should always happen
> > > with chan_lock held. However, these two callers of this function
> > > were missing this locking.
> > >
> > > This change makes sure the function calls are protected with
> > > proper locking.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > >  fs/smb/client/connect.c | 7 +++++--
> > >  fs/smb/client/smb2ops.c | 7 ++++++-
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > > index 8b7cffba1ad5..3052a208c6ca 100644
> > > --- a/fs/smb/client/connect.c
> > > +++ b/fs/smb/client/connect.c
> > > @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
> > >         spin_lock(&cifs_tcp_ses_lock);
> > >         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
> > >                 /* check if iface is still active */
> > > -               if (!cifs_chan_is_iface_active(ses, server))
> > > +               spin_lock(&ses->chan_lock);
> > > +               if (!cifs_chan_is_iface_active(ses, server)) {
> > > +                       spin_unlock(&ses->chan_lock);
> > >                         cifs_chan_update_iface(ses, server);
> > > +                       spin_lock(&ses->chan_lock);
> > > +               }
> > >
> > > -               spin_lock(&ses->chan_lock);
> > >                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
> > >                         spin_unlock(&ses->chan_lock);
> > >                         continue;
> > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > > index 441d144bd712..104c58df0368 100644
> > > --- a/fs/smb/client/smb2ops.c
> > > +++ b/fs/smb/client/smb2ops.c
> > > @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
> > >                 goto out;
> > >
> > >         /* check if iface is still active */
> > > +       spin_lock(&ses->chan_lock);
> > >         pserver = ses->chans[0].server;
> > > -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> > > +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> > > +               spin_unlock(&ses->chan_lock);
> > >                 cifs_chan_update_iface(ses, pserver);
> > > +               spin_lock(&ses->chan_lock);
> > > +       }
> > > +       spin_unlock(&ses->chan_lock);
> > >
> > >  out:
> > >         kfree(out_buf);
> > > --
> > > 2.34.1
> > >
> >
> > This one fixes two changes. Not sure if it's valid to have two Fixes tag.
>
> Yes - that is ok (two Fixes tags)
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: [PATCH 3/4] cifs: cifs_pick_channel should skip unhealthy channels

[Shyam Prasad N]

On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> cifs_pick_channel does not take into account the current
> state of the channel today. As a result, if some channels
> are unhealthy, they could still get picked, resulting
> in unnecessary errors.
>
> This change checks the channel transport status before
> making the choice. If all channels are unhealthy, the
> primary channel will be returned anyway.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/transport.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index 4f717ad7c21b..f8e6636e90a3 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c
> @@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses)
>                 if (!server || server->terminate)
>                         continue;
>
> +               /*
> +                * do not pick a channel that's not healthy.
> +                * if all channels are unhealthy, we'll use
> +                * the primary channel
> +                */
> +               spin_lock(&server->srv_lock);
> +               if (server->tcpStatus != CifsNew &&
> +                   server->tcpStatus != CifsGood) {
> +                       spin_unlock(&server->srv_lock);
> +                       continue;
> +               }
> +               spin_unlock(&server->srv_lock);
> +
>                 /*
>                  * strictly speaking, we should pick up req_lock to read
>                  * server->in_flight. But it shouldn't matter much here if we
> --
> 2.34.1
>
Please skip this patch. I'll submit a revised patch next week.

-- 
Regards,
Shyam

Re: [PATCH 3/4] cifs: cifs_pick_channel should skip unhealthy channels

[Steve French]

for-next updated to remove that patch

On Fri, Dec 29, 2023 at 11:02 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > cifs_pick_channel does not take into account the current
> > state of the channel today. As a result, if some channels
> > are unhealthy, they could still get picked, resulting
> > in unnecessary errors.
> >
> > This change checks the channel transport status before
> > making the choice. If all channels are unhealthy, the
> > primary channel will be returned anyway.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/transport.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> > index 4f717ad7c21b..f8e6636e90a3 100644
> > --- a/fs/smb/client/transport.c
> > +++ b/fs/smb/client/transport.c
> > @@ -1026,6 +1026,19 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses)
> >                 if (!server || server->terminate)
> >                         continue;
> >
> > +               /*
> > +                * do not pick a channel that's not healthy.
> > +                * if all channels are unhealthy, we'll use
> > +                * the primary channel
> > +                */
> > +               spin_lock(&server->srv_lock);
> > +               if (server->tcpStatus != CifsNew &&
> > +                   server->tcpStatus != CifsGood) {
> > +                       spin_unlock(&server->srv_lock);
> > +                       continue;
> > +               }
> > +               spin_unlock(&server->srv_lock);
> > +
> >                 /*
> >                  * strictly speaking, we should pick up req_lock to read
> >                  * server->in_flight. But it shouldn't matter much here if we
> > --
> > 2.34.1
> >
> Please skip this patch. I'll submit a revised patch next week.
>
> --
> Regards,
> Shyam



-- 
Thanks,

Steve