linux-raid.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] Reduce unnecessary opens of raid members
@ 2013-04-11 13:18 Jes.Sorensen
  2013-04-11 13:18 ` [PATCH 1/1] prevent double open(O_RDWR) on raid creation Jes.Sorensen
  0 siblings, 1 reply; 11+ messages in thread
From: Jes.Sorensen @ 2013-04-11 13:18 UTC (permalink / raw)
  To: neilb; +Cc: harald, linux-raid

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I am posting this on Harald's request. This avoids unncessary double
opens of raid members and also has the side effect of not triggering
udev notify twice.

Jes


Harald Hoyer (1):
  prevent double open(O_RDWR) on raid creation

 Kill.c        | 28 +++++++++++++++++-----------
 Manage.c      |  2 +-
 mdadm.c       |  4 ++--
 mdadm.h       |  2 +-
 super-ddf.c   |  2 +-
 super-intel.c |  2 +-
 super0.c      |  2 +-
 super1.c      |  2 +-
 8 files changed, 25 insertions(+), 19 deletions(-)

-- 
1.8.1.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-11 13:18 [PATCH 0/1] Reduce unnecessary opens of raid members Jes.Sorensen
@ 2013-04-11 13:18 ` Jes.Sorensen
  2013-04-29  0:57   ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Jes.Sorensen @ 2013-04-11 13:18 UTC (permalink / raw)
  To: neilb; +Cc: harald, linux-raid

From: Harald Hoyer <harald@redhat.com>

This does not trigger the udev inotify twice and saves a lot of blk I/O
for the raid members.

Also fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=947815

Signed-off-by: Harald Hoyer <harald@redhat.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Kill.c        | 28 +++++++++++++++++-----------
 Manage.c      |  2 +-
 mdadm.c       |  4 ++--
 mdadm.h       |  2 +-
 super-ddf.c   |  2 +-
 super-intel.c |  2 +-
 super0.c      |  2 +-
 super1.c      |  2 +-
 8 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/Kill.c b/Kill.c
index f2fdb85..b5c93ae 100644
--- a/Kill.c
+++ b/Kill.c
@@ -29,7 +29,7 @@
 #include	"md_u.h"
 #include	"md_p.h"
 
-int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
+int Kill(char *dev, int oldfd, struct supertype *st, int force, int verbose, int noexcl)
 {
 	/*
 	 * Nothing fancy about Kill.  It just zeroes out a superblock
@@ -42,21 +42,26 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
 
 	int fd, rv = 0;
 
-	if (force)
-		noexcl = 1;
-	fd = open(dev, O_RDWR|(noexcl ? 0 : O_EXCL));
-	if (fd < 0) {
-		if (verbose >= 0)
-			pr_err("Couldn't open %s for write - not zeroing\n",
-				dev);
-		return 2;
+	if (oldfd != -1) {
+		fd = oldfd;
+	} else {
+		if (force)
+			noexcl = 1;
+		fd = open(dev, O_RDWR|(noexcl ? 0 : O_EXCL));
+		if (fd < 0) {
+			if (verbose >= 0)
+				pr_err("Couldn't open %s for write - not zeroing\n",
+					dev);
+			return 2;
+		}
 	}
 	if (st == NULL)
 		st = guess_super(fd);
 	if (st == NULL || st->ss->init_super == NULL) {
 		if (verbose >= 0)
 			pr_err("Unrecognised md component device - %s\n", dev);
-		close(fd);
+		if (oldfd == -1)
+			close(fd);
 		return 2;
 	}
 	st->ignore_hw_compat = 1;
@@ -76,7 +81,8 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
 			rv = 0;
 		}
 	}
-	close(fd);
+	if (oldfd == -1)
+		close(fd);
 	return rv;
 }
 
diff --git a/Manage.c b/Manage.c
index 6267c0c..509e921 100644
--- a/Manage.c
+++ b/Manage.c
@@ -785,7 +785,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 			return -1;
 		}
 
-		Kill(dv->devname, NULL, 0, -1, 0);
+		Kill(dv->devname, -1, NULL, 0, -1, 0);
 		dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
 		if (mdmon_running(tst->container_devnm))
 			tst->update_tail = &tst->updates;
diff --git a/mdadm.c b/mdadm.c
index 214afa3..d68ee96 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1739,11 +1739,11 @@ static int misc_list(struct mddev_dev *devlist,
 			continue;
 		case KillOpt: /* Zero superblock */
 			if (ss)
-				rv |= Kill(dv->devname, ss, c->force, c->verbose,0);
+				rv |= Kill(dv->devname, -1, ss, c->force, c->verbose, 0);
 			else {
 				int v = c->verbose;
 				do {
-					rv |= Kill(dv->devname, NULL, c->force, v, 0);
+					rv |= Kill(dv->devname, -1, NULL, c->force, v, 0);
 					v = -1;
 				} while (rv == 0);
 				rv &= ~2;
diff --git a/mdadm.h b/mdadm.h
index c7b5183..e55dec5 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1156,7 +1156,7 @@ extern int Monitor(struct mddev_dev *devlist,
 		   int dosyslog, char *pidfile, int increments,
 		   int share);
 
-extern int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl);
+extern int Kill(char *dev, int oldfd, struct supertype *st, int force, int verbose, int noexcl);
 extern int Kill_subarray(char *dev, char *subarray, int verbose);
 extern int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int quiet);
 extern int Wait(char *dev);
diff --git a/super-ddf.c b/super-ddf.c
index c5f6f5c..a88699c 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2597,7 +2597,7 @@ static int write_init_super_ddf(struct supertype *st)
 	} else {
 		struct dl *d;
 		for (d = ddf->dlist; d; d=d->next)
-			while (Kill(d->devname, NULL, 0, -1, 1) == 0);
+			while (Kill(d->devname, d->fd, NULL, 0, -1, 1) == 0);
 		return __write_init_super_ddf(st);
 	}
 }
diff --git a/super-intel.c b/super-intel.c
index 24016b7..743a7fc 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5221,7 +5221,7 @@ static int write_init_super_imsm(struct supertype *st)
 	} else {
 		struct dl *d;
 		for (d = super->disks; d; d = d->next)
-			Kill(d->devname, NULL, 0, -1, 1);
+			Kill(d->devname, d->fd, NULL, 0, -1, 1);
 		return write_super_imsm(st, 1);
 	}
 }
diff --git a/super0.c b/super0.c
index 1f4dc75..57b549e 100644
--- a/super0.c
+++ b/super0.c
@@ -779,7 +779,7 @@ static int write_init_super0(struct supertype *st)
 			continue;
 		if (di->fd == -1)
 			continue;
-		while (Kill(di->devname, NULL, 0, -1, 1) == 0)
+		while (Kill(di->devname, di->fd, NULL, 0, -1, 1) == 0)
 			;
 
 		sb->disks[di->disk.number].state &= ~(1<<MD_DISK_FAULTY);
diff --git a/super1.c b/super1.c
index d0f1d5f..e3eeb80 100644
--- a/super1.c
+++ b/super1.c
@@ -1333,7 +1333,7 @@ static int write_init_super1(struct supertype *st)
 		if (di->fd < 0)
 			continue;
 
-		while (Kill(di->devname, NULL, 0, -1, 1) == 0)
+		while (Kill(di->devname, di->fd, NULL, 0, -1, 1) == 0)
 			;
 
 		sb->dev_number = __cpu_to_le32(di->disk.number);
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-11 13:18 ` [PATCH 1/1] prevent double open(O_RDWR) on raid creation Jes.Sorensen
@ 2013-04-29  0:57   ` NeilBrown
  2013-04-29  5:33     ` Harald Hoyer
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2013-04-29  0:57 UTC (permalink / raw)
  To: Jes.Sorensen; +Cc: harald, linux-raid

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

On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:

> From: Harald Hoyer <harald@redhat.com>
> 
> This does not trigger the udev inotify twice and saves a lot of blk I/O
> for the raid members.
> 
> Also fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=947815
> 
> Signed-off-by: Harald Hoyer <harald@redhat.com>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

(Sorry for delays.  Thanks for reminders).

That patch seems to make sense, but the description above is awfully thin.

Why is double-open a problem exactly?  What does it make udev do?  And how
does that related to ID_FS_TYPE being wrong as mentioned in the bugzilla
entry.

NeilBrown


> ---
>  Kill.c        | 28 +++++++++++++++++-----------
>  Manage.c      |  2 +-
>  mdadm.c       |  4 ++--
>  mdadm.h       |  2 +-
>  super-ddf.c   |  2 +-
>  super-intel.c |  2 +-
>  super0.c      |  2 +-
>  super1.c      |  2 +-
>  8 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/Kill.c b/Kill.c
> index f2fdb85..b5c93ae 100644
> --- a/Kill.c
> +++ b/Kill.c
> @@ -29,7 +29,7 @@
>  #include	"md_u.h"
>  #include	"md_p.h"
>  
> -int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
> +int Kill(char *dev, int oldfd, struct supertype *st, int force, int verbose, int noexcl)
>  {
>  	/*
>  	 * Nothing fancy about Kill.  It just zeroes out a superblock
> @@ -42,21 +42,26 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
>  
>  	int fd, rv = 0;
>  
> -	if (force)
> -		noexcl = 1;
> -	fd = open(dev, O_RDWR|(noexcl ? 0 : O_EXCL));
> -	if (fd < 0) {
> -		if (verbose >= 0)
> -			pr_err("Couldn't open %s for write - not zeroing\n",
> -				dev);
> -		return 2;
> +	if (oldfd != -1) {
> +		fd = oldfd;
> +	} else {
> +		if (force)
> +			noexcl = 1;
> +		fd = open(dev, O_RDWR|(noexcl ? 0 : O_EXCL));
> +		if (fd < 0) {
> +			if (verbose >= 0)
> +				pr_err("Couldn't open %s for write - not zeroing\n",
> +					dev);
> +			return 2;
> +		}
>  	}
>  	if (st == NULL)
>  		st = guess_super(fd);
>  	if (st == NULL || st->ss->init_super == NULL) {
>  		if (verbose >= 0)
>  			pr_err("Unrecognised md component device - %s\n", dev);
> -		close(fd);
> +		if (oldfd == -1)
> +			close(fd);
>  		return 2;
>  	}
>  	st->ignore_hw_compat = 1;
> @@ -76,7 +81,8 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
>  			rv = 0;
>  		}
>  	}
> -	close(fd);
> +	if (oldfd == -1)
> +		close(fd);
>  	return rv;
>  }
>  
> diff --git a/Manage.c b/Manage.c
> index 6267c0c..509e921 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -785,7 +785,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
>  			return -1;
>  		}
>  
> -		Kill(dv->devname, NULL, 0, -1, 0);
> +		Kill(dv->devname, -1, NULL, 0, -1, 0);
>  		dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
>  		if (mdmon_running(tst->container_devnm))
>  			tst->update_tail = &tst->updates;
> diff --git a/mdadm.c b/mdadm.c
> index 214afa3..d68ee96 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1739,11 +1739,11 @@ static int misc_list(struct mddev_dev *devlist,
>  			continue;
>  		case KillOpt: /* Zero superblock */
>  			if (ss)
> -				rv |= Kill(dv->devname, ss, c->force, c->verbose,0);
> +				rv |= Kill(dv->devname, -1, ss, c->force, c->verbose, 0);
>  			else {
>  				int v = c->verbose;
>  				do {
> -					rv |= Kill(dv->devname, NULL, c->force, v, 0);
> +					rv |= Kill(dv->devname, -1, NULL, c->force, v, 0);
>  					v = -1;
>  				} while (rv == 0);
>  				rv &= ~2;
> diff --git a/mdadm.h b/mdadm.h
> index c7b5183..e55dec5 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1156,7 +1156,7 @@ extern int Monitor(struct mddev_dev *devlist,
>  		   int dosyslog, char *pidfile, int increments,
>  		   int share);
>  
> -extern int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl);
> +extern int Kill(char *dev, int oldfd, struct supertype *st, int force, int verbose, int noexcl);
>  extern int Kill_subarray(char *dev, char *subarray, int verbose);
>  extern int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int quiet);
>  extern int Wait(char *dev);
> diff --git a/super-ddf.c b/super-ddf.c
> index c5f6f5c..a88699c 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -2597,7 +2597,7 @@ static int write_init_super_ddf(struct supertype *st)
>  	} else {
>  		struct dl *d;
>  		for (d = ddf->dlist; d; d=d->next)
> -			while (Kill(d->devname, NULL, 0, -1, 1) == 0);
> +			while (Kill(d->devname, d->fd, NULL, 0, -1, 1) == 0);
>  		return __write_init_super_ddf(st);
>  	}
>  }
> diff --git a/super-intel.c b/super-intel.c
> index 24016b7..743a7fc 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5221,7 +5221,7 @@ static int write_init_super_imsm(struct supertype *st)
>  	} else {
>  		struct dl *d;
>  		for (d = super->disks; d; d = d->next)
> -			Kill(d->devname, NULL, 0, -1, 1);
> +			Kill(d->devname, d->fd, NULL, 0, -1, 1);
>  		return write_super_imsm(st, 1);
>  	}
>  }
> diff --git a/super0.c b/super0.c
> index 1f4dc75..57b549e 100644
> --- a/super0.c
> +++ b/super0.c
> @@ -779,7 +779,7 @@ static int write_init_super0(struct supertype *st)
>  			continue;
>  		if (di->fd == -1)
>  			continue;
> -		while (Kill(di->devname, NULL, 0, -1, 1) == 0)
> +		while (Kill(di->devname, di->fd, NULL, 0, -1, 1) == 0)
>  			;
>  
>  		sb->disks[di->disk.number].state &= ~(1<<MD_DISK_FAULTY);
> diff --git a/super1.c b/super1.c
> index d0f1d5f..e3eeb80 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1333,7 +1333,7 @@ static int write_init_super1(struct supertype *st)
>  		if (di->fd < 0)
>  			continue;
>  
> -		while (Kill(di->devname, NULL, 0, -1, 1) == 0)
> +		while (Kill(di->devname, di->fd, NULL, 0, -1, 1) == 0)
>  			;
>  
>  		sb->dev_number = __cpu_to_le32(di->disk.number);


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  0:57   ` NeilBrown
@ 2013-04-29  5:33     ` Harald Hoyer
  2013-04-29  6:11       ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  5:33 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 02:57 AM, NeilBrown wrote:
> On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
> 
>> From: Harald Hoyer <harald@redhat.com>
>> 
>> This does not trigger the udev inotify twice and saves a lot of blk I/O 
>> for the raid members.
>> 
>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
>> 
>> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by: Jes
>> Sorensen <Jes.Sorensen@redhat.com>
> 
> (Sorry for delays.  Thanks for reminders).
> 
> That patch seems to make sense, but the description above is awfully thin.
> 
> Why is double-open a problem exactly?  What does it make udev do?  And how 
> does that related to ID_FS_TYPE being wrong as mentioned in the bugzilla 
> entry.
> 
> NeilBrown
> 

udevd with watch enabled (inotify on /dev/sd*) gets triggered on close(), when
you opened it writeable. So, if you double open() and udev wakes up from the
first close(), not all information are written to disk yet, it will not get
the ID_FS_TYPE.

Seems like the second close() does not trigger an inotify sometimes, so it is
missing afterwards all the time.

Watch via inotify is just a lazy workaround, so we don't have to modify every
tool to emit a "change" uevent, after they changed the disk.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfgYhAAoJEANOs3ABTfJw0uUQALrm0pEjlLd6XgojMTQ6xJGy
y98MVcobi10O/WJyg3HV1RqjnYNu7wfpp+lFIzKRmE/sxIBj8X9ATFfjaopCGWYC
/MPGsdehrCpGPPOZBlt47vTdoaKWB5meKsBm3X1I0AhA+uOxgeV2qfaijoOkHeim
a4RbIUoOJIjIyvbdKCuVbs8mqcr62eMUiZBDPv/b3FcBtOOjYkWVZU14ylqujNtM
WzE+soKyd6L70DvPWVY2KzJ4/5bg/fRuvFcc464k88hAqa8U36FU6MzfTuT4K+ZH
a4FYJtpdrggL+IZuG5XToNR5lpR/YW/B1UBhfCjItXbr1dhX3alk+Y3xZCWvpbRF
FFwAA1GJfcB8UmKp3loBX0YH4gJ8h8d6EITE0Quj38VqG4MlCl89J6ClQZYgXsf8
ZCVDX+lomiQkEp5xYyC85hmwfwepibncqfqKef8+4ABc5xWswQr89gDFPVsFZUE/
PbHzCUlAkz8lvuRSNH6k54b7nFeGn116eJgO7sKESt4uygP0o1A6WpWZI+YAMMg5
CBkxrLYM/iERP7sf8kHr3Wd5EWJNTYm6UsJjVtWStGHuB7LNDo6qPBXxzf84Mkek
1fnIqBfl6QDQBcYb+02p2vGhcTA+P/byi+j+eFQmwV8g2gbkwxhV6/t0Sizj57tC
SlUZAaWHeeNK9HDgoNJ0
=zkJO
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  5:33     ` Harald Hoyer
@ 2013-04-29  6:11       ` NeilBrown
  2013-04-29  6:32         ` Harald Hoyer
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2013-04-29  6:11 UTC (permalink / raw)
  To: Harald Hoyer; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, 29 Apr 2013 07:33:21 +0200 Harald Hoyer <harald@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/29/2013 02:57 AM, NeilBrown wrote:
> > On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
> > 
> >> From: Harald Hoyer <harald@redhat.com>
> >> 
> >> This does not trigger the udev inotify twice and saves a lot of blk I/O 
> >> for the raid members.
> >> 
> >> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
> >> 
> >> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by: Jes
> >> Sorensen <Jes.Sorensen@redhat.com>
> > 
> > (Sorry for delays.  Thanks for reminders).
> > 
> > That patch seems to make sense, but the description above is awfully thin.
> > 
> > Why is double-open a problem exactly?  What does it make udev do?  And how 
> > does that related to ID_FS_TYPE being wrong as mentioned in the bugzilla 
> > entry.
> > 
> > NeilBrown
> > 
> 
> udevd with watch enabled (inotify on /dev/sd*) gets triggered on close(), when
> you opened it writeable. So, if you double open() and udev wakes up from the
> first close(), not all information are written to disk yet, it will not get
> the ID_FS_TYPE.
> 
> Seems like the second close() does not trigger an inotify sometimes, so it is
> missing afterwards all the time.
> 
> Watch via inotify is just a lazy workaround, so we don't have to modify every
> tool to emit a "change" uevent, after they changed the disk.

So udev have a "lazy workaround" so that other programs don't need to trigger
a change, and as a result, I need to add some special code to mdadm.
Doesn't seem like I'm getting any advantage out of this laziness.

How about when udev gets an inotify for a block device, it first checks
that it can open it O_EXCL.  If not, it doesn't generate the change event.
That seems like the laziest option to me :-)

NeilBrown


> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJRfgYhAAoJEANOs3ABTfJw0uUQALrm0pEjlLd6XgojMTQ6xJGy
> y98MVcobi10O/WJyg3HV1RqjnYNu7wfpp+lFIzKRmE/sxIBj8X9ATFfjaopCGWYC
> /MPGsdehrCpGPPOZBlt47vTdoaKWB5meKsBm3X1I0AhA+uOxgeV2qfaijoOkHeim
> a4RbIUoOJIjIyvbdKCuVbs8mqcr62eMUiZBDPv/b3FcBtOOjYkWVZU14ylqujNtM
> WzE+soKyd6L70DvPWVY2KzJ4/5bg/fRuvFcc464k88hAqa8U36FU6MzfTuT4K+ZH
> a4FYJtpdrggL+IZuG5XToNR5lpR/YW/B1UBhfCjItXbr1dhX3alk+Y3xZCWvpbRF
> FFwAA1GJfcB8UmKp3loBX0YH4gJ8h8d6EITE0Quj38VqG4MlCl89J6ClQZYgXsf8
> ZCVDX+lomiQkEp5xYyC85hmwfwepibncqfqKef8+4ABc5xWswQr89gDFPVsFZUE/
> PbHzCUlAkz8lvuRSNH6k54b7nFeGn116eJgO7sKESt4uygP0o1A6WpWZI+YAMMg5
> CBkxrLYM/iERP7sf8kHr3Wd5EWJNTYm6UsJjVtWStGHuB7LNDo6qPBXxzf84Mkek
> 1fnIqBfl6QDQBcYb+02p2vGhcTA+P/byi+j+eFQmwV8g2gbkwxhV6/t0Sizj57tC
> SlUZAaWHeeNK9HDgoNJ0
> =zkJO
> -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIVAwUBUX4PDDnsnt1WYoG5AQInKA/+PyFlr+sO0cHRQRxG6EMi1Uy649MAf1u+
TOnvspRNilHs+ackxKOyT3ZNHYUwyeybVKlAvKuZ8ynObtUmE+E7OnGOcqLl28Ml
As92W35y56qPl1czDhX/UDAZALjI0FrHcDBJLxef8bWLXCsEoUpPeqF5ddfmsvzL
7XR9Aiy7LHkBXQbPciYneR1kZHnuNF3zBFtt68bAnK4hV4128LuohM3v6FM4CU5q
fiZpo+gqeKdnu7oev0Xh6inaP5MfupGvYA9UTTPiRV/Rg+9tv2Y/hSHtYE/mvWbH
lLLAHq1e7Pcu1wCwXmlW0h2ph5rcs3rBOgQYpFWet65nbFk76q+NY01m0tqelVbv
6wt1JVyUxopHMy9FLypg+/cctDYLimgDFd2eG5B2YXL/lF5EBclM3PmhMDR6ITun
wWGhburperRwKYVZszv9Eo8Teu1ed/1gDnD69XlOa8dOOXtj95WcBDGz6p+1hM/Q
H7bWwXEiXreMrv3WtCvoghakM+ul9P3vZXo83Vwv7x7WDteEQ6ZYUe9CdVuM/k3r
x+CYz7kjfkg2oZEZmN11pe3UoU+eCBR3FOm1xvbEiQt5SWZm8mRd7v7Y2qVI3e3C
Q9M5rEJ8wws20JrRXHEtkOCX1FzeiYOi5calDWUzf+2AyY86NYv214SM98qWXgju
AyUaIO7u0Mg=
=zg9i
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  6:11       ` NeilBrown
@ 2013-04-29  6:32         ` Harald Hoyer
  2013-04-29  6:53           ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  6:32 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 08:11 AM, NeilBrown wrote:
> On Mon, 29 Apr 2013 07:33:21 +0200 Harald Hoyer <harald@redhat.com> wrote:
> 
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> 
>> On 04/29/2013 02:57 AM, NeilBrown wrote:
>>> On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
>>> 
>>>> From: Harald Hoyer <harald@redhat.com>
>>>> 
>>>> This does not trigger the udev inotify twice and saves a lot of blk
>>>> I/O for the raid members.
>>>> 
>>>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
>>>> 
>>>> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by: Jes 
>>>> Sorensen <Jes.Sorensen@redhat.com>
>>> 
>>> (Sorry for delays.  Thanks for reminders).
>>> 
>>> That patch seems to make sense, but the description above is awfully
>>> thin.
>>> 
>>> Why is double-open a problem exactly?  What does it make udev do?  And
>>> how does that related to ID_FS_TYPE being wrong as mentioned in the
>>> bugzilla entry.
>>> 
>>> NeilBrown
>>> 
> 
>> udevd with watch enabled (inotify on /dev/sd*) gets triggered on close(),
>> when you opened it writeable. So, if you double open() and udev wakes up
>> from the first close(), not all information are written to disk yet, it
>> will not get the ID_FS_TYPE.
> 
>> Seems like the second close() does not trigger an inotify sometimes, so
>> it is missing afterwards all the time.
> 
>> Watch via inotify is just a lazy workaround, so we don't have to modify
>> every tool to emit a "change" uevent, after they changed the disk.
> 
> So udev have a "lazy workaround" so that other programs don't need to
> trigger a change, and as a result, I need to add some special code to
> mdadm. Doesn't seem like I'm getting any advantage out of this laziness.
> 
> How about when udev gets an inotify for a block device, it first checks 
> that it can open it O_EXCL.  If not, it doesn't generate the change event. 
> That seems like the laziest option to me :-)

We cannot open with O_EXCL, because the device can be mounted, and O_EXCL
would fail there.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfhP+AAoJEANOs3ABTfJwONcP+gOzhDDvWFG0AZdocgttk04o
oV4mr2sde+NtuQMB8ac9v4Y0XP2HnpAYOVAJP7LALpkNHwUYGxtDDbqwWLGQWfPy
xXt6Zq9jE/srwmbBTmCB5sROS5lw6c+M2ft4kza2PDDY+9xyTjxVqVf2tcRGa3Fh
LIFNnKGk58qrgAI/46Muw0rbfMZOpOPuLNrI2aYwl/xmrXmQVXfGLIb+Rms9cdsY
Duk/U05t/DxYupic/fAvkuDmuUzsyBAhpuixL/774m7J3yzf6sNIadtZgy3rMiym
WNz+C8Ltx4d2H1nGmjkw9rKbmnWye15lLfsJGZQxvtt9LkWCPu6L9KbLKQyjjnc9
DBIj/6vID3JhxoLRE7H+zQXk7b9VX6sWAKPo6iFC4LcD8tESs01Jz0bV2kyMcUsZ
1HucYgcd2yzaaMBdEB9LleXd7SCtiBW74qTa/WgFK26notiyIGVWnfBdr9TYi1cB
UJLbsa9XHadq/OW4qVEBWbDbkM6Wpwkc2W7oosh3klauifbn2PXguwp5oeT/CQ1y
HORW36Xt9myAcZWY9iBjFRMwxJNztF89XYWOIzZ5j9fV1wHvCqYqNN+gtyt5vq97
N+Pn4pJ2Nuyygxu++aSv271JBga0YZCIGlIE38J1mJVgZlNAFXIGELs30NhYNhI2
GZ3eOMhawdR9k3NkQbMM
=hDz6
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  6:32         ` Harald Hoyer
@ 2013-04-29  6:53           ` NeilBrown
  2013-04-29  8:34             ` Harald Hoyer
  2013-04-29  8:40             ` Harald Hoyer
  0 siblings, 2 replies; 11+ messages in thread
From: NeilBrown @ 2013-04-29  6:53 UTC (permalink / raw)
  To: Harald Hoyer; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, 29 Apr 2013 08:32:31 +0200 Harald Hoyer <harald@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/29/2013 08:11 AM, NeilBrown wrote:
> > On Mon, 29 Apr 2013 07:33:21 +0200 Harald Hoyer <harald@redhat.com> wrote:
> > 
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> > 
> >> On 04/29/2013 02:57 AM, NeilBrown wrote:
> >>> On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
> >>> 
> >>>> From: Harald Hoyer <harald@redhat.com>
> >>>> 
> >>>> This does not trigger the udev inotify twice and saves a lot of blk
> >>>> I/O for the raid members.
> >>>> 
> >>>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
> >>>> 
> >>>> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by: Jes 
> >>>> Sorensen <Jes.Sorensen@redhat.com>
> >>> 
> >>> (Sorry for delays.  Thanks for reminders).
> >>> 
> >>> That patch seems to make sense, but the description above is awfully
> >>> thin.
> >>> 
> >>> Why is double-open a problem exactly?  What does it make udev do?  And
> >>> how does that related to ID_FS_TYPE being wrong as mentioned in the
> >>> bugzilla entry.
> >>> 
> >>> NeilBrown
> >>> 
> > 
> >> udevd with watch enabled (inotify on /dev/sd*) gets triggered on close(),
> >> when you opened it writeable. So, if you double open() and udev wakes up
> >> from the first close(), not all information are written to disk yet, it
> >> will not get the ID_FS_TYPE.
> > 
> >> Seems like the second close() does not trigger an inotify sometimes, so
> >> it is missing afterwards all the time.
> > 
> >> Watch via inotify is just a lazy workaround, so we don't have to modify
> >> every tool to emit a "change" uevent, after they changed the disk.
> > 
> > So udev have a "lazy workaround" so that other programs don't need to
> > trigger a change, and as a result, I need to add some special code to
> > mdadm. Doesn't seem like I'm getting any advantage out of this laziness.
> > 
> > How about when udev gets an inotify for a block device, it first checks 
> > that it can open it O_EXCL.  If not, it doesn't generate the change event. 
> > That seems like the laziest option to me :-)
> 
> We cannot open with O_EXCL, because the device can be mounted, and O_EXCL
> would fail there.
> 

If the device is mounted, why would you want udev to be doing anything to it?

I assumed this was for things like "mkfs" so that as soon as you mkfs a
filesystem udev could tell udisks to immediately mount it...  though I'm not
sure this is a good idea.

I'm probably missing something important: what is the particular use case for
udev mapping a close-after-write to a change event?

Thanks,
NeilBrown
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIVAwUBUX4Y/jnsnt1WYoG5AQKvwA//XoMDQJrPb+Iu3X8+IgomwT0PX58Pqhu6
gTQ6o1LGBOX7ir3ddFn0nGN7dRX05dngVdbOZkVqrXwJePm+uE8E9+tFLsb6VCwM
77mZue44eNhLpgVQ3C7HMt1LWBy8traDBq0PgJYeNu6yOXaQTh+mINrJ2w1c2dpe
hQnvYGIwYOj4SYDj9IbTubCwoMn9f6B/kHFQ7xSjkeLQDrKtE3yIm0RfYHtUIXgz
vF9Zrtu9HBl+C4zMqLt8qPtNUGWymeGApPNYU2n1RAeKqBKL/C88tj7Rfj0TR5qp
VssHPJ+utGvO9sZADrWFm4cHNA3wHuhXYCQZFLjDRSijILXe/vGcIk0ooEeqmLSj
G/n1dqMsfOnvO6+I86kHlJTbouhkpdSTKQi+0+URctjecN6FutsEhCWkyOtOsUWK
v2XFdwUmx64JZ6h3StMjpV70lEaSZohFvgnigyT75rC/fL5jPoS0ss28o7dr1urN
lUDiV4Mbrx5xVDaixe96OeKP1ZHxtXKhCV7/6rmnRdFN6DQWNrWasiFz/LOVafjn
ndb6bc5IMLl7b2++JY3ySS6j1MKEnFoqXiO7RqZk20ZPWIkj/naCfnk1tOUx6tA5
U1O3tquFn3KYGKRl/idl0Xhh0NxnDCw5gCpBySFf74N10Y0VCYKpkAHsABsBO/uV
L+TDpWZQJKE=
=dYDW
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  6:53           ` NeilBrown
@ 2013-04-29  8:34             ` Harald Hoyer
  2013-04-29  8:40             ` Harald Hoyer
  1 sibling, 0 replies; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  8:34 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 08:53 AM, NeilBrown wrote:
> On Mon, 29 Apr 2013 08:32:31 +0200 Harald Hoyer <harald@redhat.com> wrote:
> 
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> 
>> On 04/29/2013 08:11 AM, NeilBrown wrote:
>>> On Mon, 29 Apr 2013 07:33:21 +0200 Harald Hoyer <harald@redhat.com>
>>> wrote:
>>> 
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>> 
>>>> On 04/29/2013 02:57 AM, NeilBrown wrote:
>>>>> On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
>>>>> 
>>>>>> From: Harald Hoyer <harald@redhat.com>
>>>>>> 
>>>>>> This does not trigger the udev inotify twice and saves a lot of
>>>>>> blk I/O for the raid members.
>>>>>> 
>>>>>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
>>>>>> 
>>>>>> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by:
>>>>>> Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>> 
>>>>> (Sorry for delays.  Thanks for reminders).
>>>>> 
>>>>> That patch seems to make sense, but the description above is
>>>>> awfully thin.
>>>>> 
>>>>> Why is double-open a problem exactly?  What does it make udev do?
>>>>> And how does that related to ID_FS_TYPE being wrong as mentioned in
>>>>> the bugzilla entry.
>>>>> 
>>>>> NeilBrown
>>>>> 
>>> 
>>>> udevd with watch enabled (inotify on /dev/sd*) gets triggered on
>>>> close(), when you opened it writeable. So, if you double open() and
>>>> udev wakes up from the first close(), not all information are written
>>>> to disk yet, it will not get the ID_FS_TYPE.
>>> 
>>>> Seems like the second close() does not trigger an inotify sometimes,
>>>> so it is missing afterwards all the time.
>>> 
>>>> Watch via inotify is just a lazy workaround, so we don't have to
>>>> modify every tool to emit a "change" uevent, after they changed the
>>>> disk.
>>> 
>>> So udev have a "lazy workaround" so that other programs don't need to 
>>> trigger a change, and as a result, I need to add some special code to 
>>> mdadm. Doesn't seem like I'm getting any advantage out of this
>>> laziness.
>>> 
>>> How about when udev gets an inotify for a block device, it first
>>> checks that it can open it O_EXCL.  If not, it doesn't generate the
>>> change event. That seems like the laziest option to me :-)
> 
>> We cannot open with O_EXCL, because the device can be mounted, and
>> O_EXCL would fail there.
> 
> 
> If the device is mounted, why would you want udev to be doing anything to
> it?
> 
> I assumed this was for things like "mkfs" so that as soon as you mkfs a 
> filesystem udev could tell udisks to immediately mount it...  though I'm
> not sure this is a good idea.
> 
> I'm probably missing something important: what is the particular use case
> for udev mapping a close-after-write to a change event?
> 
> Thanks, NeilBrown

Well, you would not recognize LABEL or UUID or MD UUID changes, if you don't
call blkid after a mkfs or mdadm or lvm action.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfjCeAAoJEANOs3ABTfJwLQIP/iaKQFUYyE5tsw8zVVxw+ag6
QPhs08y8JM1nPNyC4hd2BKLwx687eXVIR7Kyfx8i9L2dEHLn2ry+ROTAbfn1UEr2
GjpD48wuNiohJKUZ5l8Wn3q8slboUImgHUkiZgtWII0pCk3vGo+/01WIWzSzDRlN
PsI8TS4+wBIdZhfWMtzU65uXiZYoGpaURqIv+7xIgEN90mM45kE1FA/HJF8YZOBz
H8CMI2pr+GUI/XCUjUYJK5m3y1sJCm5US5IsR4AqpQGlkU2G9pFQYRWFS4uYKOsF
tjk7Fj8wFw51gz/dWGlKM9uK+iAFHVVorjFPdDJilSdswWP88JMrwlfAkivxbKSA
6JsKEOSLlkuW5gNRl6Flg2/hLBmlJYUx3nXzT4WZWgibD4mkKAmY1uB0gnzE2/lm
YP7dEs5WVo5Rl3lM+Mv6vqDl6dJCvg60AU3u5srWwfqUau81p5rYIuqXJ5Da4GcU
njz6NUwzMm2FI91INDDi/GJzwi1bVV6CeTNiz3LNjd2+yr2dt0AFBZWQ3k1c+/rO
WJ8NhN6dW8JZEL4VN5yfEht4x+kVnxqdTedAm5NtPooSD3we2w/e5u5im3FfUkJN
GX4+Et2sE5gk70Es/u2Ws1tLOdpxZsUiU/HBgS51+8KBIumj+Me3BLZpGkvqwNm/
vV2Fov6yJhM5E0/lYAhn
=xI4C
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  6:53           ` NeilBrown
  2013-04-29  8:34             ` Harald Hoyer
@ 2013-04-29  8:40             ` Harald Hoyer
  2013-04-29  8:45               ` Harald Hoyer
  1 sibling, 1 reply; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  8:40 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 08:53 AM, NeilBrown wrote:
> On Mon, 29 Apr 2013 08:32:31 +0200 Harald Hoyer <harald@redhat.com> wrote:
> 
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> 
>> On 04/29/2013 08:11 AM, NeilBrown wrote:
>>> On Mon, 29 Apr 2013 07:33:21 +0200 Harald Hoyer <harald@redhat.com>
>>> wrote:
>>> 
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>> 
>>>> On 04/29/2013 02:57 AM, NeilBrown wrote:
>>>>> On Thu, 11 Apr 2013 15:18:33 +0200 Jes.Sorensen@redhat.com wrote:
>>>>> 
>>>>>> From: Harald Hoyer <harald@redhat.com>
>>>>>> 
>>>>>> This does not trigger the udev inotify twice and saves a lot of
>>>>>> blk I/O for the raid members.
>>>>>> 
>>>>>> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=947815
>>>>>> 
>>>>>> Signed-off-by: Harald Hoyer <harald@redhat.com> Signed-off-by:
>>>>>> Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>> 
>>>>> (Sorry for delays.  Thanks for reminders).
>>>>> 
>>>>> That patch seems to make sense, but the description above is
>>>>> awfully thin.
>>>>> 
>>>>> Why is double-open a problem exactly?  What does it make udev do?
>>>>> And how does that related to ID_FS_TYPE being wrong as mentioned in
>>>>> the bugzilla entry.
>>>>> 
>>>>> NeilBrown
>>>>> 
>>> 
>>>> udevd with watch enabled (inotify on /dev/sd*) gets triggered on
>>>> close(), when you opened it writeable. So, if you double open() and
>>>> udev wakes up from the first close(), not all information are written
>>>> to disk yet, it will not get the ID_FS_TYPE.
>>> 
>>>> Seems like the second close() does not trigger an inotify sometimes,
>>>> so it is missing afterwards all the time.
>>> 
>>>> Watch via inotify is just a lazy workaround, so we don't have to
>>>> modify every tool to emit a "change" uevent, after they changed the
>>>> disk.
>>> 
>>> So udev have a "lazy workaround" so that other programs don't need to 
>>> trigger a change, and as a result, I need to add some special code to 
>>> mdadm. Doesn't seem like I'm getting any advantage out of this
>>> laziness.
>>> 
>>> How about when udev gets an inotify for a block device, it first
>>> checks that it can open it O_EXCL.  If not, it doesn't generate the
>>> change event. That seems like the laziest option to me :-)
> 
>> We cannot open with O_EXCL, because the device can be mounted, and
>> O_EXCL would fail there.
> 
> 
> If the device is mounted, why would you want udev to be doing anything to
> it?
> 
> I assumed this was for things like "mkfs" so that as soon as you mkfs a 
> filesystem udev could tell udisks to immediately mount it...  though I'm
> not sure this is a good idea.
> 
> I'm probably missing something important: what is the particular use case
> for udev mapping a close-after-write to a change event?
> 
> Thanks, NeilBrown
> 


Anyway, if you don't want to play nicely with the inotify mechanism of udev,
you have to inject the "change" uevent manually for every device mdadm changes.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfjHrAAoJEANOs3ABTfJwgosP/i1btIR88rZ5z8eYB5P6sZqT
XU0nUP1F4yMS1K4MOWDggQbuzcuxFowcnizg4Jje26c4z3kQ7Pj75GvqWwI3qqYp
+TdG+idu7kGPeQtYa05I567pj20D6nWYxC78aGJPlBU6C0qPvA1yXb7ui4NPJcw4
2/oRH2BONpb62VCQKCB04rQhqOXnzp/9agaqAL7hJcUOsbJv8vceLW0rXD0RqTzO
uQXQjUV2bYR73ySRZQWo2evaxZ/YgWDWL91h7R1O1wYvTclMNYqv+SQB9hDyrrDi
mk4YFWxdRUmdIzyr6FkZUUTj1KSpmrW01PaaIi3ueHV27Pvmz+7+1jd5JVvw9GiM
hm8Ob3baiGPMIfFZ7mfLBLlizBu0N4QTK5mm7D2btFS9phHb9/QzNpBtdAB15CTQ
8UF4IZg9HnbkG8XAedW97D3QS40873kPp7UPtsScnFe7+VcOh05s3AzF9zznji/C
kEcpPIjthK50RLniWBEoKNEjbfdpyF1PLsvQ7GkQNIoUHSveCPJQmaG0GV8AIPht
tNteLCImjeaUf57bi/BfKk5L42dwe8wqfmBGw40nbzmavRrYHEvUK3CMMR3C7TRZ
lvGzXInwxJpR2hkN8A9nX13FOX3YtVLXUtl5R6CVvcrYM6ZK78D1z5liiZcfKv2Y
XiUwQtkeIlscVHeW1QdO
=fzOH
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  8:40             ` Harald Hoyer
@ 2013-04-29  8:45               ` Harald Hoyer
  2013-04-29  8:54                 ` Harald Hoyer
  0 siblings, 1 reply; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  8:45 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 10:40 AM, Harald Hoyer wrote:
> On 04/29/2013 08:53 AM, NeilBrown wrote:
>> On Mon, 29 Apr 2013 08:32:31 +0200 Harald Hoyer <harald@redhat.com>
>> wrote:
>>> We cannot open with O_EXCL, because the device can be mounted, and 
>>> O_EXCL would fail there.
> 
> 
>> If the device is mounted, why would you want udev to be doing anything
>> to it?
> 
>> I assumed this was for things like "mkfs" so that as soon as you mkfs a 
>> filesystem udev could tell udisks to immediately mount it...  though I'm 
>> not sure this is a good idea.
> 
>> I'm probably missing something important: what is the particular use
>> case for udev mapping a close-after-write to a change event?
> 
>> Thanks, NeilBrown
> 
> 
> 
> Anyway, if you don't want to play nicely with the inotify mechanism of
> udev, you have to inject the "change" uevent manually for every device
> mdadm changes.
> 
*rant mode on*

I think we should drop the inotify mechanism from udev and say "f*** you" to
the tools... If they change s.th. on the disk, they can also emit a "change"
uevent to the kernel themselves.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfjM3AAoJEANOs3ABTfJwRwoP/jtGvFdNonRF71txWxTzvsas
QErwTrojNdpRT850rCHIMNF9x/iYSMaVJcMGBx+H3zyZCDLiYwr7qvXUNWh9frC2
FQvJEqhpoH4ku17wT2eX4XYhIOC7/qudsS9EUfQ/NZw4her2uXbHosSJjfBjsp0f
voVaU1dXZ4jsyA5nybH2fN0L70HpMei4AU9/lhI5UtBZa6erOe3o/kc0rT+E9o9z
n4l+r6DI2zsgw/Cg0O59N6G9otd2zYuj2QLosPmG4Rx0yEcoC8Tn04ncJ8e5Qw4E
GW0sm7SvP2tSlQvZFltKpGjEMPuKMXkcmgonkmGpZGSzpxF2evH+6SyK0lN4E7IK
Guz6s5r+ecj6WWGneX7H+RD7EV4UIYHirr8txhS7QXmPiXR0+OJ4S5BPGdnxLBUf
D/ttJUwA9sSSGdVx4M5iH39o/cC3I6XdUXqa7A6gpEsNZHE44masIVfdUIOqkv9a
1Ok3sArMWFkE+RtsRltzmcnGGyj58W4wBwtMp5gzbqfSGqpONzSt4uETzs1cTJWt
qQdWMGFsb4WKjHmjoG0ohr70r5zNubpeXfQlWzAFcFSWcqPmKK4J/Wk9hrB2kBo/
TNdgAxyRJOzGh/uNF0XVbEuL2dfIxjc25AadYJs9eq3wUQrs5C3ppL3lHI4/vKFi
AAwQBFAqj/b3MBJ5FR95
=r9Au
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] prevent double open(O_RDWR) on raid creation
  2013-04-29  8:45               ` Harald Hoyer
@ 2013-04-29  8:54                 ` Harald Hoyer
  0 siblings, 0 replies; 11+ messages in thread
From: Harald Hoyer @ 2013-04-29  8:54 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid, Kay Sievers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/29/2013 10:45 AM, Harald Hoyer wrote:
> On 04/29/2013 10:40 AM, Harald Hoyer wrote:
>> On 04/29/2013 08:53 AM, NeilBrown wrote:
>>> On Mon, 29 Apr 2013 08:32:31 +0200 Harald Hoyer <harald@redhat.com> 
>>> wrote:
>>>> We cannot open with O_EXCL, because the device can be mounted, and 
>>>> O_EXCL would fail there.
> 
> 
>>> If the device is mounted, why would you want udev to be doing anything 
>>> to it?
> 
>>> I assumed this was for things like "mkfs" so that as soon as you mkfs a
>>>  filesystem udev could tell udisks to immediately mount it...  though
>>> I'm not sure this is a good idea.
> 
>>> I'm probably missing something important: what is the particular use 
>>> case for udev mapping a close-after-write to a change event?
> 
>>> Thanks, NeilBrown
> 
> 
> 
>> Anyway, if you don't want to play nicely with the inotify mechanism of 
>> udev, you have to inject the "change" uevent manually for every device 
>> mdadm changes.
> 
> *rant mode on*
> 
> I think we should drop the inotify mechanism from udev and say "f*** you"
> to the tools... If they change s.th. on the disk, they can also emit a
> "change" uevent to the kernel themselves.
> 

Oh, and the tools then also have wait for udev to settle..

Imagine a script doing:

# mdadm -A --uuid=<my-UUID> ....
# mkfs  -L mylabel /dev/disk/by-uuid/<my-MD-UUID>
# mount /dev/disk/by-label/mylabel /mnt

This is all broken by design today:
- - there is no guaranteed change event
- - there is no automatic udev settle
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRfjUtAAoJEANOs3ABTfJw/8IP/jX1RUgSQQwqM5QG1JUVXOZT
Q/Uzuf4bDSdlHMrM0FjH6KNNDgh80fqwKBnb/sCsSNVRYgHdlLJXTdFYfzzWPk4t
2q7Rw/M8xQS9OnqzFy8BhkdvChyW8t9Pl2Owt4+6B++GJ+2ZZksb66ggsGca4bUE
XjLDUVJ8zk+VjVyg0yFMtUMwbJ88q14I2b+eAnCenXt/RoIGqMIX5ds8PEiowQeo
zVMjo0nXHyaDZPV6DkDIGtQ2vsnLGTc2V+SWzTH8vZJwbHERQNQczkcSiVFmt1JA
kgOkcSiq80FSrNAkD/gYdmZ9Qy4Txo5y1/gnaoWubehkVctQpezdjWslnCY0HTKL
ledREAUq562m9I5etIUINWjUyWJFLyBDD3a9TI3psg1dV4nNr7MmTf1OsCBaIhZt
EvqXTyo03K0tgpQHUJCj/ZgNejgvoMJGVYIxMdIC64+gZcM3iHqMkkJjKHTvwc+7
Hh0Mn8L/XwYX5PoBrOJrg3XAAt4S97gA/Opv90q7VrEz47+04KHzI5T4PQ2B8MzA
S+CZyDGMAazwSXTmaaEenXOaQKnYepiMQ/kFMM5teyaRKOPg3HqMPHWQnJq+/+bC
ktA3X7wFQ5QLa8v5G2TpVPxvwTY+8gpmYWzXx+qhFZAMQne3RLd7zQiclNzUFkLK
Nhnvkqel+fvi/0rFVx/X
=SYBt
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-04-29  8:54 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-11 13:18 [PATCH 0/1] Reduce unnecessary opens of raid members Jes.Sorensen
2013-04-11 13:18 ` [PATCH 1/1] prevent double open(O_RDWR) on raid creation Jes.Sorensen
2013-04-29  0:57   ` NeilBrown
2013-04-29  5:33     ` Harald Hoyer
2013-04-29  6:11       ` NeilBrown
2013-04-29  6:32         ` Harald Hoyer
2013-04-29  6:53           ` NeilBrown
2013-04-29  8:34             ` Harald Hoyer
2013-04-29  8:40             ` Harald Hoyer
2013-04-29  8:45               ` Harald Hoyer
2013-04-29  8:54                 ` Harald Hoyer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).