Re: [PATCH 2/2] reiser4: block_alloc: get rid of discard-related hack in reiser4_dealloc_blocks().,

[Edward Shishkin <edward.shishkin@gmail.com>]

On 09/06/2014 12:33 PM, Ivan Shapovalov wrote:
> On Sunday 31 August 2014 at 23:19:10, Edward Shishkin wrote:	
>> On 08/18/2014 01:31 PM, Ivan Shapovalov wrote:
>>> When discard was enabled, immediate deallocations were made deferred in order
>>> to record these extents in the atom's delete set and, consequently, allow
>>> their discarding.
>>>
>>> However, this is wrong in the following way. Immediate deallocations make use
>>> of target allocation stage and ancillary flags like BA_PERMANENT and BA_FORMATTED.
>>> By converting immediate deallocations to deferred, these flags are essentially
>>> dropped, and application of deferred deallocations in reiser4_post_write_back_hook()
>>> uses an equivalent of BLOCK_NOT_COUNTED stage and BA_FORMATTED flag.
>>>
>>> Dropping this hack does not hinder efficiency of the discard procedure, because
>>> immediate deallocations are now used only to deallocate "just allocated" and not
>>> yet written blocks, which actually do not need to be discarded.
>>
>> The idea to defer every "successful" deallocation regardless of discard
>> support status looks OK, but I don't like the first patch (1/2), so
>> let's try
>> to improve it..
>>
>> So, we want to defer also deallocations in the following 2 places:
>> 1) in  dealloc_tx_list();
>> 2) in dealloc_wmap_actor().
>>
>> Here we should take care about block stages.
>> Let's take a look where and how they were allocated. There are exactly
>> 2 places:
>>
>> a) in get_more_wandered_blocks() with block_stage = BLOCK_GRABBED;
>> b) in alloc_tx() with block_stage = BLOCK_GRABBED.
>>
>> Note, that alloc_blocks() with the stage BLOCK_GRABBED calls
>> grabbed2used(). This is exactly what we want in apply_dset() called by
>> post_write_back_hook.
>>
>> That is, just adding BA_DEFER to (1) and (2) with the patch 2/2 should work.
>> Please, make sure..
> This patchset does it exactly this way.
>
> The first patch does two things:
> - adds BA_DEFER to deallocations in dealloc_tx_list() and dealloc_wmap_actor()
>    (because we really want them deferred);
> - removes BA_FORMATTED from where BA_DEFER is used
>    (this is the thing you don't like).
>
> Let me explain why I've done the second thing. When BA_DEFER is specified,
> BA_FORMATTED is not checked for (there are even no assertions). While it may be
> semantically correct to specify BA_FORMATTED as well in these cases, it is not
> checked for and thus sooner or later we'll get a case when BA_FORMATTED flag is
> missing, and no one will notice that (because it does not influence the system
> behavior). So I've decided to force-expire that moment and preemptively remove
> the flags :)
>
> Of course, you are the maintainer. If you opt for keeping BA_DEFER | BA_FORMATTED
> combination, let's do it this way (but then it would sense to add an assertion).


It can happen, that we'll use BA_FORMATTED flag in deferred deallocations.
So I suggest to not touch it..


>
>> P.S. In (1)-(2) we allocate with stage BLOCK_GRABBED, but in (a)-(b) we
>> deallocate with stage BLOCK_NOT_COUNTED. It can confuse. Let's allocate
>> with stage BLOCK_NOT_COUNTED: from the standpoint of alloc_blocks() it
>> doesn't matter.
> This would be incorrect. The space for allocations in get_more_wandered_blocks()
> and alloc_tx() is already grabbed at the beginning of commit_tx() (see wander.c:1103),
> so these allocations have to be done with stage == BLOCK_GRABBED.


Ok, agreed..

Thanks,
Edward.