[FOR INTERNAL REVIEW] [PATCH 3/4] staging: erofs: remove RADIX_TREE_EXCEPTIONAL_{ENTRY,SHIFT}

[Matthew Wilcox]

This review looks pretty external now ...

On Mon, Jul 30, 2018 at 05:18:30PM +0800, Gao Xiang wrote:
> After commit 49520d317715 ("xarray: Replace exceptional entries"),
> there are no RADIX_TREE_EXCEPTIONAL_{ENTRY,SHIFT} at all, remove them
> as a workaround but erofs actually needs an extra bit for each entry.

This patch confuses me.  You say erofs needs an extra bit, but apparently
it works well with the bit removed.  What, then, is the point of the bit?

You can use bit 0 if you need to distinguish between two types of pointers.
Indeed, you can distinguish between three different types of pointers
-- x & 3 == { 0, 1, 3 }.

> In the future, it should be converted to XArray of course.
> 
> Cc: Matthew Wilcox <willy at infradead.org>
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
> 
> - Tested with images generated by mkfs.erofs:
>    1) mount and unmount operations
>    2) md5sum `find . -type f`
> 
>  drivers/staging/erofs/utils.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/erofs/utils.c b/drivers/staging/erofs/utils.c
> index 595cf90..25ff39f 100644
> --- a/drivers/staging/erofs/utils.c
> +++ b/drivers/staging/erofs/utils.c
> @@ -47,9 +47,7 @@ struct erofs_workgroup *erofs_find_workgroup(
>  	rcu_read_lock();
>  	grp = radix_tree_lookup(&sbi->workstn_tree, index);
>  	if (grp != NULL) {
> -		*tag = radix_tree_exceptional_entry(grp);
> -		grp = (void *)((unsigned long)grp &
> -			~RADIX_TREE_EXCEPTIONAL_ENTRY);
> +		*tag = 0;
>  
>  		if (erofs_workgroup_get(grp, &oldcount)) {
>  			/* prefer to relax rcu read side */
> @@ -68,7 +66,7 @@ struct erofs_workgroup *erofs_find_workgroup(
>  
>  int erofs_register_workgroup(struct super_block *sb,
>  			     struct erofs_workgroup *grp,
> -			     bool tag)
> +			     bool __maybe_unused tag)
>  {
>  	struct erofs_sb_info *sbi;
>  	int err;
> @@ -76,6 +74,11 @@ int erofs_register_workgroup(struct super_block *sb,
>  	/* grp->refcount should not < 1 */
>  	BUG_ON(!atomic_read(&grp->refcount));
>  
> +	if (tag) {
> +		DBG_BUGON(1);
> +		return -EINVAL;
> +	}
> +
>  	err = radix_tree_preload(GFP_NOFS);
>  	if (err)
>  		return err;
> @@ -83,10 +86,6 @@ int erofs_register_workgroup(struct super_block *sb,
>  	sbi = EROFS_SB(sb);
>  	erofs_workstn_lock(sbi);
>  
> -	if (tag)
> -		grp = (void *)((unsigned long)grp |
> -			1UL << RADIX_TREE_EXCEPTIONAL_SHIFT);
> -
>  	err = radix_tree_insert(&sbi->workstn_tree,
>  		grp->index, grp);
>  
> @@ -131,9 +130,7 @@ unsigned long erofs_shrink_workstation(struct erofs_sb_info *sbi,
>  
>  	for (i = 0; i < found; ++i) {
>  		int cnt;
> -		struct erofs_workgroup *grp = (void *)
> -			((unsigned long)batch[i] &
> -				~RADIX_TREE_EXCEPTIONAL_ENTRY);
> +		struct erofs_workgroup *grp = batch[i];
>  
>  		first_index = grp->index + 1;
>  
> -- 
> 1.9.1
>