[Ocfs2-tools-devel] [PATCH 07/15] dx_dirs v11: more library support for directory indexing

[Tao Ma]

Hi Coly,

Coly Li wrote:
> 
> On 04/26/2010 09:27 AM, Tao Ma Wrote:
>> Hi Coly,
>>
>> Coly Li wrote:
>>> On 04/23/2010 10:26 PM, Tao Ma Wrote:
>>>> Hi coly,
>>>>     the 2nd part of review for this really large patch. ;)
>>>>
>>>> Coly Li wrote:
>>>>> -static void ocfs2_swap_dx_entry_list_to_cpu(struct
>>>>> ocfs2_dx_entry_list *dl_list)
>>>>> +static void ocfs2_swap_dx_entry_list(struct ocfs2_dx_entry_list
>>>>> *dl_list)
>>>>>  {
>>>>>      int i;
>>>>>  
>>>>> -    if (cpu_is_little_endian)
>>>>> -        return;
>>>>> -
>>>>>      dl_list->de_count    = bswap_16(dl_list->de_count);
>>>>>      dl_list->de_num_used    = bswap_16(dl_list->de_num_used);
>>>>>  
>>>>>      for (i = 0; i < dl_list->de_count; i++)
>>>>> -        ocfs2_swap_dx_entry_to_cpu(&dl_list->de_entries[i]);
>>>>> +        ocfs2_swap_dx_entry(&dl_list->de_entries[i]);
>>>>> +}
>>>>> +
>>>>> +static void ocfs2_swap_dx_entry_list_to_cpu(struct
>>>>> ocfs2_dx_entry_list *dl_list)
>>>>> +{
>>>>> +    if (cpu_is_little_endian)
>>>>> +        return;
>>>>> +    ocfs2_swap_dx_entry_list(dl_list);
>>>>> +}
>>>>> +
>>>>> +static void ocfs2_swap_dx_entry_list_from_cpu(struct
>>>>> ocfs2_dx_entry_list *dl_list)
>>>>> +{
>>>>> +    if (cpu_is_little_endian)
>>>>> +        return;
>>>>> +    ocfs2_swap_dx_entry_list(dl_list);
>>>>>  }
>>>> Hey, why do you change the original one? Your code is buggy now. When
>>> I change the original one, just want to avoid another
>>> ocfs2_swap_dx_entry_from_cpu().
>>>
>>>> swap_entry_list_to_cpu you need to swap de_count/de_num_used first.
>>>> While if we want to swap_entry_list_from_cpu we neet to swap the
>>>> dx_entry first.
>>> It should be safe to call ocfs2_swap_dx_entry_list() for both to_cpu
>>> and from_cpu. And the code is tested, it works here.
>> Do you test in a big endian platform like ppc? We should have a problem
>> there.
> 
> No, I don't have a big endian machine to test.
> Could you please to explain more, how it's buggy for current code ? I don't catch your mind yet.
OK, so the function is:
static void ocfs2_swap_dx_entry_list(struct ocfs2_dx_entry_list *dl_list)
{
         int i;

         dl_list->de_count       = bswap_16(dl_list->de_count);
         dl_list->de_num_used    = bswap_16(dl_list->de_num_used);

         for (i = 0; i < dl_list->de_count; i++)
                 ocfs2_swap_dx_entry(&dl_list->de_entries[i]);
}

When swapping to the cpu, it is good since we swap de_count first.
But when we swap from the cpu, the de_count is swapped first.
So after this line
         dl_list->de_count       = bswap_16(dl_list->de_count);

the de_count isn't correct any more since in a big endian platform, you 
have swapped the 'de_count' to a le16 value which is going to be stored 
in the disk. That means the following line
         for (i = 0; i < dl_list->de_count; i++)
You use the wrong de_count.

You can check ocfs2_swap_extent_list_{to,from}_cpu for detail. If 
from_cpu, we swap secondary first. While if to_cpu, we swap primary first.

Regards,
Tao