[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

`z_erofs_vle_workgroup' is heavily generated in the decompression,
for example, it resets 32 bytes redundantly for 64-bit platforms
even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
default 4, pages are stored in `z_erofs_vle_workgroup'.

As an another example, `struct mutex' takes 72 bytes for our kirin
64-bit platforms, it's unnecessary to be reseted at first and
be initialized each time.

Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
since most fields are reinitialized to meaningful values later,
and pagevec is no need to initialized at all.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 79d3ba62b298..7aa26818054a 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
 	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
 }
 
+static void init_once(void *ptr)
+{
+	struct z_erofs_vle_workgroup *grp = ptr;
+	struct z_erofs_vle_work *const work =
+		z_erofs_vle_grab_primary_work(grp);
+	unsigned int i;
+
+	mutex_init(&work->lock);
+	work->nr_pages = 0;
+	work->vcnt = 0;
+	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
+		grp->compressed_pages[i] = NULL;
+}
+
+static void init_always(struct z_erofs_vle_workgroup *grp)
+{
+	struct z_erofs_vle_work *const work =
+		z_erofs_vle_grab_primary_work(grp);
+
+	atomic_set(&grp->obj.refcount, 1);
+	grp->flags = 0;
+
+	DBG_BUGON(work->nr_pages);
+	DBG_BUGON(work->vcnt);
+}
+
 int __init z_erofs_init_zip_subsystem(void)
 {
 	z_erofs_workgroup_cachep =
 		kmem_cache_create("erofs_compress",
 		Z_EROFS_WORKGROUP_SIZE, 0,
-		SLAB_RECLAIM_ACCOUNT, NULL);
+		SLAB_RECLAIM_ACCOUNT, init_once);
 
 	if (z_erofs_workgroup_cachep != NULL) {
 		if (!init_unzip_workqueue())
@@ -369,10 +395,11 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
 	BUG_ON(grp != NULL);
 
 	/* no available workgroup, let's allocate one */
-	grp = kmem_cache_zalloc(z_erofs_workgroup_cachep, GFP_NOFS);
+	grp = kmem_cache_alloc(z_erofs_workgroup_cachep, GFP_NOFS);
 	if (unlikely(grp == NULL))
 		return ERR_PTR(-ENOMEM);
 
+	init_always(grp);
 	grp->obj.index = f->idx;
 	grp->llen = map->m_llen;
 
@@ -380,7 +407,6 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
 		(map->m_flags & EROFS_MAP_ZIPPED) ?
 			Z_EROFS_VLE_WORKGRP_FMT_LZ4 :
 			Z_EROFS_VLE_WORKGRP_FMT_PLAIN);
-	atomic_set(&grp->obj.refcount, 1);
 
 	/* new workgrps have been claimed as type 1 */
 	WRITE_ONCE(grp->next, *f->owned_head);
@@ -393,8 +419,6 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
 	work = z_erofs_vle_grab_primary_work(grp);
 	work->pageofs = f->pageofs;
 
-	mutex_init(&work->lock);
-
 	if (gnew) {
 		int err = erofs_register_workgroup(f->sb, &grp->obj, 0);
 
-- 
2.14.4

[PATCH v2 chao/erofs-dev 2/2] staging: erofs: locked before registering for all new workgroups

[Gao Xiang]

Let's make sure that the one registering a workgroup will also
take the primary work lock at first for two reasons:
  1) There's no need to introduce such a race window (and consequently
     overhead) between registering and locking, other tasks could break
     in by chance, and the race seems unnecessary (no benefit at all);

  2) It's better to take the primary work when a workgroup
     is registered to apply the cache managed policy, for example,
     if some other tasks break in, it could turn into the in-place
     decompression rather than use as the cached decompression.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 7aa26818054a..91ce07f05bd2 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -419,18 +419,21 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
 	work = z_erofs_vle_grab_primary_work(grp);
 	work->pageofs = f->pageofs;
 
+	/* lock all primary followed works before visible to others */
+	if (unlikely(!mutex_trylock(&work->lock)))
+		BUG();
+
 	if (gnew) {
 		int err = erofs_register_workgroup(f->sb, &grp->obj, 0);
 
 		if (err) {
+			mutex_unlock(&work->lock);
 			kmem_cache_free(z_erofs_workgroup_cachep, grp);
 			return ERR_PTR(-EAGAIN);
 		}
 	}
 
 	*f->owned_head = *f->grp_ret = grp;
-
-	mutex_lock(&work->lock);
 	return work;
 }
 
-- 
2.14.4

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

On 2018/10/19 14:41, Gao Xiang wrote:
> `z_erofs_vle_workgroup' is heavily generated in the decompression,
> for example, it resets 32 bytes redundantly for 64-bit platforms
> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
> default 4, pages are stored in `z_erofs_vle_workgroup'.
> 
> As an another example, `struct mutex' takes 72 bytes for our kirin
> 64-bit platforms, it's unnecessary to be reseted at first and
> be initialized each time.
> 
> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
> since most fields are reinitialized to meaningful values later,
> and pagevec is no need to initialized at all.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 79d3ba62b298..7aa26818054a 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>  }
>  
> +static void init_once(void *ptr)

How about rename it to init_vle_workgroup() for better readability?

> +{
> +	struct z_erofs_vle_workgroup *grp = ptr;
> +	struct z_erofs_vle_work *const work =
> +		z_erofs_vle_grab_primary_work(grp);
> +	unsigned int i;
> +
> +	mutex_init(&work->lock);
> +	work->nr_pages = 0;
> +	work->vcnt = 0;
> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
> +		grp->compressed_pages[i] = NULL;
> +}
> +
> +static void init_always(struct z_erofs_vle_workgroup *grp)

Ditto, maybe reset_vle_workgroup().

Otherwise, it looks good to me.

Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks,

> +{
> +	struct z_erofs_vle_work *const work =
> +		z_erofs_vle_grab_primary_work(grp);
> +
> +	atomic_set(&grp->obj.refcount, 1);
> +	grp->flags = 0;
> +
> +	DBG_BUGON(work->nr_pages);
> +	DBG_BUGON(work->vcnt);
> +}
> +
>  int __init z_erofs_init_zip_subsystem(void)
>  {
>  	z_erofs_workgroup_cachep =
>  		kmem_cache_create("erofs_compress",
>  		Z_EROFS_WORKGROUP_SIZE, 0,
> -		SLAB_RECLAIM_ACCOUNT, NULL);
> +		SLAB_RECLAIM_ACCOUNT, init_once);
>  
>  	if (z_erofs_workgroup_cachep != NULL) {
>  		if (!init_unzip_workqueue())
> @@ -369,10 +395,11 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
>  	BUG_ON(grp != NULL);
>  
>  	/* no available workgroup, let's allocate one */
> -	grp = kmem_cache_zalloc(z_erofs_workgroup_cachep, GFP_NOFS);
> +	grp = kmem_cache_alloc(z_erofs_workgroup_cachep, GFP_NOFS);
>  	if (unlikely(grp == NULL))
>  		return ERR_PTR(-ENOMEM);
>  
> +	init_always(grp);
>  	grp->obj.index = f->idx;
>  	grp->llen = map->m_llen;
>  
> @@ -380,7 +407,6 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
>  		(map->m_flags & EROFS_MAP_ZIPPED) ?
>  			Z_EROFS_VLE_WORKGRP_FMT_LZ4 :
>  			Z_EROFS_VLE_WORKGRP_FMT_PLAIN);
> -	atomic_set(&grp->obj.refcount, 1);
>  
>  	/* new workgrps have been claimed as type 1 */
>  	WRITE_ONCE(grp->next, *f->owned_head);
> @@ -393,8 +419,6 @@ z_erofs_vle_work_register(const struct z_erofs_vle_work_finder *f,
>  	work = z_erofs_vle_grab_primary_work(grp);
>  	work->pageofs = f->pageofs;
>  
> -	mutex_init(&work->lock);
> -
>  	if (gnew) {
>  		int err = erofs_register_workgroup(f->sb, &grp->obj, 0);
>  
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/10/31 10:04, Chao Yu wrote:
> On 2018/10/19 14:41, Gao Xiang wrote:
>> `z_erofs_vle_workgroup' is heavily generated in the decompression,
>> for example, it resets 32 bytes redundantly for 64-bit platforms
>> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
>> default 4, pages are stored in `z_erofs_vle_workgroup'.
>>
>> As an another example, `struct mutex' takes 72 bytes for our kirin
>> 64-bit platforms, it's unnecessary to be reseted at first and
>> be initialized each time.
>>
>> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
>> since most fields are reinitialized to meaningful values later,
>> and pagevec is no need to initialized at all.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>> ---
>>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>> index 79d3ba62b298..7aa26818054a 100644
>> --- a/drivers/staging/erofs/unzip_vle.c
>> +++ b/drivers/staging/erofs/unzip_vle.c
>> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>  }
>>  
>> +static void init_once(void *ptr)
> How about rename it to init_vle_workgroup() for better readability?
The name convension is borrowed from inode_init_always/inode_init_once of fs/inode.c...
since it is pure static, I don't add the prefix(such as z_erofs_vle_workgroup or etc.. too long...).

p.s. These days I think "vle_" is redundant... It could be renamed at a proper time...


> 
>> +{
>> +	struct z_erofs_vle_workgroup *grp = ptr;
>> +	struct z_erofs_vle_work *const work =
>> +		z_erofs_vle_grab_primary_work(grp);
>> +	unsigned int i;
>> +
>> +	mutex_init(&work->lock);
>> +	work->nr_pages = 0;
>> +	work->vcnt = 0;
>> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
>> +		grp->compressed_pages[i] = NULL;
>> +}
>> +
>> +static void init_always(struct z_erofs_vle_workgroup *grp)
> Ditto, maybe reset_vle_workgroup().
> 
> Otherwise, it looks good to me.
> 
> Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks for the review :)

Thanks,
Gao Xiang


> 
> Thanks,
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?

=====patchset======
staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
[v2] staging: erofs: fix race when the managed cache is enabled
staging: erofs: fix refcount assertion in erofs_register_workgroup

staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
staging: erofs: add a full barrier in erofs_workgroup_unfreeze

staging: erofs: separate into init_once / always
staging: erofs: locked before registering for all new workgroups

[v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
=====end patchset======


and

"staging: erofs: managed pages could be locked at the time of decompression"
could be dropped now since I will refactor the z_erofs_vle_unzip
(currently, A serious bug in it, I have fixed it internally).

I am working on the reset stability patches reported by the internal beta test,
expected to finish this week. :)

Thanks,
Gao Xiang

On 2018/10/31 10:11, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/10/31 10:04, Chao Yu wrote:
>> On 2018/10/19 14:41, Gao Xiang wrote:
>>> `z_erofs_vle_workgroup' is heavily generated in the decompression,
>>> for example, it resets 32 bytes redundantly for 64-bit platforms
>>> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
>>> default 4, pages are stored in `z_erofs_vle_workgroup'.
>>>
>>> As an another example, `struct mutex' takes 72 bytes for our kirin
>>> 64-bit platforms, it's unnecessary to be reseted at first and
>>> be initialized each time.
>>>
>>> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
>>> since most fields are reinitialized to meaningful values later,
>>> and pagevec is no need to initialized at all.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>>> index 79d3ba62b298..7aa26818054a 100644
>>> --- a/drivers/staging/erofs/unzip_vle.c
>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>>>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>>  }
>>>  
>>> +static void init_once(void *ptr)
>> How about rename it to init_vle_workgroup() for better readability?
> The name convension is borrowed from inode_init_always/inode_init_once of fs/inode.c...
> since it is pure static, I don't add the prefix(such as z_erofs_vle_workgroup or etc.. too long...).
> 
> p.s. These days I think "vle_" is redundant... It could be renamed at a proper time...
> 
> 
>>
>>> +{
>>> +	struct z_erofs_vle_workgroup *grp = ptr;
>>> +	struct z_erofs_vle_work *const work =
>>> +		z_erofs_vle_grab_primary_work(grp);
>>> +	unsigned int i;
>>> +
>>> +	mutex_init(&work->lock);
>>> +	work->nr_pages = 0;
>>> +	work->vcnt = 0;
>>> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
>>> +		grp->compressed_pages[i] = NULL;
>>> +}
>>> +
>>> +static void init_always(struct z_erofs_vle_workgroup *grp)
>> Ditto, maybe reset_vle_workgroup().
>>
>> Otherwise, it looks good to me.
>>
>> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
> 
> Thanks for the review :)
> 
> Thanks,
> Gao Xiang
> 
> 
>>
>> Thanks,
>>

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018/10/31 10:11, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/10/31 10:04, Chao Yu wrote:
>> On 2018/10/19 14:41, Gao Xiang wrote:
>>> `z_erofs_vle_workgroup' is heavily generated in the decompression,
>>> for example, it resets 32 bytes redundantly for 64-bit platforms
>>> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
>>> default 4, pages are stored in `z_erofs_vle_workgroup'.
>>>
>>> As an another example, `struct mutex' takes 72 bytes for our kirin
>>> 64-bit platforms, it's unnecessary to be reseted at first and
>>> be initialized each time.
>>>
>>> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
>>> since most fields are reinitialized to meaningful values later,
>>> and pagevec is no need to initialized at all.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>>> index 79d3ba62b298..7aa26818054a 100644
>>> --- a/drivers/staging/erofs/unzip_vle.c
>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>>>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>>  }
>>>  
>>> +static void init_once(void *ptr)
>> How about rename it to init_vle_workgroup() for better readability?
> The name convension is borrowed from inode_init_always/inode_init_once of fs/inode.c...

Yes, I guess it is. :)

> since it is pure static, I don't add the prefix(such as z_erofs_vle_workgroup or etc.. too long...).
> 
> p.s. These days I think "vle_" is redundant... It could be renamed at a proper time...

Okay, naming is not a big deal, please feal free to do that.

Thanks,

> 
> 
>>
>>> +{
>>> +	struct z_erofs_vle_workgroup *grp = ptr;
>>> +	struct z_erofs_vle_work *const work =
>>> +		z_erofs_vle_grab_primary_work(grp);
>>> +	unsigned int i;
>>> +
>>> +	mutex_init(&work->lock);
>>> +	work->nr_pages = 0;
>>> +	work->vcnt = 0;
>>> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
>>> +		grp->compressed_pages[i] = NULL;
>>> +}
>>> +
>>> +static void init_always(struct z_erofs_vle_workgroup *grp)
>> Ditto, maybe reset_vle_workgroup().
>>
>> Otherwise, it looks good to me.
>>
>> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
> 
> Thanks for the review :)
> 
> Thanks,
> Gao Xiang
> 
> 
>>
>> Thanks,
>>
> 
> .
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018/10/31 10:39, Gao Xiang wrote:
> Hi Chao,
> 
> Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?
> 
> =====patchset======
> staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
> [v2] staging: erofs: fix race when the managed cache is enabled
> staging: erofs: fix refcount assertion in erofs_register_workgroup
> 
> staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
> staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
> staging: erofs: add a full barrier in erofs_workgroup_unfreeze
> 
> staging: erofs: separate into init_once / always
> staging: erofs: locked before registering for all new workgroups
> 
> [v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
> =====end patchset======
> 
> 
> and
> 
> "staging: erofs: managed pages could be locked at the time of decompression"
> could be dropped now since I will refactor the z_erofs_vle_unzip
> (currently, A serious bug in it, I have fixed it internally).
> 
> I am working on the reset stability patches reported by the internal beta test,
> expected to finish this week. :)

No problem, let me update them at this weekend...

Thanks,

> 
> Thanks,
> Gao Xiang
> 
> On 2018/10/31 10:11, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/10/31 10:04, Chao Yu wrote:
>>> On 2018/10/19 14:41, Gao Xiang wrote:
>>>> `z_erofs_vle_workgroup' is heavily generated in the decompression,
>>>> for example, it resets 32 bytes redundantly for 64-bit platforms
>>>> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
>>>> default 4, pages are stored in `z_erofs_vle_workgroup'.
>>>>
>>>> As an another example, `struct mutex' takes 72 bytes for our kirin
>>>> 64-bit platforms, it's unnecessary to be reseted at first and
>>>> be initialized each time.
>>>>
>>>> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
>>>> since most fields are reinitialized to meaningful values later,
>>>> and pagevec is no need to initialized at all.
>>>>
>>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>>> ---
>>>>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>>>> index 79d3ba62b298..7aa26818054a 100644
>>>> --- a/drivers/staging/erofs/unzip_vle.c
>>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>>> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>>>>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>>>  }
>>>>  
>>>> +static void init_once(void *ptr)
>>> How about rename it to init_vle_workgroup() for better readability?
>> The name convension is borrowed from inode_init_always/inode_init_once of fs/inode.c...
>> since it is pure static, I don't add the prefix(such as z_erofs_vle_workgroup or etc.. too long...).
>>
>> p.s. These days I think "vle_" is redundant... It could be renamed at a proper time...
>>
>>
>>>
>>>> +{
>>>> +	struct z_erofs_vle_workgroup *grp = ptr;
>>>> +	struct z_erofs_vle_work *const work =
>>>> +		z_erofs_vle_grab_primary_work(grp);
>>>> +	unsigned int i;
>>>> +
>>>> +	mutex_init(&work->lock);
>>>> +	work->nr_pages = 0;
>>>> +	work->vcnt = 0;
>>>> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
>>>> +		grp->compressed_pages[i] = NULL;
>>>> +}
>>>> +
>>>> +static void init_always(struct z_erofs_vle_workgroup *grp)
>>> Ditto, maybe reset_vle_workgroup().
>>>
>>> Otherwise, it looks good to me.
>>>
>>> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
>>
>> Thanks for the review :)
>>
>> Thanks,
>> Gao Xiang
>>
>>
>>>
>>> Thanks,
>>>
> 
> .
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/11/2 18:26, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/10/31 10:39, Gao Xiang wrote:
>> Hi Chao,
>>
>> Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?
>>
>> =====patchset======
>> staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
>> [v2] staging: erofs: fix race when the managed cache is enabled
>> staging: erofs: fix refcount assertion in erofs_register_workgroup
>>
>> staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
>> staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
>> staging: erofs: add a full barrier in erofs_workgroup_unfreeze
>>
>> staging: erofs: separate into init_once / always
>> staging: erofs: locked before registering for all new workgroups
>>
>> [v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
>> =====end patchset======
>>
>>
>> and
>>
>> "staging: erofs: managed pages could be locked at the time of decompression"
>> could be dropped now since I will refactor the z_erofs_vle_unzip
>> (currently, A serious bug in it, I have fixed it internally).
>>
>> I am working on the reset stability patches reported by the internal beta test,
>> expected to finish this week. :)
> No problem, let me update them at this weekend...

Thanks...could take some time to update this?...
I need find a code-base to work on :)

Thanks,
Gao Xiang

> 
> Thanks,
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018-11-4 18:04, Gao Xiang via Linux-erofs wrote:
> Hi Chao,
> 
> On 2018/11/2 18:26, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/10/31 10:39, Gao Xiang wrote:
>>> Hi Chao,
>>>
>>> Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?
>>>
>>> =====patchset======
>>> staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
>>> [v2] staging: erofs: fix race when the managed cache is enabled
>>> staging: erofs: fix refcount assertion in erofs_register_workgroup
>>>
>>> staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
>>> staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
>>> staging: erofs: add a full barrier in erofs_workgroup_unfreeze
>>>
>>> staging: erofs: separate into init_once / always
>>> staging: erofs: locked before registering for all new workgroups

There is merge conflict when applying above two patches, could you rebase them
on last udpated erofs-dev branch? :)

Thanks,

>>>
>>> [v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
>>> =====end patchset======
>>>
>>>
>>> and
>>>
>>> "staging: erofs: managed pages could be locked at the time of decompression"
>>> could be dropped now since I will refactor the z_erofs_vle_unzip
>>> (currently, A serious bug in it, I have fixed it internally).
>>>
>>> I am working on the reset stability patches reported by the internal beta test,
>>> expected to finish this week. :)
>> No problem, let me update them at this weekend...
> 
> Thanks...could take some time to update this?...
> I need find a code-base to work on :)
> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/11/4 23:27, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018-11-4 18:04, Gao Xiang via Linux-erofs wrote:
>> Hi Chao,
>>
>> On 2018/11/2 18:26, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/10/31 10:39, Gao Xiang wrote:
>>>> Hi Chao,
>>>>
>>>> Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?
>>>>
>>>> =====patchset======
>>>> staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
>>>> [v2] staging: erofs: fix race when the managed cache is enabled
>>>> staging: erofs: fix refcount assertion in erofs_register_workgroup
>>>>
>>>> staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
>>>> staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
>>>> staging: erofs: add a full barrier in erofs_workgroup_unfreeze
>>>>
>>>> staging: erofs: separate into init_once / always
>>>> staging: erofs: locked before registering for all new workgroups
> 
> There is merge conflict when applying above two patches, could you rebase them
> on last udpated erofs-dev branch? :)
> 
> Thanks,
> 
>>>>
>>>> [v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
>>>> =====end patchset======
>>>>
>>>>
>>>> and
>>>>
>>>> "staging: erofs: managed pages could be locked at the time of decompression"
>>>> could be dropped now since I will refactor the z_erofs_vle_unzip
>>>> (currently, A serious bug in it, I have fixed it internally).
>>>>
>>>> I am working on the reset stability patches reported by the internal beta test,
>>>> expected to finish this week. :)
>>> No problem, let me update them at this weekend...
>>
>> Thanks...could take some time to update this?...
>> I need find a code-base to work on :)
>>
>> Thanks,
>> Gao Xiang
>>
>>>
>>> Thanks,
>>>

I could not see the content of this email,
is there some conflicts with "staging: erofs: separate into init_once / always"?

Thanks,
Gao Xiang

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018-11-4 23:32, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/11/4 23:27, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018-11-4 18:04, Gao Xiang via Linux-erofs wrote:
>>> Hi Chao,
>>>
>>> On 2018/11/2 18:26, Chao Yu wrote:
>>>> Hi Xiang,
>>>>
>>>> On 2018/10/31 10:39, Gao Xiang wrote:
>>>>> Hi Chao,
>>>>>
>>>>> Could you please update chao/erofs-dev branch based on the linus tree and add the following patch?
>>>>>
>>>>> =====patchset======
>>>>> staging: erofs: fix `trace_erofs_readpage' position    <-- trivial
>>>>> [v2] staging: erofs: fix race when the managed cache is enabled
>>>>> staging: erofs: fix refcount assertion in erofs_register_workgroup
>>>>>
>>>>> staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
>>>>> staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
>>>>> staging: erofs: add a full barrier in erofs_workgroup_unfreeze
>>>>>
>>>>> staging: erofs: separate into init_once / always
>>>>> staging: erofs: locked before registering for all new workgroups
>>
>> There is merge conflict when applying above two patches, could you rebase them
>> on last udpated erofs-dev branch? :)
>>
>> Thanks,
>>
>>>>>
>>>>> [v2 RESEND] staging: erofs: decompress asynchronously if PG_readahead page at first
>>>>> =====end patchset======
>>>>>
>>>>>
>>>>> and
>>>>>
>>>>> "staging: erofs: managed pages could be locked at the time of decompression"
>>>>> could be dropped now since I will refactor the z_erofs_vle_unzip
>>>>> (currently, A serious bug in it, I have fixed it internally).
>>>>>
>>>>> I am working on the reset stability patches reported by the internal beta test,
>>>>> expected to finish this week. :)
>>>> No problem, let me update them at this weekend...
>>>
>>> Thanks...could take some time to update this?...
>>> I need find a code-base to work on :)
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>>>
>>>> Thanks,
>>>>
> 
> I could not see the content of this email,
> is there some conflicts with "staging: erofs: separate into init_once / always"?

Yes, it needs rebase below two patches to last erofs-dev branch.

staging: erofs: separate into init_once / always
staging: erofs: locked before registering for all new workgroups

Thanks,

> 
> Thanks,
> Gao Xiang
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/11/4 23:35, Chao Yu wrote:
> Yes, it needs rebase below two patches to last erofs-dev branch.
> 
> staging: erofs: separate into init_once / always
> staging: erofs: locked before registering for all new workgroups
> 
> Thanks,

I think the erofs-dev branch is missing a upstreamed commit

staging: erofs: add the missing __init tags

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/erofs?id=0a0b7e62525fa0ad778439acc28bf2f53967453e

Could you please update the erofs and erofs-dev branch on the linus tree?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/

Since the commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/lib/lz4/lz4_decompress.c?id=2209fda323e2fd2a2d0885595fd5097717f8d2aa

has been merged in the mainstream... I have to remove our hacked lz4 code...

p.s. could you please drop the commit
"staging: erofs: managed pages could be locked at the time of decompression"?
I will rewrite the whole commit...

Thanks,
Gao Xiang

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018/11/4 23:52, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/11/4 23:35, Chao Yu wrote:
>> Yes, it needs rebase below two patches to last erofs-dev branch.
>>
>> staging: erofs: separate into init_once / always
>> staging: erofs: locked before registering for all new workgroups
>>
>> Thanks,
> 
> I think the erofs-dev branch is missing a upstreamed commit
> 
> staging: erofs: add the missing __init tags
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/erofs?id=0a0b7e62525fa0ad778439acc28bf2f53967453e
> 
> Could you please update the erofs and erofs-dev branch on the linus tree?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> 
> Since the commit
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/lib/lz4/lz4_decompress.c?id=2209fda323e2fd2a2d0885595fd5097717f8d2aa
> 
> has been merged in the mainstream... I have to remove our hacked lz4 code...
> 
> p.s. could you please drop the commit
> "staging: erofs: managed pages could be locked at the time of decompression"?
> I will rewrite the whole commit...

Oh, I forgot to rebase on upstream, will update soon.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/11/5 9:30, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/11/4 23:52, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/11/4 23:35, Chao Yu wrote:
>>> Yes, it needs rebase below two patches to last erofs-dev branch.
>>>
>>> staging: erofs: separate into init_once / always
>>> staging: erofs: locked before registering for all new workgroups
>>>
>>> Thanks,
>> I think the erofs-dev branch is missing a upstreamed commit
>>
>> staging: erofs: add the missing __init tags
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/erofs?id=0a0b7e62525fa0ad778439acc28bf2f53967453e
>>
>> Could you please update the erofs and erofs-dev branch on the linus tree?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>
>> Since the commit
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/lib/lz4/lz4_decompress.c?id=2209fda323e2fd2a2d0885595fd5097717f8d2aa
>>
>> has been merged in the mainstream... I have to remove our hacked lz4 code...
>>
>> p.s. could you please drop the commit
>> "staging: erofs: managed pages could be locked at the time of decompression"?
>> I will rewrite the whole commit...
> Oh, I forgot to rebase on upstream, will update soon.

Yes, and it seems 4.20-rc1 just came out, therefore we could base on 4.20-rc1 to update erofs-master/erofs/erofs-dev....
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/?h=staging-testing

Thanks,
Gao Xiang

> 
> Thanks,
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018/11/5 9:36, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/11/5 9:30, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/11/4 23:52, Gao Xiang wrote:
>>> Hi Chao,
>>>
>>> On 2018/11/4 23:35, Chao Yu wrote:
>>>> Yes, it needs rebase below two patches to last erofs-dev branch.
>>>>
>>>> staging: erofs: separate into init_once / always
>>>> staging: erofs: locked before registering for all new workgroups
>>>>
>>>> Thanks,
>>> I think the erofs-dev branch is missing a upstreamed commit
>>>
>>> staging: erofs: add the missing __init tags
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/erofs?id=0a0b7e62525fa0ad778439acc28bf2f53967453e
>>>
>>> Could you please update the erofs and erofs-dev branch on the linus tree?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>
>>> Since the commit
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/lib/lz4/lz4_decompress.c?id=2209fda323e2fd2a2d0885595fd5097717f8d2aa
>>>
>>> has been merged in the mainstream... I have to remove our hacked lz4 code...
>>>
>>> p.s. could you please drop the commit
>>> "staging: erofs: managed pages could be locked at the time of decompression"?
>>> I will rewrite the whole commit...
>> Oh, I forgot to rebase on upstream, will update soon.
> 
> Yes, and it seems 4.20-rc1 just came out, therefore we could base on 4.20-rc1 to update erofs-master/erofs/erofs-dev....
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/?h=staging-testing

Agreed, let me update erofs* branch this tonight.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
> 
> .
> 

[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Gao Xiang]

Hi Chao,

On 2018/11/5 9:46, Chao Yu wrote:
> Agreed, let me update erofs* branch this tonight.
> 
> Thanks,

I have rebased all the patches and send all out as the patchset
of "[PATCH 4.20-rc1 rebased xx/10]"... It begins at
https://lists.ozlabs.org/pipermail/linux-erofs/2018-November/000785.html

Thanks,
Gao Xiang