Re: [PATCH v2] btrfs: Turn name_cache radix tree into XArray in send_ctx

[Gabriel Niebler <gniebler@suse.com>]

Should I resend with the casts removed?

Am 27.04.22 um 16:36 schrieb Gabriel Niebler:
> Am 27.04.22 um 13:46 schrieb Nikolay Borisov:
>> On 26.04.22 г. 12:51 ч., Gabriel Niebler wrote:
>>> … and adjust all usages of this object to use the XArray API for the 
>>> sake
>>> of consistency.
>>>
>>> XArray API provides array semantics, so it is notionally easier to 
>>> use and
>>> understand, and it also takes care of locking for us.
>>>
>>> None of this makes a real difference in this particular patch, but it 
>>> does
>>> in other places where similar replacements are or have been made and we
>>> want to be consistent in our usage of data structures in btrfs.
>>>
>>> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
>>> ---
>>>
>>> Changes from v1:
>>>   - Let commit message begin with "btrfs: "
>>>
>>> ---
>>>   fs/btrfs/send.c | 40 +++++++++++++++++++---------------------
>>>   1 file changed, 19 insertions(+), 21 deletions(-)
>>>
>>
>> LGTM, one minor nit below though.
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>> <snip>
>>
>>> @@ -262,14 +261,14 @@ struct orphan_dir_info {
>>>   struct name_cache_entry {
>>>       struct list_head list;
>>>       /*
>>> -     * radix_tree has only 32bit entries but we need to handle 64bit 
>>> inums.
>>> -     * We use the lower 32bit of the 64bit inum to store it in the 
>>> tree. If
>>> -     * more then one inum would fall into the same entry, we use 
>>> radix_list
>>> -     * to store the additional entries. radix_list is also used to 
>>> store
>>> -     * entries where two entries have the same inum but different
>>> +     * On 32bit kernels, XArray has only 32bit indices, but we need to
>>> +     * handle 64bit inums. We use the lower 32bit of the 64bit inum 
>>> to store
>>> +     * it in the tree. If more than one inum would fall into the 
>>> same entry,
>>> +     * we use inum_aliases to store the additional entries. 
>>> inum_aliases is
>>> +     * also used to store entries with the same inum but different
>>>        * generations.
>>>        */
>>> -    struct list_head radix_list;
>>> +    struct list_head inum_aliases;
>>>       u64 ino;
>>>       u64 gen;
>>>       u64 parent_ino;
>>> @@ -2019,9 +2018,9 @@ static int did_overwrite_first_ref(struct 
>>> send_ctx *sctx, u64 ino, u64 gen)
>>>   }
>>>   /*
>>> - * Insert a name cache entry. On 32bit kernels the radix tree index 
>>> is 32bit,
>>> + * Insert a name cache entry. On 32bit kernels the XArray index is 
>>> 32bit,
>>>    * so we need to do some special handling in case we have clashes. 
>>> This function
>>> - * takes care of this with the help of name_cache_entry::radix_list.
>>> + * takes care of this with the help of name_cache_entry::inum_aliases.
>>>    * In case of error, nce is kfreed.
>>>    */
>>>   static int name_cache_insert(struct send_ctx *sctx,
>>> @@ -2030,8 +2029,7 @@ static int name_cache_insert(struct send_ctx 
>>> *sctx,
>>>       int ret = 0;
>>>       struct list_head *nce_head;
>>> -    nce_head = radix_tree_lookup(&sctx->name_cache,
>>> -            (unsigned long)nce->ino);
>>> +    nce_head = xa_load(&sctx->name_cache, (unsigned long)nce->ino);
>>
>> The casting is redundant since the function's argument is already 
>> declared as unsigned long so truncation happens anyway. The only 
>> rationale to keep is for documentation purposes but even this is 
>> somewhat dubious. But since there is already something said about that 
>> above the definition of inum_aliases I'd say lets do away with the casts.
> 
> I see your point and I agree with you, but I'd like to point out that I 
> didn't add this cast - it was already there  (and I think there may be 
> others, too).
> 
> I thought about removing it (as we had discussed and I've done in 
> another patch), but then I decided to leave it, thinking that maybe 
> there was a reason for it. Like communicating something explicitely to 
> anyone reading the code.
> 
> It's true, though, that the comment actually explains it.
> 
>>>       if (!nce_head) {
>>>           nce_head = kmalloc(sizeof(*nce_head), GFP_KERNEL);
>>>           if (!nce_head) {
>>> @@ -2040,14 +2038,15 @@ static int name_cache_insert(struct send_ctx 
>>> *sctx,
>>>           }
>>>           INIT_LIST_HEAD(nce_head);
>>> -        ret = radix_tree_insert(&sctx->name_cache, nce->ino, nce_head);
>>> +        ret = xa_insert(&sctx->name_cache, nce->ino, nce_head,
>>
>> Here nce->ino is not cast, yet the parameter is still unsigned long 
>> meaning truncation occurs (as is expected). At the very least this 
>> makes the code style inconsistent.
> 
> Yeah, true. Again, it was inconsistent before I got there, but I'll 
> admit that I didn't notice this one.
> 
> For the sake of consistency, I'm willing to remove the cast (and perhaps 
> others, would have to check) and resend.
> 
> I'll leave that up to the maintainer to decide.
>