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). > > 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. Thanks, -- Ivan Shapovalov / intelfx /
Attachment:
signature.asc
Description: This is a digitally signed message part.
