bcache-tools: minor change for bcache-register

PATCH New Rolf Fokkens 6 participants 14 messages Aug 26, 2013 – Aug 27, 2013 3 messages not in the archive bcache (block layer cache)

lore

Share & tools

lore

Rolf Fokkens

author

2013-08-26 09:14 UTC #0 lore raw

Hi!

bcache-register is called from the provided udev rules in bcache-tools. During boot it seams like some devices are registered multiple times, which results in messages during boot. To suppress these messages I redirect them to /dev/null in the following patch:

bcache-register 2013-08-25 21:31:11.707823194 +0200 renamed from bcache-register.register 2013-08-20 22:03:46.000000000 +0200 +7 -2

@@ -1,4 +1,9 @@
 #!/bin/sh
-modprobe -qba bcache
-test -f /sys/fs/bcache/register && echo "$1" > /sys/fs/bcache/register
+#
+# Utility script to be called from udev, so it's not monkey proof at all. It's
+# sole purpose is to load the bcache kernel module whenever a bcache device is
+# detected.
+#
+/usr/sbin/modprobe -qba bcache
+test -f /sys/fs/bcache/register && echo "$1" > /sys/fs/bcache/register 2>/dev/null

I can imagine there's a more elegant solution, suggestions are welcome.

Rolf

A message that is not in the archive (sent off-list or not yet received): [email protected]

Darrick J. Wong

2013-08-26 16:17 UTC #2 lore raw

Quoted 26 lines · Rolf Fokkens

Hi!
bcache-register is called from the provided udev rules in bcache-tools. During boot it seams like some devices are registered multiple times, which results in messages during boot. To suppress these messages I redirect them to /dev/null in the following patch:
--- bcache-tools-20130820/bcache-register.register 2013-08-20
22:03:46.000000000 +0200
+++ bcache-tools-20130820/bcache-register 2013-08-25
21:31:11.707823194 +0200
bcache-register 2013-08-25 21:31:11.707823194 +0200 renamed from bcache-register.register 2013-08-20 22:03:46.000000000 +0200 +2 -2
@@ -1,4 +1,9 @@
 #!/bin/sh
-modprobe -qba bcache
-test -f /sys/fs/bcache/register && echo "$1" > /sys/fs/bcache/register
+#
+# Utility script to be called from udev, so it's not monkey proof
at all. It's
+# sole purpose is to load the bcache kernel module whenever a bcache device is
bcache-register 2013-08-25 21:31:11.707823194 +0200 renamed from bcache-register.register 2013-08-20 22:03:46.000000000 +0200 +4 -0
+# detected.
+#
+/usr/sbin/modprobe -qba bcache
+test -f /sys/fs/bcache/register && echo "$1" >
/sys/fs/bcache/register 2>/dev/null

Quoted 1 line

I can imagine there's a more elegant solution, suggestions are welcome.

/sys/fs/bcache/register_quiet

--D

Quoted 7 lines

Rolf
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Gabriel de Perthuis

2013-08-26 16:31 UTC #3 lore raw

Quoted 6 lines · Darrick J. Wong

Quoted 3 lines · Rolf Fokkens
/sys/fs/bcache/register 2>/dev/null
I can imagine there's a more elegant solution, suggestions are welcome.
/sys/fs/bcache/register_quiet

Good point: the messages come from the kernel.

register_quiet would make legitimate errors harder to debug however.

I'll just send a patch to change "device already registered" to a low-priority pr_debug message rather than something that looks like an error.

Gabriel de Perthuis

2013-08-26 16:54 UTC #4 lore raw

PATCH 1/1 bcache: Downgrade "device already registered" to pr_debug

People tended to see:

(info) bcache: register_bcache() error opening /dev/blah: device already registered

as more problematic than it really is. Change it to:

(debug) bcache: register_bcache() device /dev/blah is already registered

and don't return -EINVAL in this case.

Signed-off-by Gabriel de Perthuis <[email protected]>

---
 drivers/md/bcache/super.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/md/bcache/super.c +8 -5

index a41c681..b09e973 100644
@@ -1894,21 +1894,24 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
 
 	if (!(path = kstrndup(buffer, size, GFP_KERNEL)) ||
 	    !(sb = kmalloc(sizeof(struct cache_sb), GFP_KERNEL)))
 		goto err;
 
-	err = "failed to open device";
 	bdev = blkdev_get_by_path(strim(path),
 				  FMODE_READ|FMODE_WRITE|FMODE_EXCL,
 				  sb);
 	if (IS_ERR(bdev)) {
 		if (bdev == ERR_PTR(-EBUSY)) {
 			bdev = lookup_bdev(strim(path));
-			if (!IS_ERR(bdev) && bch_is_open(bdev))
-				err = "device already registered";
-			else
-				err = "device busy";
+			if (!IS_ERR(bdev) && bch_is_open(bdev)) {
+				pr_debug("device %s is already registered",
+					 path);
+				goto out;
+			}
+			err = "device busy";
+		} else {
+			err = "failed to open device";
 		}
 		goto err;
 	}
 
 	err = "failed to set blocksize";

Signature · 1 line

1.8.4.rc0.5.g48765cc

Kent Overstreet

2013-08-26 21:48 UTC #5 lore raw

In reply to Gabriel de Perthuis

Quoted 8 lines · Gabriel de Perthuis

Quoted 6 lines · Darrick J. Wong
Quoted 3 lines · Rolf Fokkens
/sys/fs/bcache/register 2>/dev/null
I can imagine there's a more elegant solution, suggestions are welcome.
/sys/fs/bcache/register_quiet

Quoted 7 lines

Good point: the messages come from the kernel.
register_quiet would make legitimate errors harder to debug however.
I'll just send a patch to change "device already registered" to a low-priority pr_debug message rather than something that looks like an error.

register_quiet isn't supposed to suppress legit errors - if it does that's a bug

Gabriel de Perthuis

2013-08-26 22:16 UTC #6 lore raw

Quoted 6 lines

Quoted 3 lines
I'll just send a patch to change "device already registered" to a low-priority pr_debug message rather than something that looks like an error.
register_quiet isn't supposed to suppress legit errors - if it does that's a bug

It does suppress all the err = "…" messages in register_bcache from super.c; I don't know how often they come up in practice. From the read_super call on, errors get reported by the called functions.

Gabriel de Perthuis

2013-08-26 22:31 UTC #7 lore raw

PATCH 1/1 bcache: register_quiet should still report errors

register_quiet should still report all errors except "device already registered".

Before this some potential errors would be silenced.

Signed-off-by Gabriel de Perthuis <[email protected]>

---
 drivers/md/bcache/super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/md/bcache/super.c +6 -4

index a41c681..c3a3dc8 100644
@@ -1901,14 +1901,17 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
 				  FMODE_READ|FMODE_WRITE|FMODE_EXCL,
 				  sb);
 	if (IS_ERR(bdev)) {
 		if (bdev == ERR_PTR(-EBUSY)) {
 			bdev = lookup_bdev(strim(path));
-			if (!IS_ERR(bdev) && bch_is_open(bdev))
+			if (!IS_ERR(bdev) && bch_is_open(bdev)) {
+				if (attr == &ksysfs_register_quiet)
+					goto out;
 				err = "device already registered";
-			else
+			} else {
 				err = "device busy";
+			}
 		}
 		goto err;
 	}
 
 	err = "failed to set blocksize";

@@ -1942,12 +1945,11 @@ out:
 	return ret;
 
 err_close:
 	blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
 err:
-	if (attr != &ksysfs_register_quiet)
-		pr_info("error opening %s: %s", path, err);
+	pr_info("error opening %s: %s", path, err);
 	ret = -EINVAL;
 	goto out;
 }
 
 static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)

Signature · 1 line

1.8.4.rc0.5.g48765cc

Gabriel de Perthuis

2013-08-26 22:37 UTC #8 lore raw

Re: [PATCH] bcache: register_quiet should still report errors

Quoted 4 lines

register_quiet should still report all errors except "device already registered".
Before this some potential errors would be silenced.

This can replace my other patch in this thread. Switching /lib/udev/bcache-register to register_quiet without this kernel patch will suppress the aforementioned errors, but they probably aren't too common so we can live with it.

Kent Overstreet

2013-08-26 21:41 UTC #9 lore raw

In reply to a message not in the archive

Quoted 26 lines · Rolf Fokkens

Hi!
bcache-register is called from the provided udev rules in bcache-tools. During boot it seams like some devices are registered multiple times, which results in messages during boot. To suppress these messages I redirect them to /dev/null in the following patch:
--- bcache-tools-20130820/bcache-register.register 2013-08-20
22:03:46.000000000 +0200
+++ bcache-tools-20130820/bcache-register 2013-08-25
21:31:11.707823194 +0200
bcache-register 2013-08-25 21:31:11.707823194 +0200 renamed from bcache-register.register 2013-08-20 22:03:46.000000000 +0200 +2 -2
@@ -1,4 +1,9 @@
 #!/bin/sh
-modprobe -qba bcache
-test -f /sys/fs/bcache/register && echo "$1" > /sys/fs/bcache/register
+#
+# Utility script to be called from udev, so it's not monkey proof
at all. It's
+# sole purpose is to load the bcache kernel module whenever a bcache device is
bcache-register 2013-08-25 21:31:11.707823194 +0200 renamed from bcache-register.register 2013-08-20 22:03:46.000000000 +0200 +4 -0
+# detected.
+#
+/usr/sbin/modprobe -qba bcache
+test -f /sys/fs/bcache/register && echo "$1" >
/sys/fs/bcache/register 2>/dev/null

Quoted 1 line

I can imagine there's a more elegant solution, suggestions are welcome.

Just use /sys/fs/bcache/register_quiet instead of register.

I'll change it to do that.

Rolf Fokkens

author

2013-08-27 06:58 UTC #10 lore raw

Would you consider another change as well:

bcache-register 2013-08-27 08:55:48.060477014 +0200 renamed from bcache-register 2013-08-26 23:46:19.000000000 +0200 +1 -1

@@ -1,4 +1,4 @@
 #!/bin/sh
-modprobe -qba bcache
+/sbin/modprobe -qba bcache
 test -f /sys/fs/bcache/register_quiet && echo "$1" >

/sys/fs/bcache/register_quiet

/sbin is not in the $PATH for utilities started from udev. For Fedora it isn't anyway, but it won't harm other distro's neither.

Quoted 31 lines · Kent Overstreet

Quoted 26 lines · Rolf Fokkens
Hi!
bcache-register is called from the provided udev rules in bcache-tools. During boot it seams like some devices are registered multiple times, which results in messages during boot. To suppress these messages I redirect them to /dev/null in the following patch:
--- bcache-tools-20130820/bcache-register.register 2013-08-20
22:03:46.000000000 +0200
+++ bcache-tools-20130820/bcache-register 2013-08-25
21:31:11.707823194 +0200
(quoted diff) +2 -2
@@ -1,4 +1,9 @@
  #!/bin/sh
-modprobe -qba bcache
-test -f /sys/fs/bcache/register && echo "$1" > /sys/fs/bcache/register
+#
+# Utility script to be called from udev, so it's not monkey proof
at all. It's
+# sole purpose is to load the bcache kernel module whenever a bcache device is
(quoted diff) +4 -0
+# detected.
+#
+/usr/sbin/modprobe -qba bcache
+test -f /sys/fs/bcache/register && echo "$1" >
/sys/fs/bcache/register 2>/dev/null
Quoted 1 line
I can imagine there's a more elegant solution, suggestions are welcome.
Just use /sys/fs/bcache/register_quiet instead of register.
I'll change it to do that.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alex Pyrgiotis

2013-08-27 10:00 UTC #12 lore raw

Quoted 13 lines · Rolf Fokkens

Would you consider another change as well:
--- bcache-tools-20130827.orig/bcache-register 2013-08-26
23:46:19.000000000 +0200
+++ bcache-tools-20130827/bcache-register 2013-08-27
08:55:48.060477014 +0200
bcache-register 2013-08-27 08:55:48.060477014 +0200 renamed from bcache-register 2013-08-26 23:46:19.000000000 +0200 +1 -1
@@ -1,4 +1,4 @@
  #!/bin/sh
-modprobe -qba bcache
+/sbin/modprobe -qba bcache
  test -f /sys/fs/bcache/register_quiet && echo "$1" > 
/sys/fs/bcache/register_quiet

Quoted 3 lines

/sbin is not in the $PATH for utilities started from udev. For Fedora it isn't anyway, but it won't harm other distro's neither.

Hi all,

Pardon the interrupt, but this is not the case for Arch Linux:
https://www.archlinux.org/news/binaries-move-to-usrbin-requiring-update-intervention/

In my opinion, if modprobe *has* to be called with the exact path name, it would probably be clearer to use which modprobe and then call the result.

Just my two cents...

Signature · 1 line

Alex | [email protected]

Gabriel de Perthuis

2013-08-27 11:45 UTC #13 lore raw

Quoted 12 lines · Alex Pyrgiotis

Quoted 8 lines · Rolf Fokkens
bcache-register 2013-08-27 08:55:48.060477014 +0200 renamed from bcache-register 2013-08-26 23:46:19.000000000 +0200 +1 -1
-modprobe -qba bcache
+/sbin/modprobe -qba bcache
  test -f /sys/fs/bcache/register_quiet && echo "$1" > 
/sys/fs/bcache/register_quiet
/sbin is not in the $PATH for utilities started from udev. For Fedora it isn't anyway, but it won't harm other distro's neither.
Hi all,

Quoted 6 lines

Pardon the interrupt, but this is not the case for Arch Linux:
https://www.archlinux.org/news/binaries-move-to-usrbin-requiring-update-intervention/
In my opinion, if modprobe *has* to be called with the exact path name, it would probably be clearer to use which modprobe and then call the result.

IME the exact path name is unnecessary.
Rolf, your best shot at contributing is to bring up problems that have actually been encountered on Fedora. Changes need to come as a side effect of discussions, not the other way around.

Rolf Fokkens

author

2013-08-27 12:02 UTC #14 lore raw

Gabriel, I might have been unclear about this, but this __IS__ a problem on Fedora. Without a full path for modprobe (/sbin/modprobe or /usr/sbin/modprobe) in bcache-register, udev does not create the /dev/bcache* devices. So at the moment I fix it in the package.

If this is however Fedora specific I'll have the fix it in the package of course. But it's hard for me to tell, and /sbin/modprobe seemed like a cross-distro solution to me.

Quoted 27 lines · Gabriel de Perthuis

Quoted 11 lines · Alex Pyrgiotis
Quoted 8 lines · Rolf Fokkens
bcache-register 2013-08-27 08:55:48.060477014 +0200 renamed from bcache-register 2013-08-26 23:46:19.000000000 +0200 +1 -1
-modprobe -qba bcache
+/sbin/modprobe -qba bcache
   test -f /sys/fs/bcache/register_quiet && echo "$1" >
/sys/fs/bcache/register_quiet
/sbin is not in the $PATH for utilities started from udev. For Fedora it isn't anyway, but it won't harm other distro's neither.
Hi all,
Quoted 6 lines
Pardon the interrupt, but this is not the case for Arch Linux:
https://www.archlinux.org/news/binaries-move-to-usrbin-requiring-update-intervention/
In my opinion, if modprobe *has* to be called with the exact path name, it would probably be clearer to use which modprobe and then call the result.
IME the exact path name is unnecessary.
Rolf, your best shot at contributing is to bring up problems that have actually been encountered on Fedora. Changes need to come as a side effect of discussions, not the other way around.
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Gabriel de Perthuis

2013-08-27 12:11 UTC #16 lore raw

Quoted 8 lines

Gabriel, I might have been unclear about this, but this __IS__ a problem on Fedora. Without a full path for modprobe (/sbin/modprobe or /usr/sbin/modprobe) in bcache-register, udev does not create the /dev/bcache* devices. So at the moment I fix it in the package.
If this is however Fedora specific I'll have the fix it in the package of course. But it's hard for me to tell, and /sbin/modprobe seemed like a cross-distro solution to me.

Oh, sorry. Apparently it's not possible to pick the same path due to Arch/Ubuntu differences, and it's not possible to use PATH due to Fedora. I'm fine with your solution since we haven't hard from an Arch packager (the AUR package is lagging way behind).