Re: [PATCH 13/14] cifs: display the endpoint IP details in DebugData

Paulo Alcantara
2023-10-31T15:19:44Z

nspmangalore@gmail.com writes:

> From: Shyam Prasad N <sprasad@microsoft.com>
>
> With multichannel, it is useful to know the src port details
> for each channel. This change will print the ip addr and
> port details for both the socket dest and src endpoints.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
> fs/smb/client/cifs_debug.c | 73 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index e23fcabb78d6..d8362e098310 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -13,6 +13,7 @@
> #include <linux/proc_fs.h>
> #include <linux/uaccess.h>
> #include <uapi/linux/ethtool.h>
> +#include <net/inet_sock.h>
> #include "cifspdu.h"
> #include "cifsglob.h"
> #include "cifsproto.h"
> @@ -158,11 +159,37 @@ cifs_dump_channel(struct seq_file *m, int i, struct cifs_chan *chan)
> in_flight(server),
> atomic_read(&server->in_send),
> atomic_read(&server->num_waiters));
> +
> #ifdef CONFIG_NET_NS
> if (server->net)
> seq_printf(m, " Net namespace: %u ", server->net->ns.inum);
> #endif /* NET_NS */
>
> +#ifdef CONFIG_CIFS_SMB_DIRECT

No need to check for CONFIG_CIFS_SMB_DIRECT as @server->{rdma,smbd_conn}
are always defined.

> + if (!server->rdma)

cifs_rdma_enabled()? To be consistent with other places.

> + goto skip_rdma;
> +
> + if (server->smbd_conn && server->smbd_conn->id) {
> + struct rdma_addr *addr =
> + &server->smbd_conn->id->route.addr;
> + seq_printf(m, "\n\t\tIP addr: dst: %pISpc, src: %pISpc",
> + &addr->dst_addr, &addr->src_addr);
> + }
> +
> +skip_rdma:
> +#endif

The goto is no longer necessary when removing above #ifdef.

> + if (server->ssocket) {
> + struct sockaddr src;
> + int addrlen;
> +
> + addrlen = kernel_getsockname(server->ssocket, &src);
> + if (addrlen != sizeof(struct sockaddr_in) &&
> + addrlen != sizeof(struct sockaddr_in6))
> + return;
> +
> + seq_printf(m, "\n\t\tIP addr: dst: %pISpc, src: %pISpc",
> + &server->dstaddr, &src);
> + }
> }
>
> static inline const char *smb_speed_to_str(size_t bps)
> @@ -279,7 +306,7 @@ static int cifs_debug_files_proc_show(struct seq_file *m, void *v)
> static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
> {
> struct mid_q_entry *mid_entry;
> - struct TCP_Server_Info *server;
> + struct TCP_Server_Info *server, *nserver;
> struct TCP_Server_Info *chan_server;
> struct cifs_ses *ses;
> struct cifs_tcon *tcon;
> @@ -336,7 +363,7 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>
> c = 0;
> spin_lock(&cifs_tcp_ses_lock);
> - list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
> + list_for_each_entry_safe(server, nserver, &cifs_tcp_ses_list, tcp_ses_list) {
> /* channel info will be printed as a part of sessions below */
> if (SERVER_IS_CHAN(server))
> continue;
> @@ -414,8 +441,39 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
> seq_printf(m, "\nMR mr_ready_count: %x mr_used_count: %x",
> atomic_read(&server->smbd_conn->mr_ready_count),
> atomic_read(&server->smbd_conn->mr_used_count));
> + if (server->smbd_conn->id) {
> + struct rdma_addr *addr =
> + &server->smbd_conn->id->route.addr;
> + seq_printf(m, "\nIP addr: dst: %pISpc, src: %pISpc",
> + &addr->dst_addr, &addr->src_addr);
> + }
> skip_rdma:
> #endif
> + if (server->ssocket) {
> + struct sockaddr src;
> + int addrlen;
> +
> + /* kernel_getsockname can block. so drop the lock first */
> + server->srv_count++;
> + spin_unlock(&cifs_tcp_ses_lock);
> +
> + addrlen = kernel_getsockname(server->ssocket, &src);
> + if (addrlen != sizeof(struct sockaddr_in) &&
> + addrlen != sizeof(struct sockaddr_in6)) {
> + cifs_put_tcp_session(server, 0);
> + spin_lock(&cifs_tcp_ses_lock);
> +
> + goto skip_addr_details;
> + }

What about

addrlen = kernel_getsockname(server->ssocket, &src);
if (addrlen == sizeof(struct sockaddr_in) &&
addrlen == sizeof(struct sockaddr_in6)) {
seq_printf(m, "\nIP addr: dst: %pISpc, src: %pISpc",
&server->dstaddr, &src);
cifs_put_tcp_session(server, 0);
spin_lock(&cifs_tcp_ses_lock);
}

> +
> + seq_printf(m, "\nIP addr: dst: %pISpc, src: %pISpc",
> + &server->dstaddr, &src);
> +
> + cifs_put_tcp_session(server, 0);
> + spin_lock(&cifs_tcp_ses_lock);
> + }
> +
> +skip_addr_details:

Then you can get rid of this goto as well.

> seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
> server->credits,
> server->echo_credits,
> @@ -515,7 +573,18 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
> seq_printf(m, "\n\n\tExtra Channels: %zu ",
> ses->chan_count-1);
> for (j = 1; j < ses->chan_count; j++) {
> + /*
> + * kernel_getsockname can block inside
> + * cifs_dump_channel. so drop the lock first
> + */
> + server->srv_count++;
> + spin_unlock(&cifs_tcp_ses_lock);
> +
> cifs_dump_channel(m, j, &ses->chans[j]);
> +
> + cifs_put_tcp_session(server, 0);
> + spin_lock(&cifs_tcp_ses_lock);

Here you are re-acquiring @cifs_tcp_ses_lock spinlock under
@ses->chan_lock, which will introduce deadlocks in threads calling
cifs_match_super(), cifs_signal_cifsd_for_reconnect(),
cifs_mark_tcp_ses_conns_for_reconnect(), cifs_find_smb_ses(), ...