[PATCH] improve atomicity of device creation

[PATCH] improve atomicity of device creation

[Scott James Remnant]

[-- Attachment #1.1.1: Type: text/plain, Size: 199 bytes --]

This is a patch we've written and applied in Ubuntu to improve the
atomicity of devmapper device creation somewhat, and avoid races with
udev.

Scott
-- 
Scott James Remnant
scott@ubuntu.com

[-- Attachment #1.1.2: devmapper-1.02.20-atomic-mknod.patch --]
[-- Type: text/x-patch, Size: 1762 bytes --]

--- devmapper-1.02.20.orig/lib/libdm-common.c
+++ devmapper-1.02.20/lib/libdm-common.c
@@ -252,12 +252,11 @@
 static int _add_dev_node(const char *dev_name, uint32_t major, uint32_t minor,
 			 uid_t uid, gid_t gid, mode_t mode)
 {
-	char path[PATH_MAX];
+	char path[PATH_MAX], tmppath[PATH_MAX + 7];
 	struct stat info;
 	dev_t dev = MKDEV(major, minor);
 	mode_t old_mask;
-
-	_build_dev_path(path, sizeof(path), dev_name);
+	int retval;
 
 	if (stat(path, &info) >= 0) {
 		if (!S_ISBLK(info.st_mode)) {
@@ -269,31 +268,39 @@
 		/* If right inode already exists we don't touch uid etc. */
 		if (info.st_rdev == dev)
 			return 1;
-
-		if (unlink(path) < 0) {
-			log_error("Unable to unlink device node for '%s'",
-				  dev_name);
-			return 0;
-		}
 	}
 
+	_build_dev_path(path, sizeof(path), dev_name);
+	strcpy (tmppath, path);
+	strcat (tmppath, ".dm-tmp");
+
 	old_mask = umask(0);
-	if (mknod(path, S_IFBLK | mode, dev) < 0) {
-		log_error("Unable to make device node for '%s'", dev_name);
+	retval = mknod(tmppath, S_IFBLK | mode, dev);
+	umask(old_mask);
+	if (retval < 0) {
+		log_error("Unable to make temporary device node for '%s'", dev_name);
 		return 0;
 	}
-	umask(old_mask);
 
-	if (chown(path, uid, gid) < 0) {
+	if (chown(tmppath, uid, gid) < 0) {
 		log_error("%s: chown failed: %s", path, strerror(errno));
+		unlink(tmppath);
 		return 0;
 	}
 
 #ifdef HAVE_SELINUX
-	if (!dm_set_selinux_context(path, S_IFBLK))
+	if (!dm_set_selinux_context(tmppath, S_IFBLK)) {
+		unlink(tmppath);
 		return 0;
+	}
 #endif
 
+	if (rename(tmppath, path) < 0) {
+		log_error("Unable to replace device node for '%s'", dev_name);
+		unlink(tmppath);
+		return 0;
+	}
+
 	return 1;
 }
 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- Attachment #3: Type: text/plain, Size: 226 bytes --]

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 04:08:36PM +0000, Scott James Remnant wrote:
> This is a patch we've written and applied in Ubuntu to improve the
> atomicity of devmapper device creation somewhat, and avoid races with
> udev.
 
BTW I won't be applying this upstream - these nodes are designed to be
managed by libdevmapper not udev so it's not relevant.  I'm awaiting
patches that have been promised to an agreed design that will provide
for correct libdevmapper/udev integration without races/deadlocks etc.

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Scott James Remnant]

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

On Tue, 2007-12-11 at 16:25 +0000, Alasdair G Kergon wrote:

> On Tue, Dec 11, 2007 at 04:08:36PM +0000, Scott James Remnant wrote:
> > This is a patch we've written and applied in Ubuntu to improve the
> > atomicity of devmapper device creation somewhat, and avoid races with
> > udev.
>  
> BTW I won't be applying this upstream - these nodes are designed to be
> managed by libdevmapper not udev so it's not relevant.  I'm awaiting
> patches that have been promised to an agreed design that will provide
> for correct libdevmapper/udev integration without races/deadlocks etc.
> 
What is that agreed design?

Scott
-- 
Scott James Remnant
scott@ubuntu.com

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- Attachment #3: Type: text/plain, Size: 226 bytes --]

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 04:42:13PM +0000, Scott James Remnant wrote:
> What is that agreed design?
 
In simple terms:

- udev takes over full responsibility for creating nodes
[this will probably be a ./configure option]

- udev provides interface we use to wait until it has finished
processing all the outstanding requests we sent it.

Key point is that our udev requests appear in batches, then we wait for
the batch to complete, as when the system is under memory pressure, udev
userspace may find itself blocked processing the first of the batch
until just before we issue the wait at the end of the batch.
[Note how libdevmapper today pushes all the requests onto an internal
stack and processes them all together at the end.]

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Scott James Remnant]

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

On Tue, 2007-12-11 at 17:03 +0000, Alasdair G Kergon wrote:

> On Tue, Dec 11, 2007 at 04:42:13PM +0000, Scott James Remnant wrote:
> > What is that agreed design?
>  
> In simple terms:
> 
> - udev takes over full responsibility for creating nodes
> [this will probably be a ./configure option]
> 
Excellent, this is what we've wanted for a while.

> - udev provides interface we use to wait until it has finished
> processing all the outstanding requests we sent it.
> 
What's this interface?

I had a proposed patch that made udev write its sequence number to the
kernel, and thus allowed any kobject add event to have a "bottom half"
that could happen when udev had finished.  The idea was that the
existing ioctls could then just block.

The other patch we tried a couple of releases ago was have devmapper
spin until udev had caught up.

Scott
-- 
Scott James Remnant
scott@ubuntu.com

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- Attachment #3: Type: text/plain, Size: 226 bytes --]

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Kay Sievers]

On Dec 11, 2007 5:08 PM, Scott James Remnant <scott@ubuntu.com> wrote:
> This is a patch we've written and applied in Ubuntu to improve the
> atomicity of devmapper device creation somewhat, and avoid races with
> udev.

Without synchronization between libdevmapper and udev, you can't
predict which one will win in creating the node, right?
So this patch makes is more likely to create a valid device node, but
it can still happen, that libdevmapper replaces the udev nodes, which
is not what we want, right?

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Scott James Remnant]

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

On Tue, 2007-12-11 at 18:35 +0100, Kay Sievers wrote:

> On Dec 11, 2007 5:08 PM, Scott James Remnant <scott@ubuntu.com> wrote:
> > This is a patch we've written and applied in Ubuntu to improve the
> > atomicity of devmapper device creation somewhat, and avoid races with
> > udev.
> 
> Without synchronization between libdevmapper and udev, you can't
> predict which one will win in creating the node, right?
> So this patch makes is more likely to create a valid device node, but
> it can still happen, that libdevmapper replaces the udev nodes, which
> is not what we want, right?
> 
No, the patch is slightly skewed so that udev always wins.  udev will
always adopt an existing device node, adjusting permissions as
necessary.  The patch makes devmapper "back off" if the device node
exists.

So you end up with either:

 1) devmapper creates device node
 2) udev adopts device node and sets permissions

or:

 1) udev creates device node and sets permissions
 2) devmapper no-ops since it already exists

Scott
-- 
Scott James Remnant
scott@ubuntu.com

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- Attachment #3: Type: text/plain, Size: 226 bytes --]

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Kay Sievers]

On Dec 11, 2007 6:18 PM, Scott James Remnant <scott@ubuntu.com> wrote:
> On Tue, 2007-12-11 at 17:03 +0000, Alasdair G Kergon wrote:
>
> > On Tue, Dec 11, 2007 at 04:42:13PM +0000, Scott James Remnant wrote:
> > > What is that agreed design?
> >
> > In simple terms:
> >
> > - udev takes over full responsibility for creating nodes
> > [this will probably be a ./configure option]
> >
> Excellent, this is what we've wanted for a while.

Sounds fine.

> > - udev provides interface we use to wait until it has finished
> > processing all the outstanding requests we sent it.
> >
> What's this interface?
>
> I had a proposed patch that made udev write its sequence number to the
> kernel, and thus allowed any kobject add event to have a "bottom half"
> that could happen when udev had finished.  The idea was that the
> existing ioctls could then just block.

You want to block processes (doing ioctl) until udev has finished? Like a
kernel userspace transaction? Where could you see which requests you
will need to fulfill to continue the hanging processes?

It could work, but if something goes wrong, it seems pretty hard to recover
from such a failure. Just remember how firmware loading works. :)

> The other patch we tried a couple of releases ago was have devmapper
> spin until udev had caught up.

The currently running udev events are in: /dev/.udev/queue/<seqnum>. The last
seqnum that arrived in the udev daemon is: /dev/.udev/uevent_seqnum. So udev's
state is exported.

We can make the kernel's kobject_uevent() return the generated seqnum to
the caller. A devmapper ioctl, an existing one to extend, or a new one like
"create node" can return the seqnum to libdevmapper. Libdevmapper then
checks and spins until  /dev/.udev/uevent_seqnum has seen the seqnum,
and /dev/.udev/queue/<seqnum> does not exist.

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 05:40:28PM +0000, Scott James Remnant wrote:
>  1) udev creates device node and sets permissions
>  2) devmapper no-ops since it already exists
 
...because libdevmapper has no notion that something other than itself
could have created it.  That's a bug, but provided the proper udev
integration isn't much longer in coming, I shan't bother to fix it.

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Kay Sievers]

On Dec 11, 2007 6:40 PM, Scott James Remnant <scott@ubuntu.com> wrote:
>
> On Tue, 2007-12-11 at 18:35 +0100, Kay Sievers wrote:
>
> > On Dec 11, 2007 5:08 PM, Scott James Remnant <scott@ubuntu.com> wrote:
> > > This is a patch we've written and applied in Ubuntu to improve the
> > > atomicity of devmapper device creation somewhat, and avoid races with
> > > udev.
> >
> > Without synchronization between libdevmapper and udev, you can't
> > predict which one will win in creating the node, right?
> > So this patch makes is more likely to create a valid device node, but
> > it can still happen, that libdevmapper replaces the udev nodes, which
> > is not what we want, right?
> >
> No, the patch is slightly skewed so that udev always wins.  udev will
> always adopt an existing device node, adjusting permissions as
> necessary.  The patch makes devmapper "back off" if the device node
> exists.
>
> So you end up with either:
>
>  1) devmapper creates device node
>  2) udev adopts device node and sets permissions
>
> or:
>
>  1) udev creates device node and sets permissions
>  2) devmapper no-ops since it already exists

There is a  window between stat() and rename() in libdevmapper,
how can you konw that "udev always wins"?

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 06:51:31PM +0100, Kay Sievers wrote:
> We can make the kernel's kobject_uevent() return the generated seqnum to
> the caller.

Is 32 bits enough?

We added 'uint32_t padding;' recently which we I reckon we could now use for
this.

dm_kobject_uevent() and alloc_dev() could keep a new field in
struct mapped_device up-to-date, and a new function could return its
value to dm-ioctl.c code to place into a renamed 'padding' field
before returning to userspace?

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Kay Sievers]

On Tue, 2007-12-11 at 18:08 +0000, Alasdair G Kergon wrote:
> On Tue, Dec 11, 2007 at 06:51:31PM +0100, Kay Sievers wrote:
> > We can make the kernel's kobject_uevent() return the generated seqnum to
> > the caller.
> 
> Is 32 bits enough?

In the kernel it is 64 bit. Udev also uses 64bit number, but it does not
care about the value these days (netlink is never out of order,
unlike /sbin/hotplug). Only the udev queue export would break if the
number wraps around and duplicates are generated in a window of 180
seconds (the event process kills itself after that time). :)

So we would need to strip the upper 32 bit of the numbers we see from
udev, and handle the case where the 32 bit number wraps around.

> We added 'uint32_t padding;' recently which we I reckon we could now use for
> this.
> 
> dm_kobject_uevent() and alloc_dev() could keep a new field in
> struct mapped_device up-to-date, and a new function could return its
> value to dm-ioctl.c code to place into a renamed 'padding' field
> before returning to userspace?

Yes, we would return the kernel generated seqnum to the libdevmapper
ioctl.

Should we create an additional new iocl to request the device node
creation, or add that call to an existing one?

Could it be that we want to pass more properties to the event, which are
added as environment keys?

Kay


-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Scott James Remnant]

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

On Tue, 2007-12-11 at 17:53 +0000, Alasdair G Kergon wrote:

> On Tue, Dec 11, 2007 at 05:40:28PM +0000, Scott James Remnant wrote:
> >  1) udev creates device node and sets permissions
> >  2) devmapper no-ops since it already exists
>  
> ...because libdevmapper has no notion that something other than itself
> could have created it.  That's a bug, but provided the proper udev
> integration isn't much longer in coming, I shan't bother to fix it.
> 
That's ok ;)  no pressure on the patch, I just wanted to make sure that
we weren't carrying anything that hadn't at least been shown upstream to
see whether you wanted it or not.

Obviously we'd prefer the long-term solution, our patch is just a short
term one since we're already relying on udev for devmapper work today.

Scott
-- 
Scott James Remnant
scott@ubuntu.com

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- Attachment #3: Type: text/plain, Size: 226 bytes --]

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 07:25:32PM +0100, Kay Sievers wrote:
> Should we create an additional new iocl to request the device node
> creation, or add that call to an existing one?

Currently we get an ADD from alloc_dev -> add_disk -> register_disk.
But the device is not usable until the dm resume ioctl, so ideally that
one should be ignored, and it's the CHANGE issued by dm resume that udev
should act upon.  (That's not essential though - the /dev node can be
created in response to the ADD, but nothing in udev userspace should
attempt to open it until after the CHANGE is received.)

> Could it be that we want to pass more properties to the event, which are
> added as environment keys?
 
Don't think so - we'll be handing full control of ownership/permissions
over to udev.

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [dm-devel] [PATCH] improve atomicity of device creation

[Alasdair G Kergon]

On Tue, Dec 11, 2007 at 07:25:32PM +0100, Kay Sievers wrote:
> So we would need to strip the upper 32 bit of the numbers we see from
> udev, and handle the case where the 32 bit number wraps around.
 
Should be OK I think.  Otherwise we can append a 64-bit field to the 
struct - with care, this can be done without breaking compatibility.

Alasdair
-- 
agk@redhat.com

-------------------------------------------------------------------------
SF.Net email is sponsored by: 
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel