[PATCH 0/2] reiser4: block deallocation fixes.

[PATCH 0/2] reiser4: block deallocation fixes.

[Ivan Shapovalov]

Actually, the idea of converting immediate allocations into deferred when
discard is enabled was flawed. Deferred deallocations ignore block stage
and additional flags, while some immediate deallocations use non-standard
stage/flags which do not match what's done by reiser4_post_write_back_hook().

So this patchset first tweaks deallocations a bit (while at it, I've
removed redundant and unused flags in some call-sites), so that immediate
deallocations for blocks that actually need to be discarded are properly
converted into deferred deallocations. Remaining instances of immediate
deallocations handle blocks which have been just allocated and not yet written
to, so they do not need to be discarded.

The second commit actually removes the wrong "conversion".

Ivan Shapovalov (2):
  reiser4: sanitize deallocations throughout the code.
  reiser4: block_alloc: get rid of discard-related hack in reiser4_dealloc_blocks().

 fs/reiser4/block_alloc.c  |  3 +--
 fs/reiser4/plugin/txmod.c | 18 ++++++------------
 fs/reiser4/tree.c         |  3 +--
 fs/reiser4/wander.c       |  5 ++---
 4 files changed, 10 insertions(+), 19 deletions(-)

-- 
2.0.4

[PATCH 1/2] reiser4: sanitize deallocations throughout the code.

[Ivan Shapovalov]

- Deferred (BA_DEFER) deallocations do not make use of target stage and
  other flags, so remove them to avoid confusion.
- mark final deallocations in wandering log code as deferred.

With the last point, there is a clear semantic distinction between
deferred and immediate deallocations.
Deferred mode is used to deallocate blocks that were previously written,
immediate mode is used to deallocate "just allocated" blocks in error
paths or if some previously allocated blocks turn out to be unneeded.

This way, we may get rid of discard-related hack in the deallocation routine,
i. e. treating immediate deallocations as deferred (which messes up
block accounting). This is done in the next commit.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/plugin/txmod.c | 18 ++++++------------
 fs/reiser4/tree.c         |  3 +--
 fs/reiser4/wander.c       |  5 ++---
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/fs/reiser4/plugin/txmod.c b/fs/reiser4/plugin/txmod.c
index 63d461f..d8a99b7 100644
--- a/fs/reiser4/plugin/txmod.c
+++ b/fs/reiser4/plugin/txmod.c
@@ -287,8 +287,7 @@ static int forward_relocate_unformatted(flush_pos_t *flush_pos,
 		 * on relocating - free nodes which are going to be
 		 * relocated
 		 */
-		reiser4_dealloc_blocks(&start, &allocated,
-				       BLOCK_ALLOCATED, BA_DEFER);
+		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
 
 	/* assign new block numbers to protected nodes */
 	assign_real_blocknrs(flush_pos, oid, index, allocated, first_allocated);
@@ -386,16 +385,13 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
 	result = put_unit_to_end(left, key, &copy_extent);
 
 	if (result == -E_NODE_FULL) {
-		int target_block_stage;
 		/*
 		 * free blocks which were just allocated
 		 */
-		target_block_stage =
-			(state ==
-			 ALLOCATED_EXTENT) ? BLOCK_FLUSH_RESERVED :
-			BLOCK_UNALLOCATED;
 		reiser4_dealloc_blocks(&first_allocated, &allocated,
-				       target_block_stage,
+				       (state == ALLOCATED_EXTENT)
+				       ? BLOCK_FLUSH_RESERVED
+				       : BLOCK_UNALLOCATED,
 				       BA_PERMANENT);
 		/*
 		 * rewind the preceder
@@ -408,8 +404,7 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
 		/*
 		 * free nodes which were relocated
 		 */
-		reiser4_dealloc_blocks(&start, &allocated,
-				       BLOCK_ALLOCATED, BA_DEFER);
+		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
 	}
 	/*
 	 * assign new block numbers to protected nodes
@@ -689,8 +684,7 @@ static int forward_try_defragment_locality(znode * node,
 		goto exit;
 
 	if (!ZF_ISSET(node, JNODE_CREATED) &&
-	    (ret = reiser4_dealloc_block(znode_get_block(node), 0,
-					 BA_DEFER | BA_FORMATTED)))
+	    (ret = reiser4_dealloc_block(znode_get_block(node), 0, BA_DEFER)))
 		goto exit;
 
 	if (likely(!znode_is_root(node))) {
diff --git a/fs/reiser4/tree.c b/fs/reiser4/tree.c
index 27d8dc3..46072d9 100644
--- a/fs/reiser4/tree.c
+++ b/fs/reiser4/tree.c
@@ -679,8 +679,7 @@ static void uncapture_znode(znode * node)
 
 		/* An already allocated block goes right to the atom's delete set. */
 		ret =
-		    reiser4_dealloc_block(znode_get_block(node), 0,
-					  BA_DEFER | BA_FORMATTED);
+		    reiser4_dealloc_block(znode_get_block(node), 0, BA_DEFER);
 		if (ret)
 			warning("zam-942",
 				"can\'t add a block (%llu) number to atom's delete set\n",
diff --git a/fs/reiser4/wander.c b/fs/reiser4/wander.c
index 04ddec6..fc35a7c 100644
--- a/fs/reiser4/wander.c
+++ b/fs/reiser4/wander.c
@@ -482,8 +482,7 @@ static void dealloc_tx_list(struct commit_handle *ch)
 		jnode *cur = list_entry(ch->tx_list.next, jnode, capture_link);
 		list_del(&cur->capture_link);
 		ON_DEBUG(INIT_LIST_HEAD(&cur->capture_link));
-		reiser4_dealloc_block(jnode_get_block(cur), BLOCK_NOT_COUNTED,
-				      BA_FORMATTED);
+		reiser4_dealloc_block(jnode_get_block(cur), 0, BA_DEFER);
 
 		unpin_jnode_data(cur);
 		reiser4_drop_io_head(cur);
@@ -502,7 +501,7 @@ dealloc_wmap_actor(txn_atom * atom UNUSED_ARG,
 	assert("zam-500", *b != 0);
 	assert("zam-501", !reiser4_blocknr_is_fake(b));
 
-	reiser4_dealloc_block(b, BLOCK_NOT_COUNTED, BA_FORMATTED);
+	reiser4_dealloc_block(b, 0, BA_DEFER);
 	return 0;
 }
 
-- 
2.0.4

Re: [PATCH 1/2] reiser4: sanitize deallocations throughout the code.

[Edward Shishkin]

On 08/18/2014 01:31 PM, Ivan Shapovalov wrote:
> - Deferred (BA_DEFER) deallocations do not make use of target stage and
>    other flags, so remove them to avoid confusion.
> - mark final deallocations in wandering log code as deferred.
>
> With the last point, there is a clear semantic distinction between
> deferred and immediate deallocations.
> Deferred mode is used to deallocate blocks that were previously written,
> immediate mode is used to deallocate "just allocated" blocks in error
> paths or if some previously allocated blocks turn out to be unneeded.
>
> This way, we may get rid of discard-related hack in the deallocation routine,
> i. e. treating immediate deallocations as deferred (which messes up
> block accounting). This is done in the next commit.
>
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/plugin/txmod.c | 18 ++++++------------
>   fs/reiser4/tree.c         |  3 +--
>   fs/reiser4/wander.c       |  5 ++---
>   3 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/fs/reiser4/plugin/txmod.c b/fs/reiser4/plugin/txmod.c
> index 63d461f..d8a99b7 100644
> --- a/fs/reiser4/plugin/txmod.c
> +++ b/fs/reiser4/plugin/txmod.c
> @@ -287,8 +287,7 @@ static int forward_relocate_unformatted(flush_pos_t *flush_pos,
>   		 * on relocating - free nodes which are going to be
>   		 * relocated
>   		 */
> -		reiser4_dealloc_blocks(&start, &allocated,
> -				       BLOCK_ALLOCATED, BA_DEFER);
> +		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
>   
>   	/* assign new block numbers to protected nodes */
>   	assign_real_blocknrs(flush_pos, oid, index, allocated, first_allocated);
> @@ -386,16 +385,13 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
>   	result = put_unit_to_end(left, key, &copy_extent);
>   
>   	if (result == -E_NODE_FULL) {
> -		int target_block_stage;
>   		/*
>   		 * free blocks which were just allocated
>   		 */
> -		target_block_stage =
> -			(state ==
> -			 ALLOCATED_EXTENT) ? BLOCK_FLUSH_RESERVED :
> -			BLOCK_UNALLOCATED;
>   		reiser4_dealloc_blocks(&first_allocated, &allocated,
> -				       target_block_stage,
> +				       (state == ALLOCATED_EXTENT)
> +				       ? BLOCK_FLUSH_RESERVED
> +				       : BLOCK_UNALLOCATED,
>   				       BA_PERMANENT);
>   		/*
>   		 * rewind the preceder
> @@ -408,8 +404,7 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
>   		/*
>   		 * free nodes which were relocated
>   		 */
> -		reiser4_dealloc_blocks(&start, &allocated,
> -				       BLOCK_ALLOCATED, BA_DEFER);
> +		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
>   	}
>   	/*
>   	 * assign new block numbers to protected nodes
> @@ -689,8 +684,7 @@ static int forward_try_defragment_locality(znode * node,
>   		goto exit;
>   
>   	if (!ZF_ISSET(node, JNODE_CREATED) &&
> -	    (ret = reiser4_dealloc_block(znode_get_block(node), 0,
> -					 BA_DEFER | BA_FORMATTED)))
> +	    (ret = reiser4_dealloc_block(znode_get_block(node), 0, BA_DEFER)))


Here you drop BA_FORMATTED flag for the node, which is really formatted.
It makes a mess. Do you have more neat solution?


>   		goto exit;
>   
>   	if (likely(!znode_is_root(node))) {
> diff --git a/fs/reiser4/tree.c b/fs/reiser4/tree.c
> index 27d8dc3..46072d9 100644
> --- a/fs/reiser4/tree.c
> +++ b/fs/reiser4/tree.c
> @@ -679,8 +679,7 @@ static void uncapture_znode(znode * node)
>   
>   		/* An already allocated block goes right to the atom's delete set. */
>   		ret =
> -		    reiser4_dealloc_block(znode_get_block(node), 0,
> -					  BA_DEFER | BA_FORMATTED);
> +		    reiser4_dealloc_block(znode_get_block(node), 0, BA_DEFER);


ditto


>   		if (ret)
>   			warning("zam-942",
>   				"can\'t add a block (%llu) number to atom's delete set\n",
> diff --git a/fs/reiser4/wander.c b/fs/reiser4/wander.c
> index 04ddec6..fc35a7c 100644
> --- a/fs/reiser4/wander.c
> +++ b/fs/reiser4/wander.c
> @@ -482,8 +482,7 @@ static void dealloc_tx_list(struct commit_handle *ch)
>   		jnode *cur = list_entry(ch->tx_list.next, jnode, capture_link);
>   		list_del(&cur->capture_link);
>   		ON_DEBUG(INIT_LIST_HEAD(&cur->capture_link));
> -		reiser4_dealloc_block(jnode_get_block(cur), BLOCK_NOT_COUNTED,
> -				      BA_FORMATTED);
> +		reiser4_dealloc_block(jnode_get_block(cur), 0, BA_DEFER);
>   
>   		unpin_jnode_data(cur);
>   		reiser4_drop_io_head(cur);
> @@ -502,7 +501,7 @@ dealloc_wmap_actor(txn_atom * atom UNUSED_ARG,
>   	assert("zam-500", *b != 0);
>   	assert("zam-501", !reiser4_blocknr_is_fake(b));
>   
> -	reiser4_dealloc_block(b, BLOCK_NOT_COUNTED, BA_FORMATTED);
> +	reiser4_dealloc_block(b, 0, BA_DEFER);


ditto

Thanks,
Edward.

Re: [PATCH 1/2] reiser4: sanitize deallocations throughout the code.

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

On Sunday 31 August 2014 at 13:24:35, Edward Shishkin wrote:	
> 
> On 08/18/2014 01:31 PM, Ivan Shapovalov wrote:
> > - Deferred (BA_DEFER) deallocations do not make use of target stage and
> >    other flags, so remove them to avoid confusion.
> > - mark final deallocations in wandering log code as deferred.
> >
> > With the last point, there is a clear semantic distinction between
> > deferred and immediate deallocations.
> > Deferred mode is used to deallocate blocks that were previously written,
> > immediate mode is used to deallocate "just allocated" blocks in error
> > paths or if some previously allocated blocks turn out to be unneeded.
> >
> > This way, we may get rid of discard-related hack in the deallocation routine,
> > i. e. treating immediate deallocations as deferred (which messes up
> > block accounting). This is done in the next commit.
> >
> > Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> > ---
> >   fs/reiser4/plugin/txmod.c | 18 ++++++------------
> >   fs/reiser4/tree.c         |  3 +--
> >   fs/reiser4/wander.c       |  5 ++---
> >   3 files changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/reiser4/plugin/txmod.c b/fs/reiser4/plugin/txmod.c
> > index 63d461f..d8a99b7 100644
> > --- a/fs/reiser4/plugin/txmod.c
> > +++ b/fs/reiser4/plugin/txmod.c
> > @@ -287,8 +287,7 @@ static int forward_relocate_unformatted(flush_pos_t *flush_pos,
> >   		 * on relocating - free nodes which are going to be
> >   		 * relocated
> >   		 */
> > -		reiser4_dealloc_blocks(&start, &allocated,
> > -				       BLOCK_ALLOCATED, BA_DEFER);
> > +		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
> >   
> >   	/* assign new block numbers to protected nodes */
> >   	assign_real_blocknrs(flush_pos, oid, index, allocated, first_allocated);
> > @@ -386,16 +385,13 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
> >   	result = put_unit_to_end(left, key, &copy_extent);
> >   
> >   	if (result == -E_NODE_FULL) {
> > -		int target_block_stage;
> >   		/*
> >   		 * free blocks which were just allocated
> >   		 */
> > -		target_block_stage =
> > -			(state ==
> > -			 ALLOCATED_EXTENT) ? BLOCK_FLUSH_RESERVED :
> > -			BLOCK_UNALLOCATED;
> >   		reiser4_dealloc_blocks(&first_allocated, &allocated,
> > -				       target_block_stage,
> > +				       (state == ALLOCATED_EXTENT)
> > +				       ? BLOCK_FLUSH_RESERVED
> > +				       : BLOCK_UNALLOCATED,
> >   				       BA_PERMANENT);
> >   		/*
> >   		 * rewind the preceder
> > @@ -408,8 +404,7 @@ static squeeze_result squeeze_relocate_unformatted(znode *left,
> >   		/*
> >   		 * free nodes which were relocated
> >   		 */
> > -		reiser4_dealloc_blocks(&start, &allocated,
> > -				       BLOCK_ALLOCATED, BA_DEFER);
> > +		reiser4_dealloc_blocks(&start, &allocated, 0, BA_DEFER);
> >   	}
> >   	/*
> >   	 * assign new block numbers to protected nodes
> > @@ -689,8 +684,7 @@ static int forward_try_defragment_locality(znode * node,
> >   		goto exit;
> >   
> >   	if (!ZF_ISSET(node, JNODE_CREATED) &&
> > -	    (ret = reiser4_dealloc_block(znode_get_block(node), 0,
> > -					 BA_DEFER | BA_FORMATTED)))
> > +	    (ret = reiser4_dealloc_block(znode_get_block(node), 0, BA_DEFER)))
> 
> 
> Here you drop BA_FORMATTED flag for the node, which is really formatted.
> It makes a mess. Do you have more neat solution?

I've dropped BA_FORMATTED flag for one reason: if BA_DEFER is specified,
other flags are not taken into account. So it is redundant to specify those.
By the same reason 0 is already passed instead of target block stage.

Actually, only two pieces of this patch are not no-op: I've added BA_DEFER
in dealloc_tx_list() and in dealloc_wmap_actor().

If you think that BA_FORMATTED must remain despite it is unused --
then ok, so it shall be.

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

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

[Ivan Shapovalov]

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.

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/block_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 324b11c..33ca86c 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -1008,8 +1008,7 @@ reiser4_dealloc_blocks(const reiser4_block_nr * start,
 		spin_unlock_reiser4_super(sbinfo);
 	}
 
-	if ((flags & BA_DEFER) ||
-	    reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
+	if (flags & BA_DEFER) {
 		/* store deleted block numbers in the atom's deferred delete set
 		   for further actual deletion */
 		do {
-- 
2.0.4

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

[Edward Shishkin]

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..

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.

Thanks.

> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/block_alloc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> index 324b11c..33ca86c 100644
> --- a/fs/reiser4/block_alloc.c
> +++ b/fs/reiser4/block_alloc.c
> @@ -1008,8 +1008,7 @@ reiser4_dealloc_blocks(const reiser4_block_nr * start,
>   		spin_unlock_reiser4_super(sbinfo);
>   	}
>   
> -	if ((flags & BA_DEFER) ||
> -	    reiser4_is_set(reiser4_get_current_sb(), REISER4_DISCARD)) {
> +	if (flags & BA_DEFER) {
>   		/* store deleted block numbers in the atom's deferred delete set
>   		   for further actual deletion */
>   		do {

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

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

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 #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

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

[Edward Shishkin]

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.