[PATCH] ubifs: silence error output if MS_SILENT is set

[PATCH] ubifs: silence error output if MS_SILENT is set

[Daniel Golle]

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 fs/ubifs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7034995..ae32b3c 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
 	 */
 	ubi = open_ubi(name, UBI_READONLY);
 	if (IS_ERR(ubi)) {
-		pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
-		       current->pid, name, (int)PTR_ERR(ubi));
+		if (flags & MS_SILENT)
+			pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
+			       current->pid, name, (int)PTR_ERR(ubi));
 		return ERR_CAST(ubi);
 	}
 
-- 
2.8.3

Re: [PATCH] ubifs: silence error output if MS_SILENT is set

[Richard Weinberger]

On Sat, Jun 18, 2016 at 11:52 AM, Daniel Golle <daniel@makrotopia.org> wrote:
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  fs/ubifs/super.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 7034995..ae32b3c 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
>          */
>         ubi = open_ubi(name, UBI_READONLY);
>         if (IS_ERR(ubi)) {
> -               pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> -                      current->pid, name, (int)PTR_ERR(ubi));
> +               if (flags & MS_SILENT)
> +                       pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> +                              current->pid, name, (int)PTR_ERR(ubi));

This needs a more detailed explanation.
Why does UBIFS need this? Only very few filesystems care about MS_SILENT.
What problem are you trying to address?

-- 
Thanks,
//richard

Re: [PATCH] ubifs: silence error output if MS_SILENT is set

[Daniel Golle]

Hi Richard,

first of all, thanks for reviewing my patch!

On Sat, Jun 18, 2016 at 02:57:22PM +0200, Richard Weinberger wrote:
> On Sat, Jun 18, 2016 at 11:52 AM, Daniel Golle <daniel@makrotopia.org> wrote:
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  fs/ubifs/super.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > index 7034995..ae32b3c 100644
> > --- a/fs/ubifs/super.c
> > +++ b/fs/ubifs/super.c
> > @@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
> >          */
> >         ubi = open_ubi(name, UBI_READONLY);
> >         if (IS_ERR(ubi)) {
> > -               pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> > -                      current->pid, name, (int)PTR_ERR(ubi));
> > +               if (flags & MS_SILENT)
> > +                       pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> > +                              current->pid, name, (int)PTR_ERR(ubi));
> 
> This needs a more detailed explanation.

Basically this should be seen in context of
commit 90bea5a3f0bf680b87b90516f3c231997f4b8f3b
which already implements support for MS_SILENT except for that one
error message which is still being displayed despite MS_SILENT being
set.

> Why does UBIFS need this? Only very few filesystems care about MS_SILENT.

Afaik all filesystems potentially used as rootfs on Linux do support
MS_SILENT.

> What problem are you trying to address?

On OpenWrt / LEDE we are probing whether the UBI volume set to be
rootfs contains a squashfs and thus a ubiblock device is being
created and ROOT_DEV is set accordingly. Similarly, we are also
probing whether the rootfs volume contains a ubifs (which is supported
so people can use our kernel and e.g. Debian's userspace or to
waste less space compared to overlay(lower=squashfs,upper=ubifs) when
frequently updating or heavily modifying the rootfs during runtime).
Thus, users get to see that error message in their kernel logs
[    3.060000] UBIFS error (pid: 1): cannot open "ubi0:rootfs", error -19
even though every thing is fine (rootfs just happends to not be ubifs)
and that's misleading as people often blame the first error message
they see to be the cause for anything not working.


Cheers


Daniel

[PATCH v2] ubifs: silence error output if MS_SILENT is set

[Daniel Golle]

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: logic was accidentally inverted, fix that.

 fs/ubifs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7034995..736dd58 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
 	 */
 	ubi = open_ubi(name, UBI_READONLY);
 	if (IS_ERR(ubi)) {
-		pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
-		       current->pid, name, (int)PTR_ERR(ubi));
+		if (!(flags & MS_SILENT))
+			pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
+			       current->pid, name, (int)PTR_ERR(ubi));
 		return ERR_CAST(ubi);
 	}
 
-- 
2.8.3

Re: [PATCH] ubifs: silence error output if MS_SILENT is set

[Richard Weinberger]

Hi!

Am 18.06.2016 um 15:42 schrieb Daniel Golle:
> Hi Richard,
> 
> first of all, thanks for reviewing my patch!
> 
> On Sat, Jun 18, 2016 at 02:57:22PM +0200, Richard Weinberger wrote:
>> On Sat, Jun 18, 2016 at 11:52 AM, Daniel Golle <daniel@makrotopia.org> wrote:
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  fs/ubifs/super.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 7034995..ae32b3c 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
>>>          */
>>>         ubi = open_ubi(name, UBI_READONLY);
>>>         if (IS_ERR(ubi)) {
>>> -               pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
>>> -                      current->pid, name, (int)PTR_ERR(ubi));
>>> +               if (flags & MS_SILENT)
>>> +                       pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
>>> +                              current->pid, name, (int)PTR_ERR(ubi));
>>
>> This needs a more detailed explanation.
> 
> Basically this should be seen in context of
> commit 90bea5a3f0bf680b87b90516f3c231997f4b8f3b
> which already implements support for MS_SILENT except for that one
> error message which is still being displayed despite MS_SILENT being
> set.

This should be part of the commit message since it is a fixup for the
said change.

>> Why does UBIFS need this? Only very few filesystems care about MS_SILENT.
> 
> Afaik all filesystems potentially used as rootfs on Linux do support
> MS_SILENT.
> 
>> What problem are you trying to address?
> 
> On OpenWrt / LEDE we are probing whether the UBI volume set to be
> rootfs contains a squashfs and thus a ubiblock device is being
> created and ROOT_DEV is set accordingly. Similarly, we are also
> probing whether the rootfs volume contains a ubifs (which is supported
> so people can use our kernel and e.g. Debian's userspace or to
> waste less space compared to overlay(lower=squashfs,upper=ubifs) when
> frequently updating or heavily modifying the rootfs during runtime).
> Thus, users get to see that error message in their kernel logs
> [    3.060000] UBIFS error (pid: 1): cannot open "ubi0:rootfs", error -19
> even though every thing is fine (rootfs just happends to not be ubifs)
> and that's misleading as people often blame the first error message
> they see to be the cause for anything not working.

While your fix is fine I think you should do better than just trial&error
probing. e.g. Identify the rootfs type from the volume label.

Thanks,
//richard

Re: [PATCH v2] ubifs: silence error output if MS_SILENT is set

[Richard Weinberger]

Am 18.06.2016 um 16:12 schrieb Daniel Golle:
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v2: logic was accidentally inverted, fix that.

I really would like to have a patch description.

Thanks,
//richard

[PATCH v3] ubifs: silence error output if MS_SILENT is set

[Daniel Golle]

This change completes
commit 90bea5a3f0bf680b87b90516f3c231997f4b8f3b
which already implements support for MS_SILENT except for that one
error message which is still being displayed despite MS_SILENT being
set. Suppress that error message as well in case MS_SILENT is set.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: logic was accidentally inverted, fix that.
v3: add patch description

 fs/ubifs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7034995..736dd58 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
 	 */
 	ubi = open_ubi(name, UBI_READONLY);
 	if (IS_ERR(ubi)) {
-		pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
-		       current->pid, name, (int)PTR_ERR(ubi));
+		if (!(flags & MS_SILENT))
+			pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
+			       current->pid, name, (int)PTR_ERR(ubi));
 		return ERR_CAST(ubi);
 	}
 
-- 
2.8.3

Re: [PATCH v3] ubifs: silence error output if MS_SILENT is set

[Richard Weinberger]

Am 18.06.2016 um 17:53 schrieb Daniel Golle:
> This change completes
> commit 90bea5a3f0bf680b87b90516f3c231997f4b8f3b
> which already implements support for MS_SILENT except for that one
> error message which is still being displayed despite MS_SILENT being
> set. Suppress that error message as well in case MS_SILENT is set.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v2: logic was accidentally inverted, fix that.
> v3: add patch description
> 
>  fs/ubifs/super.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 7034995..736dd58 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -2108,8 +2108,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
>  	 */
>  	ubi = open_ubi(name, UBI_READONLY);
>  	if (IS_ERR(ubi)) {
> -		pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> -		       current->pid, name, (int)PTR_ERR(ubi));
> +		if (!(flags & MS_SILENT))
> +			pr_err("UBIFS error (pid: %d): cannot open \"%s\", error %d",
> +			       current->pid, name, (int)PTR_ERR(ubi));
>  		return ERR_CAST(ubi);
>  	}

I'll queue this patch for v4.8.

Thanks,
//richard