Re: auto merging extents

[Jim Meyering]

Pádraig Brady wrote:

> On 09/03/11 18:23, Jim Meyering wrote:
>> Pádraig Brady wrote:
>>> -  i--;
>>> -  if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
>>> +  scan->ei_count = si;
>>> +
>>> +  si--;
>>> +  if (scan->ext_info[si].ext_flags & FIEMAP_EXTENT_LAST)
>>>      {
>>>        scan->hit_final_extent = true;
>>>        return true;
>>>      }
>>>
>>> +  i--;
>>>    scan->scan_start = fm_extents[i].fe_logical + fm_extents[i].fe_length;
>>
>> The above is all fine, but unless you know that scan->ei_count
>> is always positive, seeing "i" and "si" used as indices right
>> after being decremented may make you think there's a risk
>> of accessing some_buffer[-1].
>>
>> What do you think about adding an assertion, either on scan->ei_count
>> before the loop, or on i and/or si after it?
>
> Yes, it's not immediately obvious that i and si >= 1,
> but it's guaranteed by the early return when fiemap->fm_mapped_extents==0.
> Because of that return, an assert is overkill I think.
> Actually I think we can simplify further by just
> using a pointer to the last extent_info entry we're updating.

That sounds fine.
If clang complains, we'll address it.

> I also noticed that we shouldn't care about
> the FIEMAP_EXTENT_LAST flag when merging.

Subtle one.  Good catch.
That patch looks fine.

Have you thought of adding a test that would check for
the merged extents in the result, say using filefrag?