| Message ID | ED3E0BCACD909541BA94A34C4A164D4C5B3D11F5@post.tritech.se |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Mats, pushed the first patch out, thanks. On Mon, 2014-04-14 at 11:47 +0000, Mats Kärrman wrote: > -"continues forever.\n"; > +"continues forever.\n" > +"Whereas -p requires power cut emulation to be enabled globally, the -P option\n" > +"automatically enables the use of power cut emulation on the tested fs instance\n" > +"only (currently implemented for UBIFS only).\n"; Could we please invent an easy to understand way of naming this and the option. So essentially, this is about emulating power cuts at UBI and UBIFS levels. The "reenabling" thing is a small details which should not be visible in --help, because it is confusing anyway. I'd suggest to split this patch on 2. First patch renames the option to '--ubi-power-cut', refines help message, may be some comments and function names, if needed. The second patch introduces '--ubifs-power-cut', and explains that this makes sure that power cuts happen in the UBIFS driver, versus the underlying UBI driver. Thanks!
Hi Artem,
Ehhhh.... I think either you have confused things a bit or I totally misunderstand you.
Both the "-p" and the "-P" options enable UBIFS power-cut tests. AfaIk there is no such thing for UBI (?).
My problem is that I like to run the tests on my target platform that uses UBIFS as root FS.
If I enable "power-cut" emulation globally (i.e. echo 1 > /sys/kernel/debug/ubifs/tst_recovery) this will
enable "power-cuts" not only for the tested partition (good) but also for my root fs (bad).
UBIFS gives the opportunity to also enable "power-cuts" for a specific UBI volume (e.g.
echo 1 > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery) but this only works until the UBI partition
is reattached (the flag is initialized to 0).
What "-P" does differently from "-p" is that it re-enables the power-cut emulation for the used
partition before continuing the test after a remount.
It seems that the help-text of integck suggests that the power-cut test mode is available for
any filesystem that is "able to emulate power-cuts" so the "(currently implemented for UBIFS only)"
is there because this is the only fs integck is (currently) able to reenable emulation for. ("-p"
requires emulation to be enabled before launching integck which took me a while to figure out
so this should perhaps also be written in the help text).
BR // Mats
Hi again Artem, Another bump... I'll be leaving this project shortly but I'm prepared to give this another shot provided that I understand what you want. Please explain or otherwise I'll just drop this now. Best Regards, Mats On Tuesday, April 15, 2014 5:07 PM, Mats Kärrman wrote: >Hi Artem, > >Ehhhh.... I think either you have confused things a bit or I totally misunderstand you. > >Both the "-p" and the "-P" options enable UBIFS power-cut tests. AfaIk there is no such thing for UBI (?). >My problem is that I like to run the tests on my target platform that uses UBIFS as root FS. >If I enable "power-cut" emulation globally (i.e. echo 1 > /sys/kernel/debug/ubifs/tst_recovery) this will >enable "power-cuts" not only for the tested partition (good) but also for my root fs (bad). >UBIFS gives the opportunity to also enable "power-cuts" for a specific UBI volume (e.g. >echo 1 > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery) but this only works until the UBI partition >is reattached (the flag is initialized to 0). >What "-P" does differently from "-p" is that it re-enables the power-cut emulation for the used >partition before continuing the test after a remount. > >It seems that the help-text of integck suggests that the power-cut test mode is available for >any filesystem that is "able to emulate power-cuts" so the "(currently implemented for UBIFS only)" >is there because this is the only fs integck is (currently) able to reenable emulation for. ("-p" >requires emulation to be enabled before launching integck which took me a while to figure out >so this should perhaps also be written in the help text). > >BR // Mats > >________________________________________ >From: Artem Bityutskiy [dedekind1@gmail.com] >Sent: Tuesday, April 15, 2014 3:12 PM >To: Mats Kärrman >Cc: linux-mtd@lists.infradead.org >Subject: Re: [PATCH 2/2] mtd-utils: integck: add support for volume specific power-cut test > >Hi Mats, > >pushed the first patch out, thanks. > >On Mon, 2014-04-14 at 11:47 +0000, Mats Kärrman wrote: >> -"continues forever.\n"; >> +"continues forever.\n" >> +"Whereas -p requires power cut emulation to be enabled globally, the -P option\n" >> +"automatically enables the use of power cut emulation on the tested fs instance\n" >> +"only (currently implemented for UBIFS only).\n"; > >Could we please invent an easy to understand way of naming this and the >option. > >So essentially, this is about emulating power cuts at UBI and UBIFS >levels. The "reenabling" thing is a small details which should not be >visible in --help, because it is confusing anyway. > >I'd suggest to split this patch on 2. First patch renames the option to >'--ubi-power-cut', refines help message, may be some comments and >function names, if needed. > >The second patch introduces '--ubifs-power-cut', and explains that this >makes sure that power cuts happen in the UBIFS driver, versus the >underlying UBI driver. > >Thanks! > >-- >Best Regards, >Artem Bityutskiy
Hi Mats, yes, I was a bit confused. But generally I do not immediately like this patch is that it adds complexity to already complex, not very well-structured and not very well-commented code. And it adds another option, which means that the potential user will have to think about using it or not. On Tue, 2014-04-15 at 15:07 +0000, Mats Kärrman wrote: > Both the "-p" and the "-P" options enable UBIFS power-cut tests. AfaIk there is no such thing for UBI (?). > My problem is that I like to run the tests on my target platform that uses UBIFS as root FS. > If I enable "power-cut" emulation globally (i.e. echo 1 > /sys/kernel/debug/ubifs/tst_recovery) this will > enable "power-cuts" not only for the tested partition (good) but also for my root fs (bad). OK, thanks for explaining. > UBIFS gives the opportunity to also enable "power-cuts" for a specific UBI volume (e.g. > echo 1 > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery) but this only works until the UBI partition > is reattached (the flag is initialized to 0). Why wouldn't we have this to be the default, and the only way? > What "-P" does differently from "-p" is that it re-enables the power-cut emulation for the used > partition before continuing the test after a remount. Do we really need 2 options for this test? May be we can just use per-FS knob in the old option? > It seems that the help-text of integck suggests that the power-cut test mode is available for > any filesystem that is "able to emulate power-cuts" so the "(currently implemented for UBIFS only)" Well, this was probably a mistake to add this comment, feel free to kill it. This is ubifs-specific. > is there because this is the only fs integck is (currently) able to reenable emulation for. ("-p" > requires emulation to be enabled before launching integck which took me a while to figure out > so this should perhaps also be written in the help text). You are always welcome with a patch :-)
On Sent: Tuesday, May 13, 2014 1:00 PM, Artem Bityutskiy wrote: > On Tue, 2014-04-15 at 15:07 +0000, Mats Kärrman wrote: >> Both the "-p" and the "-P" options enable UBIFS power-cut tests. AfaIk there is no such thing for UBI (?). >> My problem is that I like to run the tests on my target platform that uses UBIFS as root FS. >> If I enable "power-cut" emulation globally (i.e. echo 1 > /sys/kernel/debug/ubifs/tst_recovery) this will >> enable "power-cuts" not only for the tested partition (good) but also for my root fs (bad). > > OK, thanks for explaining. > >> UBIFS gives the opportunity to also enable "power-cuts" for a specific UBI volume (e.g. >> echo 1 > /sys/kernel/debug/ubifs/ubi1_0/tst_recovery) but this only works until the UBI partition >> is reattached (the flag is initialized to 0). > > Why wouldn't we have this to be the default, and the only way? I assume we're still talking about changing integck and not about changing the UBIFS linux kernel driver ability to enable power-cuts globally. > >> What "-P" does differently from "-p" is that it re-enables the power-cut emulation for the used >> partition before continuing the test after a remount. > > Do we really need 2 options for this test? May be we can just use per-FS > knob in the old option? Perhaps I'm overly concerned about backwards compatibility... Also I was uncertain about if it may be relevant whether power-cut emulation is enabled during the remount but I guess that in that case it is still possible to have power-cuts enabled globally. > >> It seems that the help-text of integck suggests that the power-cut test mode is available for >> any filesystem that is "able to emulate power-cuts" so the "(currently implemented for UBIFS only)" > > Well, this was probably a mistake to add this comment, feel free to kill > it. This is ubifs-specific. > >> is there because this is the only fs integck is (currently) able to reenable emulation for. ("-p" >> requires emulation to be enabled before launching integck which took me a while to figure out >> so this should perhaps also be written in the help text). > > You are always welcome with a patch :-) So, what you're suggesting is that we should change integck so that: a) it has only one power-cut emulation switch: -p b) this switch should enable (and re-enable) power-cut emulation per fs instance (always). Right? BR // mats
On Tue, 2014-05-13 at 12:59 +0000, Mats Kärrman wrote: > > So, what you're suggesting is that we should change integck so that: > a) it has only one power-cut emulation switch: -p > b) this switch should enable (and re-enable) power-cut emulation per > fs instance (always). Yes, I think so.
diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c index 8badd1f..90ccbbc 100644 --- a/tests/fs-tests/integrity/integck.c +++ b/tests/fs-tests/integrity/integck.c @@ -61,6 +61,9 @@ /* Maximum buffer size for a single read/write operation */ #define IO_BUFFER_SIZE 32768 +/* Kernel debug fs mount point */ +#define DEBUGFS_MOUNT_POINT "/sys/kernel/debug" + /* * Check if a condition is true and die if not. */ @@ -94,7 +97,7 @@ /* The variables below are set by command line arguments */ static struct { long repeat_cnt; - int power_cut_mode; + int power_cut_mode; /* 0=off, 1=on, 2=auto enable */ int verify_ops; int reattach; int mtdn; @@ -123,6 +126,8 @@ static struct { * options as stored as a set of flags) * mount_point: tested file-system mount point path * test_dir: the directory on the tested file-system where we test + * pc_reenable_path: path to the debugfs control knob file used to re-enable + * power-cut mode for the tested file-system */ static struct { int max_name_len; @@ -137,6 +142,7 @@ static struct { unsigned long mount_flags; char *mount_point; char *test_dir; + char *pc_reenable_path; } fsinfo = { .nospc_size_ok = 1, .can_mmap = 1, @@ -2580,6 +2586,26 @@ 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(fsinfo.pc_reenable_path, O_WRONLY); + if (fd == -1) { + errmsg("failed to re-enable power-cut: %d", errno); + return -1; + } + + v("re-enabling power-cut for %s", fsinfo.fsdev); + 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. */ @@ -2693,6 +2719,13 @@ static int remount_tested_fs(void) } CHECK(chdir(fsinfo.mount_point) == 0); + + if (args.power_cut_mode == 2) { + ret = reenable_power_cut(); + if (ret) + return -1; + } + return 0; } @@ -2850,6 +2883,115 @@ static int integck(void) } /* + * This is a helper function for 'make_ubifs_power_cut_reenable_path()' that + * uses libubi to find out the ubifs volume number from a volume name. + */ +static int get_ubi_vol_num_from_name(int devnum, const char * name) +{ + libubi_t libubi; + struct ubi_vol_info vol_info; + int err, vol_num; + + libubi = libubi_open(); + if (!libubi) { + if (errno == 0) + errmsg("UBI is not present in the system"); + else + sys_errmsg("cannot open libubi"); + return -1; + } + + err = ubi_get_vol_info1_nm(libubi, devnum, name, &vol_info); + if (err) { + sys_errmsg("cannot find UBI volume \"%s\"", name); + vol_num = -1; + } else { + vol_num = vol_info.vol_id; + } + + libubi_close(libubi); + return vol_num; +} + +/* + * Make a path for an ubifs power-cut enable knob file according to the + * device and volume number of the tested file-system. + */ +static char * make_ubifs_power_cut_reenable_path(void) +{ + int ubidev, ubivol; + char *p; + char path_tail[32]; + + /* + * Try to find the ubifs device and volume number using the + * fsinfo device string. + * Valid formats are (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. + * The first format may also be preceeded by the path to the dev node + * directory, e.g. "/dev/ubiX_Y". + */ + ubivol = -1; + p = strstr(fsinfo.fsdev, "ubi"); + if (p) { + p += 3; + if (isdigit(*p)) { + if (p[1] == '_' && isdigit(p[2]) && p[3] == '\0') { + ubidev = *p - '0'; + ubivol = p[2] - '0'; + } else if (p[1] == '\0') { + ubidev = 0; + ubivol = *p - '0'; + } else if (p[1] == ':') { + ubidev = *p - '0'; + ubivol = get_ubi_vol_num_from_name(ubidev, p+2); + } + } else if (*p == ':') { + ubidev = 0; + ubivol = get_ubi_vol_num_from_name(ubidev, p+1); + } + } + + if (ubivol == -1) { + errmsg("failed to parse ubifs device string '%s'", + fsinfo.fsdev); + return NULL; + } + + sprintf(path_tail, "/ubifs/ubi%d_%d/tst_recovery", ubidev, ubivol); + return cat_strings(DEBUGFS_MOUNT_POINT, path_tail); +} + +/* + * Set path to the power-cut enable knob file according to the type of + * the tested file system. + */ +static void set_power_cut_reenable_path(void) +{ + char * path; + struct stat st; + + if (!strcmp("ubifs", fsinfo.fstype)) { + path = make_ubifs_power_cut_reenable_path(); + } else { + errmsg("automatic power-cut re-enable not supported for %s", + fsinfo.fstype); + CHECK(0); + } + + if (stat(path, &st) == -1) { + sys_errmsg("failed to stat power-cut re-enable knob '%s': %d", + path, errno); + CHECK(0); + } + + fsinfo.pc_reenable_path = path; +} + +/* * This is a helper function for 'get_tested_fs_info()'. It parses file-system * mount options string, extracts standard mount options from there, and saves * them in the 'fsinfo.mount_flags' variable, and non-standard mount options @@ -3015,12 +3157,17 @@ 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" +"automatically enables the use of power cut emulation on the tested fs instance\n" +"only (currently implemented for UBIFS 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 Like -p but power-cut mode is re-enabled by writing\n" +" '1' to the associated 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 +3178,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 = 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' }, { NULL, 0, NULL, 0}, }; @@ -3052,7 +3200,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:pPm:evVh?", long_options, NULL); if (key == -1) break; @@ -3063,7 +3211,11 @@ static int parse_opts(int argc, char * const argv[]) return errmsg("bad repeat count: \"%s\"", optarg); break; case 'p': - args.power_cut_mode = 1; + if (!args.power_cut_mode) + args.power_cut_mode = 1; + break; + case 'P': + args.power_cut_mode = 2; break; case 'm': args.mtdn = simple_strtoul(optarg, &error); @@ -3261,6 +3413,12 @@ static int recover_tested_fs(void) } } + if (args.power_cut_mode == 2) { + ret = reenable_power_cut(); + if (ret) + return -1; + } + return 0; } @@ -3285,6 +3443,8 @@ int main(int argc, char *argv[]) return EXIT_FAILURE; get_tested_fs_info(); + if (args.power_cut_mode == 2) + set_power_cut_reenable_path(); /* Seed the random generator with out PID */ random_seed = getpid();