bcache-tools: minor change for bcache-register

bcache-tools: minor change for bcache-register

[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
@@ -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

Re: bcache-tools: minor change for bcache-register

[Darrick J. Wong]

On Mon, Aug 26, 2013 at 11:14:06AM +0200, Rolf Fokkens wrote:
> 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
> @@ -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.

/sys/fs/bcache/register_quiet

--D
> 
> Rolf
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bcache-tools: minor change for bcache-register

[Gabriel de Perthuis]

On Mon, 26 Aug 2013 09:17:58 -0700, Darrick J. Wong wrote:
> On Mon, Aug 26, 2013 at 11:14:06AM +0200, Rolf Fokkens wrote:
>> /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.

[PATCH] bcache: Downgrade "device already registered" to pr_debug

[Gabriel de Perthuis]

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 <g2p.code-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/md/bcache/super.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index a41c681..b09e973 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -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";
-- 
1.8.4.rc0.5.g48765cc

Re: bcache-tools: minor change for bcache-register

[Kent Overstreet]

On Mon, Aug 26, 2013 at 11:14:06AM +0200, Rolf Fokkens wrote:
> 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
> @@ -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.

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

I'll change it to do that.

Re: bcache-tools: minor change for bcache-register

[Kent Overstreet]

On Mon, Aug 26, 2013 at 04:31:25PM +0000, Gabriel de Perthuis wrote:
> On Mon, 26 Aug 2013 09:17:58 -0700, Darrick J. Wong wrote:
> > On Mon, Aug 26, 2013 at 11:14:06AM +0200, Rolf Fokkens wrote:
> >> /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.

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

Re: bcache-tools: minor change for bcache-register

[Gabriel de Perthuis]

>> 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.

[PATCH] bcache: register_quiet should still report errors

[Gabriel de Perthuis]

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 <g2p.code-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/md/bcache/super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index a41c681..c3a3dc8 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -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)
-- 
1.8.4.rc0.5.g48765cc

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

[Gabriel de Perthuis]

> 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.

Re: bcache-tools: minor change for bcache-register

[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
@@ -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.

On 08/26/2013 11:41 PM, Kent Overstreet wrote:
> On Mon, Aug 26, 2013 at 11:14:06AM +0200, Rolf Fokkens wrote:
>> 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
>> @@ -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.
> Just use /sys/fs/bcache/register_quiet instead of register.
>
> I'll change it to do that.

Re: bcache-tools: minor change for bcache-register

[Alex Pyrgiotis]

On 08/27/2013 09:58 AM, Rolf Fokkens wrote:
> 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
> @@ -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.
> 

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...

-- 
Alex | apyrgio-Sqt7GMbKoOQ@public.gmane.org

Re: bcache-tools: minor change for bcache-register

[Gabriel de Perthuis]

On Tue, 27 Aug 2013 13:00:24 +0300, Alex Pyrgiotis wrote:
> On 08/27/2013 09:58 AM, Rolf Fokkens wrote:
>> -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,
> 
> 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.

Re: bcache-tools: minor change for bcache-register

[Rolf Fokkens]

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.

On 08/27/2013 01:45 PM, Gabriel de Perthuis wrote:
> On Tue, 27 Aug 2013 13:00:24 +0300, Alex Pyrgiotis wrote:
>> On 08/27/2013 09:58 AM, Rolf Fokkens wrote:
>>> -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,
>>
>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: bcache-tools: minor change for bcache-register

[Gabriel de Perthuis]

> 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).