ubifs: respect MS_SILENT mount flag

PATCH Superseded Daniel Golle 5 participants 19 messages May 17, 2014 – Jun 2, 2014

Subsystems (2)

Memory technology devices (MTD) UBI file system (UBIFS)

lore

Share & tools

lore

Daniel Golle

author

2014-05-17 01:56 UTC #0 lore raw

PATCH 1/1 ubifs: respect MS_SILENT mount flag

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing an additional parameter in ubifs_read_node and use it to pass down the MS_SILENT flag in ubifs_read_sb_node.

Signed-off-by Daniel Golle <[email protected]>

---
 fs/ubifs/commit.c   |  4 ++--
 fs/ubifs/io.c       | 23 ++++++++++++++---------
 fs/ubifs/sb.c       |  5 +++--
 fs/ubifs/tnc_misc.c |  4 ++--
 fs/ubifs/ubifs.h    |  2 +-
 5 files changed, 22 insertions(+), 16 deletions(-)

fs/ubifs/commit.c +2 -2

index ff82293..865d13f 100644
@@ -542,7 +542,7 @@ int dbg_old_index_check_init(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 	if (!idx)
 		return -ENOMEM;
 
-	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs, 0);
 	if (err)
 		goto out;

@@ -610,7 +610,7 @@ int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 		list_add_tail(&i->list, &list);
 		/* Read the index node */
 		idx = &i->idx;
-		err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+		err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs, 0);
 		if (err)
 			goto out_free;
 		/* Validate index node */

fs/ubifs/io.c +14 -9

index e18b988..51c4072 100644
@@ -912,7 +912,7 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 	if (!overlap) {
 		/* We may safely unlock the write-buffer and read the data */
 		spin_unlock(&wbuf->lock);
-		return ubifs_read_node(c, buf, type, len, lnum, offs);
+		return ubifs_read_node(c, buf, type, len, lnum, offs, 0);
 	}
 
 	/* Don't read under wbuf */

@@ -966,13 +966,14 @@ out:
  * @len: node length (not aligned)
  * @lnum: logical eraseblock number
  * @offs: offset within the logical eraseblock
+ * @silent: suppress error messages
  *
  * This function reads a node of known type and and length, checks it and
  * stores in @buf. Returns zero in case of success, %-EUCLEAN if CRC mismatched
  * and a negative error code in case of failure.
  */
 int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
-		    int lnum, int offs)
+		    int lnum, int offs, int silent)
 {
 	int err, l;
 	struct ubifs_ch *ch = buf;

@@ -988,30 +989,34 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
+		if (!silent) ubifs_err("bad node type (%d but expected %d)",
 			  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		if (!silent)
+			ubifs_err("expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		if (!silent)
+			ubifs_err("bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
-		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	if (!silent) {
+		ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum,
+			  offs, ubi_is_mapped(c->ubi, lnum));
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/sb.c +3 -2

index 4c37607..b46847d 100644
@@ -482,14 +482,15 @@ failed:
 struct ubifs_sb_node *ubifs_read_sb_node(struct ubifs_info *c)
 {
 	struct ubifs_sb_node *sup;
-	int err;
+	int silent, err;
 
 	sup = kmalloc(ALIGN(UBIFS_SB_NODE_SZ, c->min_io_size), GFP_NOFS);
 	if (!sup)
 		return ERR_PTR(-ENOMEM);
 
+	silent = !!(c->vfs_sb->s_flags & MS_SILENT);
 	err = ubifs_read_node(c, sup, UBIFS_SB_NODE, UBIFS_SB_NODE_SZ,
-			      UBIFS_SB_LNUM, 0);
+			      UBIFS_SB_LNUM, 0, silent);
 	if (err) {
 		kfree(sup);
 		return ERR_PTR(err);

fs/ubifs/tnc_misc.c +2 -2

index f6bf899..e128689 100644
@@ -280,7 +280,7 @@ static int read_znode(struct ubifs_info *c, int lnum, int offs, int len,
 	if (!idx)
 		return -ENOMEM;
 
-	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs, 0);
 	if (err < 0) {
 		kfree(idx);
 		return err;

@@ -472,7 +472,7 @@ int ubifs_tnc_read_node(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 					   zbr->lnum, zbr->offs);
 	else
 		err = ubifs_read_node(c, node, type, zbr->len, zbr->lnum,
-				      zbr->offs);
+				      zbr->offs, 0);
 
 	if (err) {
 		dbg_tnck(key, "key ");

fs/ubifs/ubifs.h +1 -1

index e8c8cfe..85fdd11 100644
@@ -1481,7 +1481,7 @@ int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len);
 int ubifs_wbuf_seek_nolock(struct ubifs_wbuf *wbuf, int lnum, int offs);
 int ubifs_wbuf_init(struct ubifs_info *c, struct ubifs_wbuf *wbuf);
 int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
-		    int lnum, int offs);
+		    int lnum, int offs, int silent);
 int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 			 int lnum, int offs);
 int ubifs_write_node(struct ubifs_info *c, void *node, int len, int lnum,

Signature · 1 line

1.9.2

Artem Bityutskiy

2014-05-27 12:19 UTC #1 lore raw

Quoted 7 lines · Daniel Golle

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing an additional parameter in ubifs_read_node and use it to pass down the MS_SILENT flag in ubifs_read_sb_node.
Signed-off-by Daniel Golle <[email protected]>

This looks reasonable. Could you please send v2 with checkpatch.pl complaints addressed?

Thanks!

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Daniel Golle

author

2014-05-27 14:12 UTC #2 lore raw

PATCH v2 1/1 ubifs: respect MS_SILENT mount flag

Signed-off-by Daniel Golle <[email protected]>

---
v2: fixed checkpatch.pl issues, not sure about the newline in
fs/ubifs/commit.c:610 which leaves the single 0 looking a bit lonely...

 fs/ubifs/commit.c   |  5 +++--
 fs/ubifs/io.c       | 26 ++++++++++++++++----------
 fs/ubifs/sb.c       |  5 +++--
 fs/ubifs/tnc_misc.c |  4 ++--
 fs/ubifs/ubifs.h    |  2 +-
 5 files changed, 25 insertions(+), 17 deletions(-)

fs/ubifs/commit.c +3 -2

index ff82293..936617f 100644
@@ -542,7 +542,7 @@ int dbg_old_index_check_init(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 	if (!idx)
 		return -ENOMEM;
 
-	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs, 0);
 	if (err)
 		goto out;

@@ -610,7 +610,8 @@ int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 		list_add_tail(&i->list, &list);
 		/* Read the index node */
 		idx = &i->idx;
-		err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+		err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs
+				      0);
 		if (err)
 			goto out_free;
 		/* Validate index node */

fs/ubifs/io.c +16 ~~-10~~

index e18b988..8c5dea1 100644
@@ -912,7 +912,7 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 	if (!overlap) {
 		/* We may safely unlock the write-buffer and read the data */
 		spin_unlock(&wbuf->lock);
-		return ubifs_read_node(c, buf, type, len, lnum, offs);
+		return ubifs_read_node(c, buf, type, len, lnum, offs, 0);
 	}
 
 	/* Don't read under wbuf */

@@ -966,13 +966,14 @@ out:
  * @len: node length (not aligned)
  * @lnum: logical eraseblock number
  * @offs: offset within the logical eraseblock
+ * @silent: suppress error messages
  *
  * This function reads a node of known type and and length, checks it and
  * stores in @buf. Returns zero in case of success, %-EUCLEAN if CRC mismatched
  * and a negative error code in case of failure.
  */
 int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
-		    int lnum, int offs)
+		    int lnum, int offs, int silent)
 {
 	int err, l;
 	struct ubifs_ch *ch = buf;

@@ -988,30 +989,35 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
-			  ch->node_type, type);
+		if (!silent)
+			ubifs_err("bad node type (%d but expected %d)",
+				  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		if (!silent)
+			ubifs_err("expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		if (!silent)
+			ubifs_err("bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
-		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	if (!silent) {
+		ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum,
+			  offs, ubi_is_mapped(c->ubi, lnum));
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/sb.c +3 -2

index 4c37607..b46847d 100644
@@ -482,14 +482,15 @@ failed:
 struct ubifs_sb_node *ubifs_read_sb_node(struct ubifs_info *c)
 {
 	struct ubifs_sb_node *sup;
-	int err;
+	int silent, err;
 
 	sup = kmalloc(ALIGN(UBIFS_SB_NODE_SZ, c->min_io_size), GFP_NOFS);
 	if (!sup)
 		return ERR_PTR(-ENOMEM);
 
+	silent = !!(c->vfs_sb->s_flags & MS_SILENT);
 	err = ubifs_read_node(c, sup, UBIFS_SB_NODE, UBIFS_SB_NODE_SZ,
-			      UBIFS_SB_LNUM, 0);
+			      UBIFS_SB_LNUM, 0, silent);
 	if (err) {
 		kfree(sup);
 		return ERR_PTR(err);

fs/ubifs/tnc_misc.c +2 -2

index f6bf899..e128689 100644
@@ -280,7 +280,7 @@ static int read_znode(struct ubifs_info *c, int lnum, int offs, int len,
 	if (!idx)
 		return -ENOMEM;
 
-	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs);
+	err = ubifs_read_node(c, idx, UBIFS_IDX_NODE, len, lnum, offs, 0);
 	if (err < 0) {
 		kfree(idx);
 		return err;

@@ -472,7 +472,7 @@ int ubifs_tnc_read_node(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 					   zbr->lnum, zbr->offs);
 	else
 		err = ubifs_read_node(c, node, type, zbr->len, zbr->lnum,
-				      zbr->offs);
+				      zbr->offs, 0);
 
 	if (err) {
 		dbg_tnck(key, "key ");

fs/ubifs/ubifs.h +1 -1

index e8c8cfe..85fdd11 100644
@@ -1481,7 +1481,7 @@ int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len);
 int ubifs_wbuf_seek_nolock(struct ubifs_wbuf *wbuf, int lnum, int offs);
 int ubifs_wbuf_init(struct ubifs_info *c, struct ubifs_wbuf *wbuf);
 int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
-		    int lnum, int offs);
+		    int lnum, int offs, int silent);
 int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 			 int lnum, int offs);
 int ubifs_write_node(struct ubifs_info *c, void *node, int len, int lnum,

Signature · 1 line

1.9.3

Artem Bityutskiy

2014-05-27 14:56 UTC #3 lore raw

Quoted 7 lines · Daniel Golle

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing an additional parameter in ubifs_read_node and use it to pass down the MS_SILENT flag in ubifs_read_sb_node.
Signed-off-by Daniel Golle <[email protected]>

Thanks. How did you test this patch?

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Daniel

author

2014-05-27 16:04 UTC #4 lore raw

Quoted 10 lines · Artem Bityutskiy

Quoted 7 lines · Daniel Golle
When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing an additional parameter in ubifs_read_node and use it to pass down the MS_SILENT flag in ubifs_read_sb_node.
Signed-off-by Daniel Golle <[email protected]>
Thanks. How did you test this patch?

You can test this by trying to mount a non-empty volume which does not contain a UBIFS superblock (but e.g. squashfs or a U-Boot environment) with mount -t ubifs -o silent /dev/ubiX_Y /mnt
This should fail without creating any klog lines.

The reason that I want this is that I'm working on integration of UBI support in OpenWrt, including auto-mounting the "rootfs" volume by default (if it exists) in case the rootfs and/or rootfstype parameters are not passed-down by the bootloader, see
https://gitorious.org/openwrt-oxnas/openwrt-oxnas/commit/e1306d7b9bee8a39a33147d93cb399a4621bf3aa

The idea is to have the same level of features and comfort also on devices where UBI is being used, for MTD devices OpenWrt does something similar
https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.14/480-mtd-set-rootfs-to-be-root-dev.patch

However, this is probably distribution-specific hackery, but independently of that, UBIFS should still respect the MS_SILENT flag just like all other filesystems do.

hujianyang

2014-05-28 02:13 UTC #5 lore raw

Hi Daniel and Artem,

Quoted 6 lines

You can test this by trying to mount a non-empty volume which does not contain a UBIFS superblock (but e.g. squashfs or a U-Boot environment) with mount -t ubifs -o silent /dev/ubiX_Y /mnt
This should fail without creating any klog lines.

I think disabling log message in this case is needed. But I'm sorry to say I don't like just adding a new parameter to ubifs_read_node in this way because this silent flag seems only used during mount. This adding makes the parameters different in ubifs_read_node and ubifs_write_node, also not good for reading code.

How about to add a separate func ubifs_read_node_silent to instead this ubifs_read_node in ubifs_read_sb_node? I think we could do more proper work in this new function.

Quoted 5 lines

However, this is probably distribution-specific hackery, but independently of that, UBIFS should still respect the MS_SILENT flag just like all other filesystems do.

Do you get some more error messages not only in ubifs_read_node during mount? I think this is a good suggestion. Current code do not use @silent in ubifs_fill_super and we should do some work to enable it like other filesystems.

Thanks!

Artem Bityutskiy

2014-05-28 08:02 UTC #6 lore raw

Quoted 9 lines · hujianyang

Hi Daniel and Artem,
Quoted 6 lines
You can test this by trying to mount a non-empty volume which does not contain a UBIFS superblock (but e.g. squashfs or a U-Boot environment) with mount -t ubifs -o silent /dev/ubiX_Y /mnt
This should fail without creating any klog lines.

Quoted 5 lines

I think disabling log message in this case is needed. But I'm sorry to say I don't like just adding a new parameter to ubifs_read_node in this way because this silent flag seems only used during mount. This adding makes the parameters different in ubifs_read_node and ubifs_write_node, also not good for reading code.

So you bring up 2 points.

1. This patch only makes the read part of the mount code-path silent, everything else stays normal.
2. Another parameter makes code less readable.

WRT the former, do we really need more than that? IIUC, this is exactly the meaning of the 'silent' flag, and exactly its purpose. IIUC, it serves a single use-case: file-system probing. And the patch does exactly that.

Quoted 3 lines

How about to add a separate func ubifs_read_node_silent to instead this ubifs_read_node in ubifs_read_sb_node? I think we could do more proper work in this new function.

This would introduce code duplication, and I am not sure if this is much better than what Daniel suggests.

How about introducing a per-file-system flag, like c->probing or something, and make do something like this:

fs/ubifs/super.c +5 -0

index a81c7b5..a627476 100644
@@ -1210,7 +1210,12 @@ static int mount_ubifs(struct ubifs_info *c)
 
        c->mounting = 1;
 
+       /*
+        * Good commend describing what we are doing.
+        */
+       c->probing = 1
        err = ubifs_read_superblock(c);
+       c->probing = 0
        if (err)
                goto out_free;

After all, we already have a number of "state" flags like 'c->mounting', so adding another one is not that big deal.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Bityutskiy, Artem

2014-05-28 08:08 UTC #7 lore raw

fs/ubifs/super.c static int mount_ubifs(struct ubifs_info *c) · line 1216

Quoted 4 lines · Artem Bityutskiy

fs/ubifs/super.c +4 -0

+       /*
+        * Good commend describing what we are doing.
+        */
+       c->probing = 1

Err, actually:

          c->probing = silent;

Quoted 4 lines

        err = ubifs_read_superblock(c);
+       c->probing = 0
        if (err)
                goto out_free;

Signature · 12 lines

Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Artem Bityutskiy

2014-05-28 08:15 UTC #8 lore raw

Quoted 14 lines · Artem

fs/ubifs/super.c static int mount_ubifs(struct ubifs_info *c) · line 1216
Quoted 4 lines · Artem Bityutskiy
fs/ubifs/super.c +4 -0
+       /*
+        * Good commend describing what we are doing.
+        */
+       c->probing = 1
Err, actually:
          c->probing = silent;
Quoted 4 lines
        err = ubifs_read_superblock(c);
+       c->probing = 0
        if (err)
                goto out_free;

I guess you guys got the idea, but just in case, you also will need to add "if (!c->probing) { ubifs_error() }" in the relevant places.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Artem Bityutskiy

2014-05-28 08:29 UTC #9 lore raw

Quoted 11 lines · Artem Bityutskiy

Quoted 10 lines · Artem
Review
Quoted 4 lines · Artem Bityutskiy
(quoted diff) +4 -0
+       /*
+        * Good commend describing what we are doing.
+        */
+       c->probing = 1
Err, actually:
          c->probing = silent;

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
        err = ubifs_read_superblock(c);
+       c->probing = 0
        if (err)
                goto out_free;
I guess you guys got the idea, but just in case, you also will need to add "if (!c->probing) { ubifs_error() }" in the relevant places.

Or even introduce a new version of the error macro, something like 'ubifs_errc(), and use that in the relevant places. Not sure what is going to look better, though. Here is a sketch:

fs/ubifs/ubifs.h +8 -0

index e8c8cfe..60cffa7 100644
@@ -52,6 +52,14 @@
        pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
                current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+       if (!(c)->probing)                                          \
+               ubifs_err(fmt, ##__VA_ARGS__)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

Signature · 2 lines

Best Regards,
Artem Bityutskiy

hujianyang

2014-05-28 08:44 UTC #10 lore raw

Hi Artem,

Quoted 15 lines

Or even introduce a new version of the error macro, something like 'ubifs_errc(), and use that in the relevant places. Not sure what is going to look better, though. Here is a sketch:
fs/ubifs/ubifs.h +1 -0
index e8c8cfe..60cffa7 100644
@@ -52,6 +52,14 @@
        pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
                current->pid, __func__, ##__VA_ARGS__)
 
+/*

fs/ubifs/ubifs.h line 65

Quoted 10 lines

fs/ubifs/ubifs.h +7 -0

+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+       if (!(c)->probing)                                          \
+               ubifs_err(fmt, ##__VA_ARGS__)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

I have to say I was just writing to you about adding a new marco to instead. Ha~

In this way we can disable unnecessary messages during mount if we use "slient" flag. I have a simple question，Is this @probing safe during mount? Are there some probably race conditions of it? I have no experiences of these.

I think you should find out which ubifs_err should be replace into your "ubifs_errc" in the next step.

Thank you very much!

Artem Bityutskiy

2014-05-28 09:29 UTC #11 lore raw

Quoted 3 lines · hujianyang

In this way we can disable unnecessary messages during mount if we use "slient" flag. I have a simple question，Is this @probing safe during mount? Are there some probably race conditions of it? I have no experiences of these.

I do not think so, the mount path is "single-stream".

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Daniel Golle

author

2014-05-30 22:02 UTC #12 lore raw

PATCH v3 1/1 ubifs: respect MS_SILENT mount flag

In reply to Artem Bityutskiy

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.

Signed-off-by Daniel Golle <[email protected]>

---
v3: use state variable in ubifs_info instead of

 fs/ubifs/io.c    | 14 ++++++++------
 fs/ubifs/super.c |  4 ++++
 fs/ubifs/ubifs.h |  9 +++++++++
 3 files changed, 21 insertions(+), 6 deletions(-)

fs/ubifs/io.c +8 -6

index e18b988..e6a31faa 100644
@@ -988,30 +988,32 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
+		ubifs_errc(c, "bad node type (%d but expected %d)",
 			  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		ubifs_errc(c, "expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		ubifs_errc(c, "bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
+	ubifs_errc(c, "bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
 		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	if (!c->probing) {
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/super.c +4 -0

index a81c7b5..e88bbb6 100644
@@ -1149,6 +1149,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	size_t sz;
 
 	c->ro_mount = !!(c->vfs_sb->s_flags & MS_RDONLY);
+	c->probing = !!(c->vfs_sb->s_flags & MS_SILENT);
+
 	err = init_constants_early(c);
 	if (err)
 		return err;

@@ -1214,6 +1216,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (err)
 		goto out_free;
 
+	c->probing = 0;
+
 	/*
 	 * Make sure the compressor which is set as default in the superblock
 	 * or overridden by mount options is actually compiled in.

fs/ubifs/ubifs.h +9 -0

index e8c8cfe..6aa1550 100644
@@ -52,6 +52,14 @@
 	pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
 		current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+	if (!(c)->probing)                                         \
+		ubifs_err(fmt, ##__VA_ARGS__)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

@@ -1441,6 +1449,7 @@ struct ubifs_info {
 	unsigned int replaying:1;
 	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
+	unsigned int probing:1;
 	struct list_head replay_list;
 	struct list_head replay_buds;
 	unsigned long long cs_sqnum;

Signature · 1 line

1.9.3

Brian Norris

2014-05-30 23:21 UTC #13 lore raw

Hi Daniel,

Quoted 10 lines · Daniel Golle

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.
Signed-off-by Daniel Golle <[email protected]>
---
v3: use state variable in ubifs_info instead of

Instead of what?? I'm dying to know!

(Just kidding; I can read your previous conversations. Hint: the answer is "a function argument.")

Quoted 5 lines

 fs/ubifs/io.c    | 14 ++++++++------
 fs/ubifs/super.c |  4 ++++
 fs/ubifs/ubifs.h |  9 +++++++++
 3 files changed, 21 insertions(+), 6 deletions(-)

[...]

Quoted 5 lines

fs/ubifs/ubifs.h +0 -0
index e8c8cfe..6aa1550 100644

fs/ubifs/ubifs.h line 61

Quoted 10 lines

fs/ubifs/ubifs.h +7 -0

@@ -52,6 +52,14 @@
 	pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
 		current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+	if (!(c)->probing)                                         \
+		ubifs_err(fmt, ##__VA_ARGS__)

Nitpick: might you want to wrap the whole condition in do { } while (0), so that a user can't shoot themselves in the foot with something like:

	if (condition)
		ubifs_errc(c, "hello world");
	else
		do_something_else();

which will expand to the following unexpected code:

	if (condition)
		if (!c->probing)
			ubifs_err("hello world");
		else
			do_something_else();

fs/ubifs/ubifs.h line 65

Quoted 4 lines

fs/ubifs/ubifs.h +1 -0

+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

Brian

Daniel Golle

author

2014-05-30 23:33 UTC #14 lore raw

PATCH v4 1/1 ubifs: respect MS_SILENT mount flag

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.

Signed-off-by Daniel Golle <[email protected]>

---
v4: enclose conditional error printing macro by do { } while (0)
v3: use state variable in ubifs_info instead of a function parameter

 fs/ubifs/io.c    | 14 ++++++++------
 fs/ubifs/super.c |  4 ++++
 fs/ubifs/ubifs.h | 11 +++++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

fs/ubifs/io.c +8 -6

index e18b988..e6a31faa 100644
@@ -988,30 +988,32 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
+		ubifs_errc(c, "bad node type (%d but expected %d)",
 			  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		ubifs_errc(c, "expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		ubifs_errc(c, "bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
+	ubifs_errc(c, "bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
 		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	if (!c->probing) {
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/super.c +4 -0

index a81c7b5..e88bbb6 100644
@@ -1149,6 +1149,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	size_t sz;
 
 	c->ro_mount = !!(c->vfs_sb->s_flags & MS_RDONLY);
+	c->probing = !!(c->vfs_sb->s_flags & MS_SILENT);
+
 	err = init_constants_early(c);
 	if (err)
 		return err;

@@ -1214,6 +1216,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (err)
 		goto out_free;
 
+	c->probing = 0;
+
 	/*
 	 * Make sure the compressor which is set as default in the superblock
 	 * or overridden by mount options is actually compiled in.

fs/ubifs/ubifs.h +11 -0

index e8c8cfe..d944d4d 100644
@@ -52,6 +52,16 @@
 	pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
 		current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+	do {                                                       \
+		if (!(c)->probing)                                 \
+			ubifs_err(fmt, ##__VA_ARGS__)              \
+	} while (0)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

@@ -1441,6 +1451,7 @@ struct ubifs_info {
 	unsigned int replaying:1;
 	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
+	unsigned int probing:1;
 	struct list_head replay_list;
 	struct list_head replay_buds;
 	unsigned long long cs_sqnum;

Signature · 1 line

1.9.3

Daniel Golle

author

2014-05-31 00:01 UTC #15 lore raw

PATCH v5 1/1 ubifs: respect MS_SILENT mount flag

In reply to Brian Norris

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.

Signed-off-by Daniel Golle <[email protected]>

---
v5: added missing semi-colon and fixed a style issue
v4: enclose conditional error printing macro by do { } while (0)
v3: use state variable in ubifs_info instead of a function parameter

 fs/ubifs/io.c    | 16 +++++++++-------
 fs/ubifs/super.c |  4 ++++
 fs/ubifs/ubifs.h | 11 +++++++++++
 3 files changed, 24 insertions(+), 7 deletions(-)

fs/ubifs/io.c +9 -7

index e18b988..3205929 100644
@@ -988,30 +988,32 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
+		ubifs_errc(c, "bad node type (%d but expected %d)",
 			  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		ubifs_errc(c, "expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		ubifs_errc(c, "bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
-		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	ubifs_errc(c, "bad node at LEB %d:%d, LEB mapping status %d", lnum,
+		  offs, ubi_is_mapped(c->ubi, lnum));
+	if (!c->probing) {
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/super.c +4 -0

index a81c7b5..e88bbb6 100644
@@ -1149,6 +1149,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	size_t sz;
 
 	c->ro_mount = !!(c->vfs_sb->s_flags & MS_RDONLY);
+	c->probing = !!(c->vfs_sb->s_flags & MS_SILENT);
+
 	err = init_constants_early(c);
 	if (err)
 		return err;

@@ -1214,6 +1216,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (err)
 		goto out_free;
 
+	c->probing = 0;
+
 	/*
 	 * Make sure the compressor which is set as default in the superblock
 	 * or overridden by mount options is actually compiled in.

fs/ubifs/ubifs.h +11 -0

index e8c8cfe..145dc6d 100644
@@ -52,6 +52,16 @@
 	pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
 		current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+	do {                                                       \
+		if (!(c)->probing)                                 \
+			ubifs_err(fmt, ##__VA_ARGS__);             \
+	} while (0)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

@@ -1441,6 +1451,7 @@ struct ubifs_info {
 	unsigned int replaying:1;
 	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
+	unsigned int probing:1;
 	struct list_head replay_list;
 	struct list_head replay_buds;
 	unsigned long long cs_sqnum;

Signature · 1 line

1.9.3

Artem Bityutskiy

2014-06-02 07:59 UTC #16 lore raw

Thanks Daniel, this looks nicer than the original patch. Would you please do few more cosmetic changes, though.

fs/ubifs/super.c static int mount_ubifs(struct ubifs_info *c) · line 1152

Quoted 9 lines · Daniel Golle

fs/ubifs/super.c +1 -0

index a81c7b5..e88bbb6 100644
@@ -1149,6 +1149,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	size_t sz;
 
 	c->ro_mount = !!(c->vfs_sb->s_flags & MS_RDONLY);
+	c->probing = !!(c->vfs_sb->s_flags & MS_SILENT);

Would you add a comment on top of this saying what MS_SILENT is about. The kernel people who read the code do not necessary know the "FS probing" use-case. E.g., I did not know about it.

fs/ubifs/super.c struct ubifs_info { · line 1454

Quoted 5 lines

fs/ubifs/super.c +1 -0

@@ -1441,6 +1451,7 @@ struct ubifs_info {
 	unsigned int replaying:1;
 	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
+	unsigned int probing:1;

We have kerneldoc-style comment on top of this structure, could you please document the new field there.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Daniel Golle

author

2014-06-02 13:51 UTC #17 lore raw

PATCH v6 1/1 ubifs: respect MS_SILENT mount flag

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.

Signed-off-by Daniel Golle <[email protected]>

---
v6: added comments
v5: added missing semi-colon and fixed a style issue
v4: enclose conditional error printing macro by do { } while (0)
v3: use state variable in ubifs_info instead of a function parameter

 fs/ubifs/io.c    | 16 +++++++++-------
 fs/ubifs/super.c |  5 +++++
 fs/ubifs/ubifs.h | 12 ++++++++++++
 3 files changed, 26 insertions(+), 7 deletions(-)

fs/ubifs/io.c +9 -7

index e18b988..3205929 100644
@@ -988,30 +988,32 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		return err;
 
 	if (type != ch->node_type) {
-		ubifs_err("bad node type (%d but expected %d)",
+		ubifs_errc(c, "bad node type (%d but expected %d)",
 			  ch->node_type, type);
 		goto out;
 	}
 
 	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
 	if (err) {
-		ubifs_err("expected node type %d", type);
+		ubifs_errc(c, "expected node type %d", type);
 		return err;
 	}
 
 	l = le32_to_cpu(ch->len);
 	if (l != len) {
-		ubifs_err("bad node length %d, expected %d", l, len);
+		ubifs_errc(c, "bad node length %d, expected %d", l, len);
 		goto out;
 	}
 
 	return 0;
 
 out:
-	ubifs_err("bad node at LEB %d:%d, LEB mapping status %d", lnum, offs,
-		  ubi_is_mapped(c->ubi, lnum));
-	ubifs_dump_node(c, buf);
-	dump_stack();
+	ubifs_errc(c, "bad node at LEB %d:%d, LEB mapping status %d", lnum,
+		  offs, ubi_is_mapped(c->ubi, lnum));
+	if (!c->probing) {
+		ubifs_dump_node(c, buf);
+		dump_stack();
+	}
 	return -EINVAL;
 }

fs/ubifs/super.c +5 -0

index a81c7b5..3904c85 100644
@@ -1149,6 +1149,9 @@ static int mount_ubifs(struct ubifs_info *c)
 	size_t sz;
 
 	c->ro_mount = !!(c->vfs_sb->s_flags & MS_RDONLY);
+	/* Suppress error messages while probing if MS_SILENT is set */
+	c->probing = !!(c->vfs_sb->s_flags & MS_SILENT);
+
 	err = init_constants_early(c);
 	if (err)
 		return err;

@@ -1214,6 +1217,8 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (err)
 		goto out_free;
 
+	c->probing = 0;
+
 	/*
 	 * Make sure the compressor which is set as default in the superblock
 	 * or overridden by mount options is actually compiled in.

fs/ubifs/ubifs.h +12 -0

index e8c8cfe..f87ab74 100644
@@ -52,6 +52,16 @@
 	pr_warn("UBIFS warning (pid %d): %s: " fmt "\n",            \
 		current->pid, __func__, ##__VA_ARGS__)
 
+/*
+ * A variant of 'ubifs_err()' which takes the UBIFS file-sytem description
+ * object as an argument.
+ */
+#define ubifs_errc(c, fmt, ...)                                    \
+	do {                                                       \
+		if (!(c)->probing)                                 \
+			ubifs_err(fmt, ##__VA_ARGS__);             \
+	} while (0)
+
 /* UBIFS file system VFS magic number */
 #define UBIFS_SUPER_MAGIC 0x24051905

@@ -1209,6 +1219,7 @@ struct ubifs_debug_info;
  * @need_recovery: %1 if the file-system needs recovery
  * @replaying: %1 during journal replay
  * @mounting: %1 while mounting
+ * @probing: %1 while attempting to mount if MS_SILENT mount flag is set
  * @remounting_rw: %1 while re-mounting from R/O mode to R/W mode
  * @replay_list: temporary list used during journal replay
  * @replay_buds: list of buds to replay

@@ -1441,6 +1452,7 @@ struct ubifs_info {
 	unsigned int replaying:1;
 	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
+	unsigned int probing:1;
 	struct list_head replay_list;
 	struct list_head replay_buds;
 	unsigned long long cs_sqnum;

Signature · 1 line

1.9.3

Artem Bityutskiy

Applied

2014-06-02 15:02 UTC #18 lore raw

Quoted 8 lines · Daniel Golle

When attempting to mount a non-ubifs formatted volume, lots of error messages (including a stack dump) are thrown to the kernel log even if the MS_SILENT mount flag is set.
Fix this by introducing adding an additional state-variable in struct ubifs_info and suppress error messages in ubifs_read_node if MS_SILENT is set.
Signed-off-by Daniel Golle <[email protected]>

Pushed to linux-ubifs.git with a couple of minor amendments.

Thanks!

Signature · 2 lines

Best Regards,
Artem Bityutskiy