| Message ID | CAC5umyg44tQxrFEYRL-tjzBMA2+KNXjB+nQveH3KcB_LPmYVHw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 July 2011 17:58, Akinobu Mita <akinobu.mita@gmail.com> wrote: > 2011/7/21 Per Forlin <per.forlin@linaro.org>: >> This adds support to inject data errors after a completed host transfer. >> The mmc core will return error even though the host transfer is successful. >> This simple fault injection proved to be very useful to test the >> non-blocking error handling in the mmc_blk_issue_rw_rq(). >> Random faults can also test how the host driver handles pre_req() >> and post_req() in case of errors. > > Looks good to me. > Thanks, >> @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) >> flush_workqueue(workqueue); >> } >> >> +#ifdef CONFIG_FAIL_MMC_REQUEST >> + >> +static DECLARE_FAULT_ATTR(fail_mmc_request); > > I think the fail_attr should be defined for each mmc_host like make_it_fail > in struct mmc_host and debugfs entries should also be created in a > subdirectory of mmc host debugfs. > I looked at blk-core.c and used the same code here. Current code creates the entry under the debugfs root. This means one fail_attr for all hosts. I agree, it's more clean to move the fail_attr to the host-debugfs-entry which require the fail_attr to be stored same way as make_it_fail. > And I know that init_fault_attr_dentries() can only create a > subdirectory in debugfs root directory. But I have a patch which > support for creating it in arbitrary directory. Could you take a look > at this? (Note that this patch is based on mmotm and not yet tested) > Thanks, I will check it out. Regards, Per
Hi Akinobu, On 25 July 2011 21:19, Per Forlin <per.forlin@linaro.org> wrote: > On 25 July 2011 17:58, Akinobu Mita <akinobu.mita@gmail.com> wrote: >> 2011/7/21 Per Forlin <per.forlin@linaro.org>: >>> This adds support to inject data errors after a completed host transfer. >>> The mmc core will return error even though the host transfer is successful. >>> This simple fault injection proved to be very useful to test the >>> non-blocking error handling in the mmc_blk_issue_rw_rq(). >>> Random faults can also test how the host driver handles pre_req() >>> and post_req() in case of errors. >> >> Looks good to me. >> > Thanks, > >>> @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) >>> flush_workqueue(workqueue); >>> } >>> >>> +#ifdef CONFIG_FAIL_MMC_REQUEST >>> + >>> +static DECLARE_FAULT_ATTR(fail_mmc_request); >> >> I think the fail_attr should be defined for each mmc_host like make_it_fail >> in struct mmc_host and debugfs entries should also be created in a >> subdirectory of mmc host debugfs. >> > I looked at blk-core.c and used the same code here. Current code > creates the entry under the debugfs root. This means one fail_attr for > all hosts. > I agree, it's more clean to move the fail_attr to the > host-debugfs-entry which require the fail_attr to be stored same way > as make_it_fail. > >> And I know that init_fault_attr_dentries() can only create a >> subdirectory in debugfs root directory. But I have a patch which >> support for creating it in arbitrary directory. Could you take a look >> at this? (Note that this patch is based on mmotm and not yet tested) >> I looked at your patch and it raised two questions. I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the heap. It looks like setup_fault_attr(attr, str) will fail if str is NULL. How can I initialise the fault_attrs if not stack allocated? About the boot param initialisation of fault attr. There can only be one fault_mmc_request boot param for the entire kernel but there is one fault_attr per host, and there may be many hosts. It would be convenient if setup_fault_attrs would take (attr, boot_param_name), look up boot_param_name and use that otherwise set default values. Regards, Per
2011/7/26 Per Forlin <per.forlin@linaro.org>: >>> And I know that init_fault_attr_dentries() can only create a >>> subdirectory in debugfs root directory. But I have a patch which >>> support for creating it in arbitrary directory. Could you take a look >>> at this? (Note that this patch is based on mmotm and not yet tested) >>> > I looked at your patch and it raised two questions. > I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the > heap. It looks like setup_fault_attr(attr, str) will fail if str is > NULL. How can I initialise the fault_attrs if not stack allocated? > About the boot param initialisation of fault attr. There can only be > one fault_mmc_request boot param for the entire kernel but there is > one fault_attr per host, and there may be many hosts. It would be > convenient if setup_fault_attrs would take (attr, boot_param_name), > look up boot_param_name and use that otherwise set default values. I think you can define one default fail_attr for boot time configuration and copy it to per-host fail_attr in mmc_add_host_debugfs(). /* pseudo-code */ static DECLARE_FAULT_ATTR(default_mmc_fail_attr); static int __init setup_fail_mmc_request(char *str) { return setup_fault_attr(&default_mmc_fail_attr, str); } __setup("fail_mmc_request=", setup_fail_mmc_request); ... void mmc_add_host_debugfs(struct mmc_host *host) { ... #ifdef CONFIG_FAIL_MMC_REQUEST host->fail_attr = default_mmc_fail_attr; if (!debugfs_create_fault_attr("fail_mmc_request", root, &host->fail_attr)) goto err_node; #endif ... }
On 26 July 2011 03:41, Akinobu Mita <akinobu.mita@gmail.com> wrote: > 2011/7/26 Per Forlin <per.forlin@linaro.org>: >>>> And I know that init_fault_attr_dentries() can only create a >>>> subdirectory in debugfs root directory. But I have a patch which >>>> support for creating it in arbitrary directory. Could you take a look >>>> at this? (Note that this patch is based on mmotm and not yet tested) >>>> >> I looked at your patch and it raised two questions. >> I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the >> heap. It looks like setup_fault_attr(attr, str) will fail if str is >> NULL. How can I initialise the fault_attrs if not stack allocated? >> About the boot param initialisation of fault attr. There can only be >> one fault_mmc_request boot param for the entire kernel but there is >> one fault_attr per host, and there may be many hosts. It would be >> convenient if setup_fault_attrs would take (attr, boot_param_name), >> look up boot_param_name and use that otherwise set default values. > > I think you can define one default fail_attr for boot time configuration > and copy it to per-host fail_attr in mmc_add_host_debugfs(). > You're right. This works fine. I'll prepare a new version of my patch.
On 25 July 2011 17:58, Akinobu Mita <akinobu.mita@gmail.com> wrote: > 2011/7/21 Per Forlin <per.forlin@linaro.org>: >> This adds support to inject data errors after a completed host transfer. >> The mmc core will return error even though the host transfer is successful. >> This simple fault injection proved to be very useful to test the >> non-blocking error handling in the mmc_blk_issue_rw_rq(). >> Random faults can also test how the host driver handles pre_req() >> and post_req() in case of errors. > > Looks good to me. > >> @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) >> flush_workqueue(workqueue); >> } >> >> +#ifdef CONFIG_FAIL_MMC_REQUEST >> + >> +static DECLARE_FAULT_ATTR(fail_mmc_request); > > I think the fail_attr should be defined for each mmc_host like make_it_fail > in struct mmc_host and debugfs entries should also be created in a > subdirectory of mmc host debugfs. > > And I know that init_fault_attr_dentries() can only create a > subdirectory in debugfs root directory. But I have a patch which > support for creating it in arbitrary directory. Could you take a look > at this? (Note that this patch is based on mmotm and not yet tested) > I tested your patch on a Snowball board. I had two active mmc cards and did various fault injection configurations on the two cards together with dd. I did only test MMC IO fault injection and not any of the other fault injection clients. Tested-by: Per Forlin <per.forlin@linaro.org> Regards, Per
From 79fdd83b2af932d6fd155ae918e20572e6784bb8 Mon Sep 17 00:00:00 2001 From: Akinobu Mita <akinobu.mita@gmail.com> Date: Mon, 25 Jul 2011 23:51:20 +0900 Subject: [PATCH] fault-injection: support for creating debugfs entries in arbitrary directory Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- Documentation/fault-injection/fault-injection.txt | 3 +-- block/blk-core.c | 6 ++++-- block/blk-timeout.c | 5 ++++- include/linux/fault-inject.h | 18 +++++------------- lib/fault-inject.c | 20 +++++++------------- mm/failslab.c | 14 +++++++------- mm/page_alloc.c | 13 +++++-------- 7 files changed, 33 insertions(+), 46 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 7be15e4..dda989e 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -143,8 +143,7 @@ o provide a way to configure fault attributes failslab, fail_page_alloc, and fail_make_request use this way. Helper functions: - init_fault_attr_dentries(entries, attr, name); - void cleanup_fault_attr_dentries(entries); + debugfs_create_fault_attr(name, parent, attr); - module parameters diff --git a/block/blk-core.c b/block/blk-core.c index 07988b4..498c525 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1367,8 +1367,10 @@ static bool should_fail_request(struct hd_struct *part, unsigned int bytes) static int __init fail_make_request_debugfs(void) { - return init_fault_attr_dentries(&fail_make_request, - "fail_make_request"); + struct dentry *dir = debugfs_create_fault_attr("fail_make_request", + NULL, &fail_make_request); + + return IS_ERR(dir) ? PTR_ERR(dir) : 0; } late_initcall(fail_make_request_debugfs); diff --git a/block/blk-timeout.c b/block/blk-timeout.c index 4f0c06c..6397e2e 100644 --- a/block/blk-timeout.c +++ b/block/blk-timeout.c @@ -28,7 +28,10 @@ int blk_should_fake_timeout(struct request_queue *q) static int __init fail_io_timeout_debugfs(void) { - return init_fault_attr_dentries(&fail_io_timeout, "fail_io_timeout"); + struct dentry *dir = debugfs_create_fault_attr("fail_io_timeout", + NULL, &fail_io_timeout); + + return IS_ERR(dir) ? PTR_ERR(dir) : 0; } late_initcall(fail_io_timeout_debugfs); diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h index a842db6..3f583df 100644 --- a/include/linux/fault-inject.h +++ b/include/linux/fault-inject.h @@ -25,10 +25,6 @@ struct fault_attr { unsigned long reject_end; unsigned long count; - -#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS - struct dentry *dir; -#endif }; #define FAULT_ATTR_INITIALIZER { \ @@ -45,19 +41,15 @@ bool should_fail(struct fault_attr *attr, ssize_t size); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS -int init_fault_attr_dentries(struct fault_attr *attr, const char *name); -void cleanup_fault_attr_dentries(struct fault_attr *attr); +struct dentry *debugfs_create_fault_attr(const char *name, + struct dentry *parent, struct fault_attr *attr); #else /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -static inline int init_fault_attr_dentries(struct fault_attr *attr, - const char *name) -{ - return -ENODEV; -} - -static inline void cleanup_fault_attr_dentries(struct fault_attr *attr) +static inline struct dentry *debugfs_create_fault_attr(const char *name, + struct dentry *parent, struct fault_attr *attr); { + return ERR_PTR(-ENODEV); } #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ diff --git a/lib/fault-inject.c b/lib/fault-inject.c index da61bb5..95399e7 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -192,21 +192,15 @@ static struct dentry *debugfs_create_atomic_t(const char *name, mode_t mode, return debugfs_create_file(name, mode, parent, value, &fops_atomic_t); } -void cleanup_fault_attr_dentries(struct fault_attr *attr) -{ - debugfs_remove_recursive(attr->dir); -} - -int init_fault_attr_dentries(struct fault_attr *attr, const char *name) +struct dentry *debugfs_create_fault_attr(const char *name, + struct dentry *parent, struct fault_attr *attr) { mode_t mode = S_IFREG | S_IRUSR | S_IWUSR; struct dentry *dir; - dir = debugfs_create_dir(name, NULL); + dir = debugfs_create_dir(name, parent); if (!dir) - return -ENOMEM; - - attr->dir = dir; + return ERR_PTR(-ENOMEM); if (!debugfs_create_ul("probability", mode, dir, &attr->probability)) goto fail; @@ -238,11 +232,11 @@ int init_fault_attr_dentries(struct fault_attr *attr, const char *name) #endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */ - return 0; + return dir; fail: - debugfs_remove_recursive(attr->dir); + debugfs_remove_recursive(dir); - return -ENOMEM; + return ERR_PTR(-ENOMEM); } #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ diff --git a/mm/failslab.c b/mm/failslab.c index 1ce58c2..59a3093 100644 --- a/mm/failslab.c +++ b/mm/failslab.c @@ -34,23 +34,23 @@ __setup("failslab=", setup_failslab); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS static int __init failslab_debugfs_init(void) { + struct dentry *dir; mode_t mode = S_IFREG | S_IRUSR | S_IWUSR; - int err; - err = init_fault_attr_dentries(&failslab.attr, "failslab"); - if (err) - return err; + dir = debugfs_create_fault_attr("failslab", NULL, &failslab.attr); + if (IS_ERR(dir)) + return PTR_ERR(dir); - if (!debugfs_create_bool("ignore-gfp-wait", mode, failslab.attr.dir, + if (!debugfs_create_bool("ignore-gfp-wait", mode, dir, &failslab.ignore_gfp_wait)) goto fail; - if (!debugfs_create_bool("cache-filter", mode, failslab.attr.dir, + if (!debugfs_create_bool("cache-filter", mode, dir, &failslab.cache_filter)) goto fail; return 0; fail: - cleanup_fault_attr_dentries(&failslab.attr); + debugfs_remove_recursive(dir); return -ENOMEM; } diff --git a/mm/page_alloc.c b/mm/page_alloc.c index ed59a2e..00cf04f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1409,14 +1409,11 @@ static int __init fail_page_alloc_debugfs(void) { mode_t mode = S_IFREG | S_IRUSR | S_IWUSR; struct dentry *dir; - int err; - err = init_fault_attr_dentries(&fail_page_alloc.attr, - "fail_page_alloc"); - if (err) - return err; - - dir = fail_page_alloc.attr.dir; + dir = debugfs_create_fault_attr("fail_page_alloc", NULL, + &fail_page_alloc.attr); + if (IS_ERR(dir)) + return PTR_ERR(dir); if (!debugfs_create_bool("ignore-gfp-wait", mode, dir, &fail_page_alloc.ignore_gfp_wait)) @@ -1430,7 +1427,7 @@ static int __init fail_page_alloc_debugfs(void) return 0; fail: - cleanup_fault_attr_dentries(&fail_page_alloc.attr); + debugfs_remove_recursive(dir); return -ENOMEM; } -- 1.7.4.4