[PATCH 1/6] cifs: fix status checks in cifs_tree_connect

[PATCH 1/6] cifs: fix status checks in cifs_tree_connect

[Shyam Prasad N]

The ordering of status checks at the beginning of
cifs_tree_connect is wrong. As a result, a tcon
which is good may stay marked as needing reconnect
infinitely.

Fixes: 2f0e4f034220 ("cifs: check only tcon status on tcon related functions")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/connect.c | 9 +++++----
 fs/smb/client/dfs.c     | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 8e9a672320ab..1250d156619b 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4086,16 +4086,17 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
 
 	/* only send once per connect */
 	spin_lock(&tcon->tc_lock);
+	if (tcon->status == TID_GOOD) {
+		spin_unlock(&tcon->tc_lock);
+		return 0;
+	}
+
 	if (tcon->status != TID_NEW &&
 	    tcon->status != TID_NEED_TCON) {
 		spin_unlock(&tcon->tc_lock);
 		return -EHOSTDOWN;
 	}
 
-	if (tcon->status == TID_GOOD) {
-		spin_unlock(&tcon->tc_lock);
-		return 0;
-	}
 	tcon->status = TID_IN_TCON;
 	spin_unlock(&tcon->tc_lock);
 
diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index 2f93bf8c3325..2390b2fedd6a 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -575,16 +575,17 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
 
 	/* only send once per connect */
 	spin_lock(&tcon->tc_lock);
+	if (tcon->status == TID_GOOD) {
+		spin_unlock(&tcon->tc_lock);
+		return 0;
+	}
+
 	if (tcon->status != TID_NEW &&
 	    tcon->status != TID_NEED_TCON) {
 		spin_unlock(&tcon->tc_lock);
 		return -EHOSTDOWN;
 	}
 
-	if (tcon->status == TID_GOOD) {
-		spin_unlock(&tcon->tc_lock);
-		return 0;
-	}
 	tcon->status = TID_IN_TCON;
 	spin_unlock(&tcon->tc_lock);
 
-- 
2.34.1

[PATCH 2/6] cifs: print all credit counters in DebugData

[Shyam Prasad N]

Output of /proc/fs/cifs/DebugData shows only the per-connection
counter for the number of credits of regular type. i.e. the
credits reserved for echo and oplocks are not displayed.

There have been situations recently where having this info
would have been useful. This change prints the credit counters
of all three types: regular, echo, oplocks.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cifs_debug.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 5034b862cec2..17c884724590 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -130,12 +130,14 @@ cifs_dump_channel(struct seq_file *m, int i, struct cifs_chan *chan)
 	struct TCP_Server_Info *server = chan->server;
 
 	seq_printf(m, "\n\n\t\tChannel: %d ConnectionId: 0x%llx"
-		   "\n\t\tNumber of credits: %d Dialect 0x%x"
+		   "\n\t\tNumber of credits: %d,%d,%d Dialect 0x%x"
 		   "\n\t\tTCP status: %d Instance: %d"
 		   "\n\t\tLocal Users To Server: %d SecMode: 0x%x Req On Wire: %d"
 		   "\n\t\tIn Send: %d In MaxReq Wait: %d",
 		   i+1, server->conn_id,
 		   server->credits,
+		   server->echo_credits,
+		   server->oplock_credits,
 		   server->dialect,
 		   server->tcpStatus,
 		   server->reconnect_instance,
@@ -350,8 +352,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 			atomic_read(&server->smbd_conn->mr_used_count));
 skip_rdma:
 #endif
-		seq_printf(m, "\nNumber of credits: %d Dialect 0x%x",
-			server->credits,  server->dialect);
+		seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
+			server->credits,
+			server->echo_credits,
+			server->oplock_credits,
+			server->dialect);
 		if (server->compress_algorithm == SMB3_COMPRESS_LZNT1)
 			seq_printf(m, " COMPRESS_LZNT1");
 		else if (server->compress_algorithm == SMB3_COMPRESS_LZ77)
-- 
2.34.1

Re: [PATCH 2/6] cifs: print all credit counters in DebugData

[Steve French]

tentatively merged into cifs-2.6.git for-next pending additional testing

On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Output of /proc/fs/cifs/DebugData shows only the per-connection
> counter for the number of credits of regular type. i.e. the
> credits reserved for echo and oplocks are not displayed.
>
> There have been situations recently where having this info
> would have been useful. This change prints the credit counters
> of all three types: regular, echo, oplocks.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cifs_debug.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> index 5034b862cec2..17c884724590 100644
> --- a/fs/smb/client/cifs_debug.c
> +++ b/fs/smb/client/cifs_debug.c
> @@ -130,12 +130,14 @@ cifs_dump_channel(struct seq_file *m, int i, struct cifs_chan *chan)
>         struct TCP_Server_Info *server = chan->server;
>
>         seq_printf(m, "\n\n\t\tChannel: %d ConnectionId: 0x%llx"
> -                  "\n\t\tNumber of credits: %d Dialect 0x%x"
> +                  "\n\t\tNumber of credits: %d,%d,%d Dialect 0x%x"
>                    "\n\t\tTCP status: %d Instance: %d"
>                    "\n\t\tLocal Users To Server: %d SecMode: 0x%x Req On Wire: %d"
>                    "\n\t\tIn Send: %d In MaxReq Wait: %d",
>                    i+1, server->conn_id,
>                    server->credits,
> +                  server->echo_credits,
> +                  server->oplock_credits,
>                    server->dialect,
>                    server->tcpStatus,
>                    server->reconnect_instance,
> @@ -350,8 +352,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
>                         atomic_read(&server->smbd_conn->mr_used_count));
>  skip_rdma:
>  #endif
> -               seq_printf(m, "\nNumber of credits: %d Dialect 0x%x",
> -                       server->credits,  server->dialect);
> +               seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
> +                       server->credits,
> +                       server->echo_credits,
> +                       server->oplock_credits,
> +                       server->dialect);
>                 if (server->compress_algorithm == SMB3_COMPRESS_LZNT1)
>                         seq_printf(m, " COMPRESS_LZNT1");
>                 else if (server->compress_algorithm == SMB3_COMPRESS_LZ77)
> --
> 2.34.1
>


-- 
Thanks,

Steve

[PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Shyam Prasad N]

We've seen the in-flight count go into negative with some
internal stress testing in Microsoft.

Adding a WARN when this happens, in hope of understanding
why this happens when it happens.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2ops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 6e3be58cfe49..43162915e03c 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
 					    server->conn_id, server->hostname, *val,
 					    add, server->in_flight);
 	}
+	WARN_ON(server->in_flight == 0);
 	server->in_flight--;
 	if (server->in_flight == 0 &&
 	   ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
-- 
2.34.1

Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Steve French]

should this be a warn once? Could it get very noisy?

On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> We've seen the in-flight count go into negative with some
> internal stress testing in Microsoft.
>
> Adding a WARN when this happens, in hope of understanding
> why this happens when it happens.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/smb2ops.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 6e3be58cfe49..43162915e03c 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
>                                             server->conn_id, server->hostname, *val,
>                                             add, server->in_flight);
>         }
> +       WARN_ON(server->in_flight == 0);
>         server->in_flight--;
>         if (server->in_flight == 0 &&
>            ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
> --
> 2.34.1
>


-- 
Thanks,

Steve

Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Shyam Prasad N]

On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote:
>
> should this be a warn once? Could it get very noisy?
>
> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > We've seen the in-flight count go into negative with some
> > internal stress testing in Microsoft.
> >
> > Adding a WARN when this happens, in hope of understanding
> > why this happens when it happens.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/smb2ops.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 6e3be58cfe49..43162915e03c 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
> >                                             server->conn_id, server->hostname, *val,
> >                                             add, server->in_flight);
> >         }
> > +       WARN_ON(server->in_flight == 0);
> >         server->in_flight--;
> >         if (server->in_flight == 0 &&
> >            ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
> > --
> > 2.34.1
> >
>
>
> --
> Thanks,
>
> Steve

Makes sense. We can have a warn once.

-- 
Regards,
Shyam

Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Tom Talpey]

On 6/11/2023 4:01 AM, Shyam Prasad N wrote:
> On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote:
>>
>> should this be a warn once? Could it get very noisy?
>>
>> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>>
>>> We've seen the in-flight count go into negative with some
>>> internal stress testing in Microsoft.
>>>
>>> Adding a WARN when this happens, in hope of understanding
>>> why this happens when it happens.
>>>
>>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
>>> ---
>>>   fs/smb/client/smb2ops.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>>> index 6e3be58cfe49..43162915e03c 100644
>>> --- a/fs/smb/client/smb2ops.c
>>> +++ b/fs/smb/client/smb2ops.c
>>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
>>>                                              server->conn_id, server->hostname, *val,
>>>                                              add, server->in_flight);
>>>          }
>>> +       WARN_ON(server->in_flight == 0);
>>>          server->in_flight--;
>>>          if (server->in_flight == 0 &&
>>>             ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
>>> --
>>> 2.34.1
>>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
> 
> Makes sense. We can have a warn once.

Which sounds great, but isn't this connection basically toast?
It's not super helpful to just whine. Why not clamp it at zero?

Tom.

Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Shyam Prasad N]

On Fri, Jun 23, 2023 at 9:52 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/11/2023 4:01 AM, Shyam Prasad N wrote:
> > On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote:
> >>
> >> should this be a warn once? Could it get very noisy?
> >>
> >> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >>>
> >>> We've seen the in-flight count go into negative with some
> >>> internal stress testing in Microsoft.
> >>>
> >>> Adding a WARN when this happens, in hope of understanding
> >>> why this happens when it happens.
> >>>
> >>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> >>> ---
> >>>   fs/smb/client/smb2ops.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> >>> index 6e3be58cfe49..43162915e03c 100644
> >>> --- a/fs/smb/client/smb2ops.c
> >>> +++ b/fs/smb/client/smb2ops.c
> >>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
> >>>                                              server->conn_id, server->hostname, *val,
> >>>                                              add, server->in_flight);
> >>>          }
> >>> +       WARN_ON(server->in_flight == 0);
> >>>          server->in_flight--;
> >>>          if (server->in_flight == 0 &&
> >>>             ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
> >>> --
> >>> 2.34.1
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >>
> >> Steve
> >
> > Makes sense. We can have a warn once.
>
> Which sounds great, but isn't this connection basically toast?
> It's not super helpful to just whine. Why not clamp it at zero?
>
> Tom.

So there's no "legal" way that this count can go negative.
If it has, that's definitely because there's a bug. The WARN will
hopefully help us catch and fix the bug.
We could also have a clamp at 0. I'll send an updated patch.

-- 
Regards,
Shyam

Re: [PATCH 3/6] cifs: add a warning when the in-flight count goes negative

[Tom Talpey]

On 6/26/2023 2:33 AM, Shyam Prasad N wrote:
> On Fri, Jun 23, 2023 at 9:52 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> On 6/11/2023 4:01 AM, Shyam Prasad N wrote:
>>> On Sun, Jun 11, 2023 at 1:19 AM Steve French <smfrench@gmail.com> wrote:
>>>>
>>>> should this be a warn once? Could it get very noisy?
>>>>
>>>> On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>>>>
>>>>> We've seen the in-flight count go into negative with some
>>>>> internal stress testing in Microsoft.
>>>>>
>>>>> Adding a WARN when this happens, in hope of understanding
>>>>> why this happens when it happens.
>>>>>
>>>>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
>>>>> ---
>>>>>    fs/smb/client/smb2ops.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>>>>> index 6e3be58cfe49..43162915e03c 100644
>>>>> --- a/fs/smb/client/smb2ops.c
>>>>> +++ b/fs/smb/client/smb2ops.c
>>>>> @@ -91,6 +91,7 @@ smb2_add_credits(struct TCP_Server_Info *server,
>>>>>                                               server->conn_id, server->hostname, *val,
>>>>>                                               add, server->in_flight);
>>>>>           }
>>>>> +       WARN_ON(server->in_flight == 0);
>>>>>           server->in_flight--;
>>>>>           if (server->in_flight == 0 &&
>>>>>              ((optype & CIFS_OP_MASK) != CIFS_NEG_OP) &&
>>>>> --
>>>>> 2.34.1
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>>
>>>> Steve
>>>
>>> Makes sense. We can have a warn once.
>>
>> Which sounds great, but isn't this connection basically toast?
>> It's not super helpful to just whine. Why not clamp it at zero?
>>
>> Tom.
> 
> So there's no "legal" way that this count can go negative.
> If it has, that's definitely because there's a bug. The WARN will
> hopefully help us catch and fix the bug.

To be clear, I'm ok with the warn, it's the fact that the code
goes to all the trouble to say it but doesn't do anything to
recover.

> We could also have a clamp at 0. I'll send an updated patch.

Sounds good.

Tom.

[PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

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,
-- 
2.34.1

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Enzo Matsumiya]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 6427 bytes --]

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. :)

-- 
Regards,
Shyam

[-- Attachment #2: 0004-cifs-display-the-endpoint-IP-details-in-DebugData.patch --]
[-- Type: application/octet-stream, Size: 2212 bytes --]

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:
+#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:
 #endif
 		seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
 			server->credits,
-- 
2.34.1

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Enzo Matsumiya]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Enzo Matsumiya]

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.

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Paulo Alcantara]

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?

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Enzo Matsumiya]

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.

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

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

[-- Attachment #2: 0001-cifs-display-the-endpoint-IP-details-in-DebugData.patch --]
[-- Type: application/octet-stream, Size: 3877 bytes --]

From c9db5074c409264b6892ff92f2549e6138e589a5 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] 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 | 52 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index b279f745466e..079c1df09172 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"
@@ -147,6 +148,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:
+#endif
+
 }
 
 static inline const char *smb_speed_to_str(size_t bps)
@@ -263,7 +280,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;
@@ -318,7 +335,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 (CIFS_SERVER_IS_CHAN(server))
 			continue;
@@ -396,6 +413,26 @@ 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;
+
+			/* 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))
+				goto skip_addr_details;
+
+			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:
 #endif
 		seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
 			server->credits,
@@ -493,7 +530,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);
+
 					if (CIFS_CHAN_NEEDS_RECONNECT(ses, j))
 						seq_puts(m, "\tDISCONNECTED ");
 					if (CIFS_CHAN_IN_RECONNECT(ses, j))
-- 
2.34.1

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Steve French]

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



-- 
Thanks,

Steve

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Tom Talpey]

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.

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

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

[-- Attachment #2: 0001-cifs-display-the-endpoint-IP-details-in-DebugData.patch --]
[-- Type: application/octet-stream, Size: 4396 bytes --]

From a835cda06926885387309828dbcf5def4c990a5d 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] 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 | 68 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index bfa8950547e2..9b46ebe818b2 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"
@@ -147,6 +148,32 @@ 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_CIFS_SMB_DIRECT
+	if (!server->rdma)
+		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:
+#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))
+			return;
+
+		seq_printf(m, "\n\t\tIP addr: dst: %pISpc, src: %pISpc", &server->dstaddr, &src);
+	}
+#endif
+
 }
 
 static inline const char *smb_speed_to_str(size_t bps)
@@ -263,7 +290,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;
@@ -318,7 +345,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 (CIFS_SERVER_IS_CHAN(server))
 			continue;
@@ -396,7 +423,33 @@ 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:
+#else
+		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))
+				goto skip_addr_details;
+
+			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:
 #endif
 		seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
 			server->credits,
@@ -494,7 +547,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);
+
 					if (CIFS_CHAN_NEEDS_RECONNECT(ses, j))
 						seq_puts(m, "\tDISCONNECTED ");
 					if (CIFS_CHAN_IN_RECONNECT(ses, j))
-- 
2.34.1

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Tom Talpey]

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.

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Steve French]

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



-- 
Thanks,

Steve

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Steve French]

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



-- 
Thanks,

Steve

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Steve French]

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



-- 
Thanks,

Steve

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Steve French]

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

Re: [PATCH 4/6] cifs: display the endpoint IP details in DebugData

[Shyam Prasad N]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

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.

-- 
Regards,
Shyam

[-- Attachment #2: 0001-cifs-display-the-endpoint-IP-details-in-DebugData.patch --]
[-- Type: application/octet-stream, Size: 4522 bytes --]

From 25bbbde7f0eab7dcbdb284494a593ce1158a3011 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] 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 | 74 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index bfa8950547e2..e7baca410988 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"
@@ -147,6 +148,33 @@ 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_CIFS_SMB_DIRECT
+	if (!server->rdma)
+		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
+	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)
@@ -263,7 +291,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;
@@ -318,7 +346,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 (CIFS_SERVER_IS_CHAN(server))
 			continue;
@@ -396,8 +424,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;
+			}
+
+			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:
 		seq_printf(m, "\nNumber of credits: %d,%d,%d Dialect 0x%x",
 			server->credits,
 			server->echo_credits,
@@ -494,7 +553,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);
+
 					if (CIFS_CHAN_NEEDS_RECONNECT(ses, j))
 						seq_puts(m, "\tDISCONNECTED ");
 					if (CIFS_CHAN_IN_RECONNECT(ses, j))
-- 
2.34.1

[PATCH 5/6] cifs: fix max_credits implementation

[Shyam Prasad N]

The current implementation of max_credits on the client does
not work because the CreditRequest logic for several commands
does not take max_credits into account.

Still, we can end up asking the server for more credits, depending
on the number of credits in flight. For this, we need to
limit the credits while parsing the responses too.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2ops.c |  2 ++
 fs/smb/client/smb2pdu.c | 32 ++++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 43162915e03c..18faf267c54d 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -34,6 +34,8 @@ static int
 change_conf(struct TCP_Server_Info *server)
 {
 	server->credits += server->echo_credits + server->oplock_credits;
+	if (server->credits > server->max_credits)
+		server->credits = server->max_credits;
 	server->oplock_credits = server->echo_credits = 0;
 	switch (server->credits) {
 	case 0:
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 7063b395d22f..17fe212ab895 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -1305,7 +1305,12 @@ SMB2_sess_alloc_buffer(struct SMB2_sess_data *sess_data)
 	}
 
 	/* enough to enable echos and oplocks and one max size write */
-	req->hdr.CreditRequest = cpu_to_le16(130);
+	if (server->credits >= server->max_credits)
+		req->hdr.CreditRequest = cpu_to_le16(0);
+	else
+		req->hdr.CreditRequest = cpu_to_le16(
+			min_t(int, server->max_credits -
+			      server->credits, 130));
 
 	/* only one of SMB2 signing flags may be set in SMB2 request */
 	if (server->sign)
@@ -1899,7 +1904,12 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
 	rqst.rq_nvec = 2;
 
 	/* Need 64 for max size write so ask for more in case not there yet */
-	req->hdr.CreditRequest = cpu_to_le16(64);
+	if (server->credits >= server->max_credits)
+		req->hdr.CreditRequest = cpu_to_le16(0);
+	else
+		req->hdr.CreditRequest = cpu_to_le16(
+			min_t(int, server->max_credits -
+			      server->credits, 64));
 
 	rc = cifs_send_recv(xid, ses, server,
 			    &rqst, &resp_buftype, flags, &rsp_iov);
@@ -4227,6 +4237,7 @@ smb2_async_readv(struct cifs_readdata *rdata)
 	struct TCP_Server_Info *server;
 	struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
 	unsigned int total_len;
+	int credit_request;
 
 	cifs_dbg(FYI, "%s: offset=%llu bytes=%u\n",
 		 __func__, rdata->offset, rdata->bytes);
@@ -4258,7 +4269,13 @@ smb2_async_readv(struct cifs_readdata *rdata)
 	if (rdata->credits.value > 0) {
 		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->bytes,
 						SMB2_MAX_BUFFER_SIZE));
-		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
+		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
+		if (server->credits >= server->max_credits)
+			shdr->CreditRequest = cpu_to_le16(0);
+		else
+			shdr->CreditRequest = cpu_to_le16(
+				min_t(int, server->max_credits -
+						server->credits, credit_request));
 
 		rc = adjust_credits(server, &rdata->credits, rdata->bytes);
 		if (rc)
@@ -4468,6 +4485,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
 	unsigned int total_len;
 	struct cifs_io_parms _io_parms;
 	struct cifs_io_parms *io_parms = NULL;
+	int credit_request;
 
 	if (!wdata->server)
 		server = wdata->server = cifs_pick_channel(tcon->ses);
@@ -4572,7 +4590,13 @@ smb2_async_writev(struct cifs_writedata *wdata,
 	if (wdata->credits.value > 0) {
 		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(wdata->bytes,
 						    SMB2_MAX_BUFFER_SIZE));
-		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
+		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
+		if (server->credits >= server->max_credits)
+			shdr->CreditRequest = cpu_to_le16(0);
+		else
+			shdr->CreditRequest = cpu_to_le16(
+				min_t(int, server->max_credits -
+						server->credits, credit_request));
 
 		rc = adjust_credits(server, &wdata->credits, io_parms->length);
 		if (rc)
-- 
2.34.1

Re: [PATCH 5/6] cifs: fix max_credits implementation

[Tom Talpey]

On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> The current implementation of max_credits on the client does
> not work because the CreditRequest logic for several commands
> does not take max_credits into account.
> 
> Still, we can end up asking the server for more credits, depending
> on the number of credits in flight. For this, we need to
> limit the credits while parsing the responses too.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>   fs/smb/client/smb2ops.c |  2 ++
>   fs/smb/client/smb2pdu.c | 32 ++++++++++++++++++++++++++++----
>   2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 43162915e03c..18faf267c54d 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -34,6 +34,8 @@ static int
>   change_conf(struct TCP_Server_Info *server)
>   {
>   	server->credits += server->echo_credits + server->oplock_credits;
> +	if (server->credits > server->max_credits)
> +		server->credits = server->max_credits;
>   	server->oplock_credits = server->echo_credits = 0;
>   	switch (server->credits) {
>   	case 0:
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index 7063b395d22f..17fe212ab895 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -1305,7 +1305,12 @@ SMB2_sess_alloc_buffer(struct SMB2_sess_data *sess_data)
>   	}
>   
>   	/* enough to enable echos and oplocks and one max size write */
> -	req->hdr.CreditRequest = cpu_to_le16(130);
> +	if (server->credits >= server->max_credits)
> +		req->hdr.CreditRequest = cpu_to_le16(0);
> +	else
> +		req->hdr.CreditRequest = cpu_to_le16(
> +			min_t(int, server->max_credits -
> +			      server->credits, 130));

This identical processing appears several times below. It would be
very bad if the copies got out of sync. Let's factor these as a
single function, perhaps inline but it might make sense as a client
common entry.

Tom.


>   	/* only one of SMB2 signing flags may be set in SMB2 request */
>   	if (server->sign)
> @@ -1899,7 +1904,12 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>   	rqst.rq_nvec = 2;
>   
>   	/* Need 64 for max size write so ask for more in case not there yet */
> -	req->hdr.CreditRequest = cpu_to_le16(64);
> +	if (server->credits >= server->max_credits)
> +		req->hdr.CreditRequest = cpu_to_le16(0);
> +	else
> +		req->hdr.CreditRequest = cpu_to_le16(
> +			min_t(int, server->max_credits -
> +			      server->credits, 64));
>   
>   	rc = cifs_send_recv(xid, ses, server,
>   			    &rqst, &resp_buftype, flags, &rsp_iov);
> @@ -4227,6 +4237,7 @@ smb2_async_readv(struct cifs_readdata *rdata)
>   	struct TCP_Server_Info *server;
>   	struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
>   	unsigned int total_len;
> +	int credit_request;
>   
>   	cifs_dbg(FYI, "%s: offset=%llu bytes=%u\n",
>   		 __func__, rdata->offset, rdata->bytes);
> @@ -4258,7 +4269,13 @@ smb2_async_readv(struct cifs_readdata *rdata)
>   	if (rdata->credits.value > 0) {
>   		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->bytes,
>   						SMB2_MAX_BUFFER_SIZE));
> -		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> +		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> +		if (server->credits >= server->max_credits)
> +			shdr->CreditRequest = cpu_to_le16(0);
> +		else
> +			shdr->CreditRequest = cpu_to_le16(
> +				min_t(int, server->max_credits -
> +						server->credits, credit_request));
>   
>   		rc = adjust_credits(server, &rdata->credits, rdata->bytes);
>   		if (rc)
> @@ -4468,6 +4485,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
>   	unsigned int total_len;
>   	struct cifs_io_parms _io_parms;
>   	struct cifs_io_parms *io_parms = NULL;
> +	int credit_request;
>   
>   	if (!wdata->server)
>   		server = wdata->server = cifs_pick_channel(tcon->ses);
> @@ -4572,7 +4590,13 @@ smb2_async_writev(struct cifs_writedata *wdata,
>   	if (wdata->credits.value > 0) {
>   		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(wdata->bytes,
>   						    SMB2_MAX_BUFFER_SIZE));
> -		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> +		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> +		if (server->credits >= server->max_credits)
> +			shdr->CreditRequest = cpu_to_le16(0);
> +		else
> +			shdr->CreditRequest = cpu_to_le16(
> +				min_t(int, server->max_credits -
> +						server->credits, credit_request));
>   
>   		rc = adjust_credits(server, &wdata->credits, io_parms->length);
>   		if (rc)

Re: [PATCH 5/6] cifs: fix max_credits implementation

[Shyam Prasad N]

On Fri, Jun 23, 2023 at 9:30 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> > The current implementation of max_credits on the client does
> > not work because the CreditRequest logic for several commands
> > does not take max_credits into account.
> >
> > Still, we can end up asking the server for more credits, depending
> > on the number of credits in flight. For this, we need to
> > limit the credits while parsing the responses too.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >   fs/smb/client/smb2ops.c |  2 ++
> >   fs/smb/client/smb2pdu.c | 32 ++++++++++++++++++++++++++++----
> >   2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 43162915e03c..18faf267c54d 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -34,6 +34,8 @@ static int
> >   change_conf(struct TCP_Server_Info *server)
> >   {
> >       server->credits += server->echo_credits + server->oplock_credits;
> > +     if (server->credits > server->max_credits)
> > +             server->credits = server->max_credits;
> >       server->oplock_credits = server->echo_credits = 0;
> >       switch (server->credits) {
> >       case 0:
> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > index 7063b395d22f..17fe212ab895 100644
> > --- a/fs/smb/client/smb2pdu.c
> > +++ b/fs/smb/client/smb2pdu.c
> > @@ -1305,7 +1305,12 @@ SMB2_sess_alloc_buffer(struct SMB2_sess_data *sess_data)
> >       }
> >
> >       /* enough to enable echos and oplocks and one max size write */
> > -     req->hdr.CreditRequest = cpu_to_le16(130);
> > +     if (server->credits >= server->max_credits)
> > +             req->hdr.CreditRequest = cpu_to_le16(0);
> > +     else
> > +             req->hdr.CreditRequest = cpu_to_le16(
> > +                     min_t(int, server->max_credits -
> > +                           server->credits, 130));
>
> This identical processing appears several times below. It would be
> very bad if the copies got out of sync. Let's factor these as a
> single function, perhaps inline but it might make sense as a client
> common entry.
>
> Tom.
>

Thanks for the review, Tom.
I'll prepare an updated patch.

>
> >       /* only one of SMB2 signing flags may be set in SMB2 request */
> >       if (server->sign)
> > @@ -1899,7 +1904,12 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
> >       rqst.rq_nvec = 2;
> >
> >       /* Need 64 for max size write so ask for more in case not there yet */
> > -     req->hdr.CreditRequest = cpu_to_le16(64);
> > +     if (server->credits >= server->max_credits)
> > +             req->hdr.CreditRequest = cpu_to_le16(0);
> > +     else
> > +             req->hdr.CreditRequest = cpu_to_le16(
> > +                     min_t(int, server->max_credits -
> > +                           server->credits, 64));
> >
> >       rc = cifs_send_recv(xid, ses, server,
> >                           &rqst, &resp_buftype, flags, &rsp_iov);
> > @@ -4227,6 +4237,7 @@ smb2_async_readv(struct cifs_readdata *rdata)
> >       struct TCP_Server_Info *server;
> >       struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
> >       unsigned int total_len;
> > +     int credit_request;
> >
> >       cifs_dbg(FYI, "%s: offset=%llu bytes=%u\n",
> >                __func__, rdata->offset, rdata->bytes);
> > @@ -4258,7 +4269,13 @@ smb2_async_readv(struct cifs_readdata *rdata)
> >       if (rdata->credits.value > 0) {
> >               shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->bytes,
> >                                               SMB2_MAX_BUFFER_SIZE));
> > -             shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> > +             credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> > +             if (server->credits >= server->max_credits)
> > +                     shdr->CreditRequest = cpu_to_le16(0);
> > +             else
> > +                     shdr->CreditRequest = cpu_to_le16(
> > +                             min_t(int, server->max_credits -
> > +                                             server->credits, credit_request));
> >
> >               rc = adjust_credits(server, &rdata->credits, rdata->bytes);
> >               if (rc)
> > @@ -4468,6 +4485,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
> >       unsigned int total_len;
> >       struct cifs_io_parms _io_parms;
> >       struct cifs_io_parms *io_parms = NULL;
> > +     int credit_request;
> >
> >       if (!wdata->server)
> >               server = wdata->server = cifs_pick_channel(tcon->ses);
> > @@ -4572,7 +4590,13 @@ smb2_async_writev(struct cifs_writedata *wdata,
> >       if (wdata->credits.value > 0) {
> >               shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(wdata->bytes,
> >                                                   SMB2_MAX_BUFFER_SIZE));
> > -             shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> > +             credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> > +             if (server->credits >= server->max_credits)
> > +                     shdr->CreditRequest = cpu_to_le16(0);
> > +             else
> > +                     shdr->CreditRequest = cpu_to_le16(
> > +                             min_t(int, server->max_credits -
> > +                                             server->credits, credit_request));
> >
> >               rc = adjust_credits(server, &wdata->credits, io_parms->length);
> >               if (rc)



-- 
Regards,
Shyam

[PATCH 6/6] cifs: fix sockaddr comparison in iface_cmp

[Shyam Prasad N]

iface_cmp used to simply do a memcmp of the two
provided struct sockaddrs. The comparison needs to do more
based on the address family. Similar logic was already
present in cifs_match_ipaddr. Doing something similar now.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
---
 fs/smb/client/cifsglob.h  | 37 -----------------------------
 fs/smb/client/cifsproto.h |  1 +
 fs/smb/client/connect.c   | 50 +++++++++++++++++++++++++++++++++++++++
 fs/smb/client/smb2ops.c   | 37 +++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 0d84bb1a8cd9..b212a4e16b39 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -970,43 +970,6 @@ release_iface(struct kref *ref)
 	kfree(iface);
 }
 
-/*
- * compare two interfaces a and b
- * return 0 if everything matches.
- * return 1 if a has higher link speed, or rdma capable, or rss capable
- * return -1 otherwise.
- */
-static inline int
-iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
-{
-	int cmp_ret = 0;
-
-	WARN_ON(!a || !b);
-	if (a->speed == b->speed) {
-		if (a->rdma_capable == b->rdma_capable) {
-			if (a->rss_capable == b->rss_capable) {
-				cmp_ret = memcmp(&a->sockaddr, &b->sockaddr,
-						 sizeof(a->sockaddr));
-				if (!cmp_ret)
-					return 0;
-				else if (cmp_ret > 0)
-					return 1;
-				else
-					return -1;
-			} else if (a->rss_capable > b->rss_capable)
-				return 1;
-			else
-				return -1;
-		} else if (a->rdma_capable > b->rdma_capable)
-			return 1;
-		else
-			return -1;
-	} else if (a->speed > b->speed)
-		return 1;
-	else
-		return -1;
-}
-
 struct cifs_chan {
 	unsigned int in_reconnect : 1; /* if session setup in progress for this channel */
 	struct TCP_Server_Info *server;
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index c1c704990b98..d127aded2f28 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -87,6 +87,7 @@ extern int cifs_handle_standard(struct TCP_Server_Info *server,
 				struct mid_q_entry *mid);
 extern int smb3_parse_devname(const char *devname, struct smb3_fs_context *ctx);
 extern int smb3_parse_opt(const char *options, const char *key, char **val);
+extern int cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs);
 extern bool cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs);
 extern int cifs_discard_remaining_data(struct TCP_Server_Info *server);
 extern int cifs_call_async(struct TCP_Server_Info *server,
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 1250d156619b..9d16626e7a66 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p)
 	module_put_and_kthread_exit(0);
 }
 
+int
+cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)
+{
+	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
+	struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
+	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
+	struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs;
+
+	switch (srcaddr->sa_family) {
+	case AF_UNSPEC:
+		switch (rhs->sa_family) {
+		case AF_UNSPEC:
+			return 0;
+		case AF_INET:
+		case AF_INET6:
+			return 1;
+		default:
+			return -1;
+		}
+	case AF_INET: {
+		switch (rhs->sa_family) {
+		case AF_UNSPEC:
+			return -1;
+		case AF_INET:
+			return memcmp(saddr4, vaddr4,
+				      sizeof(struct sockaddr_in));
+		case AF_INET6:
+			return 1;
+		default:
+			return -1;
+		}
+	}
+	case AF_INET6: {
+		switch (rhs->sa_family) {
+		case AF_UNSPEC:
+		case AF_INET:
+			return -1;
+		case AF_INET6:
+			return memcmp(saddr6,
+				      vaddr6,
+				      sizeof(struct sockaddr_in6));
+		default:
+			return -1;
+		}
+	}
+	default:
+		return -1; /* don't expect to be here */
+	}
+}
+
 /*
  * Returns true if srcaddr isn't specified and rhs isn't specified, or
  * if srcaddr is specified and matches the IP address of the rhs argument
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 18faf267c54d..046341115add 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -513,6 +513,43 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx)
 	return rsize;
 }
 
+/*
+ * compare two interfaces a and b
+ * return 0 if everything matches.
+ * return 1 if a is rdma capable, or rss capable, or has higher link speed
+ * return -1 otherwise.
+ */
+static int
+iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
+{
+	int cmp_ret = 0;
+
+	WARN_ON(!a || !b);
+	if (a->rdma_capable == b->rdma_capable) {
+		if (a->rss_capable == b->rss_capable) {
+			if (a->speed == b->speed) {
+				cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
+							  (struct sockaddr *) &b->sockaddr);
+				if (!cmp_ret)
+					return 0;
+				else if (cmp_ret > 0)
+					return 1;
+				else
+					return -1;
+			} else if (a->speed > b->speed)
+				return 1;
+			else
+				return -1;
+		} else if (a->rss_capable > b->rss_capable)
+			return 1;
+		else
+			return -1;
+	} else if (a->rdma_capable > b->rdma_capable)
+		return 1;
+	else
+		return -1;
+}
+
 static int
 parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 			size_t buf_len, struct cifs_ses *ses, bool in_mount)
-- 
2.34.1

Re: [PATCH 6/6] cifs: fix sockaddr comparison in iface_cmp

[Tom Talpey]

On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> iface_cmp used to simply do a memcmp of the two
> provided struct sockaddrs. The comparison needs to do more
> based on the address family. Similar logic was already
> present in cifs_match_ipaddr. Doing something similar now.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> ---
>   fs/smb/client/cifsglob.h  | 37 -----------------------------
>   fs/smb/client/cifsproto.h |  1 +
>   fs/smb/client/connect.c   | 50 +++++++++++++++++++++++++++++++++++++++
>   fs/smb/client/smb2ops.c   | 37 +++++++++++++++++++++++++++++
>   4 files changed, 88 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 0d84bb1a8cd9..b212a4e16b39 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -970,43 +970,6 @@ release_iface(struct kref *ref)
>   	kfree(iface);
>   }
>   
> -/*
> - * compare two interfaces a and b
> - * return 0 if everything matches.
> - * return 1 if a has higher link speed, or rdma capable, or rss capable
> - * return -1 otherwise.
> - */
> -static inline int
> -iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
> -{
> -	int cmp_ret = 0;
> -
> -	WARN_ON(!a || !b);
> -	if (a->speed == b->speed) {
> -		if (a->rdma_capable == b->rdma_capable) {
> -			if (a->rss_capable == b->rss_capable) {
> -				cmp_ret = memcmp(&a->sockaddr, &b->sockaddr,
> -						 sizeof(a->sockaddr));
> -				if (!cmp_ret)
> -					return 0;
> -				else if (cmp_ret > 0)
> -					return 1;
> -				else
> -					return -1;
> -			} else if (a->rss_capable > b->rss_capable)
> -				return 1;
> -			else
> -				return -1;
> -		} else if (a->rdma_capable > b->rdma_capable)
> -			return 1;
> -		else
> -			return -1;
> -	} else if (a->speed > b->speed)
> -		return 1;
> -	else
> -		return -1;
> -}
> -
>   struct cifs_chan {
>   	unsigned int in_reconnect : 1; /* if session setup in progress for this channel */
>   	struct TCP_Server_Info *server;
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index c1c704990b98..d127aded2f28 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -87,6 +87,7 @@ extern int cifs_handle_standard(struct TCP_Server_Info *server,
>   				struct mid_q_entry *mid);
>   extern int smb3_parse_devname(const char *devname, struct smb3_fs_context *ctx);
>   extern int smb3_parse_opt(const char *options, const char *key, char **val);
> +extern int cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs);
>   extern bool cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs);
>   extern int cifs_discard_remaining_data(struct TCP_Server_Info *server);
>   extern int cifs_call_async(struct TCP_Server_Info *server,
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 1250d156619b..9d16626e7a66 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p)
>   	module_put_and_kthread_exit(0);
>   }
>   
> +int
> +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)

Please, please, please, let's not add a new shared entry starting with
this four-letter word.

> +{
> +	struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
> +	struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> +	struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
> +	struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs;

I don't see the value in these variables. Why not simply add the casts
in the very few places they're used?

> +
> +	switch (srcaddr->sa_family) {
> +	case AF_UNSPEC:
> +		switch (rhs->sa_family) {
> +		case AF_UNSPEC:
> +			return 0;
> +		case AF_INET:
> +		case AF_INET6:
> +			return 1;
> +		default:
> +			return -1;
> +		}
> +	case AF_INET: {
> +		switch (rhs->sa_family) {
> +		case AF_UNSPEC:
> +			return -1;
> +		case AF_INET:
> +			return memcmp(saddr4, vaddr4,
> +				      sizeof(struct sockaddr_in));
> +		case AF_INET6:
> +			return 1;
> +		default:
> +			return -1;
> +		}
> +	}
> +	case AF_INET6: {
> +		switch (rhs->sa_family) {
> +		case AF_UNSPEC:
> +		case AF_INET:
> +			return -1;
> +		case AF_INET6:
> +			return memcmp(saddr6,
> +				      vaddr6,
> +				      sizeof(struct sockaddr_in6));
> +		default:
> +			return -1;
> +		}
> +	}
> +	default:
> +		return -1; /* don't expect to be here */
> +	}
> +}
> +
>   /*
>    * Returns true if srcaddr isn't specified and rhs isn't specified, or
>    * if srcaddr is specified and matches the IP address of the rhs argument
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 18faf267c54d..046341115add 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -513,6 +513,43 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx)
>   	return rsize;
>   }
>   
> +/*
> + * compare two interfaces a and b
> + * return 0 if everything matches.
> + * return 1 if a is rdma capable, or rss capable, or has higher link speed
> + * return -1 otherwise.
> + */
> +static int
> +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
> +{
> +	int cmp_ret = 0;
> +
> +	WARN_ON(!a || !b);
> +	if (a->rdma_capable == b->rdma_capable) {
> +		if (a->rss_capable == b->rss_capable) {
> +			if (a->speed == b->speed) {
> +				cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
> +							  (struct sockaddr *) &b->sockaddr);
> +				if (!cmp_ret)
> +					return 0;
> +				else if (cmp_ret > 0)
> +					return 1;
> +				else
> +					return -1;
> +			} else if (a->speed > b->speed)
> +				return 1;
> +			else
> +				return -1;
> +		} else if (a->rss_capable > b->rss_capable)
> +			return 1;
> +		else
> +			return -1;
> +	} else if (a->rdma_capable > b->rdma_capable)
> +		return 1;
> +	else
> +		return -1;
> +}
> +

The { <0 / 0 / >0 } behavior of this code has been a source of
incorrect comparisons in the past, and it still makes my head hurt
to attempt a review.

So I'll ask, have you thoroughly tested this to be certain that it
doesn't result in new channels being created needlessly?

>   static int
>   parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
>   			size_t buf_len, struct cifs_ses *ses, bool in_mount)

Tom.

Re: [PATCH 6/6] cifs: fix sockaddr comparison in iface_cmp

[Dan Carpenter]

On Fri, Jun 23, 2023 at 12:09:12PM -0400, Tom Talpey wrote:
> On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 1250d156619b..9d16626e7a66 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p)
> >   	module_put_and_kthread_exit(0);
> >   }
> > +int
> > +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)
> 
> Please, please, please, let's not add a new shared entry starting with
> this four-letter word.
> 

What would you suggest instead?

> > +/*
> > + * compare two interfaces a and b
> > + * return 0 if everything matches.
> > + * return 1 if a is rdma capable, or rss capable, or has higher link speed
> > + * return -1 otherwise.
> > + */
> > +static int
> > +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
> > +{
> > +	int cmp_ret = 0;
> > +
> > +	WARN_ON(!a || !b);
> > +	if (a->rdma_capable == b->rdma_capable) {
> > +		if (a->rss_capable == b->rss_capable) {
> > +			if (a->speed == b->speed) {
> > +				cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
> > +							  (struct sockaddr *) &b->sockaddr);
> > +				if (!cmp_ret)
> > +					return 0;
> > +				else if (cmp_ret > 0)
> > +					return 1;
> > +				else
> > +					return -1;
> > +			} else if (a->speed > b->speed)
> > +				return 1;
> > +			else
> > +				return -1;
> > +		} else if (a->rss_capable > b->rss_capable)
> > +			return 1;
> > +		else
> > +			return -1;
> > +	} else if (a->rdma_capable > b->rdma_capable)
> > +		return 1;
> > +	else
> > +		return -1;
> > +}
> > +
> 
> The { <0 / 0 / >0 } behavior of this code has been a source of
> incorrect comparisons in the past, and it still makes my head hurt
> to attempt a review.
> 
> So I'll ask, have you thoroughly tested this to be certain that it
> doesn't result in new channels being created needlessly?

I was not a huge fan of this function and the move makes it harder to
review.  But I didn't see anything wrong with it....  Here is a slightly
simplified diff that I use to review.

regards,
dan carpenter

 iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
 {
        int cmp_ret = 0;
 
        WARN_ON(!a || !b);
-       if (a->speed == b->speed) {
                if (a->rdma_capable == b->rdma_capable) {
                        if (a->rss_capable == b->rss_capable) {
-                               cmp_ret = memcmp(&a->sockaddr, &b->sockaddr,
-                                                sizeof(a->sockaddr));
+                       if (a->speed == b->speed) {
+                               cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
+                                                         (struct sockaddr *) &b->sockaddr);
                                if (!cmp_ret)
                                        return 0;
                                else if (cmp_ret > 0)
                                        return 1;
                                else
                                        return -1;
-                       } else if (a->rss_capable > b->rss_capable)
+                       } else if (a->speed > b->speed)
                                return 1;
                        else
                                return -1;
-               } else if (a->rdma_capable > b->rdma_capable)
+               } else if (a->rss_capable > b->rss_capable)
                        return 1;
                else
                        return -1;
-       } else if (a->speed > b->speed)
+       } else if (a->rdma_capable > b->rdma_capable)
                return 1;
        else
                return -1;
 }

regards,
dan carpenter

Re: [PATCH 6/6] cifs: fix sockaddr comparison in iface_cmp

[Tom Talpey]

On 6/26/2023 7:12 AM, Dan Carpenter wrote:
> On Fri, Jun 23, 2023 at 12:09:12PM -0400, Tom Talpey wrote:
>> On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
>>> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
>>> index 1250d156619b..9d16626e7a66 100644
>>> --- a/fs/smb/client/connect.c
>>> +++ b/fs/smb/client/connect.c
>>> @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p)
>>>    	module_put_and_kthread_exit(0);
>>>    }
>>> +int
>>> +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)
>>
>> Please, please, please, let's not add a new shared entry starting with
>> this four-letter word.
>>
> 
> What would you suggest instead?

"smb" would be fine, but honestly it's only referenced in one
place and used to be static inline, so it kind of doesn't matter
what it's called. It's also unclear to me why it's being moved
to a .c file and made visible.

Anything but "cifs" though.

Tom.

> 
>>> +/*
>>> + * compare two interfaces a and b
>>> + * return 0 if everything matches.
>>> + * return 1 if a is rdma capable, or rss capable, or has higher link speed
>>> + * return -1 otherwise.
>>> + */
>>> +static int
>>> +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
>>> +{
>>> +	int cmp_ret = 0;
>>> +
>>> +	WARN_ON(!a || !b);
>>> +	if (a->rdma_capable == b->rdma_capable) {
>>> +		if (a->rss_capable == b->rss_capable) {
>>> +			if (a->speed == b->speed) {
>>> +				cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
>>> +							  (struct sockaddr *) &b->sockaddr);
>>> +				if (!cmp_ret)
>>> +					return 0;
>>> +				else if (cmp_ret > 0)
>>> +					return 1;
>>> +				else
>>> +					return -1;
>>> +			} else if (a->speed > b->speed)
>>> +				return 1;
>>> +			else
>>> +				return -1;
>>> +		} else if (a->rss_capable > b->rss_capable)
>>> +			return 1;
>>> +		else
>>> +			return -1;
>>> +	} else if (a->rdma_capable > b->rdma_capable)
>>> +		return 1;
>>> +	else
>>> +		return -1;
>>> +}
>>> +
>>
>> The { <0 / 0 / >0 } behavior of this code has been a source of
>> incorrect comparisons in the past, and it still makes my head hurt
>> to attempt a review.
>>
>> So I'll ask, have you thoroughly tested this to be certain that it
>> doesn't result in new channels being created needlessly?
> 
> I was not a huge fan of this function and the move makes it harder to
> review.  But I didn't see anything wrong with it....  Here is a slightly
> simplified diff that I use to review.
> 
> regards,
> dan carpenter
> 
>   iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
>   {
>          int cmp_ret = 0;
>   
>          WARN_ON(!a || !b);
> -       if (a->speed == b->speed) {
>                  if (a->rdma_capable == b->rdma_capable) {
>                          if (a->rss_capable == b->rss_capable) {
> -                               cmp_ret = memcmp(&a->sockaddr, &b->sockaddr,
> -                                                sizeof(a->sockaddr));
> +                       if (a->speed == b->speed) {
> +                               cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
> +                                                         (struct sockaddr *) &b->sockaddr);
>                                  if (!cmp_ret)
>                                          return 0;
>                                  else if (cmp_ret > 0)
>                                          return 1;
>                                  else
>                                          return -1;
> -                       } else if (a->rss_capable > b->rss_capable)
> +                       } else if (a->speed > b->speed)
>                                  return 1;
>                          else
>                                  return -1;
> -               } else if (a->rdma_capable > b->rdma_capable)
> +               } else if (a->rss_capable > b->rss_capable)
>                          return 1;
>                  else
>                          return -1;
> -       } else if (a->speed > b->speed)
> +       } else if (a->rdma_capable > b->rdma_capable)
>                  return 1;
>          else
>                  return -1;
>   }
> 
> regards,
> dan carpenter
> 
> 

Re: [PATCH 1/6] cifs: fix status checks in cifs_tree_connect

[Steve French]

tentatively merged into cifs-2.6.git for-next

On Fri, Jun 9, 2023 at 12:47 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> The ordering of status checks at the beginning of
> cifs_tree_connect is wrong. As a result, a tcon
> which is good may stay marked as needing reconnect
> infinitely.
>
> Fixes: 2f0e4f034220 ("cifs: check only tcon status on tcon related functions")
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/connect.c | 9 +++++----
>  fs/smb/client/dfs.c     | 9 +++++----
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 8e9a672320ab..1250d156619b 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -4086,16 +4086,17 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
>
>         /* only send once per connect */
>         spin_lock(&tcon->tc_lock);
> +       if (tcon->status == TID_GOOD) {
> +               spin_unlock(&tcon->tc_lock);
> +               return 0;
> +       }
> +
>         if (tcon->status != TID_NEW &&
>             tcon->status != TID_NEED_TCON) {
>                 spin_unlock(&tcon->tc_lock);
>                 return -EHOSTDOWN;
>         }
>
> -       if (tcon->status == TID_GOOD) {
> -               spin_unlock(&tcon->tc_lock);
> -               return 0;
> -       }
>         tcon->status = TID_IN_TCON;
>         spin_unlock(&tcon->tc_lock);
>
> diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
> index 2f93bf8c3325..2390b2fedd6a 100644
> --- a/fs/smb/client/dfs.c
> +++ b/fs/smb/client/dfs.c
> @@ -575,16 +575,17 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
>
>         /* only send once per connect */
>         spin_lock(&tcon->tc_lock);
> +       if (tcon->status == TID_GOOD) {
> +               spin_unlock(&tcon->tc_lock);
> +               return 0;
> +       }
> +
>         if (tcon->status != TID_NEW &&
>             tcon->status != TID_NEED_TCON) {
>                 spin_unlock(&tcon->tc_lock);
>                 return -EHOSTDOWN;
>         }
>
> -       if (tcon->status == TID_GOOD) {
> -               spin_unlock(&tcon->tc_lock);
> -               return 0;
> -       }
>         tcon->status = TID_IN_TCON;
>         spin_unlock(&tcon->tc_lock);
>
> --
> 2.34.1
>


-- 
Thanks,

Steve