Here is the set of important multichannel fixes that Shyam recently
sent out, ordered the way he requested (and a few updated with missing
tags as requested). They are applied to cifs-2.6.git for-next now
--
Thanks,
Steve
From fc0f3584ce1962305f60b8a5603a133087f61788 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 29 Dec 2023 11:16:18 +0000
Subject: [PATCH 5/5] cifs: fix in logging in cifs_chan_update_iface
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@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
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.40.1
From 3fd43c432aa122bee38a43c3081d0037423d8706 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 29 Dec 2023 11:16:17 +0000
Subject: [PATCH 4/5] cifs: cifs_pick_channel should skip unhealthy channels
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@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
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.40.1
From 09eeb0723f219fbd96d8865bf9b935e03ee2ec22 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 29 Dec 2023 11:16:16 +0000
Subject: [PATCH 3/5] cifs: do not depend on release_iface for maintaining
iface_list
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")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
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 55b3ce944022..5e32c79f03a7 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -994,7 +994,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 c8722e82274f..14bc745de199 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.40.1
From 7257bcf3bdc785eabc4eef1f329a59815b032508 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 29 Dec 2023 11:16:15 +0000
Subject: [PATCH 2/5] 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@xxxxxxxxxxxxxxx
Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
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 560624189c8b..dc9b95ca71e6 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 66b310208545..c8722e82274f 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.40.1
From 27e1fd343f80168ff456785c2443136b6b7ca3cc Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 29 Dec 2023 11:30:07 +0000
Subject: [PATCH 1/5] cifs: after disabling multichannel, mark tcon for
reconnect
Once the server disables multichannel for an active multichannel
session, on the following reconnect, the client would reduce
the number of channels to 1. However, it could be the case that
the tree connect was active on one of these disabled channels.
This results in an unrecoverable state.
This change fixes that by making sure that whenever a channel
is being terminated, the session and tcon are marked for
reconnect too. This could mean a few redundant tree connect
calls to the server, but considering that this is not a frequent
event, we should be okay.
Fixes: ee1d21794e55 ("cifs: handle when server stops supporting multichannel")
Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/smb/client/connect.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index dd2a1fb65e71..560624189c8b 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -216,17 +216,21 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
/* If server is a channel, select the primary channel */
pserver = SERVER_IS_CHAN(server) ? server->primary_server : server;
+ /*
+ * if the server has been marked for termination, there is a
+ * chance that the remaining channels all need reconnect. To be
+ * on the safer side, mark the session and trees for reconnect
+ * for this scenario. This might cause a few redundant session
+ * setup and tree connect requests, but it is better than not doing
+ * a tree connect when needed, and all following requests failing
+ */
+ if (server->terminate) {
+ mark_smb_session = true;
+ server = pserver;
+ }
spin_lock(&cifs_tcp_ses_lock);
list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
- /*
- * if channel has been marked for termination, nothing to do
- * for the channel. in fact, we cannot find the channel for the
- * server. So safe to exit here
- */
- if (server->terminate)
- break;
-
/* check if iface is still active */
if (!cifs_chan_is_iface_active(ses, server))
cifs_chan_update_iface(ses, server);
--
2.40.1
