Re: [PATCH 2/2] staging: erofs: remove redundant unlikely annotation in unzip_vle.c

2019-02-13 Thread cgxu519 



On 2/13/19 2:36 PM, Chao Yu wrote:

On 2019/2/12 11:24, Chengguang Xu wrote:

unlikely has already included in IS_ERR(),
so just remove it.

Signed-off-by: Chengguang Xu 

It looks like we don't need to send two patch to fix two similar
issues, if you can merge them, it will be better.
I agree with you but I noticed the patches have already merged to Greg's 
staging tree,

so I'm not sure if it cause inconvenience to his management.

Thanks,
Chengguang


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: How can I get user space tools of erofs?

2018-11-11 Thread cgxu519 




Hi Chengguang,

Good question! It's in the final stage of preparation to open source erofs-mkfs 
(actually they are now struggle at how to properly spilt
into reasonable patches this week), hopefully the implementation could be 
released at the next week. (sorry I didn't mean to delay,
I have to put it in the first place --- successfully launch our linux-erofs 
products to the market)

@Guifu Li  is the original author of erofs-mkfs, he 
will post the original mkfs source code to linux-erofs mailing list
and he will maintain erofs-mkfs together with @Wei Fang  
later. You can contact them for further informations.
---  these piece of code is actually not clean enough (a lot hacked/dirty code 
compared to the kernel code) so a lot of cleanup will be done then.

currently, you can get erofs-mkfs binary from (still sorry to say that...):
https://github.com/hsiangkao/erofs_mkfs_binary

erofs is now in productization for these months, if all things go well, you'll 
see that HUAWEI mobile phones on the market run in erofs in few months. :)

These months I'm busy in solving bugs found by internal beta users and tuning 
memory policy in heavy memory workload for the best performance compared to ext4
(we have native in-place decompression compared with squashfs/btrfs, thus less 
extra memory allocation results in lower memory memory reclaim / page-writeback
for devices with limited memory, see: 
https://lists.ozlabs.org/pipermail/linux-erofs/2018-August/000494.html) in 
order to gain the competitive user experience
comparing to uncompressed filesystem solutions. I will update a document to 
describe our core design and linux-erofs future roadmap in this linux-4.21 
round.


Hi Xiang,

Thanks for your detail explanations!

Chengguang



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-19 Thread cgxu519 

On 9/19/18 11:22 PM, Gao Xiang wrote:

Hi Chengguang,

On 2018/9/19 22:53, Chengguang Xu wrote:

Hi Greg, Xiang

I rebased code on latest erofs-master branch and that branch
has already merged the first patch in my previous patchset,
so this time I only post rest 3 patches.

Great, at the most time Chao's erofs-master is the same as Greg's
staging tree (currently it is), but I personally think all patches
could be better directly based on the final staging tree if these
patches has no real/effective dependency with some submitted patches
but haven't been applied by Greg (p.s. it should be avoided as
much as possible because community guys could find something
important like the yesterday patches).

Once again, the detail information of branches is described in
   https://lists.ozlabs.org/listinfo/linux-erofs/

I have nothing more to say, good luck :)


Thanks for the guide!

Chengguang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/6] staging: erofs: code cleanup for option parsing of fault_injection

2018-09-18 Thread cgxu519 

On 09/18/2018 03:07 PM, Gao Xiang wrote:

Hi Chengguang,

On 2018/9/17 23:34, 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 
---
  drivers/staging/erofs/super.c | 33 -
  1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 9e421536cbdf..7ce2fd3d49f3 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -145,10 +145,14 @@ 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);
@@ -157,6 +161,15 @@ static void erofs_build_fault_attr(struct erofs_sb_info 
*sbi,
} else {
memset(ffi, 0, sizeof(struct erofs_fault_info));
}
+
+   set_opt(sbi, FAILt_INJECTION);

drivers/staging/erofs/super.c: In function ‘__erofs_build_fault_attr’:
drivers/staging/erofs/internal.h:176:51: error: ‘EROFS_MOUNT_FAILt_INJECTION’ 
undeclared (first use in this function)
  #define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
^
drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
   set_opt(sbi, FAILt_INJECTION);
   ^
drivers/staging/erofs/internal.h:176:51: note: each undeclared identifier is 
reported only once for each function it appears in
  #define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
^
drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
   set_opt(sbi, FAILt_INJECTION);
   ^


Hi Xiang,

I'm really sorry for that, I'm curious how it passed my building test.
I deleted all existing config and  binary files and tested with/without 
INJECTION config this time.



Thanks,
Chengguang



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-17 Thread 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 
---
   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






___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread 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 
---
  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






___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread 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 
---
  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


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread 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 
---
   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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread cgxu519 

On 09/12/2018 10:25 PM, Dan Carpenter wrote:

On Wed, Sep 12, 2018 at 10:05:26PM +0800, cgxu519 wrote:

On 09/12/2018 05:16 PM, Dan Carpenter wrote:

On Wed, Sep 12, 2018 at 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread 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 
---
  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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2018-09-12 Thread cgxu519 

On 09/12/2018 05:16 PM, Dan Carpenter wrote:

On Wed, Sep 12, 2018 at 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: option validation in remount

2018-09-11 Thread cgxu519 

On 09/11/2018 07:08 PM, Gao Xiang wrote:

Hi Chengguang,

Thanks for your patch.

The patch title should be "staging: erofs: " since erofs is still in staging.

Hi Xiang,

Thanks for your review. I'll add the tag from next version.



The same as your previous patch "erofs: surround fault_injection ralted option
parsing using CONFIG_EROFS_FAULT_INJECTION".

On 2018/9/11 18:51, Chengguang Xu wrote:

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 
---
  drivers/staging/erofs/super.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 1aec509c805f..8bab077381ad 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -625,10 +625,27 @@ 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);
+   struct erofs_fault_info *ffi = &sbi->fault_info;
+   unsigned int orig_mount_opt = sbi->mount_opt;
+   unsigned int orig_inject_rate = ffi->inject_rate;
+   int err;
+
BUG_ON(!sb_rdonly(sb));
  
+	err = parse_options(sb, data);

+   if (err)
+   goto out;
+
*flags |= MS_RDONLY;
return 0;
+
+out:
+   if (ffi->inject_rate != orig_inject_rate)
+   erofs_build_fault_attr(sbi, orig_inject_rate);

Currently should be with "#ifdef CONFIG_EROFS_FAULT_INJECTION"?
and have you tried to compile without EROFS_FAULT_INJECTION?

Ah, It must be enabled in my environment.
However, adding the macro in every calling place seems not convenient,
I'll try to do some code cleanups, so please hold on this and previous 
patch for

a moment, I'll resend those in a patch set.

Thanks,
Chengguang

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel