| Message ID | ED3E0BCACD909541BA94A34C4A164D4C5B401901@post.tritech.se |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, 2014-05-15 at 08:21 +0000, Mats Kärrman wrote: > From a3c9a9be5fe136d1a8e1c029a9f8cc3de634afd2 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Mats=20K=C3=A4rrman?= <mats.karrman@tritech.se> > Date: Thu, 15 May 2014 10:03:56 +0200 > Subject: [PATCH v2] mtd-utils: integck: add support for volume specific power-cut > test > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Problem: > The current power-cut test requires power-cut emulation to be enabled globally > (e.g. for all UBIFS volumes) since the power-cut mode is otherwise lost when > the file-system is re-mounted. However this may lead to some practical > problems when the root fs of the tested machine is also of the same type. > > Solution: > Modify the -p option so that it automatically enables power-cut testing > selectively for the tested filesystem and then re-enables it after every > re-mount. Enabling is done by writing "1" to the associated control file. > The file to use is deduced from the mounted device and the assumption that > debugfs is mounted on /sys/kernel/debug. > > Signed-off-by: Mats Kärrman <mats.karrman@tritech.se> Mats, firs of all, my apologies for slowness. And my apologies for leading you to this direction. But I think I see the problem with this solution now. The problem is that with this we won't get power cut emulated while you mount the file system. Before this change, we could have something like: 1. Working with the FS, power-cut 2. Unmount the FS, start mounting -> power-cut in the middle of mount. So we could test power cuts while we are in the middle of recovery from the previous power cuts, in a way "nested" power-cuts. With your change, we never have them.
Artem, On Tuesday, May 27, 2014 2:37 PM, Artem Bityutskiy wrote: > But I think I see the problem with this solution now. > > The problem is that with this we won't get power cut emulated while you > mount the file system. > > Before this change, we could have something like: > > 1. Working with the FS, power-cut > 2. Unmount the FS, start mounting -> power-cut in the middle of mount. > > So we could test power cuts while we are in the middle of recovery from > the previous power cuts, in a way "nested" power-cuts. > > With your change, we never have them. With the old version we didn't get a power-cut at all, unless power-cuts were first globally enable by writing "1" to /sys/kernel/debug/ubifs/tst_recovery. This is still possible! What wasn't possible before was to test only one filesystem image if there are more. The way I see it, my change extends what is possible, it doesn't limit it. You are correct of course, that power-cut during remount isn't tested if the test isn't run with global enable, but remounting is not a normal use-case (!?) for someone using ubifs as the rootfs. BR // Mats
On Tue, 2014-05-27 at 14:15 +0000, Mats Kärrman wrote: > Artem, > > On Tuesday, May 27, 2014 2:37 PM, Artem Bityutskiy wrote: > > But I think I see the problem with this solution now. > > > > The problem is that with this we won't get power cut emulated while you > > mount the file system. > > > > Before this change, we could have something like: > > > > 1. Working with the FS, power-cut > > 2. Unmount the FS, start mounting -> power-cut in the middle of mount. > > > > So we could test power cuts while we are in the middle of recovery from > > the previous power cuts, in a way "nested" power-cuts. > > > > With your change, we never have them. > > With the old version we didn't get a power-cut at all, unless power-cuts > were first globally enable by writing "1" to > /sys/kernel/debug/ubifs/tst_recovery. > > This is still possible! Ah, yes, it is enabled _outside_ of the test. And with this patch, the per-FS one is enabled _inside_ the test. With this patch we have the following issues: 1. -p enables per-FS power cut emulation automatically, however, you have bad coverage (the most problematic code in the UBIFS driver is related to the "nested" cuts, IIRC) 2. For better coverage, you have to to manually enable global power cuts. So, those who test UBIFS with nandsim, may mistakenly think -p is all they need, and they miss a fair amount of coverage. Not good. And we have an inconsistency: the test is able to touch the per-FS power cuts debugfs files, which is nice and user-friendly, but the global power cuts debugfs file has to be touched manually. Not nice. How about this compromise solution which should be simple enough to implement, but I am not sure it is good enough to you. Change -p so that it takes an _argument_. The argument should be a path to the debugfs file which controls the power cuts emulation. This can be either global or the per-FS switch. All the test will do is writing '1' to this file as soon as it starts, and as soon as it mounts or re-mounts the FS. Now, it is your choice which file path to give - the global or the per-FS. What do you think?
On Tuesday, May 27, 2014 4:55 PM, Artem Bityutskiy wrote: > How about this compromise solution which should be simple enough to > implement, but I am not sure it is good enough to you. > > Change -p so that it takes an _argument_. The argument should be a path > to the debugfs file which controls the power cuts emulation. This can be > either global or the per-FS switch. > > All the test will do is writing '1' to this file as soon as it starts, > and as soon as it mounts or re-mounts the FS. > > Now, it is your choice which file path to give - the global or the > per-FS. I guess you mean something in the line of http://comments.gmane.org/gmane.linux.drivers.mtd/47872 ;-) // Mats
On Wed, 2014-05-28 at 08:40 +0000, Mats Kärrman wrote: > On Tuesday, May 27, 2014 4:55 PM, Artem Bityutskiy wrote: > > How about this compromise solution which should be simple enough to > > implement, but I am not sure it is good enough to you. > > > > Change -p so that it takes an _argument_. The argument should be a path > > to the debugfs file which controls the power cuts emulation. This can be > > either global or the per-FS switch. > > > > All the test will do is writing '1' to this file as soon as it starts, > > and as soon as it mounts or re-mounts the FS. > > > > Now, it is your choice which file path to give - the global or the > > per-FS. > > I guess you mean something in the line of > http://comments.gmane.org/gmane.linux.drivers.mtd/47872 Yes, but without introducing a new option, but just changing the old one and add the path argument there.
On Wednesday, May 28, 2014 11:30 AM, Artem Bityutskiy wrote: >> I guess you mean something in the line of >> http://comments.gmane.org/gmane.linux.drivers.mtd/47872 > > Yes, but without introducing a new option, but just changing the old one > and add the path argument there. Well, you did have some valid complaints back then... How about having two options, one for global and one for per-fs enable, both automatically selecting the appropriate sysfs file and with an improved help text? I think the feature is no use anyway unless you understand something about the implementation behind it. BR // Mats
On Wed, 2014-05-28 at 10:52 +0000, Mats Kärrman wrote: > How about having two options, one for global and one for per-fs enable, > both automatically selecting the appropriate sysfs file and with an > improved help text? Should work too.
diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c index 8badd1f..3f6b8ad 100644 --- a/tests/fs-tests/integrity/integck.c +++ b/tests/fs-tests/integrity/integck.c @@ -38,7 +38,7 @@ #include <sys/mount.h> #include <sys/statvfs.h> -#define PROGRAM_VERSION "1.1" +#define PROGRAM_VERSION "1.2" #define PROGRAM_NAME "integck" #include "common.h" #include "libubi.h" @@ -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. */ @@ -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_enable_path: path to the debugfs control knob file used to 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_enable_path; } fsinfo = { .nospc_size_ok = 1, .can_mmap = 1, @@ -2580,6 +2586,24 @@ static int rm_minus_rf_dir(const char *dir_name) } /** + * Enable the power cut emulation mode on the tested file system + */ +static void enable_power_cut(void) +{ + int fd; + + fd = open(fsinfo.pc_enable_path, O_WRONLY); + if (fd == -1) { + errmsg("failed to enable power-cut: %d", errno); + CHECK(0); + } + + v("enabling power-cut for %s", fsinfo.fsdev); + CHECK(write(fd, "1\n", 2) == 2); + CHECK(close(fd) == 0); +} + +/** * Re-mount the test file-system. This function randomly select how to * re-mount. */ @@ -2693,6 +2717,10 @@ static int remount_tested_fs(void) } CHECK(chdir(fsinfo.mount_point) == 0); + + if (args.power_cut_mode) + enable_power_cut(); + return 0; } @@ -2850,6 +2878,115 @@ static int integck(void) } /* + * This is a helper function for 'make_ubifs_power_cut_enable_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_enable_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_enable_path(void) +{ + char * path; + struct stat st; + + if (!strcmp("ubifs", fsinfo.fstype)) { + path = make_ubifs_power_cut_enable_path(); + } else { + errmsg("power-cut enable not supported for %s", + fsinfo.fstype); + CHECK(0); + } + + if (stat(path, &st) == -1) { + sys_errmsg("failed to stat power-cut enable knob '%s': %d", + path, errno); + CHECK(0); + } + + fsinfo.pc_enable_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,7 +3152,8 @@ 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" +"Currently power cut testing is implemented for UBIFS only.\n"; static const char optionsstr[] = "-n, --repeat=<count> repeat count, default is 1; zero value - repeat forever\n" @@ -3261,6 +3399,9 @@ static int recover_tested_fs(void) } } + if (args.power_cut_mode) + enable_power_cut(); + return 0; } @@ -3285,6 +3426,10 @@ int main(int argc, char *argv[]) return EXIT_FAILURE; get_tested_fs_info(); + if (args.power_cut_mode) { + set_power_cut_enable_path(); + enable_power_cut(); + } /* Seed the random generator with out PID */ random_seed = getpid(); @@ -3380,5 +3525,7 @@ out_free: free(fsinfo.fstype); free(fsinfo.fsdev); free(fsinfo.test_dir); + if (fsinfo.pc_enable_path) + free(fsinfo.pc_enable_path); return ret ? EXIT_FAILURE : EXIT_SUCCESS; }