diff mbox series

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

Message ID 20231030110020.45627-8-sprasad@microsoft.com (mailing list archive)
State New, archived
Headers show
Series [01/14] cifs: print server capabilities in DebugData | expand

Commit Message

Shyam Prasad N Oct. 30, 2023, 11 a.m. UTC
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(+)

Comments

Paulo Alcantara Nov. 8, 2023, 3:44 p.m. UTC | #1
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.
Steve French Nov. 8, 2023, 6:16 p.m. UTC | #2
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.
Paulo Alcantara Nov. 8, 2023, 7:03 p.m. UTC | #3
Steve French <smfrench@gmail.com> writes:

> Updated patch attached. Let me know if any objections.
>
> 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

Looks good, thanks!
diff mbox series

Patch

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;
+
+			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);
+		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",
@@ -380,6 +397,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 9aeecee6b91b..e7e1e0e5bdfe 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;