| Message ID | 20231030110020.45627-11-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/14] cifs: print server capabilities in DebugData | expand |
merged into cifs-2.6.git for-next pending more testing added cc:stable On Mon, Oct 30, 2023 at 6:00 AM <nspmangalore@gmail.com> wrote: > > From: Shyam Prasad N <sprasad@microsoft.com> > > When the user mounts with multichannel option, but the > server does not support it, there can be a time in future > where it can be supported. > > With this change, such a case is handled. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/cifsproto.h | 4 ++++ > fs/smb/client/connect.c | 6 +++++- > fs/smb/client/smb2pdu.c | 31 ++++++++++++++++++++++++++++--- > 3 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h > index 65c84b3d1a65..5a4c1f1e0d91 100644 > --- a/fs/smb/client/cifsproto.h > +++ b/fs/smb/client/cifsproto.h > @@ -132,6 +132,10 @@ extern int SendReceiveBlockingLock(const unsigned int xid, > struct smb_hdr *in_buf, > struct smb_hdr *out_buf, > int *bytes_returned); > + > +void > +smb2_query_server_interfaces(struct work_struct *work); > + > void > cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server, > bool all_channels); > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > index e71aa33bf026..149cde77500e 100644 > --- a/fs/smb/client/connect.c > +++ b/fs/smb/client/connect.c > @@ -116,7 +116,8 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server) > return rc; > } > > -static void smb2_query_server_interfaces(struct work_struct *work) > +void > +smb2_query_server_interfaces(struct work_struct *work) > { > int rc; > int xid; > @@ -134,6 +135,9 @@ static void smb2_query_server_interfaces(struct work_struct *work) > if (rc) { > cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > __func__, rc); > + > + if (rc == -EOPNOTSUPP) > + return; > } > > queue_delayed_work(cifsiod_wq, &tcon->query_interfaces, > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c > index b7665155f4e2..2617437a4627 100644 > --- a/fs/smb/client/smb2pdu.c > +++ b/fs/smb/client/smb2pdu.c > @@ -163,6 +163,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > int rc = 0; > struct nls_table *nls_codepage = NULL; > struct cifs_ses *ses; > + int xid; > > /* > * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so > @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > tcon->need_reopen_files = true; > > rc = cifs_tree_connect(0, tcon, nls_codepage); > - mutex_unlock(&ses->session_mutex); > > cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); > if (rc) { > /* If sess reconnected but tcon didn't, something strange ... */ > + mutex_unlock(&ses->session_mutex); > cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); > goto out; > } > > - if (smb2_command != SMB2_INTERNAL_CMD) > - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > + if (!rc && > + (server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) { > + mutex_unlock(&ses->session_mutex); > + > + /* > + * query server network interfaces, in case they change > + */ > + xid = get_xid(); > + rc = SMB3_request_interfaces(xid, tcon, false); > + free_xid(xid); > + > + if (rc) > + cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > + __func__, rc); > + > + if (ses->chan_max > ses->chan_count && > + !SERVER_IS_CHAN(server)) { > + if (ses->chan_count == 1) > + cifs_dbg(VFS, "server %s supports multichannel now\n", > + ses->server->hostname); > + > + cifs_try_adding_channels(tcon->cifs_sb, ses); > + } > + } else { > + mutex_unlock(&ses->session_mutex); > + } > > atomic_inc(&tconInfoReconnectCount); > out: > -- > 2.34.1 >
nspmangalore@gmail.com writes: > From: Shyam Prasad N <sprasad@microsoft.com> > > When the user mounts with multichannel option, but the > server does not support it, there can be a time in future > where it can be supported. > > With this change, such a case is handled. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/smb/client/cifsproto.h | 4 ++++ > fs/smb/client/connect.c | 6 +++++- > fs/smb/client/smb2pdu.c | 31 ++++++++++++++++++++++++++++--- > 3 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h > index 65c84b3d1a65..5a4c1f1e0d91 100644 > --- a/fs/smb/client/cifsproto.h > +++ b/fs/smb/client/cifsproto.h > @@ -132,6 +132,10 @@ extern int SendReceiveBlockingLock(const unsigned int xid, > struct smb_hdr *in_buf, > struct smb_hdr *out_buf, > int *bytes_returned); > + > +void > +smb2_query_server_interfaces(struct work_struct *work); > + Why are you exporting this? smb2_query_server_interfaces() seems to be used only in fs/smb/client/connect.c. > void > cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server, > bool all_channels); > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > index e71aa33bf026..149cde77500e 100644 > --- a/fs/smb/client/connect.c > +++ b/fs/smb/client/connect.c > @@ -116,7 +116,8 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server) > return rc; > } > > -static void smb2_query_server_interfaces(struct work_struct *work) > +void > +smb2_query_server_interfaces(struct work_struct *work) > { Ditto. > int rc; > int xid; > @@ -134,6 +135,9 @@ static void smb2_query_server_interfaces(struct work_struct *work) > if (rc) { > cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > __func__, rc); > + > + if (rc == -EOPNOTSUPP) > + return; Maybe also get rid of cifs_dbg() when rc == -EOPNOTSUPP? > } > > queue_delayed_work(cifsiod_wq, &tcon->query_interfaces, > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c > index b7665155f4e2..2617437a4627 100644 > --- a/fs/smb/client/smb2pdu.c > +++ b/fs/smb/client/smb2pdu.c > @@ -163,6 +163,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > int rc = 0; > struct nls_table *nls_codepage = NULL; > struct cifs_ses *ses; > + int xid; > > /* > * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so > @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > tcon->need_reopen_files = true; > > rc = cifs_tree_connect(0, tcon, nls_codepage); > - mutex_unlock(&ses->session_mutex); > > cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); > if (rc) { > /* If sess reconnected but tcon didn't, something strange ... */ > + mutex_unlock(&ses->session_mutex); > cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); > goto out; > } > > - if (smb2_command != SMB2_INTERNAL_CMD) > - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); Why are you removing this optimisation? For example, session/tcon reconnect will no longer be triggered after returning back to userspace, only when next I/O comes in.
nspmangalore@gmail.com writes: > @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > tcon->need_reopen_files = true; > > rc = cifs_tree_connect(0, tcon, nls_codepage); > - mutex_unlock(&ses->session_mutex); > > cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); > if (rc) { > /* If sess reconnected but tcon didn't, something strange ... */ > + mutex_unlock(&ses->session_mutex); > cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); > goto out; > } > > - if (smb2_command != SMB2_INTERNAL_CMD) > - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > + if (!rc && > + (server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) { > + mutex_unlock(&ses->session_mutex); > + > + /* > + * query server network interfaces, in case they change > + */ > + xid = get_xid(); > + rc = SMB3_request_interfaces(xid, tcon, false); > + free_xid(xid); > + > + if (rc) > + cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > + __func__, rc); > + > + if (ses->chan_max > ses->chan_count && > + !SERVER_IS_CHAN(server)) { > + if (ses->chan_count == 1) > + cifs_dbg(VFS, "server %s supports multichannel now\n", > + ses->server->hostname); Sorry, forgot to mention that you should call cifs_dbg_server() which protects access of @server->hostname as cifsd thread could have it freed before you access it.
removed cc:stable and changed > + cifs_dbg(VFS, "server %s supports multichannel now\n", > + ses->server->hostname); to` + cifs_server_dbg(VFS, "supports multichannel now\n"); Let me know if that is ok for you. (See attached updated patch) On Thu, Nov 2, 2023 at 3:28 PM Paulo Alcantara <pc@manguebit.com> wrote: > > nspmangalore@gmail.com writes: > > > @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > > tcon->need_reopen_files = true; > > > > rc = cifs_tree_connect(0, tcon, nls_codepage); > > - mutex_unlock(&ses->session_mutex); > > > > cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); > > if (rc) { > > /* If sess reconnected but tcon didn't, something strange ... */ > > + mutex_unlock(&ses->session_mutex); > > cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); > > goto out; > > } > > > > - if (smb2_command != SMB2_INTERNAL_CMD) > > - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > > + if (!rc && > > + (server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) { > > + mutex_unlock(&ses->session_mutex); > > + > > + /* > > + * query server network interfaces, in case they change > > + */ > > + xid = get_xid(); > > + rc = SMB3_request_interfaces(xid, tcon, false); > > + free_xid(xid); > > + > > + if (rc) > > + cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > > + __func__, rc); > > + > > + if (ses->chan_max > ses->chan_count && > > + !SERVER_IS_CHAN(server)) { > > + if (ses->chan_count == 1) > > + cifs_dbg(VFS, "server %s supports multichannel now\n", > > + ses->server->hostname); > > Sorry, forgot to mention that you should call cifs_dbg_server() which > protects access of @server->hostname as cifsd thread could have it freed > before you access it. -- Thanks, Steve
Steve French <smfrench@gmail.com> writes: > removed cc:stable and changed > >> + cifs_dbg(VFS, "server %s supports multichannel now\n", >> + ses->server->hostname); > > to` > > + cifs_server_dbg(VFS, "supports > multichannel now\n"); Looks good, thanks. > Let me know if that is ok for you. (See attached updated patch) For the s/cifs_dbg/cifs_server_dbg/ change, it is.
On Wed, Nov 1, 2023 at 9:22 PM Paulo Alcantara <pc@manguebit.com> wrote: > > nspmangalore@gmail.com writes: > > > From: Shyam Prasad N <sprasad@microsoft.com> > > > > When the user mounts with multichannel option, but the > > server does not support it, there can be a time in future > > where it can be supported. > > > > With this change, such a case is handled. > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > --- > > fs/smb/client/cifsproto.h | 4 ++++ > > fs/smb/client/connect.c | 6 +++++- > > fs/smb/client/smb2pdu.c | 31 ++++++++++++++++++++++++++++--- > > 3 files changed, 37 insertions(+), 4 deletions(-) > > > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h > > index 65c84b3d1a65..5a4c1f1e0d91 100644 > > --- a/fs/smb/client/cifsproto.h > > +++ b/fs/smb/client/cifsproto.h > > @@ -132,6 +132,10 @@ extern int SendReceiveBlockingLock(const unsigned int xid, > > struct smb_hdr *in_buf, > > struct smb_hdr *out_buf, > > int *bytes_returned); > > + > > +void > > +smb2_query_server_interfaces(struct work_struct *work); > > + > > Why are you exporting this? smb2_query_server_interfaces() seems to be > used only in fs/smb/client/connect.c. In an earlier version of this change, I was calling smb2_query_server_interfaces from smb2_reconnect when multichannel is reenabled. That needed the export. However, I had to change that. I will reset this back. > > > void > > cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server, > > bool all_channels); > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > > index e71aa33bf026..149cde77500e 100644 > > --- a/fs/smb/client/connect.c > > +++ b/fs/smb/client/connect.c > > @@ -116,7 +116,8 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server) > > return rc; > > } > > > > -static void smb2_query_server_interfaces(struct work_struct *work) > > +void > > +smb2_query_server_interfaces(struct work_struct *work) > > { > > Ditto. > > > int rc; > > int xid; > > @@ -134,6 +135,9 @@ static void smb2_query_server_interfaces(struct work_struct *work) > > if (rc) { > > cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", > > __func__, rc); > > + > > + if (rc == -EOPNOTSUPP) > > + return; > > Maybe also get rid of cifs_dbg() when rc == -EOPNOTSUPP? > > > } > > > > queue_delayed_work(cifsiod_wq, &tcon->query_interfaces, > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c > > index b7665155f4e2..2617437a4627 100644 > > --- a/fs/smb/client/smb2pdu.c > > +++ b/fs/smb/client/smb2pdu.c > > @@ -163,6 +163,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > > int rc = 0; > > struct nls_table *nls_codepage = NULL; > > struct cifs_ses *ses; > > + int xid; > > > > /* > > * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so > > @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, > > tcon->need_reopen_files = true; > > > > rc = cifs_tree_connect(0, tcon, nls_codepage); > > - mutex_unlock(&ses->session_mutex); > > > > cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); > > if (rc) { > > /* If sess reconnected but tcon didn't, something strange ... */ > > + mutex_unlock(&ses->session_mutex); > > cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); > > goto out; > > } > > > > - if (smb2_command != SMB2_INTERNAL_CMD) > > - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > > Why are you removing this optimisation? For example, session/tcon > reconnect will no longer be triggered after returning back to userspace, > only when next I/O comes in.
Paulo Alcantara <pc@manguebit.com> writes: > Steve French <smfrench@gmail.com> writes: > >> removed cc:stable and changed >> >>> + cifs_dbg(VFS, "server %s supports multichannel now\n", >>> + ses->server->hostname); >> >> to` >> >> + cifs_server_dbg(VFS, "supports >> multichannel now\n"); > > Looks good, thanks. > >> Let me know if that is ok for you. (See attached updated patch) > > For the s/cifs_dbg/cifs_server_dbg/ change, it is. BTW, this patch in for-next branch still contains (1) misleading export of smb2_query_server_interfaces() (2) removal of mod_delayed_work() when reconnect failed (3) logging of failed network interface queries even when server doesn't support it.
I just updated the patch in for-next (see attached) I removed the unneeded export of smb2_query_server_interfaces() but it looks like the logging of failed network interface queries is fine (and is an FYI message so not turned on automatically) - I could definitely see cases where FYI logging is turned on to see why query server interfaces failed even in the EOPNOTSUPP case - so the debug FYI looks ok to me On Wed, Nov 8, 2023 at 10:02 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Paulo Alcantara <pc@manguebit.com> writes: > > > Steve French <smfrench@gmail.com> writes: > > > >> removed cc:stable and changed > >> > >>> + cifs_dbg(VFS, "server %s supports multichannel now\n", > >>> + ses->server->hostname); > >> > >> to` > >> > >> + cifs_server_dbg(VFS, "supports > >> multichannel now\n"); > > > > Looks good, thanks. > > > >> Let me know if that is ok for you. (See attached updated patch) > > > > For the s/cifs_dbg/cifs_server_dbg/ change, it is. > > BTW, this patch in for-next branch still contains > > (1) misleading export of smb2_query_server_interfaces() > > (2) removal of mod_delayed_work() when reconnect failed > > (3) logging of failed network interface queries even when server > doesn't support it. >
Steve French <smfrench@gmail.com> writes: > I just updated the patch in for-next (see attached) Thanks. > I removed the unneeded export of smb2_query_server_interfaces() but it > looks like the logging of failed network interface queries is fine > (and is an FYI message so not turned on automatically) - I could > definitely see cases where FYI logging is turned on to see why query > server interfaces failed even in the EOPNOTSUPP case - so the debug > FYI looks ok to me Yeah, no problem.
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h index 65c84b3d1a65..5a4c1f1e0d91 100644 --- a/fs/smb/client/cifsproto.h +++ b/fs/smb/client/cifsproto.h @@ -132,6 +132,10 @@ extern int SendReceiveBlockingLock(const unsigned int xid, struct smb_hdr *in_buf, struct smb_hdr *out_buf, int *bytes_returned); + +void +smb2_query_server_interfaces(struct work_struct *work); + void cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server, bool all_channels); diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c index e71aa33bf026..149cde77500e 100644 --- a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c @@ -116,7 +116,8 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server) return rc; } -static void smb2_query_server_interfaces(struct work_struct *work) +void +smb2_query_server_interfaces(struct work_struct *work) { int rc; int xid; @@ -134,6 +135,9 @@ static void smb2_query_server_interfaces(struct work_struct *work) if (rc) { cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", __func__, rc); + + if (rc == -EOPNOTSUPP) + return; } queue_delayed_work(cifsiod_wq, &tcon->query_interfaces, diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c index b7665155f4e2..2617437a4627 100644 --- a/fs/smb/client/smb2pdu.c +++ b/fs/smb/client/smb2pdu.c @@ -163,6 +163,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, int rc = 0; struct nls_table *nls_codepage = NULL; struct cifs_ses *ses; + int xid; /* * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so @@ -307,17 +308,41 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon, tcon->need_reopen_files = true; rc = cifs_tree_connect(0, tcon, nls_codepage); - mutex_unlock(&ses->session_mutex); cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc); if (rc) { /* If sess reconnected but tcon didn't, something strange ... */ + mutex_unlock(&ses->session_mutex); cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc); goto out; } - if (smb2_command != SMB2_INTERNAL_CMD) - mod_delayed_work(cifsiod_wq, &server->reconnect, 0); + if (!rc && + (server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) { + mutex_unlock(&ses->session_mutex); + + /* + * query server network interfaces, in case they change + */ + xid = get_xid(); + rc = SMB3_request_interfaces(xid, tcon, false); + free_xid(xid); + + if (rc) + cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n", + __func__, rc); + + if (ses->chan_max > ses->chan_count && + !SERVER_IS_CHAN(server)) { + if (ses->chan_count == 1) + cifs_dbg(VFS, "server %s supports multichannel now\n", + ses->server->hostname); + + cifs_try_adding_channels(tcon->cifs_sb, ses); + } + } else { + mutex_unlock(&ses->session_mutex); + } atomic_inc(&tconInfoReconnectCount); out: