diff mbox

[RFC] mtd-utils: integck: add support for volume specific power-cut test

Message ID ED3E0BCACD909541BA94A34C4A164D4C4FCDC571@post.tritech.se
State RFC
Headers show

Commit Message

Mats Kärrman Aug. 19, 2013, 8:50 a.m. UTC 
Problem:
The current power-cut test requires power-cut emulation to be enabled globally
(e.g. for all UBIFS volumes) but this may lead to some practical problems when
the root fs of the tested machine is also of the same type.

Proposed solution:
Add a new option -P that extends the existing -p with the ability to
automatically re-enable power-cut emulation after every re-mount by writing
"1" to the specified control file. E.g.

# ./integck -eP /sys/kernel/debug/ubifs/ubi1_0/tst_recovery -m 1 ./ubifs

Note:
The following patch includes some perhaps unrelated changes that maybe should
be handles separately:
- #include <linux/fs.h> : I couldn't build integck without it due to some missing
  defines; to be investigated.
- usleep(MOUNT_DELAY) : To avoid error 16 (Device or resource busy) when
  remounting. I have not seen any problems with this on my target system with
  real flash but when running tests on my PC with mtdram the tests keep failing
  within a minute or so without these delays.

Comments

Artem Bityutskiy Aug. 19, 2013, 1:36 p.m. UTC | #1 
On Mon, 2013-08-19 at 08:50 +0000, Mats Kärrman wrote:
> Problem:
> The current power-cut test requires power-cut emulation to be enabled globally
> (e.g. for all UBIFS volumes) but this may lead to some practical problems when
> the root fs of the tested machine is also of the same type.

Hi, sorry, I do not understand the problem. First of all, I never ran
this on a device which uses an UBIFS file-system as rootfs. I only ran
it on a PC where UBIFS has only been on top of a nandsim.

So, AFAICS, power cut emulation is per-mountpoint. So you should enable
it only for the the file-system under test. Power-cut should switch only
the file-system under test to R/O mode, not the rootfs.

What are the practical problems you mention?
Mats Kärrman Aug. 19, 2013, 2:24 p.m. UTC | #2 
Hi Artem,

Well, as far as I understand, and the tests I made support this, you can
either enable the tests for all UBIFS mounts using:

# echo "1" > /sys/kernel/debug/ubifs/tst_recovery

or you can enable it for one specific mount using something like:

# echo "1" > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery

However, in the latter case, the flag is reset when the file system is remounted
so the test continues without tst_recovery enabled.

Or am I missing something here?

BR // Mats
Artem Bityutskiy Aug. 19, 2013, 2:50 p.m. UTC | #3 
On Mon, 2013-08-19 at 14:24 +0000, Mats Kärrman wrote:
> # echo "1" > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery
> 
> However, in the latter case, the flag is reset when the file system is
> remounted
> so the test continues without tst_recovery enabled.

Ah, I see it now.

Did you try if you can resolve the mount path into 'ubiX_Y' in integck,
so that one wouldn't need to pass this -P <loong/path/string> option?
Artem Bityutskiy Aug. 19, 2013, 2:55 p.m. UTC | #4 
On Mon, 2013-08-19 at 17:50 +0300, Artem Bityutskiy wrote:
> On Mon, 2013-08-19 at 14:24 +0000, Mats Kärrman wrote:
> > # echo "1" > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery
> > 
> > However, in the latter case, the flag is reset when the file system is
> > remounted
> > so the test continues without tst_recovery enabled.
> 
> Ah, I see it now.
> 
> Did you try if you can resolve the mount path into 'ubiX_Y' in integck,
> so that one wouldn't need to pass this -P <loong/path/string> option?

Check the recover_tested_fs() which uses get_tested_fs_mntent() which
parses /proc/mounts. Can you do a similar thing and find the 'ubiX_Y'
part, and then just write 1 to the sysfs path. -P would not be needed
then at all.
Mats Kärrman Aug. 19, 2013, 3:01 p.m. UTC | #5 
On Mon, 2013-08-19 at 17:55 +0300, Artem Bityutskiy wrote:
> On Mon, 2013-08-19 at 17:50 +0300, Artem Bityutskiy wrote:
> > On Mon, 2013-08-19 at 14:24 +0000, Mats Kärrman wrote:
> > > # echo "1" > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery
> > >
> > > However, in the latter case, the flag is reset when the file system is
> > > remounted
> > > so the test continues without tst_recovery enabled.
> >
> > Ah, I see it now.
> >
> > Did you try if you can resolve the mount path into 'ubiX_Y' in integck,
> > so that one wouldn't need to pass this -P <loong/path/string> option?
> 
> Check the recover_tested_fs() which uses get_tested_fs_mntent() which
> parses /proc/mounts. Can you do a similar thing and find the 'ubiX_Y'
> part, and then just write 1 to the sysfs path. -P would not be needed
> then at all.
> 

Actually this was what I first had in mind, but I stopped for the -P solution
for two reasons:
1) It saved me a lot of time ;)
2) Integck seem to be written with the aim of being a general purpose test
program (even if some comments in the code hint in another direction). If
so, it would not be safe to assume there is any ubifs/ubi folder or that the
knob file is called tst_recovery. Also it is possible to mount the debugfs
anywhere (but i guess that could also be parsed from /proc/mounts).

So: general or ubifs specific?

BR // Mats
Artem Bityutskiy Aug. 19, 2013, 3:16 p.m. UTC | #6 
On Mon, 2013-08-19 at 15:01 +0000, Mats Kärrman wrote:
> Actually this was what I first had in mind, but I stopped for the -P solution
> for two reasons:
> 1) It saved me a lot of time ;)
> 2) Integck seem to be written with the aim of being a general purpose test
> program (even if some comments in the code hint in another direction). If
> so, it would not be safe to assume there is any ubifs/ubi folder or that the
> knob file is called tst_recovery.

OK, but there is also an "fstype" field, could you use and do the trick
only for UBIFS?

>  Also it is possible to mount the debugfs
> anywhere (but i guess that could also be parsed from /proc/mounts).

I would not worry about this and I'd assume "/sys/kernel/debug" now, I
think this is more than good enough.
Mats Kärrman Aug. 19, 2013, 3:37 p.m. UTC | #7 
On Mon, 2013-08-19 at 15:16 +0000, Artem Bityutskiy wrote:
> OK, but there is also an "fstype" field, could you use and do the trick
> only for UBIFS?

I guess so, but there is also an alternative mounting syntax:

# mount -t ubifs ubi0:integ_chk /mnt/ubifs

resulting in a /proc/mounts line of:

ubi0:integ_chk /mnt/ubifs ubifs rw,relatime 0 0

Do you see an easy way to resolve this into ubi0_0 or should this case
also be ignored?
Personally I have never seen the use of multiple ubi volumes inside of
one mtd partition so "0" could be assumed but someone else might
disagree.

BR // Mats
Artem Bityutskiy Aug. 20, 2013, 12:23 p.m. UTC | #8 
On Mon, 2013-08-19 at 15:37 +0000, Mats Kärrman wrote:
> On Mon, 2013-08-19 at 15:16 +0000, Artem Bityutskiy wrote:
> > OK, but there is also an "fstype" field, could you use and do the trick
> > only for UBIFS?
> 
> I guess so, but there is also an alternative mounting syntax:
> 
> # mount -t ubifs ubi0:integ_chk /mnt/ubifs
> 
> resulting in a /proc/mounts line of:
> 
> ubi0:integ_chk /mnt/ubifs ubifs rw,relatime 0 0

OK, you are right. Here are all the possible ways to refer an UBI
volume, from comments in the 'open_ubi()' function:

 * o ubiX_Y    - mount UBI device number X, volume Y;
 * o ubiY      - mount UBI device number 0, volume Y;
 * o ubiX:NAME - mount UBI device X, volume with name NAME;
 * o ubi:NAME  - mount UBI device 0, volume with name NAME.

> Do you see an easy way to resolve this into ubi0_0 or should this case
> also be ignored?

Not sure about the easy part, but, there is libubi in mtd-utils, and
integck actually already uses it.

In libubi there are many useful helper functions which parse the ubi
sysfs directory and fetch various kind of info from it.

And you can use libubi for finding X_Y by NAME.

And it is actually easy now, when I think about this. First, you find
out the UBI device number, it is either X or 0 (let's call it 'ubinum').

Then if you have name instead of the volume number (Y), you use libubi
and resolve it like this:

libubi_t libubi; 
struct ubi_vol_info vol_info;

libubi = libubi_open();
if (!libubi) { blah-blah }

err = ubi_get_vol_info1_nm(libubi, ubinum,  name, &vol_info);
if (err) { blah-blah }

And now you have volume ID (Y) in "vol_info.vol_id".

And you construct the debugfs path.

Hmm?

> Personally I have never seen the use of multiple ubi volumes inside of
> one mtd partition so "0" could be assumed but someone else might
> disagree.

Well... I'd be happier to see something like I proposed above, if you
can submit such a patch.
Artem Bityutskiy Aug. 20, 2013, 1:10 p.m. UTC | #9 
On Tue, 2013-08-20 at 15:23 +0300, Artem Bityutskiy wrote:
> Then if you have name instead of the volume number (Y), you use libubi
> and resolve it like this:

Take a loot at ubirmvol.c in mtd-utils and use it as an example, it is
doing a similar thing, but not exactly the same.
Mats Kärrman Aug. 21, 2013, 6:21 a.m. UTC | #10 
On Tue, 2013-08-20 at 15:23 +0300, Artem Bityutskiy wrote:
> Well... I'd be happier to see something like I proposed above, if you
> can submit such a patch.

I'll give it a shot, but maybe not this week.

BR // Mats
diff mbox

Patch

diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c
index 8badd1f..fa94f44 100644
--- a/tests/fs-tests/integrity/integck.c
+++ b/tests/fs-tests/integrity/integck.c
@@ -37,6 +37,7 @@ 
 #include <sys/vfs.h>
 #include <sys/mount.h>
 #include <sys/statvfs.h>
+#include <linux/fs.h>
 
 #define PROGRAM_VERSION "1.1"
 #define PROGRAM_NAME "integck"
@@ -61,6 +62,9 @@ 
 /* Maximum buffer size for a single read/write operation */
 #define IO_BUFFER_SIZE 32768
 
+/* To avoid trouble with error 16 (Device or resource busy) */
+#define MOUNT_DELAY 10000
+
 /*
  * Check if a condition is true and die if not.
  */
@@ -95,6 +99,7 @@ 
 static struct {
 	long repeat_cnt;
 	int power_cut_mode;
+	const char * pc_reenable;
 	int verify_ops;
 	int reattach;
 	int mtdn;
@@ -2580,6 +2585,25 @@  static int rm_minus_rf_dir(const char *dir_name)
 }
 
 /**
+ * Re-enable the power cut emulation mode on the tested file system
+ */
+static int reenable_power_cut(void)
+{
+	int fd;
+
+	fd = open(args.pc_reenable, O_WRONLY);
+	if (fd == -1) {
+		errmsg("failed to reenable power-cut: %d", errno);
+		return -1;
+	}
+
+	CHECK(write(fd, "1\n", 2) == 2);
+	CHECK(close(fd) == 0);
+
+	return 0;
+}
+
+/**
  * Re-mount the test file-system. This function randomly select how to
  * re-mount.
  */
@@ -2604,6 +2628,7 @@  static int remount_tested_fs(void)
 
 	if (rorw1) {
 		flags = fsinfo.mount_flags | MS_RDONLY | MS_REMOUNT;
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -2614,6 +2639,7 @@  static int remount_tested_fs(void)
 
 		flags = fsinfo.mount_flags | MS_REMOUNT;
 		flags &= ~((unsigned long)MS_RDONLY);
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -2626,6 +2652,7 @@  static int remount_tested_fs(void)
 	if (um) {
 		if (um_ro) {
 			flags = fsinfo.mount_flags | MS_RDONLY | MS_REMOUNT;
+			usleep(MOUNT_DELAY);
 			ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 				    fsinfo.fstype, flags, fsinfo.mount_opts);
 			if (ret) {
@@ -2635,6 +2662,7 @@  static int remount_tested_fs(void)
 			}
 		}
 
+		usleep(MOUNT_DELAY);
 		ret = umount(fsinfo.mount_point);
 		if (ret) {
 			pcv("cannot unmount %s", fsinfo.mount_point);
@@ -2642,6 +2670,7 @@  static int remount_tested_fs(void)
 		}
 
 		if (!um_rorw) {
+			usleep(MOUNT_DELAY);
 			ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 				    fsinfo.fstype, fsinfo.mount_flags,
 				    fsinfo.mount_opts);
@@ -2651,6 +2680,7 @@  static int remount_tested_fs(void)
 				return -1;
 			}
 		} else {
+			usleep(MOUNT_DELAY);
 			ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 				    fsinfo.fstype, fsinfo.mount_flags | MS_RDONLY,
 				    fsinfo.mount_opts);
@@ -2662,6 +2692,7 @@  static int remount_tested_fs(void)
 
 			flags = fsinfo.mount_flags | MS_REMOUNT;
 			flags &= ~((unsigned long)MS_RDONLY);
+			usleep(MOUNT_DELAY);
 			ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 				    fsinfo.fstype, flags, fsinfo.mount_opts);
 			if (ret) {
@@ -2674,6 +2705,7 @@  static int remount_tested_fs(void)
 
 	if (rorw2) {
 		flags = fsinfo.mount_flags | MS_RDONLY | MS_REMOUNT;
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -2683,6 +2715,7 @@  static int remount_tested_fs(void)
 
 		flags = fsinfo.mount_flags | MS_REMOUNT;
 		flags &= ~((unsigned long)MS_RDONLY);
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -2693,6 +2726,13 @@  static int remount_tested_fs(void)
 	}
 
 	CHECK(chdir(fsinfo.mount_point) == 0);
+
+	if (args.pc_reenable) {
+		ret = reenable_power_cut();
+		if (ret)
+			return -1;
+	}
+
 	return 0;
 }
 
@@ -3015,12 +3055,16 @@  static const char doc[] = PROGRAM_NAME " version " PROGRAM_VERSION
 "file-system error) for all operations which modify it. In this case this test\n"
 "program re-mounts the file-system and checks that all files and directories\n"
 "which have been successfully synchronized before the power cut. And the test\n"
-"continues forever.\n";
+"continues forever.\n"
+"Whereas -p requires power cut emulation to be enabled globally, the -P option\n"
+"enables the use of power cut emulation on the tested fs instance only.\n";
 
 static const char optionsstr[] =
 "-n, --repeat=<count> repeat count, default is 1; zero value - repeat forever\n"
 "-p, --power-cut      power cut testing mode (-n parameter is ignored and the\n"
 "                     test continues forever)\n"
+"-P, --pc-reenable=<sysfile> Like -p but power-cut mode is re-enabled by writing\n"
+"                     '1' to the specified sysfs file after every re-mount.\n"
 "-e, --verify-ops     verify all operations, e.g., every time a file is written\n"
 "                     to, read the data back and verify it, every time a\n"
 "                     directory is created, check that it exists, etc\n"
@@ -3031,13 +3075,14 @@  static const char optionsstr[] =
 "-V, --version        print program version\n";
 
 static const struct option long_options[] = {
-	{ .name = "repeat",     .has_arg = 1, .flag = NULL, .val = 'n' },
-	{ .name = "power-cut",  .has_arg = 0, .flag = NULL, .val = 'p' },
-	{ .name = "verify-ops", .has_arg = 0, .flag = NULL, .val = 'e' },
-	{ .name = "reattach",   .has_arg = 1, .flag = NULL, .val = 'm' },
-	{ .name = "verbose",    .has_arg = 0, .flag = NULL, .val = 'v' },
-	{ .name = "help",       .has_arg = 0, .flag = NULL, .val = 'h' },
-	{ .name = "version",    .has_arg = 0, .flag = NULL, .val = 'V' },
+	{ .name = "repeat",      .has_arg = 1, .flag = NULL, .val = 'n' },
+	{ .name = "power-cut",   .has_arg = 0, .flag = NULL, .val = 'p' },
+	{ .name = "pc-reenable", .has_arg = 1, .flag = NULL, .val = 'P' },
+	{ .name = "verify-ops",  .has_arg = 0, .flag = NULL, .val = 'e' },
+	{ .name = "reattach",    .has_arg = 1, .flag = NULL, .val = 'm' },
+	{ .name = "verbose",     .has_arg = 0, .flag = NULL, .val = 'v' },
+	{ .name = "help",        .has_arg = 0, .flag = NULL, .val = 'h' },
+	{ .name = "version",     .has_arg = 0, .flag = NULL, .val = 'V' },
 	{ NULL, 0, NULL, 0},
 };
 
@@ -3052,7 +3097,7 @@  static int parse_opts(int argc, char * const argv[])
 	while (1) {
 		int key, error = 0;
 
-		key = getopt_long(argc, argv, "n:pm:evVh?", long_options, NULL);
+		key = getopt_long(argc, argv, "n:pP:m:evVh?", long_options, NULL);
 		if (key == -1)
 			break;
 
@@ -3065,6 +3110,12 @@  static int parse_opts(int argc, char * const argv[])
 		case 'p':
 			args.power_cut_mode = 1;
 			break;
+		case 'P':
+			args.pc_reenable = optarg;
+			if (args.pc_reenable[0] == '-')
+				return errmsg("bad power-cut enable file: \"%s\"", optarg);
+			args.power_cut_mode = 1;
+			break;
 		case 'm':
 			args.mtdn = simple_strtoul(optarg, &error);
 			if (error || args.mtdn < 0)
@@ -3205,13 +3256,16 @@  static int recover_tested_fs(void)
 	 * while mounting in 'remount_tested_fs()'.
 	 */
 	mntent = get_tested_fs_mntent();
-	if (mntent)
+	if (mntent) {
+		usleep(MOUNT_DELAY);
 		CHECK(umount(fsinfo.mount_point) != -1);
+	}
 
 	if (args.reattach)
 		CHECK(reattach() == 0);
 
 	if (!um_rorw) {
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 			    fsinfo.fstype, fsinfo.mount_flags,
 			    fsinfo.mount_opts);
@@ -3221,6 +3275,7 @@  static int recover_tested_fs(void)
 			return -1;
 		}
 	} else {
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 			    fsinfo.fstype, fsinfo.mount_flags | MS_RDONLY,
 			    fsinfo.mount_opts);
@@ -3232,6 +3287,7 @@  static int recover_tested_fs(void)
 
 		flags = fsinfo.mount_flags | MS_REMOUNT;
 		flags &= ~((unsigned long)MS_RDONLY);
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point,
 			    fsinfo.fstype, flags, fsinfo.mount_opts);
 		if (ret) {
@@ -3243,6 +3299,7 @@  static int recover_tested_fs(void)
 
 	if (rorw2) {
 		flags = fsinfo.mount_flags | MS_RDONLY | MS_REMOUNT;
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -3252,6 +3309,7 @@  static int recover_tested_fs(void)
 
 		flags = fsinfo.mount_flags | MS_REMOUNT;
 		flags &= ~((unsigned long)MS_RDONLY);
+		usleep(MOUNT_DELAY);
 		ret = mount(fsinfo.fsdev, fsinfo.mount_point, fsinfo.fstype,
 			    flags, fsinfo.mount_opts);
 		if (ret) {
@@ -3261,6 +3319,12 @@  static int recover_tested_fs(void)
 		}
 	}
 
+	if (args.pc_reenable) {
+		ret = reenable_power_cut();
+		if (ret)
+			return -1;
+	}
+
 	return 0;
 }