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 <[email protected]>
>> Signed-off-by: Gao Xiang <[email protected]>
>> ---
>>
>> - 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
>>
