* [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups
@ 2018-09-12 5:10 Chengguang Xu
2018-09-12 5:10 ` [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc() Chengguang Xu
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
This patch set mainly does option validation for remount and at
the same time does related code cleanups. Currently when we call
fault injection related code we have to surround it with macro
CONFIG_EROFS_FAULT_INJECTION in every calling place, after this
patch set we don't have to that, so the code looks clean and more
understandable.
Chengguang Xu (7):
staging: erofs: code cleanup for erofs_kmalloc()
staging: erofs: code cleanup for option parsing of fault_injection
staging: erofs: introduce a new helper __erofs_build_fault_attr()
staging: erofs: return -EINVAL when specifying fault rate to 0
staging: erofs: introduce a new helper erofs_get_fault_rate()
staging: erofs: code cleanup for erofs_show_options()
staging: erofs: option validation in remount
drivers/staging/erofs/internal.h | 16 ++++---
drivers/staging/erofs/super.c | 74 +++++++++++++++++++++++++-------
2 files changed, 69 insertions(+), 21 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-13 2:04 ` Chao Yu
2018-09-12 5:10 ` [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection Chengguang Xu
` (5 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Define a dummy function of time_to_inject(), so that we don't
have to check macro CONFIG_EROFS_FAULT_INJECTION in calling place.
Base on above adjustment, do proper code cleanup for erofs_kmalloc().
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/internal.h | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 367b39fe46e5..1bb2e9e96143 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -42,12 +42,12 @@
#define DBG_BUGON(...) ((void)0)
#endif
-#ifdef CONFIG_EROFS_FAULT_INJECTION
enum {
FAULT_KMALLOC,
FAULT_MAX,
};
+#ifdef CONFIG_EROFS_FAULT_INJECTION
extern char *erofs_fault_name[FAULT_MAX];
#define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
@@ -139,21 +139,25 @@ static inline bool time_to_inject(struct erofs_sb_info *sbi, int type)
atomic_inc(&ffi->inject_ops);
if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
atomic_set(&ffi->inject_ops, 0);
+ erofs_show_injection_info(type);
return true;
}
return false;
}
+
+#else
+static inline bool time_to_inject(struct erofs_sb_info *sbi, int type)
+{
+ return false;
+}
#endif
static inline void *erofs_kmalloc(struct erofs_sb_info *sbi,
size_t size, gfp_t flags)
{
-#ifdef CONFIG_EROFS_FAULT_INJECTION
- if (time_to_inject(sbi, FAULT_KMALLOC)) {
- erofs_show_injection_info(FAULT_KMALLOC);
+ if (time_to_inject(sbi, FAULT_KMALLOC))
return NULL;
- }
-#endif
+
return kmalloc(size, flags);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
2018-09-12 5:10 ` [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc() Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-13 2:15 ` Chao Yu
2018-09-12 5:10 ` [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr() Chengguang Xu
` (4 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Define a dummpy function of erofs_build_fault_attr() when macro
CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
check the macro in calling place. Based on above adjustment,
do proper code cleanup for option parsing of fault_injection.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 1aec509c805f..14dbb6517b8d 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
[FAULT_KMALLOC] = "kmalloc",
};
-static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
- unsigned int rate)
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ substring_t *args)
{
struct erofs_fault_info *ffi = &sbi->fault_info;
+ int rate = 0;
+
+ if (args->from && match_int(args, &rate))
+ return -EINVAL;
if (rate) {
atomic_set(&ffi->inject_ops, 0);
ffi->inject_rate = rate;
ffi->inject_type = (1 << FAULT_MAX) - 1;
+ set_opt(sbi, FAULT_INJECTION);
} else {
memset(ffi, 0, sizeof(struct erofs_fault_info));
+ clear_opt(sbi, FAULT_INJECTION);
}
+
+ return 0;
+}
+#else
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ substring_t *args)
+{
+ infoln("fault_injection options not supported");
+ return 0;
}
#endif
@@ -192,7 +207,7 @@ static int parse_options(struct super_block *sb, char *options)
{
substring_t args[MAX_OPT_ARGS];
char *p;
- int arg = 0;
+ int err;
if (!options)
return 0;
@@ -238,15 +253,11 @@ static int parse_options(struct super_block *sb, char *options)
break;
#endif
case Opt_fault_injection:
- if (args->from && match_int(args, &arg))
- return -EINVAL;
-#ifdef CONFIG_EROFS_FAULT_INJECTION
- erofs_build_fault_attr(EROFS_SB(sb), arg);
- set_opt(EROFS_SB(sb), FAULT_INJECTION);
-#else
- infoln("FAULT_INJECTION was not selected");
-#endif
+ err = erofs_build_fault_attr(EROFS_SB(sb), args);
+ if (err)
+ return err;
break;
+
default:
errln("Unrecognized mount option \"%s\" "
"or missing value", p);
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
2018-09-12 5:10 ` [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc() Chengguang Xu
2018-09-12 5:10 ` [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-12 11:22 ` Gao Xiang
2018-09-12 5:10 ` [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0 Chengguang Xu
` (3 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Introduce a new helper __erofs_build_fault_attr() to handle set/clear
erofs_fault_info, we need this funciton for internal use case.
for example, reset fault_injection option in remount.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 14dbb6517b8d..d2dbc1fd3abb 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
[FAULT_KMALLOC] = "kmalloc",
};
-static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
- substring_t *args)
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ unsigned int rate)
{
- struct erofs_fault_info *ffi = &sbi->fault_info;
- int rate = 0;
-
- if (args->from && match_int(args, &rate))
- return -EINVAL;
-
if (rate) {
atomic_set(&ffi->inject_ops, 0);
ffi->inject_rate = rate;
@@ -162,10 +156,26 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
memset(ffi, 0, sizeof(struct erofs_fault_info));
clear_opt(sbi, FAULT_INJECTION);
}
+}
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ substring_t *args)
+{
+ struct erofs_fault_info *ffi = &sbi->fault_info;
+ int rate = 0;
+
+ if (args->from && match_int(args, &rate))
+ return -EINVAL;
+
+ __erofs_build_fault_attr(sbi, rate);
return 0;
}
#else
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ unsigned int rate)
+{
+}
+
static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
substring_t *args)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
` (2 preceding siblings ...)
2018-09-12 5:10 ` [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr() Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-12 9:16 ` Dan Carpenter
2018-09-12 5:10 ` [PATCH 5/7] staging: erofs: introduce a new helper erofs_get_fault_rate() Chengguang Xu
` (2 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Set fault rate to 0 is useless and confusable, so add check to
avoid it.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index d2dbc1fd3abb..8df680aee55a 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -166,6 +166,8 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
if (args->from && match_int(args, &rate))
return -EINVAL;
+ if (!rate)
+ return -EINVAL;
__erofs_build_fault_attr(sbi, rate);
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/7] staging: erofs: introduce a new helper erofs_get_fault_rate()
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
` (3 preceding siblings ...)
2018-09-12 5:10 ` [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0 Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-12 5:10 ` [PATCH 6/7] staging: erofs: code cleanup for erofs_show_options() Chengguang Xu
2018-09-12 5:10 ` [PATCH 7/7] staging: erofs: option validation in remount Chengguang Xu
6 siblings, 0 replies; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Introduce a new helper for getting fault rate, so that we
don't have to check macro in calling place.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 8df680aee55a..a6ae60564c65 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -144,6 +144,11 @@ char *erofs_fault_name[FAULT_MAX] = {
[FAULT_KMALLOC] = "kmalloc",
};
+static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
+{
+ return sbi->fault_info.inject_rate;
+}
+
static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
unsigned int rate)
{
@@ -173,6 +178,11 @@ static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
return 0;
}
#else
+static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
+{
+ return 0;
+}
+
static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
unsigned int rate)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/7] staging: erofs: code cleanup for erofs_show_options()
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
` (4 preceding siblings ...)
2018-09-12 5:10 ` [PATCH 5/7] staging: erofs: introduce a new helper erofs_get_fault_rate() Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
2018-09-12 5:10 ` [PATCH 7/7] staging: erofs: option validation in remount Chengguang Xu
6 siblings, 0 replies; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Call erofs_get_fault_rate() to get fault rate instead of
directly getting it from sbi, so we can remove the macro
check surrounding it.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index a6ae60564c65..702098c80446 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -648,11 +648,9 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
else
seq_puts(seq, ",noacl");
#endif
-#ifdef CONFIG_EROFS_FAULT_INJECTION
if (test_opt(sbi, FAULT_INJECTION))
seq_printf(seq, ",fault_injection=%u",
- sbi->fault_info.inject_rate);
-#endif
+ erofs_get_fault_rate(sbi));
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/7] staging: erofs: option validation in remount
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
` (5 preceding siblings ...)
2018-09-12 5:10 ` [PATCH 6/7] staging: erofs: code cleanup for erofs_show_options() Chengguang Xu
@ 2018-09-12 5:10 ` Chengguang Xu
6 siblings, 0 replies; 23+ messages in thread
From: Chengguang Xu @ 2018-09-12 5:10 UTC (permalink / raw)
Add option validation in remount. After this patch, remount
can change recognized options, and for unknown options remount
will fail and report error.
Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
---
drivers/staging/erofs/super.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 702098c80446..953408fb82c8 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -656,10 +656,23 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
static int erofs_remount(struct super_block *sb, int *flags, char *data)
{
+ struct erofs_sb_info *sbi = EROFS_SB(sb);
+ unsigned int org_mnt_opt = sbi->mount_opt;
+ unsigned int org_inject_rate = erofs_get_fault_rate(sbi);
+ int err;
+
BUG_ON(!sb_rdonly(sb));
+ err = parse_options(sb, data);
+ if (err)
+ goto out;
*flags |= MS_RDONLY;
return 0;
+out:
+ __erofs_build_fault_attr(sbi, org_inject_rate);
+ sbi->mount_opt = org_mnt_opt;
+
+ return err;
}
const struct super_operations erofs_sops = {
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0
2018-09-12 5:10 ` [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0 Chengguang Xu
@ 2018-09-12 9:16 ` Dan Carpenter
2018-09-12 14:05 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2018-09-12 9:16 UTC (permalink / raw)
On Wed, Sep 12, 2018@01:10:31PM +0800, Chengguang Xu wrote:
> Set fault rate to 0 is useless and confusable, so add check to
> avoid it.
>
I would have assumed setting rate to zero just disabled it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
2018-09-12 5:10 ` [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr() Chengguang Xu
@ 2018-09-12 11:22 ` Gao Xiang
2018-09-12 14:23 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Gao Xiang @ 2018-09-12 11:22 UTC (permalink / raw)
Hi Chengguang,
On 2018/9/12 13:10, Chengguang Xu wrote:
> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
> erofs_fault_info, we need this funciton for internal use case.
> for example, reset fault_injection option in remount.
>
> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
> ---
> drivers/staging/erofs/super.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 14dbb6517b8d..d2dbc1fd3abb 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - substring_t *args)
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
> {
> - struct erofs_fault_info *ffi = &sbi->fault_info;
> - int rate = 0;
> -
> - if (args->from && match_int(args, &rate))
> - return -EINVAL;
> -
> if (rate) {
I get some compile error of this patch...
drivers/staging/erofs/super.c: In function ?__erofs_build_fault_attr?:
drivers/staging/erofs/super.c:156:15: error: ?ffi? undeclared (first use in this function)
atomic_set(&ffi->inject_ops, 0);
^
drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is reported only once for each function it appears in
drivers/staging/erofs/super.c: In function ?erofs_build_fault_attr?:
drivers/staging/erofs/super.c:169:27: warning: unused variable ?ffi? [-Wunused-variable]
struct erofs_fault_info *ffi = &sbi->fault_info;
p.s. could you please rebase your patch on Thomas's [PATCH v4] staging: erofs: use explicit unsigned int type ?
since I'm rebasing the rest PREVIEW patches on this commit now.
p.p.s. I'd like to get Chao's idea of this fault injection patchset first :)
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0
2018-09-12 9:16 ` Dan Carpenter
@ 2018-09-12 14:05 ` cgxu519
2018-09-12 14:25 ` Dan Carpenter
0 siblings, 1 reply; 23+ messages in thread
From: cgxu519 @ 2018-09-12 14:05 UTC (permalink / raw)
On 09/12/2018 05:16 PM, Dan Carpenter wrote:
> On Wed, Sep 12, 2018@01:10:31PM +0800, Chengguang Xu wrote:
>> Set fault rate to 0 is useless and confusable, so add check to
>> avoid it.
>>
> I would have assumed setting rate to zero just disabled it.
I think currently it is useless because we have not implemented
option parsing in remount yet, maybe it's better adding another
option 'no_fault_injection' to explicitly disable it.
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
2018-09-12 11:22 ` Gao Xiang
@ 2018-09-12 14:23 ` cgxu519
2018-09-12 14:50 ` Gao Xiang
0 siblings, 1 reply; 23+ messages in thread
From: cgxu519 @ 2018-09-12 14:23 UTC (permalink / raw)
On 09/12/2018 07:22 PM, Gao Xiang wrote:
> Hi Chengguang,
>
> On 2018/9/12 13:10, Chengguang Xu wrote:
>> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
>> erofs_fault_info, we need this funciton for internal use case.
>> for example, reset fault_injection option in remount.
>>
>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>> ---
>> drivers/staging/erofs/super.c | 26 ++++++++++++++++++--------
>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index 14dbb6517b8d..d2dbc1fd3abb 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
>> [FAULT_KMALLOC] = "kmalloc",
>> };
>>
>> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> - substring_t *args)
>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> + unsigned int rate)
>> {
>> - struct erofs_fault_info *ffi = &sbi->fault_info;
>> - int rate = 0;
>> -
>> - if (args->from && match_int(args, &rate))
>> - return -EINVAL;
>> -
>> if (rate) {
> I get some compile error of this patch...
> drivers/staging/erofs/super.c: In function ?__erofs_build_fault_attr?:
> drivers/staging/erofs/super.c:156:15: error: ?ffi? undeclared (first use in this function)
> atomic_set(&ffi->inject_ops, 0);
> ^
> drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is reported only once for each function it appears in
> drivers/staging/erofs/super.c: In function ?erofs_build_fault_attr?:
> drivers/staging/erofs/super.c:169:27: warning: unused variable ?ffi? [-Wunused-variable]
> struct erofs_fault_info *ffi = &sbi->fault_info;
Sorry for that, I'll fix it in rebased version.
>
> p.s. could you please rebase your patch on Thomas's [PATCH v4] staging: erofs: use explicit unsigned int type ?
> since I'm rebasing the rest PREVIEW patches on this commit now.
I noticed Thomas' patch had already committed to erofs-master branch in
Chao's linux git repo, also my
previous patch but not in erofs-dev branch. So should I rebase on
erofs-master?
Could you give me a little more guide for it?
>
> p.p.s. I'd like to get Chao's idea of this fault injection patchset first :)
No problem, let's wait for a while, then I'll rebase the code according
to the comments.
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0
2018-09-12 14:05 ` cgxu519
@ 2018-09-12 14:25 ` Dan Carpenter
2018-09-12 14:41 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2018-09-12 14:25 UTC (permalink / raw)
On Wed, Sep 12, 2018@10:05:26PM +0800, cgxu519 wrote:
> On 09/12/2018 05:16 PM, Dan Carpenter wrote:
> > On Wed, Sep 12, 2018@01:10:31PM +0800, Chengguang Xu wrote:
> > > Set fault rate to 0 is useless and confusable, so add check to
> > > avoid it.
> > >
> > I would have assumed setting rate to zero just disabled it.
>
> I think currently it is useless because we have not implemented
> option parsing in remount yet, maybe it's better adding another
> option 'no_fault_injection' to explicitly disable it.
That's like the AC on my car, where I can't turn the fan from one to
zero, I have to press disable. I don't like the explicit disable, I
wish they would just remove that button.
But I also don't think most people will ever use the fault injection
interface so it doesn't really matter. Do whatever seems good to you.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0
2018-09-12 14:25 ` Dan Carpenter
@ 2018-09-12 14:41 ` cgxu519
0 siblings, 0 replies; 23+ messages in thread
From: cgxu519 @ 2018-09-12 14:41 UTC (permalink / raw)
On 09/12/2018 10:25 PM, Dan Carpenter wrote:
> On Wed, Sep 12, 2018@10:05:26PM +0800, cgxu519 wrote:
>> On 09/12/2018 05:16 PM, Dan Carpenter wrote:
>>> On Wed, Sep 12, 2018@01:10:31PM +0800, Chengguang Xu wrote:
>>>> Set fault rate to 0 is useless and confusable, so add check to
>>>> avoid it.
>>>>
>>> I would have assumed setting rate to zero just disabled it.
>> I think currently it is useless because we have not implemented
>> option parsing in remount yet, maybe it's better adding another
>> option 'no_fault_injection' to explicitly disable it.
> That's like the AC on my car, where I can't turn the fan from one to
> zero, I have to press disable. I don't like the explicit disable, I
> wish they would just remove that button.
Interesting, It seems same logic everywhere...
>
> But I also don't think most people will ever use the fault injection
> interface so it doesn't really matter. Do whatever seems good to you.
More background:
The main reason I touch this option is in remount sometimes
we need to restore original option setting when we detecting error
during option parsing, so after this patch we can easily determine
the option is explicitly set or just keep default value.
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
2018-09-12 14:23 ` cgxu519
@ 2018-09-12 14:50 ` Gao Xiang
2018-09-12 15:01 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Gao Xiang @ 2018-09-12 14:50 UTC (permalink / raw)
On 2018/9/12 22:23, cgxu519 wrote:
> On 09/12/2018 07:22 PM, Gao Xiang wrote:
>> Hi Chengguang,
>>
>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
>>> erofs_fault_info, we need this funciton for internal use case.
>>> for example, reset fault_injection option in remount.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>>> ---
>>> ? drivers/staging/erofs/super.c | 26 ++++++++++++++++++--------
>>> ? 1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>> index 14dbb6517b8d..d2dbc1fd3abb 100644
>>> --- a/drivers/staging/erofs/super.c
>>> +++ b/drivers/staging/erofs/super.c
>>> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
>>> ????? [FAULT_KMALLOC]??????? = "kmalloc",
>>> ? };
>>> ? -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> -??????????????????? substring_t *args)
>>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> +??????????????????? unsigned int rate)
>>> ? {
>>> -??? struct erofs_fault_info *ffi = &sbi->fault_info;
>>> -??? int rate = 0;
>>> -
>>> -??? if (args->from && match_int(args, &rate))
>>> -??????? return -EINVAL;
>>> -
>>> ????? if (rate) {
>> I get some compile error of this patch...
>> drivers/staging/erofs/super.c: In function ?__erofs_build_fault_attr?:
>> drivers/staging/erofs/super.c:156:15: error: ?ffi? undeclared (first use in this function)
>> ??? atomic_set(&ffi->inject_ops, 0);
>> ??????????????? ^
>> drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/staging/erofs/super.c: In function ?erofs_build_fault_attr?:
>> drivers/staging/erofs/super.c:169:27: warning: unused variable ?ffi? [-Wunused-variable]
>> ?? struct erofs_fault_info *ffi = &sbi->fault_info;
> Sorry for that, I'll fix it in rebased version.
>
>>
>> p.s. could you please rebase your patch on Thomas's [PATCH v4] staging: erofs: use explicit unsigned int type ?
>> since I'm rebasing the rest PREVIEW patches on this commit now.
>
> I noticed Thomas' patch had already committed to erofs-master branch in Chao's linux git repo, also my
> previous patch but not in erofs-dev branch. So should I rebase on erofs-master?
> Could you give me a little more guide for it?
Hi Chengguang,
Recently many cleanup patches submitted to Greg's staging tree and I need to
rebase the rest erofs preview patches for linux-4.20 on these accepted
cleanup patches.
I think you could make your patch based on Thomas's patch (erofs-master
branch in Chao's linux git repo), you could also tell Chao drop your
previous patch.
Thanks,
Gao Xiang
>>
>> p.p.s. I'd like to get Chao's idea of this fault injection patchset first :)
> No problem, let's wait for a while, then I'll rebase the code according to the comments.
>
>
> Thanks,
> Chengguang
>
> _______________________________________________
> devel mailing list
> devel at linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
2018-09-12 14:50 ` Gao Xiang
@ 2018-09-12 15:01 ` cgxu519
0 siblings, 0 replies; 23+ messages in thread
From: cgxu519 @ 2018-09-12 15:01 UTC (permalink / raw)
On 09/12/2018 10:50 PM, Gao Xiang wrote:
>
> On 2018/9/12 22:23, cgxu519 wrote:
>> On 09/12/2018 07:22 PM, Gao Xiang wrote:
>>> Hi Chengguang,
>>>
>>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>>> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
>>>> erofs_fault_info, we need this funciton for internal use case.
>>>> for example, reset fault_injection option in remount.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>>>> ---
>>>> ? drivers/staging/erofs/super.c | 26 ++++++++++++++++++--------
>>>> ? 1 file changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>>> index 14dbb6517b8d..d2dbc1fd3abb 100644
>>>> --- a/drivers/staging/erofs/super.c
>>>> +++ b/drivers/staging/erofs/super.c
>>>> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
>>>> ????? [FAULT_KMALLOC]??????? = "kmalloc",
>>>> ? };
>>>> ? -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>> -??????????????????? substring_t *args)
>>>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>> +??????????????????? unsigned int rate)
>>>> ? {
>>>> -??? struct erofs_fault_info *ffi = &sbi->fault_info;
>>>> -??? int rate = 0;
>>>> -
>>>> -??? if (args->from && match_int(args, &rate))
>>>> -??????? return -EINVAL;
>>>> -
>>>> ????? if (rate) {
>>> I get some compile error of this patch...
>>> drivers/staging/erofs/super.c: In function ?__erofs_build_fault_attr?:
>>> drivers/staging/erofs/super.c:156:15: error: ?ffi? undeclared (first use in this function)
>>> ??? atomic_set(&ffi->inject_ops, 0);
>>> ??????????????? ^
>>> drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is reported only once for each function it appears in
>>> drivers/staging/erofs/super.c: In function ?erofs_build_fault_attr?:
>>> drivers/staging/erofs/super.c:169:27: warning: unused variable ?ffi? [-Wunused-variable]
>>> ?? struct erofs_fault_info *ffi = &sbi->fault_info;
>> Sorry for that, I'll fix it in rebased version.
>>
>>> p.s. could you please rebase your patch on Thomas's [PATCH v4] staging: erofs: use explicit unsigned int type ?
>>> since I'm rebasing the rest PREVIEW patches on this commit now.
>> I noticed Thomas' patch had already committed to erofs-master branch in Chao's linux git repo, also my
>> previous patch but not in erofs-dev branch. So should I rebase on erofs-master?
>> Could you give me a little more guide for it?
> Hi Chengguang,
>
> Recently many cleanup patches submitted to Greg's staging tree and I need to
> rebase the rest erofs preview patches for linux-4.20 on these accepted
> cleanup patches.
>
> I think you could make your patch based on Thomas's patch (erofs-master
> branch in Chao's linux git repo), you could also tell Chao drop your
> previous patch.
Hi Xiang,
Thanks for explanation, that will be fine.
Hi Chao,
What do you think for these patches? If you think they are worth to do
then please
revert my previous patch and I'll resend rebased version.
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()
2018-09-12 5:10 ` [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc() Chengguang Xu
@ 2018-09-13 2:04 ` Chao Yu
2018-09-13 5:37 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Chao Yu @ 2018-09-13 2:04 UTC (permalink / raw)
On 2018/9/12 13:10, Chengguang Xu wrote:
> Define a dummy function of time_to_inject(), so that we don't
> have to check macro CONFIG_EROFS_FAULT_INJECTION in calling place.
> Base on above adjustment, do proper code cleanup for erofs_kmalloc().
>
> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
> ---
> drivers/staging/erofs/internal.h | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index 367b39fe46e5..1bb2e9e96143 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -42,12 +42,12 @@
> #define DBG_BUGON(...) ((void)0)
> #endif
>
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
> enum {
> FAULT_KMALLOC,
> FAULT_MAX,
> };
>
> +#ifdef CONFIG_EROFS_FAULT_INJECTION
> extern char *erofs_fault_name[FAULT_MAX];
> #define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
>
> @@ -139,21 +139,25 @@ static inline bool time_to_inject(struct erofs_sb_info *sbi, int type)
> atomic_inc(&ffi->inject_ops);
> if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
> atomic_set(&ffi->inject_ops, 0);
> + erofs_show_injection_info(type);
I prefer to show injection info in original place, where we can show real
caller of time_to_inject().
Thanks,
> return true;
> }
> return false;
> }
> +
> +#else
> +static inline bool time_to_inject(struct erofs_sb_info *sbi, int type)
> +{
> + return false;
> +}
> #endif
>
> static inline void *erofs_kmalloc(struct erofs_sb_info *sbi,
> size_t size, gfp_t flags)
> {
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
> - if (time_to_inject(sbi, FAULT_KMALLOC)) {
> - erofs_show_injection_info(FAULT_KMALLOC);
> + if (time_to_inject(sbi, FAULT_KMALLOC))
> return NULL;
> - }
> -#endif
> +
> return kmalloc(size, flags);
> }
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-12 5:10 ` [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection Chengguang Xu
@ 2018-09-13 2:15 ` Chao Yu
2018-09-13 5:46 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Chao Yu @ 2018-09-13 2:15 UTC (permalink / raw)
On 2018/9/12 13:10, Chengguang Xu wrote:
> Define a dummpy function of erofs_build_fault_attr() when macro
> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
> check the macro in calling place. Based on above adjustment,
> do proper code cleanup for option parsing of fault_injection.
>
> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
> ---
> drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
> 1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 1aec509c805f..14dbb6517b8d 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - unsigned int rate)
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
> {
> struct erofs_fault_info *ffi = &sbi->fault_info;
> + int rate = 0;
> +
> + if (args->from && match_int(args, &rate))
> + return -EINVAL;
>
> if (rate) {
> atomic_set(&ffi->inject_ops, 0);
> ffi->inject_rate = rate;
> ffi->inject_type = (1 << FAULT_MAX) - 1;
> + set_opt(sbi, FAULT_INJECTION);
> } else {
> memset(ffi, 0, sizeof(struct erofs_fault_info));
> + clear_opt(sbi, FAULT_INJECTION);
Hmmm, if user mounts/remounts image with -o fault_injection=0, user can not
check such info in anywhere, as we skip showing this option due to lack of
EROFS_MOUNT_FAULT_INJECTION bit. How about keeping this bit?
Thanks,
> }
> +
> + return 0;
> +}
> +#else
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
> +{
> + infoln("fault_injection options not supported");
> + return 0;
> }
> #endif
>
> @@ -192,7 +207,7 @@ static int parse_options(struct super_block *sb, char *options)
> {
> substring_t args[MAX_OPT_ARGS];
> char *p;
> - int arg = 0;
> + int err;
>
> if (!options)
> return 0;
> @@ -238,15 +253,11 @@ static int parse_options(struct super_block *sb, char *options)
> break;
> #endif
> case Opt_fault_injection:
> - if (args->from && match_int(args, &arg))
> - return -EINVAL;
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
> - erofs_build_fault_attr(EROFS_SB(sb), arg);
> - set_opt(EROFS_SB(sb), FAULT_INJECTION);
> -#else
> - infoln("FAULT_INJECTION was not selected");
> -#endif
> + err = erofs_build_fault_attr(EROFS_SB(sb), args);
> + if (err)
> + return err;
> break;
> +
> default:
> errln("Unrecognized mount option \"%s\" "
> "or missing value", p);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()
2018-09-13 2:04 ` Chao Yu
@ 2018-09-13 5:37 ` cgxu519
0 siblings, 0 replies; 23+ messages in thread
From: cgxu519 @ 2018-09-13 5:37 UTC (permalink / raw)
On 09/13/2018 10:04 AM, Chao Yu wrote:
> On 2018/9/12 13:10, Chengguang Xu wrote:
>> Define a dummy function of time_to_inject(), so that we don't
>> have to check macro CONFIG_EROFS_FAULT_INJECTION in calling place.
>> Base on above adjustment, do proper code cleanup for erofs_kmalloc().
>>
>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>> ---
>> drivers/staging/erofs/internal.h | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>> index 367b39fe46e5..1bb2e9e96143 100644
>> --- a/drivers/staging/erofs/internal.h
>> +++ b/drivers/staging/erofs/internal.h
>> @@ -42,12 +42,12 @@
>> #define DBG_BUGON(...) ((void)0)
>> #endif
>>
>> -#ifdef CONFIG_EROFS_FAULT_INJECTION
>> enum {
>> FAULT_KMALLOC,
>> FAULT_MAX,
>> };
>>
>> +#ifdef CONFIG_EROFS_FAULT_INJECTION
>> extern char *erofs_fault_name[FAULT_MAX];
>> #define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
>>
>> @@ -139,21 +139,25 @@ static inline bool time_to_inject(struct erofs_sb_info *sbi, int type)
>> atomic_inc(&ffi->inject_ops);
>> if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
>> atomic_set(&ffi->inject_ops, 0);
>> + erofs_show_injection_info(type);
> I prefer to show injection info in original place, where we can show real
> caller of time_to_inject().
>
OK, I agree with your suggestion.
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-13 2:15 ` Chao Yu
@ 2018-09-13 5:46 ` cgxu519
2018-09-14 15:22 ` Chao Yu
0 siblings, 1 reply; 23+ messages in thread
From: cgxu519 @ 2018-09-13 5:46 UTC (permalink / raw)
On 09/13/2018 10:15 AM, Chao Yu wrote:
> On 2018/9/12 13:10, Chengguang Xu wrote:
>> Define a dummpy function of erofs_build_fault_attr() when macro
>> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
>> check the macro in calling place. Based on above adjustment,
>> do proper code cleanup for option parsing of fault_injection.
>>
>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>> ---
>> drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
>> 1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index 1aec509c805f..14dbb6517b8d 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
>> [FAULT_KMALLOC] = "kmalloc",
>> };
>>
>> -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> - unsigned int rate)
>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> + substring_t *args)
>> {
>> struct erofs_fault_info *ffi = &sbi->fault_info;
>> + int rate = 0;
>> +
>> + if (args->from && match_int(args, &rate))
>> + return -EINVAL;
>>
>> if (rate) {
>> atomic_set(&ffi->inject_ops, 0);
>> ffi->inject_rate = rate;
>> ffi->inject_type = (1 << FAULT_MAX) - 1;
>> + set_opt(sbi, FAULT_INJECTION);
>> } else {
>> memset(ffi, 0, sizeof(struct erofs_fault_info));
>> + clear_opt(sbi, FAULT_INJECTION);
> Hmmm, if user mounts/remounts image with -o fault_injection=0, user can not
> check such info in anywhere, as we skip showing this option due to lack of
> EROFS_MOUNT_FAULT_INJECTION bit. How about keeping this bit?
>
IIUC, the purpose of fault_injection=0 is for disabling fault injection
function,
so isn't it the same as default? Should we distinguish explicit setting
value 0 and default value 0?
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-13 5:46 ` cgxu519
@ 2018-09-14 15:22 ` Chao Yu
2018-09-17 13:52 ` cgxu519
0 siblings, 1 reply; 23+ messages in thread
From: Chao Yu @ 2018-09-14 15:22 UTC (permalink / raw)
On 2018/9/13 13:46, cgxu519 wrote:
> On 09/13/2018 10:15 AM, Chao Yu wrote:
>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>> Define a dummpy function of erofs_build_fault_attr() when macro
>>> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
>>> check the macro in calling place. Based on above adjustment,
>>> do proper code cleanup for option parsing of fault_injection.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>>> ---
>>> ? drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
>>> ? 1 file changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>> index 1aec509c805f..14dbb6517b8d 100644
>>> --- a/drivers/staging/erofs/super.c
>>> +++ b/drivers/staging/erofs/super.c
>>> @@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
>>> ????? [FAULT_KMALLOC]??????? = "kmalloc",
>>> ? };
>>> ? -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> -??????????????????????? unsigned int rate)
>>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> +??????????????????? substring_t *args)
>>> ? {
>>> ????? struct erofs_fault_info *ffi = &sbi->fault_info;
>>> +??? int rate = 0;
>>> +
>>> +??? if (args->from && match_int(args, &rate))
>>> +??????? return -EINVAL;
>>> ? ????? if (rate) {
>>> ????????? atomic_set(&ffi->inject_ops, 0);
>>> ????????? ffi->inject_rate = rate;
>>> ????????? ffi->inject_type = (1 << FAULT_MAX) - 1;
>>> +??????? set_opt(sbi, FAULT_INJECTION);
>>> ????? } else {
>>> ????????? memset(ffi, 0, sizeof(struct erofs_fault_info));
>>> +??????? clear_opt(sbi, FAULT_INJECTION);
>> Hmmm, if user mounts/remounts image with -o fault_injection=0, user can not
>> check such info in anywhere, as we skip showing this option due to lack of
>> EROFS_MOUNT_FAULT_INJECTION bit. How about keeping this bit?
>>
>
> IIUC, the purpose of fault_injection=0 is for disabling fault injection function,
> so isn't it the same as default? Should we distinguish explicit setting value 0
IMO, if user set fault_injection=0 during mount, it means user want to enable
this feature but currently user want to set the rate to zero, later user may
change it to non-zero. And with EROFS_MOUNT_FAULT_INJECTION being set, user can
check current rate value via ->show_options.
Thanks,
> and default value 0?
>
> Thanks,
> Chengguang
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-14 15:22 ` Chao Yu
@ 2018-09-17 13:52 ` cgxu519
2018-09-17 14:28 ` Chao Yu
0 siblings, 1 reply; 23+ messages in thread
From: cgxu519 @ 2018-09-17 13:52 UTC (permalink / raw)
On 09/14/2018 11:22 PM, Chao Yu wrote:
> On 2018/9/13 13:46, cgxu519 wrote:
>> On 09/13/2018 10:15 AM, Chao Yu wrote:
>>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>>> Define a dummpy function of erofs_build_fault_attr() when macro
>>>> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
>>>> check the macro in calling place. Based on above adjustment,
>>>> do proper code cleanup for option parsing of fault_injection.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>>>> ---
>>>> ? drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
>>>> ? 1 file changed, 22 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>>> index 1aec509c805f..14dbb6517b8d 100644
>>>> --- a/drivers/staging/erofs/super.c
>>>> +++ b/drivers/staging/erofs/super.c
>>>> @@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
>>>> ????? [FAULT_KMALLOC]??????? = "kmalloc",
>>>> ? };
>>>> ? -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>> -??????????????????????? unsigned int rate)
>>>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>> +??????????????????? substring_t *args)
>>>> ? {
>>>> ????? struct erofs_fault_info *ffi = &sbi->fault_info;
>>>> +??? int rate = 0;
>>>> +
>>>> +??? if (args->from && match_int(args, &rate))
>>>> +??????? return -EINVAL;
>>>> ? ????? if (rate) {
>>>> ????????? atomic_set(&ffi->inject_ops, 0);
>>>> ????????? ffi->inject_rate = rate;
>>>> ????????? ffi->inject_type = (1 << FAULT_MAX) - 1;
>>>> +??????? set_opt(sbi, FAULT_INJECTION);
>>>> ????? } else {
>>>> ????????? memset(ffi, 0, sizeof(struct erofs_fault_info));
>>>> +??????? clear_opt(sbi, FAULT_INJECTION);
>>> Hmmm, if user mounts/remounts image with -o fault_injection=0, user can not
>>> check such info in anywhere, as we skip showing this option due to lack of
>>> EROFS_MOUNT_FAULT_INJECTION bit. How about keeping this bit?
>>>
>> IIUC, the purpose of fault_injection=0 is for disabling fault injection function,
>> so isn't it the same as default? Should we distinguish explicit setting value 0
> IMO, if user set fault_injection=0 during mount, it means user want to enable
> this feature but currently user want to set the rate to zero, later user may
> change it to non-zero. And with EROFS_MOUNT_FAULT_INJECTION being set, user can
> check current rate value via ->show_options.
Hi Chao,
Do you mean user can modify the value without remount? or maybe in the
future?
Thanks,
Chengguang
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection
2018-09-17 13:52 ` cgxu519
@ 2018-09-17 14:28 ` Chao Yu
0 siblings, 0 replies; 23+ messages in thread
From: Chao Yu @ 2018-09-17 14:28 UTC (permalink / raw)
On 2018/9/17 21:52, cgxu519 wrote:
> On 09/14/2018 11:22 PM, Chao Yu wrote:
>> On 2018/9/13 13:46, cgxu519 wrote:
>>> On 09/13/2018 10:15 AM, Chao Yu wrote:
>>>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>>>> Define a dummpy function of erofs_build_fault_attr() when macro
>>>>> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
>>>>> check the macro in calling place. Based on above adjustment,
>>>>> do proper code cleanup for option parsing of fault_injection.
>>>>>
>>>>> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
>>>>> ---
>>>>> ?? drivers/staging/erofs/super.c | 33 ++++++++++++++++++++++-----------
>>>>> ?? 1 file changed, 22 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>>>> index 1aec509c805f..14dbb6517b8d 100644
>>>>> --- a/drivers/staging/erofs/super.c
>>>>> +++ b/drivers/staging/erofs/super.c
>>>>> @@ -144,18 +144,33 @@ char *erofs_fault_name[FAULT_MAX] = {
>>>>> ?????? [FAULT_KMALLOC]??????? = "kmalloc",
>>>>> ?? };
>>>>> ?? -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>>> -??????????????????????? unsigned int rate)
>>>>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>>>> +??????????????????? substring_t *args)
>>>>> ?? {
>>>>> ?????? struct erofs_fault_info *ffi = &sbi->fault_info;
>>>>> +??? int rate = 0;
>>>>> +
>>>>> +??? if (args->from && match_int(args, &rate))
>>>>> +??????? return -EINVAL;
>>>>> ?? ????? if (rate) {
>>>>> ?????????? atomic_set(&ffi->inject_ops, 0);
>>>>> ?????????? ffi->inject_rate = rate;
>>>>> ?????????? ffi->inject_type = (1 << FAULT_MAX) - 1;
>>>>> +??????? set_opt(sbi, FAULT_INJECTION);
>>>>> ?????? } else {
>>>>> ?????????? memset(ffi, 0, sizeof(struct erofs_fault_info));
>>>>> +??????? clear_opt(sbi, FAULT_INJECTION);
>>>> Hmmm, if user mounts/remounts image with -o fault_injection=0, user can not
>>>> check such info in anywhere, as we skip showing this option due to lack of
>>>> EROFS_MOUNT_FAULT_INJECTION bit. How about keeping this bit?
>>>>
>>> IIUC, the purpose of fault_injection=0 is for disabling fault injection
>>> function,
>>> so isn't it the same as default? Should we distinguish explicit setting value 0
>> IMO, if user set fault_injection=0 during mount, it means user want to enable
>> this feature but currently user want to set the rate to zero, later user may
>> change it to non-zero. And with EROFS_MOUNT_FAULT_INJECTION being set, user can
>> check current rate value via ->show_options.
>
> Hi Chao,
>
> Do you mean user can modify the value without remount? or maybe in the future?
Yeah, like we did in f2fs, we can export sysfs node for configuring fault
injection rate/type.
Thanks,
>
> Thanks,
> Chengguang
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-09-17 14:28 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-12 5:10 [PATCH 0/7] staging: erofs: option validation for remount and some code cleanups Chengguang Xu
2018-09-12 5:10 ` [PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc() Chengguang Xu
2018-09-13 2:04 ` Chao Yu
2018-09-13 5:37 ` cgxu519
2018-09-12 5:10 ` [PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection Chengguang Xu
2018-09-13 2:15 ` Chao Yu
2018-09-13 5:46 ` cgxu519
2018-09-14 15:22 ` Chao Yu
2018-09-17 13:52 ` cgxu519
2018-09-17 14:28 ` Chao Yu
2018-09-12 5:10 ` [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr() Chengguang Xu
2018-09-12 11:22 ` Gao Xiang
2018-09-12 14:23 ` cgxu519
2018-09-12 14:50 ` Gao Xiang
2018-09-12 15:01 ` cgxu519
2018-09-12 5:10 ` [PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0 Chengguang Xu
2018-09-12 9:16 ` Dan Carpenter
2018-09-12 14:05 ` cgxu519
2018-09-12 14:25 ` Dan Carpenter
2018-09-12 14:41 ` cgxu519
2018-09-12 5:10 ` [PATCH 5/7] staging: erofs: introduce a new helper erofs_get_fault_rate() Chengguang Xu
2018-09-12 5:10 ` [PATCH 6/7] staging: erofs: code cleanup for erofs_show_options() Chengguang Xu
2018-09-12 5:10 ` [PATCH 7/7] staging: erofs: option validation in remount Chengguang Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.