smb: client: add only valid ifaces to ses->iface_list

This allows less locking and an easier to parse received list.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>

4 files changed, 85 insertions, 104 deletions

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index d5e66391c26f..8ecba11aea78 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -618,7 +618,7 @@ enum securityEnum cifs_select_sectype(struct TCP_Server_Info *,
 int cifs_alloc_hash(const char *name, struct shash_desc **sdesc);
 void cifs_free_hash(struct shash_desc **sdesc);
 
-int cifs_try_adding_channels(struct cifs_ses *ses);
+int cifs_try_adding_channels(struct cifs_ses *ses, struct list_head *ifaces);
 bool is_ses_using_iface(struct cifs_ses *ses, struct cifs_server_iface *iface);
 void cifs_ses_mark_for_reconnect(struct cifs_ses *ses);
 
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index ded2e38f97c0..5cff0fbf1982 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -149,17 +149,23 @@ cifs_chan_is_iface_active(struct cifs_ses *ses,
  * >0: number of channels remaining to be opened
  * <0: error from cifs_ses_add_channel()
  */
-static int try_adding_channels(struct cifs_ses *ses)
+static int try_adding_channels(struct cifs_ses *ses, struct list_head *ifaces)
 {
-	size_t iface_weight = 0, iface_min_speed = 0;
-	struct cifs_server_iface *iface = NULL, *niface = NULL;
-	struct cifs_server_iface *last_iface = NULL;
+	size_t weight = 0, min_speed = 0;
+	struct cifs_server_iface *iface = NULL, *niface = NULL, *last;
 	int rc = 0, left = ses->chan_max - ses->chan_count;
 
-	last_iface = list_last_entry(&ses->iface_list, struct cifs_server_iface, iface_head);
-	iface_min_speed = last_iface->speed;
+	if (list_empty(ifaces))
+		return 0;
 
-	list_for_each_entry_safe(iface, niface, &ses->iface_list, iface_head) {
+	last = list_last_entry(ifaces, struct cifs_server_iface, iface_head);
+	min_speed = last->speed;
+	if (!list_empty(&ses->iface_list)) {
+		last = list_last_entry(&ses->iface_list, struct cifs_server_iface, iface_head);
+		min_speed = min_t(size_t, min_speed, last->speed);
+	}
+
+	list_for_each_entry_safe(iface, niface, ifaces, iface_head) {
 		rc = 0;
 
 		/* do not mix rdma and non-rdma interfaces */
@@ -167,53 +173,43 @@ static int try_adding_channels(struct cifs_ses *ses)
 			continue;
 
 		/* skip ifaces that are unusable */
-		if (!iface->is_active || (is_ses_using_iface(ses, iface) && !iface->rss_capable))
+		if (!iface->rss_capable)
 			continue;
 
-		spin_lock(&ses->iface_lock);
-		if (!ses->iface_count) {
-			spin_unlock(&ses->iface_lock);
-			cifs_dbg(ONCE, "server %s does not advertise interfaces\n",
-				      ses->server->hostname);
-			break;
-		}
-
 		/* check if we already allocated enough channels */
-		iface_weight = iface->speed / iface_min_speed;
-		if (iface->weight_fulfilled >= iface_weight) {
-			spin_unlock(&ses->iface_lock);
+		weight = iface->speed / min_speed;
+		if (iface->weight_fulfilled >= weight)
 			continue;
-		}
 
-		/* take ref before unlock */
-		kref_get(&iface->refcount);
-
-		spin_unlock(&ses->iface_lock);
 		rc = cifs_ses_add_channel(ses, iface);
-		spin_lock(&ses->iface_lock);
-
 		if (!rc) {
 			cifs_info("successfully opened new channel on iface:%pIS\n", &iface->sockaddr);
+			ses->iface_count++;
+			iface->is_active = 1;
 			iface->num_channels++;
 			iface->weight_fulfilled++;
-			left--;
+			kref_get(&iface->refcount);
+
+			spin_lock(&ses->iface_lock);
+			list_move(&iface->iface_head, &ses->iface_list);
+			ses->iface_last_update = jiffies;
+			spin_unlock(&ses->iface_lock);
+
+			if (--left == 0)
+				break;
 		} else {
 			cifs_dbg(VFS, "failed to open extra channel on iface:%pIS rc=%d\n",
 				 &iface->sockaddr, rc);
-			kref_put(&iface->refcount, release_iface);
 			iface->weight_fulfilled++;
-			if (rc != -EAGAIN) {
-				spin_unlock(&ses->iface_lock);
+			if (rc != -EAGAIN)
 				break;
-			}
 		}
-		spin_unlock(&ses->iface_lock);
 	}
 
 	return rc ?: left;
 }
 
-int cifs_try_adding_channels(struct cifs_ses *ses)
+int cifs_try_adding_channels(struct cifs_ses *ses, struct list_head *ifaces)
 {
 	int rc, retries = 0;
 
@@ -226,7 +222,7 @@ try_again:
 	}
 	spin_unlock(&ses->chan_lock);
 
-	rc = try_adding_channels(ses);
+	rc = try_adding_channels(ses, ifaces);
 	if (!rc)
 		return 0;
 
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 0878d45d5c57..25f75146985f 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -617,9 +617,15 @@ static int iface_cmp(void *unused, const struct list_head *a, const struct list_
 	return __iface_cmp(ifa, ifb);
 }
 
+static inline bool ifaces_need_update(unsigned long last_update)
+{
+	return (last_update &&
+		time_before(jiffies, last_update + (SMB_INTERFACE_POLL_INTERVAL * HZ)));
+}
+
 static int
-parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
-			size_t buf_len, struct cifs_ses *ses)
+parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf, size_t buf_len,
+			struct cifs_ses *ses, struct list_head *ifaces)
 {
 	struct network_interface_info_ioctl_rsp *p;
 	struct sockaddr_in *addr4;
@@ -629,32 +635,13 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 	struct cifs_server_iface *info = NULL, *iface = NULL, *niface = NULL;
 	struct cifs_server_iface tmp_iface;
 	ssize_t bytes_left;
-	size_t next = 0, old_count;
+	size_t next = 0;
 	int nb_iface = 0;
 	int rc = 0, ret = 0;
 
 	bytes_left = buf_len;
 	p = buf;
 
-	spin_lock(&ses->iface_lock);
-	/* do not query too frequently, this time with lock held */
-	if (ses->iface_last_update &&
-	    time_before(jiffies, ses->iface_last_update +
-			(SMB_INTERFACE_POLL_INTERVAL * HZ))) {
-		spin_unlock(&ses->iface_lock);
-		return 0;
-	}
-
-	/*
-	 * Go through iface_list and mark them as inactive
-	 */
-	list_for_each_entry_safe(iface, niface, &ses->iface_list,
-				 iface_head)
-		iface->is_active = 0;
-
-	old_count = ses->iface_count;
-	spin_unlock(&ses->iface_lock);
-
 	/*
 	 * Samba server e.g. can return an empty interface list in some cases,
 	 * which would only be a problem if we were requesting multichannel
@@ -672,6 +659,12 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 		tmp_iface.rdma_capable = le32_to_cpu(p->Capability & RDMA_CAPABLE) ? 1 : 0;
 		tmp_iface.rss_capable = le32_to_cpu(p->Capability & RSS_CAPABLE) ? 1 : 0;
 
+		if (!tmp_iface.rss_capable)
+			goto next_iface;
+
+		if (tmp_iface.rdma_capable != ses->server->rdma)
+			goto next_iface;
+
 		switch (p->Family) {
 		/*
 		 * The kernel and wire socket structures have the same
@@ -712,26 +705,29 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 		}
 
 		/*
-		 * The iface_list is assumed to be sorted by speed.
+		 * iface_list is sorted by speed.
 		 * Check if the new interface exists in that list.
-		 * NEVER change iface. it could be in use.
-		 * Add a new one instead
 		 */
 		spin_lock(&ses->iface_lock);
-		list_for_each_entry_safe(iface, niface, &ses->iface_list,
-					 iface_head) {
-			ret = __iface_cmp(iface, &tmp_iface);
-			if (!ret) {
-				iface->is_active = 1;
-				spin_unlock(&ses->iface_lock);
-				goto next_iface;
+		list_for_each_entry_safe(iface, niface, &ses->iface_list, iface_head) {
+			if (likely(iface->is_active)) {
+				ret = __iface_cmp(iface, &tmp_iface);
+				if (!ret) {
+					spin_unlock(&ses->iface_lock);
+					goto next_iface;
+				}
+				continue;
 			}
+
+			/* Take the opportunity and drop inactive ifaces. */
+			list_del(&iface->iface_head);
+			kref_put(&iface->refcount, release_iface);
+			ses->iface_count--;
 		}
 		spin_unlock(&ses->iface_lock);
 
 		/* no match. insert the entry in the list */
-		info = kmalloc(sizeof(struct cifs_server_iface),
-			       GFP_KERNEL);
+		info = kmalloc(sizeof(struct cifs_server_iface), GFP_KERNEL);
 		if (!info) {
 			rc = -ENOMEM;
 			goto out;
@@ -740,7 +736,6 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 
 		/* add this new entry to the list */
 		kref_init(&info->refcount);
-		info->is_active = 1;
 
 		cifs_dbg(FYI, "%s: adding iface[%zu]: %pIS\n", __func__, ses->iface_count,
 			 &info->sockaddr);
@@ -748,10 +743,7 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 		cifs_dbg(FYI, "%s: capabilities 0x%08x\n", __func__,
 			 le32_to_cpu(p->Capability));
 
-		spin_lock(&ses->iface_lock);
-		list_add_tail(&info->iface_head, &ses->iface_list);
-		ses->iface_count++;
-		spin_unlock(&ses->iface_lock);
+		list_add_tail(&info->iface_head, ifaces);
 next_iface:
 		nb_iface++;
 		next = le32_to_cpu(p->Next);
@@ -772,31 +764,12 @@ next_iface:
 	/* Azure rounds the buffer size up 8, to a 16 byte boundary */
 	if ((bytes_left > 8) || p->Next)
 		cifs_dbg(VFS, "%s: incomplete interface info\n", __func__);
-
-	ses->iface_last_update = jiffies;
-
 out:
-	/*
-	 * Put inactive entries, then sort the list (only if new ifaces added, to save some cycles).
-	 */
-	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--;
-		}
-	}
-
-	if (old_count != ses->iface_count)
-		list_sort(NULL, &ses->iface_list, iface_cmp);
-	spin_unlock(&ses->iface_lock);
-
 	return rc;
 }
 
-static int request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
+static int request_interfaces(const unsigned int xid, struct cifs_tcon *tcon,
+			      struct list_head *ifaces)
 {
 	int rc;
 	unsigned int ret_data_len = 0;
@@ -833,7 +806,7 @@ static int request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
 		goto out;
 	}
 
-	rc = parse_server_interfaces(out_buf, ret_data_len, ses);
+	rc = parse_server_interfaces(out_buf, ret_data_len, ses, ifaces);
 	if (rc)
 		goto out;
 
@@ -855,13 +828,17 @@ out:
 void smb2_query_server_interfaces(struct work_struct *work)
 {
 	struct cifs_tcon *tcon = container_of(work, struct cifs_tcon, query_interfaces.work);
-	struct cifs_ses *ses = tcon->ses;
+	struct cifs_ses *ses;
+	struct cifs_server_iface *iface, *q;
 	unsigned long delay = SMB_INTERFACE_POLL_INTERVAL * HZ;
 	int xid, rc;
+	LIST_HEAD(ifaces);
 
-	if (!tcon->ipc || !ses)
+	if (!tcon->ipc || !tcon->ses)
 		return;
 
+	ses = tcon->ses;
+
 	/* Hold a reference while we run as the last non-IPC tcon might free ses. */
 	spin_lock(&cifs_tcp_ses_lock);
 	cifs_smb_ses_inc_refcount(ses);
@@ -876,6 +853,7 @@ void smb2_query_server_interfaces(struct work_struct *work)
 	spin_unlock(&tcon->tc_lock);
 
 	spin_lock(&ses->ses_lock);
+	WARN_ON_ONCE(ses->tcon_ipc != tcon);
 	if (ses->ses_status != SES_GOOD) {
 		spin_unlock(&ses->ses_lock);
 		cifs_put_smb_ses(ses);
@@ -887,21 +865,24 @@ void smb2_query_server_interfaces(struct work_struct *work)
 	 * query server network interfaces, in case they change
 	 */
 	xid = get_xid();
-	rc = request_interfaces(xid, tcon);
+	rc = request_interfaces(xid, tcon, &ifaces);
 	free_xid(xid);
 
 	if (rc)
 		goto out;
 
 	/* Got interfaces, try to add channels. */
-	rc = cifs_try_adding_channels(ses);
-	if (rc) {
+	rc = cifs_try_adding_channels(ses, &ifaces);
+	if (!rc) {
+		spin_lock(&ses->iface_lock);
+		list_sort(NULL, &ses->iface_list, iface_cmp);
+		spin_unlock(&ses->iface_lock);
+	} else if (rc == -EAGAIN || rc == -EINPROGRESS) {
 		/*
 		 * Interfaces are available, but failed to negotiate/sess setup on channel(s) --
 		 * try again in 2s if not a hard error.
 		 */
-		if (rc == -EAGAIN || rc == -EINPROGRESS)
-			delay = 2 * HZ;
+		delay = 2 * HZ;
 	}
 out:
 	if (rc != -EOPNOTSUPP) {
@@ -910,6 +891,11 @@ out:
 		queue_delayed_work(cifsiod_wq, &tcon->query_interfaces, delay);
 	}
 
+	list_for_each_entry_safe(iface, q, &ifaces, iface_head) {
+		list_del(&iface->iface_head);
+		kfree(iface);
+	}
+
 	cifs_put_smb_ses(ses);
 }
 
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index ee98f6c95451..d327b76cf1b8 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -218,7 +218,6 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	       struct TCP_Server_Info *server, bool from_reconnect)
 {
 	struct cifs_ses *ses;
-	int xid;
 	int rc = 0;
 
 	/*