[Ocfs2-devel] [PATCH 1/1] Ocfs2: Optimize punching-hole codes v4.
Tao Ma
tao.ma at oracle.com
Thu Apr 8 21:28:00 PDT 2010
tristan wrote:
> Tao Ma wrote:
>> Hi Tristan,
>>
>> tristan wrote:
>>> Tao,
>>>
>>> Thanks a lot for your quick review;)
>>>
>>> Tao Ma wrote:
>>>> Hi Tristan,
>>>> Tristan Ye wrote:
>>>>> Changes from v3 to v4:
>>>>>
>>>>> 1. Fix a bug when crossing extent blocks.
>>>>>
>>>>> 2. Fix a bug when hole exists in the beginning of an extent block.
>>>>>
>>>>> 3. Apply tao's comments.
>>>>>
>>>>>
>>>>> Signed-off-by: Tristan Ye <tristan.ye at oracle.com>
>>>>> ---
>>>>> fs/ocfs2/file.c | 233
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 1 files changed, 206 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>>>>> index db2e0c9..75e087f 100644
>>>>> --- a/fs/ocfs2/file.c
>>>>> +++ b/fs/ocfs2/file.c
>>>>> @@ -1423,18 +1423,154 @@ out:
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +static void ocfs2_find_rec(struct ocfs2_extent_list *el,
>>>>> + struct ocfs2_extent_rec **rec_found,
>>>>> + u32 *pos)
>>>>> +{
>>>>> + int i, found = 0;
>>>>> + struct ocfs2_extent_rec *rec = NULL;
>>>>> +
>>>>> + for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) {
>>>>> +
>>>>> + rec = &el->l_recs[i];
>>>>> +
>>>>> + if (le32_to_cpu(rec->e_cpos) <= *pos) {
>>>>> + found = 1;
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (!found)
>>>>> + *rec_found = NULL;
>>>>> + else
>>>>> + *rec_found = &el->l_recs[i];
>>>>> +}
>>>>>
>>>> This function never returns pos now. So why you want to pass a *pos?
>>>> another issue is that now it seems that you only want to returns a rec?
>>>> then why not change this function to
>>>> static int ocfs2_find_rec(struct ocfs2_extent_list *el, u32 pos)
>>>
>>> Yes, it's confusing to use *pos, thanks for pointing this out.
>>>
>>>> and after the loop, just return i. So if i>=0, you find it, if i <
>>>> 0, no rec is found. Looks more natural?
>>>
>>> I think returning a meaty record would be more straightforward.
>> why? actually as I have said below, these 2 functions ocfs2_find_rec
>> and ocfs2_find_rec_with_holes can be integrated into one function
>> named ocfs2_find_rec or whatever. You are too nervous about holes
>> actually.
>
> Hole still needs to be handled, while I may over-treat this a little bit.
>
> I'll try to merge the 2 funcs into one.
>> So
>> static int ocfs2_find_rec(struct ocfs2_extent_list *el, u32 pos)
>> {
>> int i;
>> struct ocfs2_extent_rec *rec = NULL;
>>
>> for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) {
>> rec = &el->l_recs[i];
>>
>> if (le32_to_cpu(rec->e_cpos) < pos)
>> break;
>> }
>>
>> return i;
>> }
>
> Not enough,
>
> cases about 'we didn't find a rec' and 'we find rec[0]' both return i=0;
what do you mean? If we can't find a rec, i will be set to -1, so we
will return '-1'. It is the rule of 'for'.
Regards,
Tao
More information about the Ocfs2-devel
mailing list