| 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 |
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.
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.
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 --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;