Re: [2.6.23 PATCH 13/18] dm: netlink

Re: [2.6.23 PATCH 13/18] dm: netlink

[Andrew Morton]

On Wed, 11 Jul 2007 22:01:37 +0100
Alasdair G Kergon <agk@redhat.com> wrote:

> From: Mike Anderson <andmike@us.ibm.com>
> 
> This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> directory.
> 
> ...
>  
> +config DM_NETLINK
> +	bool "DM netlink events (EXPERIMENTAL)"
> +	depends on BLK_DEV_DM && EXPERIMENTAL
> +	---help---
> +	Generate netlink events for DM events.

Need a dependency on NET there?

> ...
>
> +#ifdef CONFIG_DM_NETLINK
> +
> +int dm_netlink_init(void);
> +void dm_netlink_exit(void);
> +
> +#else	/* CONFIG_DM_NETLINK */
> +
> +static inline int __init dm_netlink_init(void)

The __init here isn't needed (doesn't make sense, is missing the required
#include anyway)

> +{
> +	return 0;
> +}
> +static inline void dm_netlink_exit(void)
> +{
> +}
> +
> +#endif	/* CONFIG_DM_NETLINK */
> +
> +#endif	/* DM_NETLINK_H */
> Index: linux/drivers/md/dm.c
> ===================================================================
> --- linux.orig/drivers/md/dm.c	2007-07-11 21:37:47.000000000 +0100
> +++ linux/drivers/md/dm.c	2007-07-11 21:37:50.000000000 +0100
> @@ -7,6 +7,7 @@
>  
>  #include "dm.h"
>  #include "dm-bio-list.h"
> +#include "dm-netlink.h"
>  
>  #include <linux/init.h>
>  #include <linux/module.h>
> @@ -176,6 +177,7 @@ int (*_inits[])(void) __initdata = {
>  	dm_linear_init,
>  	dm_stripe_init,
>  	dm_interface_init,
> +	dm_netlink_init,
>  };
>  
>  void (*_exits[])(void) = {
> @@ -184,6 +186,7 @@ void (*_exits[])(void) = {
>  	dm_linear_exit,
>  	dm_stripe_exit,
>  	dm_interface_exit,
> +	dm_netlink_exit,
>  };

hm, so if CONFIG_DM_NETLINK=n we end up taking the address of an inlined
function.  So the __init above _did_ make sense, in a peculiar way.  I
don't know that gcc will actually put that converted-to-non-inline function
into the desired section though.

There's no way in which those inlined functions will ever get inlined.
Perhaps all this would be better if there was no implementation of
dm_netlink_init() and dm_netlink_exit() if CONFIG_DM_NETLINK=n and you just
whack the requisite ifdefs into these tables here?

>  static int __init dm_init(void)
> Index: linux/include/linux/netlink.h
> ===================================================================
> --- linux.orig/include/linux/netlink.h	2007-07-11 21:37:31.000000000 +0100
> +++ linux/include/linux/netlink.h	2007-07-11 21:37:50.000000000 +0100
> @@ -21,7 +21,7 @@
>  #define NETLINK_DNRTMSG		14	/* DECnet routing messages */
>  #define NETLINK_KOBJECT_UEVENT	15	/* Kernel messages to userspace */
>  #define NETLINK_GENERIC		16
> -/* leave room for NETLINK_DM (DM Events) */
> +#define NETLINK_DM		17	/* Device Mapper */
>  #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
>  #define NETLINK_ECRYPTFS	19

Have the net guys checked this?  

Re: [2.6.23 PATCH 13/18] dm: netlink

[Mike Anderson]

Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 11 Jul 2007 22:01:37 +0100
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
> > From: Mike Anderson <andmike@us.ibm.com>
> > 
> > This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> > directory.
> > 
> > ...
> >  
> > +config DM_NETLINK
> > +	bool "DM netlink events (EXPERIMENTAL)"
> > +	depends on BLK_DEV_DM && EXPERIMENTAL
> > +	---help---
> > +	Generate netlink events for DM events.
> 
> Need a dependency on NET there?
> 

Yes.

> > ...
> >
> > +#ifdef CONFIG_DM_NETLINK
> > +
> > +int dm_netlink_init(void);
> > +void dm_netlink_exit(void);
> > +
> > +#else	/* CONFIG_DM_NETLINK */
> > +
> > +static inline int __init dm_netlink_init(void)
> 
> The __init here isn't needed (doesn't make sense, is missing the required
> #include anyway)
> 
> > +{
> > +	return 0;
> > +}
> > +static inline void dm_netlink_exit(void)
> > +{
> > +}
> > +
> > +#endif	/* CONFIG_DM_NETLINK */
> > +
> > +#endif	/* DM_NETLINK_H */
> > Index: linux/drivers/md/dm.c
> > ===================================================================
> > --- linux.orig/drivers/md/dm.c	2007-07-11 21:37:47.000000000 +0100
> > +++ linux/drivers/md/dm.c	2007-07-11 21:37:50.000000000 +0100
> > @@ -7,6 +7,7 @@
> >  
> >  #include "dm.h"
> >  #include "dm-bio-list.h"
> > +#include "dm-netlink.h"
> >  
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> > @@ -176,6 +177,7 @@ int (*_inits[])(void) __initdata = {
> >  	dm_linear_init,
> >  	dm_stripe_init,
> >  	dm_interface_init,
> > +	dm_netlink_init,
> >  };
> >  
> >  void (*_exits[])(void) = {
> > @@ -184,6 +186,7 @@ void (*_exits[])(void) = {
> >  	dm_linear_exit,
> >  	dm_stripe_exit,
> >  	dm_interface_exit,
> > +	dm_netlink_exit,
> >  };
> 
> hm, so if CONFIG_DM_NETLINK=n we end up taking the address of an inlined
> function.  So the __init above _did_ make sense, in a peculiar way.  I
> don't know that gcc will actually put that converted-to-non-inline function
> into the desired section though.
> 
> There's no way in which those inlined functions will ever get inlined.
> Perhaps all this would be better if there was no implementation of
> dm_netlink_init() and dm_netlink_exit() if CONFIG_DM_NETLINK=n and you just
> whack the requisite ifdefs into these tables here?
> 

ok, I will switch to the ifdef CONFIG_DM_NETLINK in the tables.


> >  static int __init dm_init(void)
> > Index: linux/include/linux/netlink.h
> > ===================================================================
> > --- linux.orig/include/linux/netlink.h	2007-07-11 21:37:31.000000000 +0100
> > +++ linux/include/linux/netlink.h	2007-07-11 21:37:50.000000000 +0100
> > @@ -21,7 +21,7 @@
> >  #define NETLINK_DNRTMSG		14	/* DECnet routing messages */
> >  #define NETLINK_KOBJECT_UEVENT	15	/* Kernel messages to userspace */
> >  #define NETLINK_GENERIC		16
> > -/* leave room for NETLINK_DM (DM Events) */
> > +#define NETLINK_DM		17	/* Device Mapper */
> >  #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> >  #define NETLINK_ECRYPTFS	19
> 
> Have the net guys checked this?  

No. The support is a derivative of the netlink support in
scsi_transport_iscsi.c.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [2.6.23 PATCH 13/18] dm: netlink

[Evgeniy Polyakov]

On Wed, Jul 11, 2007 at 04:37:36PM -0700, Mike Anderson (andmike@us.ibm.com) wrote:
> > > --- linux.orig/include/linux/netlink.h	2007-07-11 21:37:31.000000000 +0100
> > > +++ linux/include/linux/netlink.h	2007-07-11 21:37:50.000000000 +0100
> > > @@ -21,7 +21,7 @@
> > >  #define NETLINK_DNRTMSG		14	/* DECnet routing messages */
> > >  #define NETLINK_KOBJECT_UEVENT	15	/* Kernel messages to userspace */
> > >  #define NETLINK_GENERIC		16
> > > -/* leave room for NETLINK_DM (DM Events) */
> > > +#define NETLINK_DM		17	/* Device Mapper */
> > >  #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> > >  #define NETLINK_ECRYPTFS	19
> > 
> > Have the net guys checked this?  
> 
> No. The support is a derivative of the netlink support in
> scsi_transport_iscsi.c.

I'm not sure about all net guys, but the first question rised after
reading this - why do you want special netlink family and do not want to
use interfaces created on top of - like connector and genetlink?

> -andmike

-- 
	Evgeniy Polyakov

Re: [2.6.23 PATCH 13/18] dm: netlink

[David Miller]

From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Date: Thu, 12 Jul 2007 12:10:29 +0400

> On Wed, Jul 11, 2007 at 04:37:36PM -0700, Mike Anderson (andmike@us.ibm.com) wrote:
> > > > --- linux.orig/include/linux/netlink.h	2007-07-11 21:37:31.000000000 +0100
> > > > +++ linux/include/linux/netlink.h	2007-07-11 21:37:50.000000000 +0100
> > > > @@ -21,7 +21,7 @@
> > > >  #define NETLINK_DNRTMSG		14	/* DECnet routing messages */
> > > >  #define NETLINK_KOBJECT_UEVENT	15	/* Kernel messages to userspace */
> > > >  #define NETLINK_GENERIC		16
> > > > -/* leave room for NETLINK_DM (DM Events) */
> > > > +#define NETLINK_DM		17	/* Device Mapper */
> > > >  #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> > > >  #define NETLINK_ECRYPTFS	19
> > > 
> > > Have the net guys checked this?  
> > 
> > No. The support is a derivative of the netlink support in
> > scsi_transport_iscsi.c.
> 
> I'm not sure about all net guys, but the first question rised after
> reading this - why do you want special netlink family and do not want to
> use interfaces created on top of - like connector and genetlink?

I agree, there is really no reason to not at least use
genetlink.

Re: [2.6.23 PATCH 13/18] dm: netlink

[Mike Anderson]

David Miller <davem@davemloft.net> wrote:
> From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> Date: Thu, 12 Jul 2007 12:10:29 +0400
> 
> > On Wed, Jul 11, 2007 at 04:37:36PM -0700, Mike Anderson (andmike@us.ibm.com) wrote:
> > > > > --- linux.orig/include/linux/netlink.h	2007-07-11 21:37:31.000000000 +0100
> > > > > +++ linux/include/linux/netlink.h	2007-07-11 21:37:50.000000000 +0100
> > > > > @@ -21,7 +21,7 @@
> > > > >  #define NETLINK_DNRTMSG		14	/* DECnet routing messages */
> > > > >  #define NETLINK_KOBJECT_UEVENT	15	/* Kernel messages to userspace */
> > > > >  #define NETLINK_GENERIC		16
> > > > > -/* leave room for NETLINK_DM (DM Events) */
> > > > > +#define NETLINK_DM		17	/* Device Mapper */
> > > > >  #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> > > > >  #define NETLINK_ECRYPTFS	19
> > > > 
> > > > Have the net guys checked this?  
> > > 
> > > No. The support is a derivative of the netlink support in
> > > scsi_transport_iscsi.c.
> > 
> > I'm not sure about all net guys, but the first question rised after
> > reading this - why do you want special netlink family and do not want to
> > use interfaces created on top of - like connector and genetlink?
> 
> I agree, there is really no reason to not at least use
> genetlink.

ok, I will switch over to using genetlink.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [2.6.23 PATCH 13/18] dm: netlink

[Matt Mackall]

On Wed, Jul 11, 2007 at 02:27:12PM -0700, Andrew Morton wrote:
> On Wed, 11 Jul 2007 22:01:37 +0100
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
> > From: Mike Anderson <andmike@us.ibm.com>
> > 
> > This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> > directory.
> > 
> > ...
> >  
> > +config DM_NETLINK
> > +	bool "DM netlink events (EXPERIMENTAL)"
> > +	depends on BLK_DEV_DM && EXPERIMENTAL
> > +	---help---
> > +	Generate netlink events for DM events.
> 
> Need a dependency on NET there?

It's really sad to make DM dependent on the network layer.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [2.6.23 PATCH 13/18] dm: netlink

[Andrew Morton]

On Thu, 12 Jul 2007 18:19:20 -0500
Matt Mackall <mpm@selenic.com> wrote:

> On Wed, Jul 11, 2007 at 02:27:12PM -0700, Andrew Morton wrote:
> > On Wed, 11 Jul 2007 22:01:37 +0100
> > Alasdair G Kergon <agk@redhat.com> wrote:
> > 
> > > From: Mike Anderson <andmike@us.ibm.com>
> > > 
> > > This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> > > directory.
> > > 
> > > ...
> > >  
> > > +config DM_NETLINK
> > > +	bool "DM netlink events (EXPERIMENTAL)"
> > > +	depends on BLK_DEV_DM && EXPERIMENTAL
> > > +	---help---
> > > +	Generate netlink events for DM events.
> > 
> > Need a dependency on NET there?
> 
> It's really sad to make DM dependent on the network layer.
> 

Yes, it would be somewhat sad.  However one can presumably continue to use
DM, just without "DM netlink events".

But that probably means that one will not be able to use the standard DM
admin tools without networking.  Maybe there will remain alternative but
cruder ways to get things done?

Re: [2.6.23 PATCH 13/18] dm: netlink

[Mike Anderson]

Matt Mackall <mpm@selenic.com> wrote:
> On Wed, Jul 11, 2007 at 02:27:12PM -0700, Andrew Morton wrote:
> > On Wed, 11 Jul 2007 22:01:37 +0100
> > Alasdair G Kergon <agk@redhat.com> wrote:
> > 
> > > From: Mike Anderson <andmike@us.ibm.com>
> > > 
> > > This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> > > directory.
> > > 
> > > ...
> > >  
> > > +config DM_NETLINK
> > > +	bool "DM netlink events (EXPERIMENTAL)"
> > > +	depends on BLK_DEV_DM && EXPERIMENTAL
> > > +	---help---
> > > +	Generate netlink events for DM events.
> > 
> > Need a dependency on NET there?
> 
> It's really sad to make DM dependent on the network layer.

It wouldn't be all of dm. Only the netlink based event interface would be
dependent.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [dm-devel] Re: [2.6.23 PATCH 13/18] dm: netlink

[Mike Anderson]

Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 12 Jul 2007 18:19:20 -0500
> Matt Mackall <mpm@selenic.com> wrote:
> 
> > On Wed, Jul 11, 2007 at 02:27:12PM -0700, Andrew Morton wrote:
> > > On Wed, 11 Jul 2007 22:01:37 +0100
> > > Alasdair G Kergon <agk@redhat.com> wrote:
> > > 
> > > > From: Mike Anderson <andmike@us.ibm.com>
> > > > 
> > > > This patch adds a dm-netlink skeleton support to the Makefile, and the dm
> > > > directory.
> > > > 
> > > > ...
> > > >  
> > > > +config DM_NETLINK
> > > > +	bool "DM netlink events (EXPERIMENTAL)"
> > > > +	depends on BLK_DEV_DM && EXPERIMENTAL
> > > > +	---help---
> > > > +	Generate netlink events for DM events.
> > > 
> > > Need a dependency on NET there?
> > 
> > It's really sad to make DM dependent on the network layer.
> > 
> 
> Yes, it would be somewhat sad.  However one can presumably continue to use
> DM, just without "DM netlink events".
> 

Yes, if you deselect you just do not receive the events through netlink.

> But that probably means that one will not be able to use the standard DM
> admin tools without networking.  Maybe there will remain alternative but
> cruder ways to get things done?

No, all admin tools and interfaces function as they do today. The
dm-netlink patch series only contains 9 deletions (actual just one true
deletion of existing kernel code the others are due to break up of the
patch into compilable chunks). The intent was not to break users or force
migration.


-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [2.6.23 PATCH 13/18] dm: netlink

[Evgeniy Polyakov]

On Thu, Jul 12, 2007 at 04:31:15PM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > Need a dependency on NET there?
> > 
> > It's really sad to make DM dependent on the network layer.
> > 
> 
> Yes, it would be somewhat sad.  However one can presumably continue to use
> DM, just without "DM netlink events".

On my machine device mapper (lvm, raid) does not work without sockets,
(maybe not dm, but hotplug, which creates nodes, or configuration)
so it already depends on it. Hotplug depends on networking.

You missed the day when everyone started to depend on networking :)

-- 
	Evgeniy Polyakov

Re: [2.6.23 PATCH 13/18] dm: netlink

[Kay Sievers]

On 7/13/07, Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> On Thu, Jul 12, 2007 at 04:31:15PM -0700, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > > Need a dependency on NET there?
> > >
> > > It's really sad to make DM dependent on the network layer.
> > >
> >
> > Yes, it would be somewhat sad.  However one can presumably continue to use
> > DM, just without "DM netlink events".
>
> On my machine device mapper (lvm, raid) does not work without sockets,
> (maybe not dm, but hotplug, which creates nodes, or configuration)
> so it already depends on it. Hotplug depends on networking.
>
> You missed the day when everyone started to depend on networking :)

Right, uevents (udev) depend on networking. One could still use the
/sbin/hotplug fork-bomb, but boxes that don't want networking are
often that small, that the amount of events the kernel creates today,
leads immediately to OOM, because of too many event processes forked
at the same time.

Kay

Re: Re: [2.6.23 PATCH 13/18] dm: netlink

[Valdis.Kletnieks]

[-- Attachment #1.1: Type: text/plain, Size: 540 bytes --]

On Thu, 12 Jul 2007 19:00:59 PDT, Mike Anderson said:

> No, all admin tools and interfaces function as they do today. The
> dm-netlink patch series only contains 9 deletions (actual just one true
> deletion of existing kernel code the others are due to break up of the
> patch into compilable chunks). The intent was not to break users or force
> migration.

OK, I'll bite - if the admin tools function as before, who is *using* the
code?  Or do the admin tools have a preferred netlink component and a fallback
set of code if that fails?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 226 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]