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

[Paulo Alcantara <pc@manguebit.com>]

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(), ...