On 07/13/2014 02:47 PM, Ivan Shapovalov wrote:
> On Sunday 13 July 2014 at 03:33:57, Edward Shishkin wrote:	
>> On 07/09/2014 02:40 PM, Ivan Shapovalov wrote:
>>> On Monday 07 July 2014 at 01:47:41, Edward Shishkin wrote:	
>>>> On 06/22/2014 12:48 PM, Ivan Shapovalov wrote:
>>>> [...]
>> [...]
>>>>> + * - if a single extent is smaller than the erase unit, then this particular
>>>>> + *   extent won't be discarded even if it is surrounded by enough free blocks
>>>>> + *   to constitute a whole erase unit;
>>>> Why not to discard the aligned and padded extent, which coincides
>>>> with the whole erase unit?
>>> With a number of whole erase units.
>>>
>>>>> + * - we won't be able to merge small adjacent extents forming an extent long
>>>>> + *   enough to be discarded.
>>>> At this point we have already sorted and merged everything.
>>>> So may be it makes sense just to check the head and tail of every resulted
>>>> extent and discard the aligned and padded one?
>>> "Head and tail" is not sufficient. We may check the whole extent with a single
>>> bitmap request, but such algorithm will be very inefficient: it will miss many
>>> possibilities for discarding.
>>>
>>> Consider many-block extent, from which one block has been allocated again.
>>> In this case we miss (all-1) blocks to be discarded (if granularity = 1 block).
>>>
>>>> Please, consider such possibility. Iterating over erase units in
>>>> discard_extent()
>>>> looks suboptimal.
>>> Yes, it's costly... but I don't see any other ways to do the task efficiently.
>>
>> How about this function?  (the attached patch is against v6-series).
>> Total number of bitmap checks is in [N+1, 2N], where N is number of
>> extents  in the list. At the same time we don't leave any non-discarded
>> "garbage"...
>>
>> Edward.
>> P.S. I tested it, but not enough.
> Hm. I'm probably a dumbass, but...
>
> I don't see where the [start; start+len) region is checked for being free.


check_free_blocks() is called for this purpose.

Firstly when checking head padding. Secondly in the gluing loop
(to glue 2 extents in particular means to make sure that region
between them is free). Finally we check if the tail padding is free.


>
> Also, btw, do we need to cut the head (lines 155-163 of the patch) if headp
> is empty? It seems that it would reduce extent by one whole erase unit
> without any justification.


Yes, this is definitely a leak of not discarded erase units.
Is the attached version better?

Edward.