[v4,1/6] cifs: sort interface list by speed
diff mbox series

Message ID 20191103012112.12212-2-aaptel@suse.com
State New
Headers show
Series
  • add multichannel support
Related show

Commit Message

Aurelien Aptel Nov. 3, 2019, 1:21 a.m. UTC
New channels are going to be opened by walking the list sequentially,
so by sorting it we will connect to the fastest interfaces first.

Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/smb2ops.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Pavel Shilovsky Nov. 25, 2019, 9:29 p.m. UTC | #1
сб, 2 нояб. 2019 г. в 18:21, Aurelien Aptel <aaptel@suse.com>:
>
> New channels are going to be opened by walking the list sequentially,
> so by sorting it we will connect to the fastest interfaces first.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/smb2ops.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index cd55af9b7cc5..ea634581791a 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -10,6 +10,7 @@
>  #include <linux/falloc.h>
>  #include <linux/scatterlist.h>
>  #include <linux/uuid.h>
> +#include <linux/sort.h>
>  #include <crypto/aead.h>
>  #include "cifsglob.h"
>  #include "smb2pdu.h"
> @@ -558,6 +559,13 @@ parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
>         return rc;
>  }
>
> +static int compare_iface(const void *ia, const void *ib)
> +{
> +       const struct cifs_server_iface *a = (struct cifs_server_iface *)ia;
> +       const struct cifs_server_iface *b = (struct cifs_server_iface *)ib;
> +
> +       return a->speed == b->speed ? 0 : (a->speed > b->speed ? -1 : 1);
> +}
>
>  static int
>  SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> @@ -587,6 +595,9 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
>         if (rc)
>                 goto out;
>
> +       /* sort interfaces from fastest to slowest */
> +       sort(iface_list, iface_count, sizeof(*iface_list), compare_iface, NULL);
> +
>         spin_lock(&ses->iface_lock);
>         kfree(ses->iface_list);
>         ses->iface_list = iface_list;
> --
> 2.16.4
>

Looks good at the first glance, thanks!

@Steve, you may add

Acked-by: Pavel Shilovsky <pshilov@microsoft.com>

to this and other patches in the series.

--
Best regards,
Pavel Shilovsky
Tom Talpey Nov. 26, 2019, 4:04 p.m. UTC | #2
> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> Behalf Of Pavel Shilovsky
> Sent: Monday, November 25, 2019 4:29 PM
> To: Aurelien Aptel <aaptel@suse.com>
> Cc: linux-cifs <linux-cifs@vger.kernel.org>; Steve French
> <smfrench@gmail.com>
> Subject: [EXTERNAL] Re: [PATCH v4 1/6] cifs: sort interface list by speed
> 
> сб, 2 нояб. 2019 г. в 18:21, Aurelien Aptel <aaptel@suse.com>:
> >
> > New channels are going to be opened by walking the list sequentially,
> > so by sorting it we will connect to the fastest interfaces first.

Sorting by speed is definitely appropriate, but sorting the other
multichannel attributes is just as important. For example, if the
RDMA attribute is set, the address should actually be excluded
for non-RDMA connections (a second, non-RDMA entry is included,
if the interface supports both). And, the RSS attribute indicates a
"better" destination than non-RSS for a given speed, because it is
more efficient at local traffic management.

Suggest a followon effort to take advantage of these, by excluding
ineligible paths, and raising the sort priority of others.

Tom.

> > Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> > ---
> >  fs/cifs/smb2ops.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index cd55af9b7cc5..ea634581791a 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/falloc.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/uuid.h>
> > +#include <linux/sort.h>
> >  #include <crypto/aead.h>
> >  #include "cifsglob.h"
> >  #include "smb2pdu.h"
> > @@ -558,6 +559,13 @@ parse_server_interfaces(struct
> network_interface_info_ioctl_rsp *buf,
> >         return rc;
> >  }
> >
> > +static int compare_iface(const void *ia, const void *ib)
> > +{
> > +       const struct cifs_server_iface *a = (struct cifs_server_iface *)ia;
> > +       const struct cifs_server_iface *b = (struct cifs_server_iface *)ib;
> > +
> > +       return a->speed == b->speed ? 0 : (a->speed > b->speed ? -1 : 1);
> > +}
> >
> >  static int
> >  SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> > @@ -587,6 +595,9 @@ SMB3_request_interfaces(const unsigned int xid,
> struct cifs_tcon *tcon)
> >         if (rc)
> >                 goto out;
> >
> > +       /* sort interfaces from fastest to slowest */
> > +       sort(iface_list, iface_count, sizeof(*iface_list), compare_iface, NULL);
> > +
> >         spin_lock(&ses->iface_lock);
> >         kfree(ses->iface_list);
> >         ses->iface_list = iface_list;
> > --
> > 2.16.4
> >
> 
> Looks good at the first glance, thanks!
> 
> @Steve, you may add
> 
> Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
> 
> to this and other patches in the series.
> 
> --
> Best regards,
> Pavel Shilovsky
Aurelien Aptel Nov. 26, 2019, 4:54 p.m. UTC | #3
Hi,

Tom Talpey <ttalpey@microsoft.com> writes:
> Sorting by speed is definitely appropriate, but sorting the other
> multichannel attributes is just as important. For example, if the
> RDMA attribute is set, the address should actually be excluded
> for non-RDMA connections (a second, non-RDMA entry is included,
> if the interface supports both). And, the RSS attribute indicates a
> "better" destination than non-RSS for a given speed, because it is
> more efficient at local traffic management.

Note that the way the list is used has been altered in a later patch
here:

https://lore.kernel.org/linux-cifs/20191120161559.30295-2-aaptel@suse.com/T/#u

Cheers,
Tom Talpey Nov. 26, 2019, 6:38 p.m. UTC | #4
> -----Original Message-----
> From: Aurélien Aptel <aaptel@suse.com>
> Sent: Tuesday, November 26, 2019 11:54 AM
> To: Tom Talpey <ttalpey@microsoft.com>; Pavel Shilovsky
> <piastryyy@gmail.com>
> Cc: linux-cifs <linux-cifs@vger.kernel.org>; Steve French
> <smfrench@gmail.com>
> Subject: RE: [EXTERNAL] Re: [PATCH v4 1/6] cifs: sort interface list by speed
> 
> Hi,
> 
> Tom Talpey <ttalpey@microsoft.com> writes:
> > Sorting by speed is definitely appropriate, but sorting the other
> > multichannel attributes is just as important. For example, if the
> > RDMA attribute is set, the address should actually be excluded
> > for non-RDMA connections (a second, non-RDMA entry is included,
> > if the interface supports both). And, the RSS attribute indicates a
> > "better" destination than non-RSS for a given speed, because it is
> > more efficient at local traffic management.
> 
> Note that the way the list is used has been altered in a later patch

Hmm, that patch does help by taking RSS into account, but it
does not take RDMA into consideration. The logic for that may
be more complex, as it would need to determine whether an
RDMA mount was desired, and an adapter available. And it
looks like the code will try the RDMA addresses from other
transport protocols.

I also think it is more than a little too "heroic". Retrying seems
unnecessary, doesn't the code refresh the list from time to time
and reattempt connections? That's what the Windows client does,
anyway. I would suggest avoiding any kind of retry except on the
very first connection, and even then, many admins will want a
fast-fail. Three TCP retries can take a long time!

Tom.

> here:
> 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker
> nel.org%2Flinux-cifs%2F20191120161559.30295-2-
> aaptel%40suse.com%2FT%2F%23u&amp;data=02%7C01%7Cttalpey%40micros
> oft.com%7Cf76a99bd50ea4fbf4f7908d772914946%7C72f988bf86f141af91ab2
> d7cd011db47%7C1%7C0%7C637103840632982128&amp;sdata=OJxqwDoV8C
> CYNo69cgR9nwmc1xGR22s8J9Y9GDiV74M%3D&amp;reserved=0
> 
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Patch
diff mbox series

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index cd55af9b7cc5..ea634581791a 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -10,6 +10,7 @@ 
 #include <linux/falloc.h>
 #include <linux/scatterlist.h>
 #include <linux/uuid.h>
+#include <linux/sort.h>
 #include <crypto/aead.h>
 #include "cifsglob.h"
 #include "smb2pdu.h"
@@ -558,6 +559,13 @@  parse_server_interfaces(struct network_interface_info_ioctl_rsp *buf,
 	return rc;
 }
 
+static int compare_iface(const void *ia, const void *ib)
+{
+	const struct cifs_server_iface *a = (struct cifs_server_iface *)ia;
+	const struct cifs_server_iface *b = (struct cifs_server_iface *)ib;
+
+	return a->speed == b->speed ? 0 : (a->speed > b->speed ? -1 : 1);
+}
 
 static int
 SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
@@ -587,6 +595,9 @@  SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
 	if (rc)
 		goto out;
 
+	/* sort interfaces from fastest to slowest */
+	sort(iface_list, iface_count, sizeof(*iface_list), compare_iface, NULL);
+
 	spin_lock(&ses->iface_lock);
 	kfree(ses->iface_list);
 	ses->iface_list = iface_list;