Patchwork [v3,2/3] mmc: core: add random fault injection

login
register
mail settings
Submitter Akinobu Mita
Date July 25, 2011, 3:58 p.m.
Message ID <CAC5umyg44tQxrFEYRL-tjzBMA2+KNXjB+nQveH3KcB_LPmYVHw@mail.gmail.com>
Download mbox | patch
Permalink /patch/106712/
State New
Headers show

Comments

Akinobu Mita - July 25, 2011, 3:58 p.m.
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)
Per Forlin - July 25, 2011, 7:19 p.m.
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
Per Forlin - July 25, 2011, 9:20 p.m.
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
Akinobu Mita - July 26, 2011, 1:41 a.m.
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
	...
}
Per Forlin - July 26, 2011, 8:01 p.m.
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.
Per Forlin - July 26, 2011, 8:08 p.m.
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

Patch

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