[PATCH 1/6] staging: erofs: remove redundant CONFIG_EROFS_FS_XATTRs

[PATCH 1/6] staging: erofs: remove redundant CONFIG_EROFS_FS_XATTRs

[Gao Xiang]

some CONFIG_EROFS_FS_XATTR conditions were added because of
the historial Linux kernel compatibility, which are unneeded now.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

Hi all,

  These are cleanup patches in Chao's erofs-dev test tree for a while.
  Many of them are trivial.
  In order for all preview patches to keep up with the staging tree,
I resend them now.

Thanks,
Gao Xiang

 drivers/staging/erofs/inode.c    | 6 ------
 drivers/staging/erofs/internal.h | 6 ++----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index c46a8d4..da8693a 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -260,22 +260,16 @@ struct inode *erofs_iget(struct super_block *sb,
 const struct inode_operations erofs_generic_xattr_iops = {
 	.listxattr = erofs_listxattr,
 };
-#endif
 
-#ifdef CONFIG_EROFS_FS_XATTR
 const struct inode_operations erofs_symlink_xattr_iops = {
 	.get_link = page_get_link,
 	.listxattr = erofs_listxattr,
 };
-#endif
 
 const struct inode_operations erofs_special_inode_operations = {
-#ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
-#endif
 };
 
-#ifdef CONFIG_EROFS_FS_XATTR
 const struct inode_operations erofs_fast_symlink_xattr_iops = {
 	.get_link = simple_get_link,
 	.listxattr = erofs_listxattr,
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 0011b9d..cfcc6db 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -507,13 +507,11 @@ extern struct inode *erofs_iget(struct super_block *sb,
 int erofs_namei(struct inode *dir, struct qstr *name,
 	erofs_nid_t *nid, unsigned *d_type);
 
-/* xattr.c */
 #ifdef CONFIG_EROFS_FS_XATTR
+/* xattr.c */
 extern const struct xattr_handler *erofs_xattr_handlers[];
-#endif
 
-/* symlink */
-#ifdef CONFIG_EROFS_FS_XATTR
+/* symlink and special inode */
 extern const struct inode_operations erofs_symlink_xattr_iops;
 extern const struct inode_operations erofs_fast_symlink_xattr_iops;
 extern const struct inode_operations erofs_special_inode_operations;
-- 
1.9.1

[PATCH 2/6] staging: erofs: fold in `__update_workgrp_llen'

[Gao Xiang]

There is the only one user to use `__update_workgrp_llen'.
Fold it in `z_erofs_vle_work_iter_begin' and cleanup related code.

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

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 2b9b313..e820490 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -422,18 +422,6 @@ struct z_erofs_vle_work_finder {
 	return work;
 }
 
-static inline void __update_workgrp_llen(struct z_erofs_vle_workgroup *grp,
-					 unsigned int llen)
-{
-	while (1) {
-		unsigned int orig_llen = grp->llen;
-
-		if (orig_llen >= llen || orig_llen ==
-			cmpxchg(&grp->llen, orig_llen, llen))
-			break;
-	}
-}
-
 #define builder_is_followed(builder) \
 	((builder)->role >= Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED)
 
@@ -466,7 +454,13 @@ static int z_erofs_vle_work_iter_begin(struct z_erofs_vle_work_builder *builder,
 repeat:
 	work = z_erofs_vle_work_lookup(&finder);
 	if (work != NULL) {
-		__update_workgrp_llen(grp, map->m_llen);
+		unsigned int orig_llen;
+
+		/* increase workgroup `llen' if needed */
+		while ((orig_llen = READ_ONCE(grp->llen)) < map->m_llen &&
+		       orig_llen != cmpxchg_relaxed(&grp->llen,
+						    orig_llen, map->m_llen))
+			cpu_relax();
 		goto got_it;
 	}
 
-- 
1.9.1

Re: [PATCH 2/6] staging: erofs: fold in `__update_workgrp_llen'

[Chao Yu]

On 2018/9/19 13:49, Gao Xiang wrote:
> There is the only one user to use `__update_workgrp_llen'.
> Fold it in `z_erofs_vle_work_iter_begin' and cleanup related code.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

[PATCH 3/6] staging: erofs: drop multiref support temporarily

[Gao Xiang]

Multiref support means that a compressed page could have
more than one reference, which is designed for on-disk data
deduplication. However, mkfs doesn't support this mode
at this moment, and the kernel implementation is also broken.

Let's drop multiref support. If it is fully implemented
in the future, it can be reverted later.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 36 +++++-------------------------------
 drivers/staging/erofs/unzip_vle.h | 12 +-----------
 2 files changed, 6 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index e820490..9fe18cd3 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -301,12 +301,9 @@ struct z_erofs_vle_work_finder {
 	grp = container_of(egrp, struct z_erofs_vle_workgroup, obj);
 	*f->grp_ret = grp;
 
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 	work = z_erofs_vle_grab_work(grp, f->pageofs);
+	/* if multiref is disabled, `primary' is always true */
 	primary = true;
-#else
-	BUG();
-#endif
 
 	DBG_BUGON(work->pageofs != f->pageofs);
 
@@ -368,12 +365,9 @@ struct z_erofs_vle_work_finder {
 	struct z_erofs_vle_workgroup *grp = *f->grp_ret;
 	struct z_erofs_vle_work *work;
 
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
+	/* if multiref is disabled, grp should never be nullptr */
 	BUG_ON(grp != NULL);
-#else
-	if (grp != NULL)
-		goto skip;
-#endif
+
 	/* no available workgroup, let's allocate one */
 	grp = kmem_cache_zalloc(z_erofs_workgroup_cachep, GFP_NOFS);
 	if (unlikely(grp == NULL))
@@ -396,13 +390,7 @@ struct z_erofs_vle_work_finder {
 	*f->hosted = true;
 
 	gnew = true;
-#ifdef CONFIG_EROFS_FS_ZIP_MULTIREF
-skip:
-	/* currently unimplemented */
-	BUG();
-#else
 	work = z_erofs_vle_grab_primary_work(grp);
-#endif
 	work->pageofs = f->pageofs;
 
 	mutex_init(&work->lock);
@@ -797,9 +785,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 
 	struct z_erofs_pagevec_ctor ctor;
 	unsigned int nr_pages;
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 	unsigned int sparsemem_pages = 0;
-#endif
 	struct page *pages_onstack[Z_EROFS_VLE_VMAP_ONSTACK_PAGES];
 	struct page **pages, **compressed_pages, *page;
 	unsigned int i, llen;
@@ -811,11 +797,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 	int err;
 
 	might_sleep();
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 	work = z_erofs_vle_grab_primary_work(grp);
-#else
-	BUG();
-#endif
 	BUG_ON(!READ_ONCE(work->nr_pages));
 
 	mutex_lock(&work->lock);
@@ -866,13 +848,11 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 			pagenr = z_erofs_onlinepage_index(page);
 
 		BUG_ON(pagenr >= nr_pages);
-
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 		BUG_ON(pages[pagenr] != NULL);
-		++sparsemem_pages;
-#endif
+
 		pages[pagenr] = page;
 	}
+	sparsemem_pages = i;
 
 	z_erofs_pagevec_ctor_exit(&ctor, true);
 
@@ -902,10 +882,8 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 		pagenr = z_erofs_onlinepage_index(page);
 
 		BUG_ON(pagenr >= nr_pages);
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 		BUG_ON(pages[pagenr] != NULL);
 		++sparsemem_pages;
-#endif
 		pages[pagenr] = page;
 
 		overlapped = true;
@@ -931,12 +909,10 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 	if (err != -ENOTSUPP)
 		goto out_percpu;
 
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 	if (sparsemem_pages >= nr_pages) {
 		BUG_ON(sparsemem_pages > nr_pages);
 		goto skip_allocpage;
 	}
-#endif
 
 	for (i = 0; i < nr_pages; ++i) {
 		if (pages[i] != NULL)
@@ -945,9 +921,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 		pages[i] = __stagingpage_alloc(page_pool, GFP_NOFS);
 	}
 
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
 skip_allocpage:
-#endif
 	vout = erofs_vmap(pages, nr_pages);
 
 	err = z_erofs_vle_unzip_vmap(compressed_pages,
diff --git a/drivers/staging/erofs/unzip_vle.h b/drivers/staging/erofs/unzip_vle.h
index 3939985..3316bc3 100644
--- a/drivers/staging/erofs/unzip_vle.h
+++ b/drivers/staging/erofs/unzip_vle.h
@@ -47,13 +47,6 @@ static inline bool z_erofs_gather_if_stagingpage(struct list_head *page_pool,
 #define Z_EROFS_VLE_INLINE_PAGEVECS     3
 
 struct z_erofs_vle_work {
-	/* struct z_erofs_vle_work *left, *right; */
-
-#ifdef CONFIG_EROFS_FS_ZIP_MULTIREF
-	struct list_head list;
-
-	atomic_t refcount;
-#endif
 	struct mutex lock;
 
 	/* I: decompression offset in page */
@@ -107,10 +100,8 @@ static inline void z_erofs_vle_set_workgrp_fmt(
 	grp->flags = fmt | (grp->flags & ~Z_EROFS_VLE_WORKGRP_FMT_MASK);
 }
 
-#ifdef CONFIG_EROFS_FS_ZIP_MULTIREF
-#error multiref decompression is unimplemented yet
-#else
 
+/* definitions if multiref is disabled */
 #define z_erofs_vle_grab_primary_work(grp)	(&(grp)->work)
 #define z_erofs_vle_grab_work(grp, pageofs)	(&(grp)->work)
 #define z_erofs_vle_work_workgroup(wrk, primary)	\
@@ -118,7 +109,6 @@ static inline void z_erofs_vle_set_workgrp_fmt(
 		struct z_erofs_vle_workgroup, work) : \
 		({ BUG(); (void *)NULL; }))
 
-#endif
 
 #define Z_EROFS_WORKGROUP_SIZE       sizeof(struct z_erofs_vle_workgroup)
 
-- 
1.9.1

Re: [PATCH 3/6] staging: erofs: drop multiref support temporarily

[Chao Yu]

On 2018/9/19 13:49, Gao Xiang wrote:
> Multiref support means that a compressed page could have
> more than one reference, which is designed for on-disk data
> deduplication. However, mkfs doesn't support this mode
> at this moment, and the kernel implementation is also broken.
> 
> Let's drop multiref support. If it is fully implemented
> in the future, it can be reverted later.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

[PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Gao Xiang]

This patch introduces `__should_decompress_synchronously' to
cleanup `z_erofs_vle_normalaccess_readpages'.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/internal.h  | 11 +++++++++++
 drivers/staging/erofs/super.c     |  5 +++++
 drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index cfcc6db..c84eb97 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -95,6 +95,9 @@ struct erofs_sb_info {
 	/* the dedicated workstation for compression */
 	struct radix_tree_root workstn_tree;
 
+	/* threshold for decompression synchronously */
+	unsigned int max_sync_decompress_pages;
+
 #ifdef EROFS_FS_HAS_MANAGED_CACHE
 	struct inode *managed_cache;
 #endif
@@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
 	struct page *page);
 #endif
 
+#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
+
+static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
+						     unsigned int nr)
+{
+	return nr <= sbi->max_sync_decompress_pages;
+}
+
 #endif
 
 /* we strictly follow PAGE_SIZE and no buffer head yet */
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 802202c..5b87f59 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -162,6 +162,11 @@ static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
 
 static void default_options(struct erofs_sb_info *sbi)
 {
+	/* set up some FS parameters */
+#ifdef CONFIG_EROFS_FS_ZIP
+	sbi->max_sync_decompress_pages = DEFAULT_MAX_SYNC_DECOMPRESS_PAGES;
+#endif
+
 #ifdef CONFIG_EROFS_FS_XATTR
 	set_opt(sbi, XATTR_USER);
 #endif
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 9fe18cd3..337a82d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1308,12 +1308,14 @@ static int z_erofs_vle_normalaccess_readpage(struct file *file,
 	return 0;
 }
 
-static inline int __z_erofs_vle_normalaccess_readpages(
-	struct file *filp,
-	struct address_space *mapping,
-	struct list_head *pages, unsigned int nr_pages, bool sync)
+static int z_erofs_vle_normalaccess_readpages(struct file *filp,
+					      struct address_space *mapping,
+					      struct list_head *pages,
+					      unsigned int nr_pages)
 {
 	struct inode *const inode = mapping->host;
+	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+	const bool sync = __should_decompress_synchronously(sbi, nr_pages);
 
 	struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
 	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
@@ -1372,16 +1374,6 @@ static inline int __z_erofs_vle_normalaccess_readpages(
 	return 0;
 }
 
-static int z_erofs_vle_normalaccess_readpages(
-	struct file *filp,
-	struct address_space *mapping,
-	struct list_head *pages, unsigned int nr_pages)
-{
-	return __z_erofs_vle_normalaccess_readpages(filp,
-		mapping, pages, nr_pages,
-		nr_pages < 4 /* sync */);
-}
-
 const struct address_space_operations z_erofs_vle_normalaccess_aops = {
 	.readpage = z_erofs_vle_normalaccess_readpage,
 	.readpages = z_erofs_vle_normalaccess_readpages,
-- 
1.9.1

Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Chao Yu]

Hi Xiang,

On 2018/9/19 13:49, Gao Xiang wrote:
> This patch introduces `__should_decompress_synchronously' to
> cleanup `z_erofs_vle_normalaccess_readpages'.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  drivers/staging/erofs/internal.h  | 11 +++++++++++
>  drivers/staging/erofs/super.c     |  5 +++++
>  drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
>  3 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index cfcc6db..c84eb97 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>  	/* the dedicated workstation for compression */
>  	struct radix_tree_root workstn_tree;
>  
> +	/* threshold for decompression synchronously */
> +	unsigned int max_sync_decompress_pages;
> +
>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>  	struct inode *managed_cache;
>  #endif
> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
>  	struct page *page);
>  #endif
>  
> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
> +
> +static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
> +						     unsigned int nr)
> +{
> +	return nr <= sbi->max_sync_decompress_pages;

-		nr_pages < 4 /* sync */);

There is a little bit changed except cleanup, IIUC, would it be any difference
of performance around boundary of four pages?

Thanks,

> +}
> +
>  #endif
>  
>  /* we strictly follow PAGE_SIZE and no buffer head yet */
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 802202c..5b87f59 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -162,6 +162,11 @@ static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>  
>  static void default_options(struct erofs_sb_info *sbi)
>  {
> +	/* set up some FS parameters */
> +#ifdef CONFIG_EROFS_FS_ZIP
> +	sbi->max_sync_decompress_pages = DEFAULT_MAX_SYNC_DECOMPRESS_PAGES;
> +#endif
> +
>  #ifdef CONFIG_EROFS_FS_XATTR
>  	set_opt(sbi, XATTR_USER);
>  #endif
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 9fe18cd3..337a82d 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -1308,12 +1308,14 @@ static int z_erofs_vle_normalaccess_readpage(struct file *file,
>  	return 0;
>  }
>  
> -static inline int __z_erofs_vle_normalaccess_readpages(
> -	struct file *filp,
> -	struct address_space *mapping,
> -	struct list_head *pages, unsigned int nr_pages, bool sync)
> +static int z_erofs_vle_normalaccess_readpages(struct file *filp,
> +					      struct address_space *mapping,
> +					      struct list_head *pages,
> +					      unsigned int nr_pages)
>  {
>  	struct inode *const inode = mapping->host;
> +	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
> +	const bool sync = __should_decompress_synchronously(sbi, nr_pages);
>  
>  	struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
>  	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
> @@ -1372,16 +1374,6 @@ static inline int __z_erofs_vle_normalaccess_readpages(
>  	return 0;
>  }
>  
> -static int z_erofs_vle_normalaccess_readpages(
> -	struct file *filp,
> -	struct address_space *mapping,
> -	struct list_head *pages, unsigned int nr_pages)
> -{
> -	return __z_erofs_vle_normalaccess_readpages(filp,
> -		mapping, pages, nr_pages,
> -		nr_pages < 4 /* sync */);
> -}
> -
>  const struct address_space_operations z_erofs_vle_normalaccess_aops = {
>  	.readpage = z_erofs_vle_normalaccess_readpage,
>  	.readpages = z_erofs_vle_normalaccess_readpages,
> 

Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Gao Xiang]

Hi Chao,

On 2018/9/19 23:26, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/9/19 13:49, Gao Xiang wrote:
>> This patch introduces `__should_decompress_synchronously' to
>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>>  drivers/staging/erofs/internal.h  | 11 +++++++++++
>>  drivers/staging/erofs/super.c     |  5 +++++
>>  drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
>>  3 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>> index cfcc6db..c84eb97 100644
>> --- a/drivers/staging/erofs/internal.h
>> +++ b/drivers/staging/erofs/internal.h
>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>  	/* the dedicated workstation for compression */
>>  	struct radix_tree_root workstn_tree;
>>  
>> +	/* threshold for decompression synchronously */
>> +	unsigned int max_sync_decompress_pages;
>> +
>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>  	struct inode *managed_cache;
>>  #endif
>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
>>  	struct page *page);
>>  #endif
>>  
>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
>> +
>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
>> +						     unsigned int nr)
>> +{
>> +	return nr <= sbi->max_sync_decompress_pages;
> -		nr_pages < 4 /* sync */);
> 
> There is a little bit changed except cleanup, IIUC, would it be any difference
> of performance around boundary of four pages?

No.. Once synchronous decompression is applied for 1,2,3 pages for no special reasons.
But I think it could be better to adjust it to the power of two --- 1,2,3,4 is preferred.
Since I have no idea to measure which is better or what value is best for all platform or use cases...

Therefore I tune it in this patch since I don't like the number
DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...

Thanks,
Gao Xiang

> 
> Thanks,
> 

Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Gao Xiang]

Hi Chao,

On 2018/9/19 23:32, Gao Xiang via Linux-erofs wrote:
> Hi Chao,
> 
> On 2018/9/19 23:26, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/9/19 13:49, Gao Xiang wrote:
>>> This patch introduces `__should_decompress_synchronously' to
>>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> ---
>>>  drivers/staging/erofs/internal.h  | 11 +++++++++++
>>>  drivers/staging/erofs/super.c     |  5 +++++
>>>  drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
>>>  3 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>>> index cfcc6db..c84eb97 100644
>>> --- a/drivers/staging/erofs/internal.h
>>> +++ b/drivers/staging/erofs/internal.h
>>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>>  	/* the dedicated workstation for compression */
>>>  	struct radix_tree_root workstn_tree;
>>>  
>>> +	/* threshold for decompression synchronously */
>>> +	unsigned int max_sync_decompress_pages;
>>> +
>>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>>  	struct inode *managed_cache;
>>>  #endif
>>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
>>>  	struct page *page);
>>>  #endif
>>>  
>>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
>>> +
>>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
>>> +						     unsigned int nr)
>>> +{
>>> +	return nr <= sbi->max_sync_decompress_pages;
>> -		nr_pages < 4 /* sync */);
>>
>> There is a little bit changed except cleanup, IIUC, would it be any difference
>> of performance around boundary of four pages?
> 
> No.. Once synchronous decompression is applied for 1,2,3 pages for no special reasons.
> But I think it could be better to adjust it to the power of two --- 1,2,3,4 is preferred.
> Since I have no idea to measure which is better or what value is best for all platform or use cases...
> 
> Therefore I tune it in this patch since I don't like the number
> DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...

p.s. If you think that is not the cleanup meaning, I will resend this patch with
the original 3. Both are ok for me. it is a minor tuning and I don't have a way
to measure which is better and the best boundary.

Thanks,
Gao Xiang

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

Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Chao Yu]

Hi Xiang,

On 2018/9/19 23:32, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/9/19 23:26, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/9/19 13:49, Gao Xiang wrote:
>>> This patch introduces `__should_decompress_synchronously' to
>>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> ---
>>>  drivers/staging/erofs/internal.h  | 11 +++++++++++
>>>  drivers/staging/erofs/super.c     |  5 +++++
>>>  drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
>>>  3 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>>> index cfcc6db..c84eb97 100644
>>> --- a/drivers/staging/erofs/internal.h
>>> +++ b/drivers/staging/erofs/internal.h
>>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>>  	/* the dedicated workstation for compression */
>>>  	struct radix_tree_root workstn_tree;
>>>  
>>> +	/* threshold for decompression synchronously */
>>> +	unsigned int max_sync_decompress_pages;
>>> +
>>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>>  	struct inode *managed_cache;
>>>  #endif
>>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
>>>  	struct page *page);
>>>  #endif
>>>  
>>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
>>> +
>>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
>>> +						     unsigned int nr)
>>> +{
>>> +	return nr <= sbi->max_sync_decompress_pages;
>> -		nr_pages < 4 /* sync */);
>>
>> There is a little bit changed except cleanup, IIUC, would it be any difference
>> of performance around boundary of four pages?
> 
> No.. Once synchronous decompression is applied for 1,2,3 pages for no special reasons.
> But I think it could be better to adjust it to the power of two --- 1,2,3,4 is preferred.
> Since I have no idea to measure which is better or what value is best for all platform or use cases...
> 
> Therefore I tune it in this patch since I don't like the number
> DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...

Yeah, how about adding one more patch to tune this magic number first? as that
change would not be part of cleanup thing. I guess later maybe we could export a
sysfs node to provider tuning ability on that threshold.

Thanks,

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

Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Gao Xiang]

Hi Chao,

On 2018/9/19 23:45, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/9/19 23:32, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/9/19 23:26, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/9/19 13:49, Gao Xiang wrote:
>>>> This patch introduces `__should_decompress_synchronously' to
>>>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>>>
>>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>>> ---
>>>>  drivers/staging/erofs/internal.h  | 11 +++++++++++
>>>>  drivers/staging/erofs/super.c     |  5 +++++
>>>>  drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
>>>>  3 files changed, 22 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>>>> index cfcc6db..c84eb97 100644
>>>> --- a/drivers/staging/erofs/internal.h
>>>> +++ b/drivers/staging/erofs/internal.h
>>>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>>>  	/* the dedicated workstation for compression */
>>>>  	struct radix_tree_root workstn_tree;
>>>>  
>>>> +	/* threshold for decompression synchronously */
>>>> +	unsigned int max_sync_decompress_pages;
>>>> +
>>>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>>>  	struct inode *managed_cache;
>>>>  #endif
>>>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
>>>>  	struct page *page);
>>>>  #endif
>>>>  
>>>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	4
>>>> +
>>>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
>>>> +						     unsigned int nr)
>>>> +{
>>>> +	return nr <= sbi->max_sync_decompress_pages;
>>> -		nr_pages < 4 /* sync */);
>>>
>>> There is a little bit changed except cleanup, IIUC, would it be any difference
>>> of performance around boundary of four pages?
>>
>> No.. Once synchronous decompression is applied for 1,2,3 pages for no special reasons.
>> But I think it could be better to adjust it to the power of two --- 1,2,3,4 is preferred.
>> Since I have no idea to measure which is better or what value is best for all platform or use cases...
>>
>> Therefore I tune it in this patch since I don't like the number
>> DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...
> 
> Yeah, how about adding one more patch to tune this magic number first? as that
> change would not be part of cleanup thing. I guess later maybe we could export a
> sysfs node to provider tuning ability on that threshold.

OK, I think we can leave the original "3" for now. I will fix that value and resend this patch soon.

The sysfs interface could be necessary later since it seems some arguments
involved in decompression could impact the decompression performance.

Thanks,
Gao Xiang

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

[PATCH v2 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

This patch introduces `__should_decompress_synchronously' to
cleanup `z_erofs_vle_normalaccess_readpages'.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
change log v2:
 - Leave the original threshold "3" for DEFAULT_MAX_SYNC_DECOMPRESS_PAGES
   and just do the cleanup work.

 drivers/staging/erofs/internal.h  | 11 +++++++++++
 drivers/staging/erofs/super.c     |  5 +++++
 drivers/staging/erofs/unzip_vle.c | 20 ++++++--------------
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index cfcc6db..c2cc4a016 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -95,6 +95,9 @@ struct erofs_sb_info {
 	/* the dedicated workstation for compression */
 	struct radix_tree_root workstn_tree;
 
+	/* threshold for decompression synchronously */
+	unsigned int max_sync_decompress_pages;
+
 #ifdef EROFS_FS_HAS_MANAGED_CACHE
 	struct inode *managed_cache;
 #endif
@@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct address_space *mapping,
 	struct page *page);
 #endif
 
+#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	3
+
+static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
+						     unsigned int nr)
+{
+	return nr <= sbi->max_sync_decompress_pages;
+}
+
 #endif
 
 /* we strictly follow PAGE_SIZE and no buffer head yet */
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 802202c..5b87f59 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -162,6 +162,11 @@ static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
 
 static void default_options(struct erofs_sb_info *sbi)
 {
+	/* set up some FS parameters */
+#ifdef CONFIG_EROFS_FS_ZIP
+	sbi->max_sync_decompress_pages = DEFAULT_MAX_SYNC_DECOMPRESS_PAGES;
+#endif
+
 #ifdef CONFIG_EROFS_FS_XATTR
 	set_opt(sbi, XATTR_USER);
 #endif
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 9fe18cd3..337a82d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1308,12 +1308,14 @@ static int z_erofs_vle_normalaccess_readpage(struct file *file,
 	return 0;
 }
 
-static inline int __z_erofs_vle_normalaccess_readpages(
-	struct file *filp,
-	struct address_space *mapping,
-	struct list_head *pages, unsigned int nr_pages, bool sync)
+static int z_erofs_vle_normalaccess_readpages(struct file *filp,
+					      struct address_space *mapping,
+					      struct list_head *pages,
+					      unsigned int nr_pages)
 {
 	struct inode *const inode = mapping->host;
+	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+	const bool sync = __should_decompress_synchronously(sbi, nr_pages);
 
 	struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
 	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
@@ -1372,16 +1374,6 @@ static inline int __z_erofs_vle_normalaccess_readpages(
 	return 0;
 }
 
-static int z_erofs_vle_normalaccess_readpages(
-	struct file *filp,
-	struct address_space *mapping,
-	struct list_head *pages, unsigned int nr_pages)
-{
-	return __z_erofs_vle_normalaccess_readpages(filp,
-		mapping, pages, nr_pages,
-		nr_pages < 4 /* sync */);
-}
-
 const struct address_space_operations z_erofs_vle_normalaccess_aops = {
 	.readpage = z_erofs_vle_normalaccess_readpage,
 	.readpages = z_erofs_vle_normalaccess_readpages,
-- 
2.7.4

Re: [PATCH v2 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'

[Chao Yu]

On 2018/9/20 0:06, Gao Xiang wrote:
> From: Gao Xiang <gaoxiang25@huawei.com>
> 
> This patch introduces `__should_decompress_synchronously' to
> cleanup `z_erofs_vle_normalaccess_readpages'.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

[PATCH 5/6] staging: erofs: add some comments for xattr subsystem

[Gao Xiang]

As Dan Carpenter pointed out, it is better to document what
return values of these callbacks in `struct xattr_iter_handlers'
mean and why it->ofs is increased regardless of success or
failure in `xattr_foreach'.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/xattr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 4942ca1..7b1367e 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -109,6 +109,13 @@ static int init_inode_xattrs(struct inode *inode)
 	return 0;
 }
 
+/*
+ * the general idea for these return values is
+ * if    0 is returned, go on processing the current xattr;
+ *       1 (> 0) is returned, skip this round to process the next xattr;
+ *    -err (< 0) is returned, an error (maybe ENOXATTR) occurred
+ *                            and need to be handled
+ */
 struct xattr_iter_handlers {
 	int (*entry)(struct xattr_iter *, struct erofs_xattr_entry *);
 	int (*name)(struct xattr_iter *, unsigned int, char *, unsigned int);
@@ -164,6 +171,10 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	return vi->xattr_isize - xattr_header_sz;
 }
 
+/*
+ * Regardless of success or failure, `xattr_foreach' will end up with
+ * `ofs' pointing to the next xattr item rather than an arbitrary position.
+ */
 static int xattr_foreach(struct xattr_iter *it,
 	const struct xattr_iter_handlers *op, unsigned int *tlimit)
 {
@@ -255,7 +266,7 @@ static int xattr_foreach(struct xattr_iter *it,
 	}
 
 out:
-	/* we assume that ofs is aligned with 4 bytes */
+	/* xattrs should be 4-byte aligned (on-disk constraint) */
 	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
 	return err;
 }
-- 
1.9.1

Re: [PATCH 5/6] staging: erofs: add some comments for xattr subsystem

[Chao Yu]

On 2018/9/19 13:49, Gao Xiang wrote:
> As Dan Carpenter pointed out, it is better to document what
> return values of these callbacks in `struct xattr_iter_handlers'
> mean and why it->ofs is increased regardless of success or
> failure in `xattr_foreach'.
> 
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---

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

Thanks,

[PATCH 6/6] staging: erofs: simplify return value of `xattr_foreach'

[Gao Xiang]

As Dan Carpenter pointed out, there is no need to propagate positive
return values back to its callers.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/xattr.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 7b1367e..80dca6a 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -268,7 +268,7 @@ static int xattr_foreach(struct xattr_iter *it,
 out:
 	/* xattrs should be 4-byte aligned (on-disk constraint) */
 	it->ofs = EROFS_XATTR_ALIGN(it->ofs);
-	return err;
+	return err < 0 ? err : 0;
 }
 
 struct getxattr_iter {
@@ -333,15 +333,12 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 	remaining = ret;
 	while (remaining) {
 		ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
-		if (ret >= 0)
-			break;
-
-		if (ret != -ENOATTR)	/* -ENOMEM, -EIO, etc. */
+		if (ret != -ENOATTR)
 			break;
 	}
 	xattr_iter_end_final(&it->it);
 
-	return ret < 0 ? ret : it->buffer_size;
+	return ret ? ret : it->buffer_size;
 }
 
 static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
@@ -371,16 +368,13 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 		}
 
 		ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
-		if (ret >= 0)
-			break;
-
-		if (ret != -ENOATTR)	/* -ENOMEM, -EIO, etc. */
+		if (ret != -ENOATTR)
 			break;
 	}
 	if (vi->xattr_shared_count)
 		xattr_iter_end_final(&it->it);
 
-	return ret < 0 ? ret : it->buffer_size;
+	return ret ? ret : it->buffer_size;
 }
 
 static bool erofs_xattr_user_list(struct dentry *dentry)
@@ -567,11 +561,11 @@ static int inline_listxattr(struct listxattr_iter *it)
 	remaining = ret;
 	while (remaining) {
 		ret = xattr_foreach(&it->it, &list_xattr_handlers, &remaining);
-		if (ret < 0)
+		if (ret)
 			break;
 	}
 	xattr_iter_end_final(&it->it);
-	return ret < 0 ? ret : it->buffer_ofs;
+	return ret ? ret : it->buffer_ofs;
 }
 
 static int shared_listxattr(struct listxattr_iter *it)
@@ -601,13 +595,13 @@ static int shared_listxattr(struct listxattr_iter *it)
 		}
 
 		ret = xattr_foreach(&it->it, &list_xattr_handlers, NULL);
-		if (ret < 0)
+		if (ret)
 			break;
 	}
 	if (vi->xattr_shared_count)
 		xattr_iter_end_final(&it->it);
 
-	return ret < 0 ? ret : it->buffer_ofs;
+	return ret ? ret : it->buffer_ofs;
 }
 
 ssize_t erofs_listxattr(struct dentry *dentry,
-- 
1.9.1

Re: [PATCH 6/6] staging: erofs: simplify return value of `xattr_foreach'

[Chao Yu]

On 2018/9/19 13:49, Gao Xiang wrote:
> As Dan Carpenter pointed out, there is no need to propagate positive
> return values back to its callers.
> 
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

Re: [PATCH 1/6] staging: erofs: remove redundant CONFIG_EROFS_FS_XATTRs

[Chao Yu]

On 2018/9/19 13:49, Gao Xiang wrote:
> some CONFIG_EROFS_FS_XATTR conditions were added because of
> the historial Linux kernel compatibility, which are unneeded now.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,