Re: [PATCH 08/14] cifs: account for primary channel in the interface list

[Steve French <smfrench@gmail.com>]

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

Updated patch attached. Let me know if any objections.

Paulo,
I made minor updates to Shyam's patch following your suggestion of
changing the logging level you suggested, and saving off dstaddr so we
don't have to hold a lock on it

On Wed, Nov 8, 2023 at 9:44 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> nspmangalore@gmail.com writes:
>
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > The refcounting of server interfaces should account
> > for the primary channel too. Although this is not
> > strictly necessary, doing so will account for the primary
> > channel in DebugData.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/sess.c    | 23 +++++++++++++++++++++++
> >  fs/smb/client/smb2ops.c |  6 ++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
> > index d009994f82cf..6843deed6119 100644
> > --- a/fs/smb/client/sess.c
> > +++ b/fs/smb/client/sess.c
> > @@ -332,6 +332,16 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
> >
> >       /* then look for a new one */
> >       list_for_each_entry(iface, &ses->iface_list, iface_head) {
> > +             if (!chan_index) {
> > +                     /* if we're trying to get the updated iface for primary channel */
> > +                     if (!cifs_match_ipaddr((struct sockaddr *) &server->dstaddr,
> > +                                            (struct sockaddr *) &iface->sockaddr))
> > +                             continue;
>
> You should hold @server->srv_lock to protect access of @server->dstaddr
> as it might change over reconnect or mount.
>
> > +
> > +                     kref_get(&iface->refcount);
> > +                     break;
> > +             }
> > +
> >               /* do not mix rdma and non-rdma interfaces */
> >               if (iface->rdma_capable != server->rdma)
> >                       continue;
> > @@ -358,6 +368,13 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
> >               cifs_dbg(FYI, "unable to find a suitable iface\n");
> >       }
> >
> > +     if (!chan_index && !iface) {
> > +             cifs_dbg(VFS, "unable to get the interface matching: %pIS\n",
> > +                      &server->dstaddr);
>
> Ditto.
>
> Also, I think you should log in FYI level as the above doesn't seem
> unlikely to happen.



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-account-for-primary-channel-in-the-interface-li.patch --]
[-- Type: text/x-patch, Size: 3829 bytes --]

From 8023cb3ca155551abcef7125cd53e626022fc53b Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Tue, 14 Mar 2023 11:14:58 +0000
Subject: [PATCH] cifs: account for primary channel in the interface list

The refcounting of server interfaces should account
for the primary channel too. Although this is not
strictly necessary, doing so will account for the primary
channel in DebugData.

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/sess.c    | 28 ++++++++++++++++++++++++++++
 fs/smb/client/smb2ops.c |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index 04d300e9a779..21e75ca47151 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -303,6 +303,7 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 	struct cifs_server_iface *iface = NULL;
 	struct cifs_server_iface *old_iface = NULL;
 	struct cifs_server_iface *last_iface = NULL;
+	struct sockaddr_storage ss;
 	int rc = 0;
 
 	spin_lock(&ses->chan_lock);
@@ -321,6 +322,10 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 	}
 	spin_unlock(&ses->chan_lock);
 
+	spin_lock(&server->srv_lock);
+	ss = server->dstaddr;
+	spin_unlock(&server->srv_lock);
+
 	spin_lock(&ses->iface_lock);
 	if (!ses->iface_count) {
 		spin_unlock(&ses->iface_lock);
@@ -334,6 +339,16 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 
 	/* then look for a new one */
 	list_for_each_entry(iface, &ses->iface_list, iface_head) {
+		if (!chan_index) {
+			/* if we're trying to get the updated iface for primary channel */
+			if (!cifs_match_ipaddr((struct sockaddr *) &ss,
+					       (struct sockaddr *) &iface->sockaddr))
+				continue;
+
+			kref_get(&iface->refcount);
+			break;
+		}
+
 		/* do not mix rdma and non-rdma interfaces */
 		if (iface->rdma_capable != server->rdma)
 			continue;
@@ -360,6 +375,13 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 		cifs_dbg(FYI, "unable to find a suitable iface\n");
 	}
 
+	if (!chan_index && !iface) {
+		cifs_dbg(FYI, "unable to get the interface matching: %pIS\n",
+			 &ss);
+		spin_unlock(&ses->iface_lock);
+		return 0;
+	}
+
 	/* now drop the ref to the current iface */
 	if (old_iface && iface) {
 		cifs_dbg(FYI, "replacing iface: %pIS with %pIS\n",
@@ -382,6 +404,12 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
 			old_iface->weight_fulfilled--;
 
 		kref_put(&old_iface->refcount, release_iface);
+	} else if (!chan_index) {
+		/* special case: update interface for primary channel */
+		cifs_dbg(FYI, "referencing primary channel iface: %pIS\n",
+			 &iface->sockaddr);
+		iface->num_channels++;
+		iface->weight_fulfilled++;
 	} else {
 		WARN_ON(!iface);
 		cifs_dbg(FYI, "adding new iface: %pIS\n", &iface->sockaddr);
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 601e7a187f87..a959ed2c9b22 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -756,6 +756,7 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
 	unsigned int ret_data_len = 0;
 	struct network_interface_info_ioctl_rsp *out_buf = NULL;
 	struct cifs_ses *ses = tcon->ses;
+	struct TCP_Server_Info *pserver;
 
 	/* do not query too frequently */
 	if (ses->iface_last_update &&
@@ -780,6 +781,11 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
 	if (rc)
 		goto out;
 
+	/* check if iface is still active */
+	pserver = ses->chans[0].server;
+	if (pserver && !cifs_chan_is_iface_active(ses, pserver))
+		cifs_chan_update_iface(ses, pserver);
+
 out:
 	kfree(out_buf);
 	return rc;
-- 
2.39.2