[PATCH 0/7] staging: erofs: option validation for remount and some code cleanups

[PATCH 0/7] staging: erofs: option validation for remount and some code cleanups

[Chengguang Xu]

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

[PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()

[Chengguang Xu]

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

[PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()

[Chao Yu]

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);
>  }
>  
> 

[PATCH 1/7] staging: erofs: code cleanup for erofs_kmalloc()

[cgxu519]

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

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[Chengguang Xu]

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

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[Chao Yu]

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);
> 

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[cgxu519]

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

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[Chao Yu]

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
> 
> 
> 
> 
> 
> 

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[cgxu519]

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

[PATCH 2/7] staging: erofs: code cleanup for option parsing of fault_injection

[Chao Yu]

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
> 
> 
> 
> 
> 
> 

[PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()

[Chengguang Xu]

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

[PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()

[Gao Xiang]

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

[PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()

[cgxu519]

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

[PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()

[Gao Xiang]

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

[PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()

[cgxu519]

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

[PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0

[Chengguang Xu]

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

[PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0

[Dan Carpenter]

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

[PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0

[cgxu519]

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

[PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0

[Dan Carpenter]

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

[PATCH 4/7] staging: erofs: return -EINVAL when specifying fault rate to 0

[cgxu519]

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

[PATCH 5/7] staging: erofs: introduce a new helper erofs_get_fault_rate()

[Chengguang Xu]

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

[PATCH 6/7] staging: erofs: code cleanup for erofs_show_options()

[Chengguang Xu]

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

[PATCH 7/7] staging: erofs: option validation in remount

[Chengguang Xu]

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