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

[Gao Xiang]

Hi Matthew,

On 2018/7/30 21:19, Matthew Wilcox wrote:
> 
> This review looks pretty external now ...
> 

:-'( Internal means currently in the linux-erofs mailing list for preview.
I haven't sent to the linux-kernel mailing list yet..

> 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?
> 
The previous version of erofs used these bits for differentiating (un)cached entry...
Link: https://lists.ozlabs.org/pipermail/linux-erofs/2018-July/000093.html

It has no use currently, but I plan to reserve an extra bit for the future
VLE/non-VLE hybrid decompression...

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

Thanks, I will look into the radix tree implementation and have a try.
But if XArray is merged into 4.19, I will also try to convert into the XArray...

Thanks,
Gao Xiang

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