| Message ID | 20230609174659.60327-4-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] cifs: fix status checks in cifs_tree_connect | expand |
Hi Shyam, On 06/09, Shyam Prasad N wrote: >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 | 46 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > >diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c >index 17c884724590..d5fd3681f56e 100644 >--- a/fs/smb/client/cifs_debug.c >+++ b/fs/smb/client/cifs_debug.c >@@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/proc_fs.h> > #include <linux/uaccess.h> >+#include <net/inet_sock.h> > #include "cifspdu.h" > #include "cifsglob.h" > #include "cifsproto.h" >@@ -146,6 +147,30 @@ 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)); >+ >+#ifndef CONFIG_CIFS_SMB_DIRECT >+ if (server->ssocket) { >+ if (server->dstaddr.ss_family == AF_INET6) { >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; >+ struct sock *sk = server->ssocket->sk; >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); >+ seq_printf(m, "\n\t\tIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", >+ &ipv6->sin6_addr, >+ ntohs(ipv6->sin6_port), >+ &sk->sk_v6_rcv_saddr.s6_addr32, >+ ntohs(inet->inet_sport)); >+ } else { >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); >+ seq_printf(m, "\n\t\tIPv4 Dest: %pI4:%d Src: %pI4:%d", >+ &ipv4->sin_addr, >+ ntohs(ipv4->sin_port), >+ &inet->inet_saddr, >+ ntohs(inet->inet_sport)); >+ } >+ } >+#endif >+ > } > > static void >@@ -351,6 +376,27 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) > atomic_read(&server->smbd_conn->mr_ready_count), > atomic_read(&server->smbd_conn->mr_used_count)); > skip_rdma: >+#else >+ if (server->ssocket) { >+ if (server->dstaddr.ss_family == AF_INET6) { >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; >+ struct sock *sk = server->ssocket->sk; >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); >+ seq_printf(m, "\nIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", >+ &ipv6->sin6_addr, >+ ntohs(ipv6->sin6_port), >+ &sk->sk_v6_rcv_saddr.s6_addr32, >+ ntohs(inet->inet_sport)); >+ } else { >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); >+ seq_printf(m, "\nIPv4 Dest: %pI4:%d Src: %pI4:%d", >+ &ipv4->sin_addr, >+ ntohs(ipv4->sin_port), >+ &inet->inet_saddr, >+ ntohs(inet->inet_sport)); >+ } >+ } > #endif > seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", > server->credits, You could save a lot of lines by using the generic formats for IP addresses (Documentation/printk-formats.txt, look for "IPv4/IPv6 addresses (generic, with port, flowinfo, scope)"). e.g. using %pISpc will give you: 1.2.3.4:12345 for IPv4 or [1:2:3:4:5:6:7:8]:12345 for IPv6, just by passing &server->dstaddr (without any casts), and you don't have to check address family every time as well. And to properly get the source IP being used by the socket there's kernel_getpeername(). e.g.: { ... struct sockaddr src; int addrlen; addrlen = kernel_getpeername(server->ssocket, &src); if (addrlen != sizeof(struct sockaddr_in) && addrlen != sizeof(struct sockaddr_in6)) continue; // skip or "return addrlen < 0 ? addrlen : -EAFNOSUPPORT;" ... seq_print(m, "IP: src=%pISpc, dest=%pISpc", &server->dstaddr, &src); ... } Cheers, Enzo
On Fri, Jun 9, 2023 at 11:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > Hi Shyam, > > On 06/09, Shyam Prasad N wrote: > >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 | 46 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > >diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c > >index 17c884724590..d5fd3681f56e 100644 > >--- a/fs/smb/client/cifs_debug.c > >+++ b/fs/smb/client/cifs_debug.c > >@@ -12,6 +12,7 @@ > > #include <linux/module.h> > > #include <linux/proc_fs.h> > > #include <linux/uaccess.h> > >+#include <net/inet_sock.h> > > #include "cifspdu.h" > > #include "cifsglob.h" > > #include "cifsproto.h" > >@@ -146,6 +147,30 @@ 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)); > >+ > >+#ifndef CONFIG_CIFS_SMB_DIRECT > >+ if (server->ssocket) { > >+ if (server->dstaddr.ss_family == AF_INET6) { > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > >+ struct sock *sk = server->ssocket->sk; > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > >+ seq_printf(m, "\n\t\tIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > >+ &ipv6->sin6_addr, > >+ ntohs(ipv6->sin6_port), > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > >+ ntohs(inet->inet_sport)); > >+ } else { > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > >+ seq_printf(m, "\n\t\tIPv4 Dest: %pI4:%d Src: %pI4:%d", > >+ &ipv4->sin_addr, > >+ ntohs(ipv4->sin_port), > >+ &inet->inet_saddr, > >+ ntohs(inet->inet_sport)); > >+ } > >+ } > >+#endif > >+ > > } > > > > static void > >@@ -351,6 +376,27 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) > > atomic_read(&server->smbd_conn->mr_ready_count), > > atomic_read(&server->smbd_conn->mr_used_count)); > > skip_rdma: > >+#else > >+ if (server->ssocket) { > >+ if (server->dstaddr.ss_family == AF_INET6) { > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > >+ struct sock *sk = server->ssocket->sk; > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > >+ seq_printf(m, "\nIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > >+ &ipv6->sin6_addr, > >+ ntohs(ipv6->sin6_port), > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > >+ ntohs(inet->inet_sport)); > >+ } else { > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > >+ seq_printf(m, "\nIPv4 Dest: %pI4:%d Src: %pI4:%d", > >+ &ipv4->sin_addr, > >+ ntohs(ipv4->sin_port), > >+ &inet->inet_saddr, > >+ ntohs(inet->inet_sport)); > >+ } > >+ } > > #endif > > seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", > > server->credits, > > You could save a lot of lines by using the generic formats for IP > addresses (Documentation/printk-formats.txt, look for "IPv4/IPv6 > addresses (generic, with port, flowinfo, scope)"). > > e.g. using %pISpc will give you: > 1.2.3.4:12345 for IPv4 or [1:2:3:4:5:6:7:8]:12345 for IPv6, just by > passing &server->dstaddr (without any casts), and you don't have to > check address family every time as well. > > And to properly get the source IP being used by the socket there's > kernel_getpeername(). > > e.g.: > { > ... > struct sockaddr src; > int addrlen; > > addrlen = kernel_getpeername(server->ssocket, &src); > if (addrlen != sizeof(struct sockaddr_in) && addrlen != sizeof(struct sockaddr_in6)) > continue; // skip or "return addrlen < 0 ? addrlen : -EAFNOSUPPORT;" > ... > seq_print(m, "IP: src=%pISpc, dest=%pISpc", &server->dstaddr, &src); > ... > } > > > Cheers, > > Enzo Hi Enzo, Thanks for the review. Very good points. Let me test out with these changes.
On Sun, Jun 11, 2023 at 1:32 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Fri, Jun 9, 2023 at 11:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > > Hi Shyam, > > > > On 06/09, Shyam Prasad N wrote: > > >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 | 46 ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > > > >diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c > > >index 17c884724590..d5fd3681f56e 100644 > > >--- a/fs/smb/client/cifs_debug.c > > >+++ b/fs/smb/client/cifs_debug.c > > >@@ -12,6 +12,7 @@ > > > #include <linux/module.h> > > > #include <linux/proc_fs.h> > > > #include <linux/uaccess.h> > > >+#include <net/inet_sock.h> > > > #include "cifspdu.h" > > > #include "cifsglob.h" > > > #include "cifsproto.h" > > >@@ -146,6 +147,30 @@ 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)); > > >+ > > >+#ifndef CONFIG_CIFS_SMB_DIRECT > > >+ if (server->ssocket) { > > >+ if (server->dstaddr.ss_family == AF_INET6) { > > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > > >+ struct sock *sk = server->ssocket->sk; > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > >+ seq_printf(m, "\n\t\tIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > > >+ &ipv6->sin6_addr, > > >+ ntohs(ipv6->sin6_port), > > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > > >+ ntohs(inet->inet_sport)); > > >+ } else { > > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > >+ seq_printf(m, "\n\t\tIPv4 Dest: %pI4:%d Src: %pI4:%d", > > >+ &ipv4->sin_addr, > > >+ ntohs(ipv4->sin_port), > > >+ &inet->inet_saddr, > > >+ ntohs(inet->inet_sport)); > > >+ } > > >+ } > > >+#endif > > >+ > > > } > > > > > > static void > > >@@ -351,6 +376,27 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) > > > atomic_read(&server->smbd_conn->mr_ready_count), > > > atomic_read(&server->smbd_conn->mr_used_count)); > > > skip_rdma: > > >+#else > > >+ if (server->ssocket) { > > >+ if (server->dstaddr.ss_family == AF_INET6) { > > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > > >+ struct sock *sk = server->ssocket->sk; > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > >+ seq_printf(m, "\nIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > > >+ &ipv6->sin6_addr, > > >+ ntohs(ipv6->sin6_port), > > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > > >+ ntohs(inet->inet_sport)); > > >+ } else { > > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > >+ seq_printf(m, "\nIPv4 Dest: %pI4:%d Src: %pI4:%d", > > >+ &ipv4->sin_addr, > > >+ ntohs(ipv4->sin_port), > > >+ &inet->inet_saddr, > > >+ ntohs(inet->inet_sport)); > > >+ } > > >+ } > > > #endif > > > seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", > > > server->credits, > > > > You could save a lot of lines by using the generic formats for IP > > addresses (Documentation/printk-formats.txt, look for "IPv4/IPv6 > > addresses (generic, with port, flowinfo, scope)"). > > > > e.g. using %pISpc will give you: > > 1.2.3.4:12345 for IPv4 or [1:2:3:4:5:6:7:8]:12345 for IPv6, just by > > passing &server->dstaddr (without any casts), and you don't have to > > check address family every time as well. > > > > And to properly get the source IP being used by the socket there's > > kernel_getpeername(). > > > > e.g.: > > { > > ... > > struct sockaddr src; > > int addrlen; > > > > addrlen = kernel_getpeername(server->ssocket, &src); > > if (addrlen != sizeof(struct sockaddr_in) && addrlen != sizeof(struct sockaddr_in6)) > > continue; // skip or "return addrlen < 0 ? addrlen : -EAFNOSUPPORT;" > > ... > > seq_print(m, "IP: src=%pISpc, dest=%pISpc", &server->dstaddr, &src); > > ... > > } > > > > > > Cheers, > > > > Enzo > > Hi Enzo, > > Thanks for the review. Very good points. > Let me test out with these changes. > > -- > Regards, > Shyam Hi Enzo, Attached the updated patch. Please review. I had to use kernel_getsockname to get socket source details, not kernel_getpeername. Here's what the new output looks like: IP addr: dst: 192.168.10.1:445, src: 192.168.10.12:57966
On Mon, Jun 12, 2023 at 1:29 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Sun, Jun 11, 2023 at 1:32 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Fri, Jun 9, 2023 at 11:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > > > > Hi Shyam, > > > > > > On 06/09, Shyam Prasad N wrote: > > > >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 | 46 ++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 46 insertions(+) > > > > > > > >diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c > > > >index 17c884724590..d5fd3681f56e 100644 > > > >--- a/fs/smb/client/cifs_debug.c > > > >+++ b/fs/smb/client/cifs_debug.c > > > >@@ -12,6 +12,7 @@ > > > > #include <linux/module.h> > > > > #include <linux/proc_fs.h> > > > > #include <linux/uaccess.h> > > > >+#include <net/inet_sock.h> > > > > #include "cifspdu.h" > > > > #include "cifsglob.h" > > > > #include "cifsproto.h" > > > >@@ -146,6 +147,30 @@ 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)); > > > >+ > > > >+#ifndef CONFIG_CIFS_SMB_DIRECT > > > >+ if (server->ssocket) { > > > >+ if (server->dstaddr.ss_family == AF_INET6) { > > > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > > > >+ struct sock *sk = server->ssocket->sk; > > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > > >+ seq_printf(m, "\n\t\tIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > > > >+ &ipv6->sin6_addr, > > > >+ ntohs(ipv6->sin6_port), > > > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > > > >+ ntohs(inet->inet_sport)); > > > >+ } else { > > > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > > >+ seq_printf(m, "\n\t\tIPv4 Dest: %pI4:%d Src: %pI4:%d", > > > >+ &ipv4->sin_addr, > > > >+ ntohs(ipv4->sin_port), > > > >+ &inet->inet_saddr, > > > >+ ntohs(inet->inet_sport)); > > > >+ } > > > >+ } > > > >+#endif > > > >+ > > > > } > > > > > > > > static void > > > >@@ -351,6 +376,27 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) > > > > atomic_read(&server->smbd_conn->mr_ready_count), > > > > atomic_read(&server->smbd_conn->mr_used_count)); > > > > skip_rdma: > > > >+#else > > > >+ if (server->ssocket) { > > > >+ if (server->dstaddr.ss_family == AF_INET6) { > > > >+ struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; > > > >+ struct sock *sk = server->ssocket->sk; > > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > > >+ seq_printf(m, "\nIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", > > > >+ &ipv6->sin6_addr, > > > >+ ntohs(ipv6->sin6_port), > > > >+ &sk->sk_v6_rcv_saddr.s6_addr32, > > > >+ ntohs(inet->inet_sport)); > > > >+ } else { > > > >+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; > > > >+ struct inet_sock *inet = inet_sk(server->ssocket->sk); > > > >+ seq_printf(m, "\nIPv4 Dest: %pI4:%d Src: %pI4:%d", > > > >+ &ipv4->sin_addr, > > > >+ ntohs(ipv4->sin_port), > > > >+ &inet->inet_saddr, > > > >+ ntohs(inet->inet_sport)); > > > >+ } > > > >+ } > > > > #endif > > > > seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", > > > > server->credits, > > > > > > You could save a lot of lines by using the generic formats for IP > > > addresses (Documentation/printk-formats.txt, look for "IPv4/IPv6 > > > addresses (generic, with port, flowinfo, scope)"). > > > > > > e.g. using %pISpc will give you: > > > 1.2.3.4:12345 for IPv4 or [1:2:3:4:5:6:7:8]:12345 for IPv6, just by > > > passing &server->dstaddr (without any casts), and you don't have to > > > check address family every time as well. > > > > > > And to properly get the source IP being used by the socket there's > > > kernel_getpeername(). > > > > > > e.g.: > > > { > > > ... > > > struct sockaddr src; > > > int addrlen; > > > > > > addrlen = kernel_getpeername(server->ssocket, &src); > > > if (addrlen != sizeof(struct sockaddr_in) && addrlen != sizeof(struct sockaddr_in6)) > > > continue; // skip or "return addrlen < 0 ? addrlen : -EAFNOSUPPORT;" > > > ... > > > seq_print(m, "IP: src=%pISpc, dest=%pISpc", &server->dstaddr, &src); > > > ... > > > } > > > > > > > > > Cheers, > > > > > > Enzo > > > > Hi Enzo, > > > > Thanks for the review. Very good points. > > Let me test out with these changes. > > > > -- > > Regards, > > Shyam > > Hi Enzo, > > Attached the updated patch. Please review. > I had to use kernel_getsockname to get socket source details, not > kernel_getpeername. > > Here's what the new output looks like: > IP addr: dst: 192.168.10.1:445, src: 192.168.10.12:57966 > > -- > Regards, > Shyam Attached the patch now. :)
Hi Shyam, On 06/12, Shyam Prasad N wrote: ... snip ... >Hi Enzo, > >Attached the updated patch. Please review. >I had to use kernel_getsockname to get socket source details, not >kernel_getpeername. > >Here's what the new output looks like: >IP addr: dst: 192.168.10.1:445, src: 192.168.10.12:57966 Yeah, I noticed I mixed src/dst when writing the email (was dealing with exactly this when I wrote it). >> > seq_print(m, "IP: src=%pISpc, dest=%pISpc", &server->dstaddr, &src); ^ is also mixed up. Sorry for the confusion.
Hi Shyam, On 06/12, Shyam Prasad N wrote: ... >Attached the patch now. :) (inlining the attached patch here) > From f7b46503129ac12cd3c1240ac58d7acb050e56ae Mon Sep 17 00:00:00 2001 > From: Shyam Prasad N <sprasad@microsoft.com> > Date: Fri, 9 Jun 2023 12:46:34 +0000 > Subject: [PATCH 4/6] cifs: display the endpoint IP details in DebugData > > 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 | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c > index 17c884724590..a8f7b4c49ae3 100644 > --- a/fs/smb/client/cifs_debug.c > +++ b/fs/smb/client/cifs_debug.c > @@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/proc_fs.h> > #include <linux/uaccess.h> > +#include <net/inet_sock.h> > #include "cifspdu.h" > #include "cifsglob.h" > #include "cifsproto.h" > @@ -146,6 +147,22 @@ 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)); > + > +#ifndef CONFIG_CIFS_SMB_DIRECT > + 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)) > + goto skip_addr_details; > + > + seq_printf(m, "\n\t\tIP addr: dst: %pISpc, src: %pISpc", &server->dstaddr, &src); > + } > + > +skip_addr_details: (nitpicking now) I'd just check if addrlen is == sizeof(struct sockadr_{in,in6}) and seq_printf() if true, then remove the skip_addr_details goto/label. > +#endif > + > } > > static void > @@ -351,6 +368,19 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) > atomic_read(&server->smbd_conn->mr_ready_count), > atomic_read(&server->smbd_conn->mr_used_count)); > skip_rdma: > +#else > + 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)) > + goto skip_addr_details; > + > + seq_printf(m, "\nIP addr: dst: %pISpc, src: %pISpc", &server->dstaddr, &src); > + } > + > +skip_addr_details: Ditto. > #endif > seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", > server->credits, > -- > 2.34.1 But it looks much cleaner now. Thanks. Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> Cheers
Shyam Prasad N <nspmangalore@gmail.com> writes: > I had to use kernel_getsockname to get socket source details, not > kernel_getpeername. Why can't you use @server->srcaddr directly?
On 06/12, Paulo Alcantara wrote: >Shyam Prasad N <nspmangalore@gmail.com> writes: > >> I had to use kernel_getsockname to get socket source details, not >> kernel_getpeername. > >Why can't you use @server->srcaddr directly? That's only filled if explicitly passed through srcaddr= mount option (to bind it later in bind_socket()). Otherwise, it stays zeroed through @server's lifetime.
On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > On 06/12, Paulo Alcantara wrote: > >Shyam Prasad N <nspmangalore@gmail.com> writes: > > > >> I had to use kernel_getsockname to get socket source details, not > >> kernel_getpeername. > > > >Why can't you use @server->srcaddr directly? > > That's only filled if explicitly passed through srcaddr= mount option > (to bind it later in bind_socket()). > > Otherwise, it stays zeroed through @server's lifetime. Yes. As Enzo mentioned, srcaddr is only useful if the user gave that mount option. Also, here's an updated version of the patch. kernel_getsockname seems to be a blocking function, and we need to drop the spinlock before calling it. -- Regards, Shyam
Tentatively merged into cifs-2.6.git for-next pending testing On Thu, Jun 22, 2023 at 11:22 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > > On 06/12, Paulo Alcantara wrote: > > >Shyam Prasad N <nspmangalore@gmail.com> writes: > > > > > >> I had to use kernel_getsockname to get socket source details, not > > >> kernel_getpeername. > > > > > >Why can't you use @server->srcaddr directly? > > > > That's only filled if explicitly passed through srcaddr= mount option > > (to bind it later in bind_socket()). > > > > Otherwise, it stays zeroed through @server's lifetime. > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > mount option. > > Also, here's an updated version of the patch. > kernel_getsockname seems to be a blocking function, and we need to > drop the spinlock before calling it. > > -- > Regards, > Shyam
On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: >> >> On 06/12, Paulo Alcantara wrote: >>> Shyam Prasad N <nspmangalore@gmail.com> writes: >>> >>>> I had to use kernel_getsockname to get socket source details, not >>>> kernel_getpeername. >>> >>> Why can't you use @server->srcaddr directly? >> >> That's only filled if explicitly passed through srcaddr= mount option >> (to bind it later in bind_socket()). >> >> Otherwise, it stays zeroed through @server's lifetime. > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > mount option. > > Also, here's an updated version of the patch. > kernel_getsockname seems to be a blocking function, and we need to > drop the spinlock before calling it. Why does this not do anything to report RDMA endpoint addresses/ports? Many RDMA protocols employ IP addressing. If it's intended to not report such information, there should be some string emitted to make it clear that this is TCP-specific. But let's not be lazy here, the smbd_connection stores the rdma_cm_id which holds similar information (the "rdma_addr"). Tom.
On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > >> > >> On 06/12, Paulo Alcantara wrote: > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > >>> > >>>> I had to use kernel_getsockname to get socket source details, not > >>>> kernel_getpeername. > >>> > >>> Why can't you use @server->srcaddr directly? > >> > >> That's only filled if explicitly passed through srcaddr= mount option > >> (to bind it later in bind_socket()). > >> > >> Otherwise, it stays zeroed through @server's lifetime. > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > mount option. > > > > Also, here's an updated version of the patch. > > kernel_getsockname seems to be a blocking function, and we need to > > drop the spinlock before calling it. > > Why does this not do anything to report RDMA endpoint addresses/ports? > Many RDMA protocols employ IP addressing. > > If it's intended to not report such information, there should be some > string emitted to make it clear that this is TCP-specific. But let's > not be lazy here, the smbd_connection stores the rdma_cm_id which > holds similar information (the "rdma_addr"). > > Tom. Hi Tom, As always, thanks for reviewing. My understanding of RDMA is very limited. That's the only reason why my patch did not contain changes for RDMA. Let me dig around to understand what needs to be done here.
On Tue, Jun 27, 2023 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > >> > > >> On 06/12, Paulo Alcantara wrote: > > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > > >>> > > >>>> I had to use kernel_getsockname to get socket source details, not > > >>>> kernel_getpeername. > > >>> > > >>> Why can't you use @server->srcaddr directly? > > >> > > >> That's only filled if explicitly passed through srcaddr= mount option > > >> (to bind it later in bind_socket()). > > >> > > >> Otherwise, it stays zeroed through @server's lifetime. > > > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > > mount option. > > > > > > Also, here's an updated version of the patch. > > > kernel_getsockname seems to be a blocking function, and we need to > > > drop the spinlock before calling it. > > > > Why does this not do anything to report RDMA endpoint addresses/ports? > > Many RDMA protocols employ IP addressing. > > > > If it's intended to not report such information, there should be some > > string emitted to make it clear that this is TCP-specific. But let's > > not be lazy here, the smbd_connection stores the rdma_cm_id which > > holds similar information (the "rdma_addr"). > > > > Tom. > > Hi Tom, > > As always, thanks for reviewing. > My understanding of RDMA is very limited. That's the only reason why > my patch did not contain changes for RDMA. > Let me dig around to understand what needs to be done here. > > -- > Regards, > Shyam Here's a version of the patch with changes for RDMA as well. But I don't know how to test this, as I do not have a setup with RDMA. @Tom Talpey Can you please review and test out this patch?
On 6/28/2023 6:20 AM, Shyam Prasad N wrote: > Here's a version of the patch with changes for RDMA as well. > But I don't know how to test this, as I do not have a setup with RDMA. > @Tom Talpey Can you please review and test out this patch? I'm happy to test but it will take me a day to find the time. Would you like a how-to recipe for setting up software RDMA between two Linux machines? It's quite easy, and can be done over RoCEv1 (rxe) and/or softiWARP (siw). Tom.
> Would you like a how-to recipe for setting up software RDMA between two Linux machines? It's quite easy, Yes! And we need to update our buildbot and local automation scripts as well. On Wed, Jun 28, 2023 at 8:39 AM Tom Talpey <tom@talpey.com> wrote: > > On 6/28/2023 6:20 AM, Shyam Prasad N wrote: > > Here's a version of the patch with changes for RDMA as well. > > But I don't know how to test this, as I do not have a setup with RDMA. > > @Tom Talpey Can you please review and test out this patch? > > I'm happy to test but it will take me a day to find the time. > > Would you like a how-to recipe for setting up software RDMA between > two Linux machines? It's quite easy, and can be done over RoCEv1 (rxe) > and/or softiWARP (siw). > > Tom.
tentatively merged updated patch in cifs-2.6.git for-next pending additional testing/review On Wed, Jun 28, 2023 at 5:20 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Tue, Jun 27, 2023 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > > > > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > >> > > > >> On 06/12, Paulo Alcantara wrote: > > > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > > > >>> > > > >>>> I had to use kernel_getsockname to get socket source details, not > > > >>>> kernel_getpeername. > > > >>> > > > >>> Why can't you use @server->srcaddr directly? > > > >> > > > >> That's only filled if explicitly passed through srcaddr= mount option > > > >> (to bind it later in bind_socket()). > > > >> > > > >> Otherwise, it stays zeroed through @server's lifetime. > > > > > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > > > mount option. > > > > > > > > Also, here's an updated version of the patch. > > > > kernel_getsockname seems to be a blocking function, and we need to > > > > drop the spinlock before calling it. > > > > > > Why does this not do anything to report RDMA endpoint addresses/ports? > > > Many RDMA protocols employ IP addressing. > > > > > > If it's intended to not report such information, there should be some > > > string emitted to make it clear that this is TCP-specific. But let's > > > not be lazy here, the smbd_connection stores the rdma_cm_id which > > > holds similar information (the "rdma_addr"). > > > > > > Tom. > > > > Hi Tom, > > > > As always, thanks for reviewing. > > My understanding of RDMA is very limited. That's the only reason why > > my patch did not contain changes for RDMA. > > Let me dig around to understand what needs to be done here. > > > > -- > > Regards, > > Shyam > > Here's a version of the patch with changes for RDMA as well. > But I don't know how to test this, as I do not have a setup with RDMA. > @Tom Talpey Can you please review and test out this patch? > > -- > Regards, > Shyam
corrected a missing unlock (near line 451 of cifs_debug.c) and tentatively put in cifs-2.6.git for-next On Wed, Jun 28, 2023 at 5:20 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Tue, Jun 27, 2023 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > > > > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > >> > > > >> On 06/12, Paulo Alcantara wrote: > > > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > > > >>> > > > >>>> I had to use kernel_getsockname to get socket source details, not > > > >>>> kernel_getpeername. > > > >>> > > > >>> Why can't you use @server->srcaddr directly? > > > >> > > > >> That's only filled if explicitly passed through srcaddr= mount option > > > >> (to bind it later in bind_socket()). > > > >> > > > >> Otherwise, it stays zeroed through @server's lifetime. > > > > > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > > > mount option. > > > > > > > > Also, here's an updated version of the patch. > > > > kernel_getsockname seems to be a blocking function, and we need to > > > > drop the spinlock before calling it. > > > > > > Why does this not do anything to report RDMA endpoint addresses/ports? > > > Many RDMA protocols employ IP addressing. > > > > > > If it's intended to not report such information, there should be some > > > string emitted to make it clear that this is TCP-specific. But let's > > > not be lazy here, the smbd_connection stores the rdma_cm_id which > > > holds similar information (the "rdma_addr"). > > > > > > Tom. > > > > Hi Tom, > > > > As always, thanks for reviewing. > > My understanding of RDMA is very limited. That's the only reason why > > my patch did not contain changes for RDMA. > > Let me dig around to understand what needs to be done here. > > > > -- > > Regards, > > Shyam > > Here's a version of the patch with changes for RDMA as well. > But I don't know how to test this, as I do not have a setup with RDMA. > @Tom Talpey Can you please review and test out this patch? > > -- > Regards, > Shyam
also removed an extra colon in two places (IP addr dst: ..." then "src:" already had the colon so didn't need "IP addr: dst: ..." On Wed, Jun 28, 2023 at 5:20 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Tue, Jun 27, 2023 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > > > > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > >> > > > >> On 06/12, Paulo Alcantara wrote: > > > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > > > >>> > > > >>>> I had to use kernel_getsockname to get socket source details, not > > > >>>> kernel_getpeername. > > > >>> > > > >>> Why can't you use @server->srcaddr directly? > > > >> > > > >> That's only filled if explicitly passed through srcaddr= mount option > > > >> (to bind it later in bind_socket()). > > > >> > > > >> Otherwise, it stays zeroed through @server's lifetime. > > > > > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > > > mount option. > > > > > > > > Also, here's an updated version of the patch. > > > > kernel_getsockname seems to be a blocking function, and we need to > > > > drop the spinlock before calling it. > > > > > > Why does this not do anything to report RDMA endpoint addresses/ports? > > > Many RDMA protocols employ IP addressing. > > > > > > If it's intended to not report such information, there should be some > > > string emitted to make it clear that this is TCP-specific. But let's > > > not be lazy here, the smbd_connection stores the rdma_cm_id which > > > holds similar information (the "rdma_addr"). > > > > > > Tom. > > > > Hi Tom, > > > > As always, thanks for reviewing. > > My understanding of RDMA is very limited. That's the only reason why > > my patch did not contain changes for RDMA. > > Let me dig around to understand what needs to be done here. > > > > -- > > Regards, > > Shyam > > Here's a version of the patch with changes for RDMA as well. > But I don't know how to test this, as I do not have a setup with RDMA. > @Tom Talpey Can you please review and test out this patch? > > -- > Regards, > Shyam
On Wed, Jun 28, 2023 at 10:41 PM Steve French <smfrench@gmail.com> wrote: > > also removed an extra colon in two places (IP addr dst: ..." then > "src:" already had the colon so didn't need "IP addr: dst: ..." > > On Wed, Jun 28, 2023 at 5:20 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Tue, Jun 27, 2023 at 5:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > On Fri, Jun 23, 2023 at 9:24 PM Tom Talpey <tom@talpey.com> wrote: > > > > > > > > On 6/23/2023 12:21 AM, Shyam Prasad N wrote: > > > > > On Mon, Jun 12, 2023 at 8:59 PM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > > > >> > > > > >> On 06/12, Paulo Alcantara wrote: > > > > >>> Shyam Prasad N <nspmangalore@gmail.com> writes: > > > > >>> > > > > >>>> I had to use kernel_getsockname to get socket source details, not > > > > >>>> kernel_getpeername. > > > > >>> > > > > >>> Why can't you use @server->srcaddr directly? > > > > >> > > > > >> That's only filled if explicitly passed through srcaddr= mount option > > > > >> (to bind it later in bind_socket()). > > > > >> > > > > >> Otherwise, it stays zeroed through @server's lifetime. > > > > > > > > > > Yes. As Enzo mentioned, srcaddr is only useful if the user gave that > > > > > mount option. > > > > > > > > > > Also, here's an updated version of the patch. > > > > > kernel_getsockname seems to be a blocking function, and we need to > > > > > drop the spinlock before calling it. > > > > > > > > Why does this not do anything to report RDMA endpoint addresses/ports? > > > > Many RDMA protocols employ IP addressing. > > > > > > > > If it's intended to not report such information, there should be some > > > > string emitted to make it clear that this is TCP-specific. But let's > > > > not be lazy here, the smbd_connection stores the rdma_cm_id which > > > > holds similar information (the "rdma_addr"). > > > > > > > > Tom. > > > > > > Hi Tom, > > > > > > As always, thanks for reviewing. > > > My understanding of RDMA is very limited. That's the only reason why > > > my patch did not contain changes for RDMA. > > > Let me dig around to understand what needs to be done here. > > > > > > -- > > > Regards, > > > Shyam > > > > Here's a version of the patch with changes for RDMA as well. > > But I don't know how to test this, as I do not have a setup with RDMA. > > @Tom Talpey Can you please review and test out this patch? > > > > -- > > Regards, > > Shyam > > > > -- > Thanks, > > Steve Here's the updated patch with the correct fixes to the problems suggested by the bot. And also a correction in refcount drop.
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c index 17c884724590..d5fd3681f56e 100644 --- a/fs/smb/client/cifs_debug.c +++ b/fs/smb/client/cifs_debug.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/proc_fs.h> #include <linux/uaccess.h> +#include <net/inet_sock.h> #include "cifspdu.h" #include "cifsglob.h" #include "cifsproto.h" @@ -146,6 +147,30 @@ 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)); + +#ifndef CONFIG_CIFS_SMB_DIRECT + if (server->ssocket) { + if (server->dstaddr.ss_family == AF_INET6) { + struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; + struct sock *sk = server->ssocket->sk; + struct inet_sock *inet = inet_sk(server->ssocket->sk); + seq_printf(m, "\n\t\tIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", + &ipv6->sin6_addr, + ntohs(ipv6->sin6_port), + &sk->sk_v6_rcv_saddr.s6_addr32, + ntohs(inet->inet_sport)); + } else { + struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; + struct inet_sock *inet = inet_sk(server->ssocket->sk); + seq_printf(m, "\n\t\tIPv4 Dest: %pI4:%d Src: %pI4:%d", + &ipv4->sin_addr, + ntohs(ipv4->sin_port), + &inet->inet_saddr, + ntohs(inet->inet_sport)); + } + } +#endif + } static void @@ -351,6 +376,27 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v) atomic_read(&server->smbd_conn->mr_ready_count), atomic_read(&server->smbd_conn->mr_used_count)); skip_rdma: +#else + if (server->ssocket) { + if (server->dstaddr.ss_family == AF_INET6) { + struct sockaddr_in6 *ipv6 = (struct sockaddr_in6 *)&server->dstaddr; + struct sock *sk = server->ssocket->sk; + struct inet_sock *inet = inet_sk(server->ssocket->sk); + seq_printf(m, "\nIPv6 Dest: [%pI6]:%d Src: [%pI6]:%d", + &ipv6->sin6_addr, + ntohs(ipv6->sin6_port), + &sk->sk_v6_rcv_saddr.s6_addr32, + ntohs(inet->inet_sport)); + } else { + struct sockaddr_in *ipv4 = (struct sockaddr_in *)&server->dstaddr; + struct inet_sock *inet = inet_sk(server->ssocket->sk); + seq_printf(m, "\nIPv4 Dest: %pI4:%d Src: %pI4:%d", + &ipv4->sin_addr, + ntohs(ipv4->sin_port), + &inet->inet_saddr, + ntohs(inet->inet_sport)); + } + } #endif seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x", server->credits,
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 | 46 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)