[PATCHv2 0/3] reiser4: space grabbing fixes.

[PATCHv2 0/3] reiser4: space grabbing fixes.

[Ivan Shapovalov]

These are mostly equivalent transformations (aside from the first patch)
I've done when I was experimenting with rwsems at zero-th iteration of
batch discard support.

v2: - disable grab in current context unconditionally, not only if !BA_FORCE

Ivan Shapovalov (3):
  reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
  reiser4: block_alloc: sanitize grab_enabled modifications.
  reiser4: do not mess with grab_enabled; instead, use BA_FORCE.

 fs/reiser4/block_alloc.c                 | 57 +++++++++++++++++---------------
 fs/reiser4/plugin/file/file.c            | 12 +++----
 fs/reiser4/plugin/file/file_conversion.c |  3 +-
 fs/reiser4/plugin/file/tail_conversion.c | 11 +++---
 fs/reiser4/plugin/item/extent_file_ops.c |  3 +-
 fs/reiser4/plugin/item/tail.c            |  3 +-
 fs/reiser4/safe_link.c                   |  4 +--
 7 files changed, 44 insertions(+), 49 deletions(-)

-- 
2.1.2

[PATCHv2 1/3] reiser4: block_alloc: improve error handling in reiser4_grab_reserved().

[Ivan Shapovalov]

In case of error in reiser4_grab_space(), return the original error code,
not -ENOSPC unconditionally.

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

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 324b11c..7f9f910 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -360,37 +360,40 @@ int reiser4_grab_reserved(struct super_block *super,
 			  __u64 count, reiser4_ba_flags_t flags)
 {
 	reiser4_super_info_data *sbinfo = get_super_private(super);
+	int ret;
 
 	assert("nikita-3175", flags & BA_CAN_COMMIT);
 
 	/* Check the delete mutex already taken by us, we assume that
 	 * reading of machine word is atomic. */
 	if (sbinfo->delete_mutex_owner == current) {
-		if (reiser4_grab_space
-		    (count, (flags | BA_RESERVED) & ~BA_CAN_COMMIT)) {
+		ret = reiser4_grab_space (count, (flags | BA_RESERVED) & ~BA_CAN_COMMIT);
+
+		if (ret != 0) {
 			warning("zam-1003",
 				"nested call of grab_reserved fails count=(%llu)",
 				(unsigned long long)count);
 			reiser4_release_reserved(super);
-			return RETERR(-ENOSPC);
 		}
-		return 0;
-	}
+	} else {
+		ret = reiser4_grab_space(count, flags);
 
-	if (reiser4_grab_space(count, flags)) {
-		mutex_lock(&sbinfo->delete_mutex);
-		assert("nikita-2929", sbinfo->delete_mutex_owner == NULL);
-		sbinfo->delete_mutex_owner = current;
+		if (ret != 0) {
+			mutex_lock(&sbinfo->delete_mutex);
+			assert("nikita-2929", sbinfo->delete_mutex_owner == NULL);
+			sbinfo->delete_mutex_owner = current;
+			ret = reiser4_grab_space(count, flags | BA_RESERVED);
 
-		if (reiser4_grab_space(count, flags | BA_RESERVED)) {
-			warning("zam-833",
-				"reserved space is not enough (%llu)",
-				(unsigned long long)count);
-			reiser4_release_reserved(super);
-			return RETERR(-ENOSPC);
+			if (ret != 0) {
+				warning("zam-833",
+					"reserved space is not enough (%llu)",
+					(unsigned long long)count);
+				reiser4_release_reserved(super);
+			}
 		}
 	}
-	return 0;
+
+	return ret;
 }
 
 void reiser4_release_reserved(struct super_block *super)
-- 
2.1.2

[PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Ivan Shapovalov]

- check and modify ctx->grab_enabled in reiser4_grab_space(), not reiser4_grab()
- do not re-enable grab before doing second attempt in BA_CAN_COMMIT sequence
  (it is unneeded given the first change)

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

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index 7f9f910..9339de7 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
 
 	assert("vs-1276", ctx == get_current_context());
 
-	/* Do not grab anything on ro-mounted fs. */
-	if (rofs_super(ctx->super)) {
-		ctx->grab_enabled = 0;
-		return 0;
-	}
-
 	sbinfo = get_super_private(ctx->super);
 
 	spin_lock_reiser4_super(sbinfo);
@@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
 
 	assert("nikita-2986", reiser4_check_block_counters(ctx->super));
 
-	/* disable grab space in current context */
-	ctx->grab_enabled = 0;
-
 unlock_and_ret:
 	spin_unlock_reiser4_super(sbinfo);
 
@@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
 	if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
 		return 0;
 
+	/* Do not grab anything on ro-mounted fs. */
+	if (rofs_super(ctx->super)) {
+		ctx->grab_enabled = 0;
+		return 0;
+	}
+
 	ret = reiser4_grab(ctx, count, flags);
 	if (ret == -ENOSPC) {
 
@@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
 		   present */
 		if (flags & BA_CAN_COMMIT) {
 			txnmgr_force_commit_all(ctx->super, 0);
-			ctx->grab_enabled = 1;
 			ret = reiser4_grab(ctx, count, flags);
 		}
 	}
+
+	if (ret == 0) {
+		/* disable grab space in current context */
+		ctx->grab_enabled = 0;
+	}
+
 	/*
 	 * allocation from reserved pool cannot fail. This is severe error.
 	 */
-- 
2.1.2

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Ivan Shapovalov]

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

On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:	
> - check and modify ctx->grab_enabled in reiser4_grab_space(), not reiser4_grab()
> - do not re-enable grab before doing second attempt in BA_CAN_COMMIT sequence
>   (it is unneeded given the first change)

...testing has revealed that this is wrong. Please see below.

> 
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>  fs/reiser4/block_alloc.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> index 7f9f910..9339de7 100644
> --- a/fs/reiser4/block_alloc.c
> +++ b/fs/reiser4/block_alloc.c
> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>  
>  	assert("vs-1276", ctx == get_current_context());
>  
> -	/* Do not grab anything on ro-mounted fs. */
> -	if (rofs_super(ctx->super)) {
> -		ctx->grab_enabled = 0;
> -		return 0;
> -	}
> -
>  	sbinfo = get_super_private(ctx->super);
>  
>  	spin_lock_reiser4_super(sbinfo);
> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>  
>  	assert("nikita-2986", reiser4_check_block_counters(ctx->super));
>  
> -	/* disable grab space in current context */
> -	ctx->grab_enabled = 0;
> -
>  unlock_and_ret:
>  	spin_unlock_reiser4_super(sbinfo);
>  
> @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
>  	if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
>  		return 0;
>  
> +	/* Do not grab anything on ro-mounted fs. */
> +	if (rofs_super(ctx->super)) {
> +		ctx->grab_enabled = 0;
> +		return 0;
> +	}
> +
>  	ret = reiser4_grab(ctx, count, flags);
>  	if (ret == -ENOSPC) {
>  
> @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
>  		   present */
>  		if (flags & BA_CAN_COMMIT) {
>  			txnmgr_force_commit_all(ctx->super, 0);
> -			ctx->grab_enabled = 1;
>  			ret = reiser4_grab(ctx, count, flags);
>  		}

Here, txnmgr_force_commit_all() performs space grabbing (with BA_FORCE flag)
and as such indirectly sets ctx->grab_enabled to 0.

Initial problem.
Situation: ctx->grab_enabled == 0
Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
* first reiser4_grab() returns -ENOSPC
* txnmgr_force_commit_all() is called
* ctx->grab_enabled is set to 1 by us
* second reiser4_grab() returns -ENOSPC as well
Result: ctx->grab_enabled == 1

Problem with this patch.
Situation: ctx->grab_enabled == 1
Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
* first reiser4_grab() returns -ENOSPC
* txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
* second reiser4_grab() returns -ENOSPC as well
Result: no grab performed, but ctx->grab_enabled == 0.
 
Solution 1:
save and restore ctx->grab_enabled across call to txnmgr_force_commit_all();

Solution 2:
make txnmgr_force_commit_all() not touch ctx->grab_enabled,
i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.

What would you suggest?

-- 
Ivan Shapovalov / intelfx /

>  	}
> +
> +	if (ret == 0) {
> +		/* disable grab space in current context */
> +		ctx->grab_enabled = 0;
> +	}
> +
>  	/*
>  	 * allocation from reserved pool cannot fail. This is severe error.
>  	 */
> 

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

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Edward Shishkin]

On 11/16/2014 06:38 AM, Ivan Shapovalov wrote:
> On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:	
>> - check and modify ctx->grab_enabled in reiser4_grab_space(), not reiser4_grab()
>> - do not re-enable grab before doing second attempt in BA_CAN_COMMIT sequence
>>    (it is unneeded given the first change)
> ...testing has revealed that this is wrong. Please see below.
>
>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>> ---
>>   fs/reiser4/block_alloc.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
>> index 7f9f910..9339de7 100644
>> --- a/fs/reiser4/block_alloc.c
>> +++ b/fs/reiser4/block_alloc.c
>> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>>   
>>   	assert("vs-1276", ctx == get_current_context());
>>   
>> -	/* Do not grab anything on ro-mounted fs. */
>> -	if (rofs_super(ctx->super)) {
>> -		ctx->grab_enabled = 0;
>> -		return 0;
>> -	}
>> -
>>   	sbinfo = get_super_private(ctx->super);
>>   
>>   	spin_lock_reiser4_super(sbinfo);
>> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, reiser4_ba_flags_t flags)
>>   
>>   	assert("nikita-2986", reiser4_check_block_counters(ctx->super));
>>   
>> -	/* disable grab space in current context */
>> -	ctx->grab_enabled = 0;
>> -
>>   unlock_and_ret:
>>   	spin_unlock_reiser4_super(sbinfo);
>>   
>> @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
>>   	if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
>>   		return 0;
>>   
>> +	/* Do not grab anything on ro-mounted fs. */
>> +	if (rofs_super(ctx->super)) {
>> +		ctx->grab_enabled = 0;
>> +		return 0;
>> +	}
>> +
>>   	ret = reiser4_grab(ctx, count, flags);
>>   	if (ret == -ENOSPC) {
>>   
>> @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
>>   		   present */
>>   		if (flags & BA_CAN_COMMIT) {
>>   			txnmgr_force_commit_all(ctx->super, 0);
>> -			ctx->grab_enabled = 1;
>>   			ret = reiser4_grab(ctx, count, flags);
>>   		}
> Here, txnmgr_force_commit_all() performs space grabbing (with BA_FORCE flag)
> and as such indirectly sets ctx->grab_enabled to 0.
>
> Initial problem.
> Situation: ctx->grab_enabled == 0
> Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
> * first reiser4_grab() returns -ENOSPC
> * txnmgr_force_commit_all() is called
> * ctx->grab_enabled is set to 1 by us
> * second reiser4_grab() returns -ENOSPC as well
> Result: ctx->grab_enabled == 1
>
> Problem with this patch.
> Situation: ctx->grab_enabled == 1
> Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
> * first reiser4_grab() returns -ENOSPC
> * txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
> * second reiser4_grab() returns -ENOSPC as well
> Result: no grab performed, but ctx->grab_enabled == 0.
>   
> Solution 1:
> save and restore ctx->grab_enabled across call to txnmgr_force_commit_all();
>
> Solution 2:
> make txnmgr_force_commit_all() not touch ctx->grab_enabled,
> i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.
>
> What would you suggest?


Could you please remind, what problem
this patch series (sanitize grab_enabled) fixes?

Thanks,
Edward.


>

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Edward Shishkin]

On 11/16/2014 10:53 AM, Edward Shishkin wrote:
>
> On 11/16/2014 06:38 AM, Ivan Shapovalov wrote:
>> On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:
>>> - check and modify ctx->grab_enabled in reiser4_grab_space(), not 
>>> reiser4_grab()
>>> - do not re-enable grab before doing second attempt in BA_CAN_COMMIT 
>>> sequence
>>>    (it is unneeded given the first change)
>> ...testing has revealed that this is wrong. Please see below.
>>
>>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>>> ---
>>>   fs/reiser4/block_alloc.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
>>> index 7f9f910..9339de7 100644
>>> --- a/fs/reiser4/block_alloc.c
>>> +++ b/fs/reiser4/block_alloc.c
>>> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 
>>> count, reiser4_ba_flags_t flags)
>>>         assert("vs-1276", ctx == get_current_context());
>>>   -    /* Do not grab anything on ro-mounted fs. */
>>> -    if (rofs_super(ctx->super)) {
>>> -        ctx->grab_enabled = 0;
>>> -        return 0;
>>> -    }
>>> -
>>>       sbinfo = get_super_private(ctx->super);
>>>         spin_lock_reiser4_super(sbinfo);
>>> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, 
>>> reiser4_ba_flags_t flags)
>>>         assert("nikita-2986", 
>>> reiser4_check_block_counters(ctx->super));
>>>   -    /* disable grab space in current context */
>>> -    ctx->grab_enabled = 0;
>>> -
>>>   unlock_and_ret:
>>>       spin_unlock_reiser4_super(sbinfo);
>>>   @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, 
>>> reiser4_ba_flags_t flags)
>>>       if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
>>>           return 0;
>>>   +    /* Do not grab anything on ro-mounted fs. */
>>> +    if (rofs_super(ctx->super)) {
>>> +        ctx->grab_enabled = 0;
>>> +        return 0;
>>> +    }
>>> +
>>>       ret = reiser4_grab(ctx, count, flags);
>>>       if (ret == -ENOSPC) {
>>>   @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, 
>>> reiser4_ba_flags_t flags)
>>>              present */
>>>           if (flags & BA_CAN_COMMIT) {
>>>               txnmgr_force_commit_all(ctx->super, 0);
>>> -            ctx->grab_enabled = 1;
>>>               ret = reiser4_grab(ctx, count, flags);
>>>           }
>> Here, txnmgr_force_commit_all() performs space grabbing (with 
>> BA_FORCE flag)
>> and as such indirectly sets ctx->grab_enabled to 0.
>>
>> Initial problem.
>> Situation: ctx->grab_enabled == 0
>> Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
>> * first reiser4_grab() returns -ENOSPC
>> * txnmgr_force_commit_all() is called
>> * ctx->grab_enabled is set to 1 by us
>> * second reiser4_grab() returns -ENOSPC as well
>> Result: ctx->grab_enabled == 1
>>
>> Problem with this patch.
>> Situation: ctx->grab_enabled == 1
>> Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
>> * first reiser4_grab() returns -ENOSPC
>> * txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
>> * second reiser4_grab() returns -ENOSPC as well
>> Result: no grab performed, but ctx->grab_enabled == 0.
>>   Solution 1:
>> save and restore ctx->grab_enabled across call to 
>> txnmgr_force_commit_all();
>>
>> Solution 2:
>> make txnmgr_force_commit_all() not touch ctx->grab_enabled,
>> i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.
>>
>> What would you suggest?
>
>
> Could you please remind, what problem
> this patch series (sanitize grab_enabled) fixes?


To be clear, I forgot why I included this patch series.
What is the system crash, or disk space leak, or something else..?

Edward.

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Edward Shishkin]

On 11/16/2014 11:45 AM, Edward Shishkin wrote:
>
> On 11/16/2014 10:53 AM, Edward Shishkin wrote:
>>
>> On 11/16/2014 06:38 AM, Ivan Shapovalov wrote:
>>> On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:
>>>> - check and modify ctx->grab_enabled in reiser4_grab_space(), not 
>>>> reiser4_grab()
>>>> - do not re-enable grab before doing second attempt in 
>>>> BA_CAN_COMMIT sequence
>>>>    (it is unneeded given the first change)
>>> ...testing has revealed that this is wrong. Please see below.
>>>
>>>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>>>> ---
>>>>   fs/reiser4/block_alloc.c | 22 ++++++++++++----------
>>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
>>>> index 7f9f910..9339de7 100644
>>>> --- a/fs/reiser4/block_alloc.c
>>>> +++ b/fs/reiser4/block_alloc.c
>>>> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 
>>>> count, reiser4_ba_flags_t flags)
>>>>         assert("vs-1276", ctx == get_current_context());
>>>>   -    /* Do not grab anything on ro-mounted fs. */
>>>> -    if (rofs_super(ctx->super)) {
>>>> -        ctx->grab_enabled = 0;
>>>> -        return 0;
>>>> -    }
>>>> -
>>>>       sbinfo = get_super_private(ctx->super);
>>>>         spin_lock_reiser4_super(sbinfo);
>>>> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 
>>>> count, reiser4_ba_flags_t flags)
>>>>         assert("nikita-2986", 
>>>> reiser4_check_block_counters(ctx->super));
>>>>   -    /* disable grab space in current context */
>>>> -    ctx->grab_enabled = 0;
>>>> -
>>>>   unlock_and_ret:
>>>>       spin_unlock_reiser4_super(sbinfo);
>>>>   @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, 
>>>> reiser4_ba_flags_t flags)
>>>>       if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
>>>>           return 0;
>>>>   +    /* Do not grab anything on ro-mounted fs. */
>>>> +    if (rofs_super(ctx->super)) {
>>>> +        ctx->grab_enabled = 0;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>>       ret = reiser4_grab(ctx, count, flags);
>>>>       if (ret == -ENOSPC) {
>>>>   @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, 
>>>> reiser4_ba_flags_t flags)
>>>>              present */
>>>>           if (flags & BA_CAN_COMMIT) {
>>>>               txnmgr_force_commit_all(ctx->super, 0);
>>>> -            ctx->grab_enabled = 1;
>>>>               ret = reiser4_grab(ctx, count, flags);
>>>>           }
>>> Here, txnmgr_force_commit_all() performs space grabbing (with 
>>> BA_FORCE flag)
>>> and as such indirectly sets ctx->grab_enabled to 0.
>>>
>>> Initial problem.
>>> Situation: ctx->grab_enabled == 0
>>> Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
>>> * first reiser4_grab() returns -ENOSPC
>>> * txnmgr_force_commit_all() is called
>>> * ctx->grab_enabled is set to 1 by us
>>> * second reiser4_grab() returns -ENOSPC as well
>>> Result: ctx->grab_enabled == 1
>>>
>>> Problem with this patch.
>>> Situation: ctx->grab_enabled == 1
>>> Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
>>> * first reiser4_grab() returns -ENOSPC
>>> * txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
>>> * second reiser4_grab() returns -ENOSPC as well
>>> Result: no grab performed, but ctx->grab_enabled == 0.
>>>   Solution 1:
>>> save and restore ctx->grab_enabled across call to 
>>> txnmgr_force_commit_all();
>>>
>>> Solution 2:
>>> make txnmgr_force_commit_all() not touch ctx->grab_enabled,
>>> i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.
>>>
>>> What would you suggest?
>>
>>
>> Could you please remind, what problem
>> this patch series (sanitize grab_enabled) fixes?
>
>
> To be clear, I forgot why I included this patch series.
> What is

^What is^Was it

> the system crash, or disk space leak, or something else..?
>
> Edward.

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Edward Shishkin]

On 11/16/2014 10:53 AM, Edward Shishkin wrote:
>
> On 11/16/2014 06:38 AM, Ivan Shapovalov wrote:
>> On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:
>>> - check and modify ctx->grab_enabled in reiser4_grab_space(), not 
>>> reiser4_grab()
>>> - do not re-enable grab before doing second attempt in BA_CAN_COMMIT 
>>> sequence
>>>    (it is unneeded given the first change)
>> ...testing has revealed that this is wrong. Please see below.
>>
>>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>>> ---
>>>   fs/reiser4/block_alloc.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
>>> index 7f9f910..9339de7 100644
>>> --- a/fs/reiser4/block_alloc.c
>>> +++ b/fs/reiser4/block_alloc.c
>>> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 
>>> count, reiser4_ba_flags_t flags)
>>>         assert("vs-1276", ctx == get_current_context());
>>>   -    /* Do not grab anything on ro-mounted fs. */
>>> -    if (rofs_super(ctx->super)) {
>>> -        ctx->grab_enabled = 0;
>>> -        return 0;
>>> -    }
>>> -
>>>       sbinfo = get_super_private(ctx->super);
>>>         spin_lock_reiser4_super(sbinfo);
>>> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, 
>>> reiser4_ba_flags_t flags)
>>>         assert("nikita-2986", 
>>> reiser4_check_block_counters(ctx->super));
>>>   -    /* disable grab space in current context */
>>> -    ctx->grab_enabled = 0;
>>> -
>>>   unlock_and_ret:
>>>       spin_unlock_reiser4_super(sbinfo);
>>>   @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, 
>>> reiser4_ba_flags_t flags)
>>>       if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
>>>           return 0;
>>>   +    /* Do not grab anything on ro-mounted fs. */
>>> +    if (rofs_super(ctx->super)) {
>>> +        ctx->grab_enabled = 0;
>>> +        return 0;
>>> +    }
>>> +
>>>       ret = reiser4_grab(ctx, count, flags);
>>>       if (ret == -ENOSPC) {
>>>   @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, 
>>> reiser4_ba_flags_t flags)
>>>              present */
>>>           if (flags & BA_CAN_COMMIT) {
>>>               txnmgr_force_commit_all(ctx->super, 0);
>>> -            ctx->grab_enabled = 1;
>>>               ret = reiser4_grab(ctx, count, flags);
>>>           }
>> Here, txnmgr_force_commit_all() performs space grabbing (with 
>> BA_FORCE flag)
>> and as such indirectly sets ctx->grab_enabled to 0.
>>
>> Initial problem.
>> Situation: ctx->grab_enabled == 0
>> Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
>> * first reiser4_grab() returns -ENOSPC
>> * txnmgr_force_commit_all() is called
>> * ctx->grab_enabled is set to 1 by us
>> * second reiser4_grab() returns -ENOSPC as well
>> Result: ctx->grab_enabled == 1
>>
>> Problem with this patch.
>> Situation: ctx->grab_enabled == 1
>> Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
>> * first reiser4_grab() returns -ENOSPC
>> * txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
>> * second reiser4_grab() returns -ENOSPC as well
>> Result: no grab performed, but ctx->grab_enabled == 0.
>>   Solution 1:
>> save and restore ctx->grab_enabled across call to 
>> txnmgr_force_commit_all();
>>
>> Solution 2:
>> make txnmgr_force_commit_all() not touch ctx->grab_enabled,
>> i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.
>>
>> What would you suggest?
>
>
> Could you please remind, what problem
> this patch series (sanitize grab_enabled) fixes?


To be clear, I forgot why I included this patch series.
What is the system crash, or disk space leak, or something else..?

Edward.

Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications.

[Ivan Shapovalov]

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

On Sunday 16 November 2014 at 11:45:47, Edward Shishkin wrote:	
> 
> On 11/16/2014 10:53 AM, Edward Shishkin wrote:
> >
> > On 11/16/2014 06:38 AM, Ivan Shapovalov wrote:
> >> On Thursday 23 October 2014 at 11:18:04, Ivan Shapovalov wrote:
> >>> - check and modify ctx->grab_enabled in reiser4_grab_space(), not 
> >>> reiser4_grab()
> >>> - do not re-enable grab before doing second attempt in BA_CAN_COMMIT 
> >>> sequence
> >>>    (it is unneeded given the first change)
> >> ...testing has revealed that this is wrong. Please see below.
> >>
> >>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> >>> ---
> >>>   fs/reiser4/block_alloc.c | 22 ++++++++++++----------
> >>>   1 file changed, 12 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
> >>> index 7f9f910..9339de7 100644
> >>> --- a/fs/reiser4/block_alloc.c
> >>> +++ b/fs/reiser4/block_alloc.c
> >>> @@ -270,12 +270,6 @@ reiser4_grab(reiser4_context * ctx, __u64 
> >>> count, reiser4_ba_flags_t flags)
> >>>         assert("vs-1276", ctx == get_current_context());
> >>>   -    /* Do not grab anything on ro-mounted fs. */
> >>> -    if (rofs_super(ctx->super)) {
> >>> -        ctx->grab_enabled = 0;
> >>> -        return 0;
> >>> -    }
> >>> -
> >>>       sbinfo = get_super_private(ctx->super);
> >>>         spin_lock_reiser4_super(sbinfo);
> >>> @@ -300,9 +294,6 @@ reiser4_grab(reiser4_context * ctx, __u64 count, 
> >>> reiser4_ba_flags_t flags)
> >>>         assert("nikita-2986", 
> >>> reiser4_check_block_counters(ctx->super));
> >>>   -    /* disable grab space in current context */
> >>> -    ctx->grab_enabled = 0;
> >>> -
> >>>   unlock_and_ret:
> >>>       spin_unlock_reiser4_super(sbinfo);
> >>>   @@ -321,6 +312,12 @@ int reiser4_grab_space(__u64 count, 
> >>> reiser4_ba_flags_t flags)
> >>>       if (!(flags & BA_FORCE) && !is_grab_enabled(ctx))
> >>>           return 0;
> >>>   +    /* Do not grab anything on ro-mounted fs. */
> >>> +    if (rofs_super(ctx->super)) {
> >>> +        ctx->grab_enabled = 0;
> >>> +        return 0;
> >>> +    }
> >>> +
> >>>       ret = reiser4_grab(ctx, count, flags);
> >>>       if (ret == -ENOSPC) {
> >>>   @@ -328,10 +325,15 @@ int reiser4_grab_space(__u64 count, 
> >>> reiser4_ba_flags_t flags)
> >>>              present */
> >>>           if (flags & BA_CAN_COMMIT) {
> >>>               txnmgr_force_commit_all(ctx->super, 0);
> >>> -            ctx->grab_enabled = 1;
> >>>               ret = reiser4_grab(ctx, count, flags);
> >>>           }
> >> Here, txnmgr_force_commit_all() performs space grabbing (with 
> >> BA_FORCE flag)
> >> and as such indirectly sets ctx->grab_enabled to 0.
> >>
> >> Initial problem.
> >> Situation: ctx->grab_enabled == 0
> >> Call: reiser4_grab_space(toomany, BA_FORCE | BA_CAN_COMMIT)
> >> * first reiser4_grab() returns -ENOSPC
> >> * txnmgr_force_commit_all() is called
> >> * ctx->grab_enabled is set to 1 by us
> >> * second reiser4_grab() returns -ENOSPC as well
> >> Result: ctx->grab_enabled == 1
> >>
> >> Problem with this patch.
> >> Situation: ctx->grab_enabled == 1
> >> Call: reiser4_grab_space(toomany, BA_CAN_COMMIT)
> >> * first reiser4_grab() returns -ENOSPC
> >> * txnmgr_force_commit_all() is called and sets ctx->grab_enabled to 0
> >> * second reiser4_grab() returns -ENOSPC as well
> >> Result: no grab performed, but ctx->grab_enabled == 0.
> >>   Solution 1:
> >> save and restore ctx->grab_enabled across call to 
> >> txnmgr_force_commit_all();
> >>
> >> Solution 2:
> >> make txnmgr_force_commit_all() not touch ctx->grab_enabled,
> >> i. e. set ctx->grab_enabled to 0 only if !BA_FORCE.
> >>
> >> What would you suggest?
> >
> >
> > Could you please remind, what problem
> > this patch series (sanitize grab_enabled) fixes?
> 
> 
> To be clear, I forgot why I included this patch series.

This patch series hasn't yet been merged, has it?

> What is the system crash, or disk space leak, or something else..?

There is no particular problem. You can think it's a kind of "refactoring".

-- 
Ivan Shapovalov / intelfx /

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

[PATCHv2 3/3] reiser4: do not mess with grab_enabled; instead, use BA_FORCE.

[Ivan Shapovalov]

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/plugin/file/file.c            | 12 ++++--------
 fs/reiser4/plugin/file/file_conversion.c |  3 +--
 fs/reiser4/plugin/file/tail_conversion.c | 11 +++++------
 fs/reiser4/plugin/item/extent_file_ops.c |  3 +--
 fs/reiser4/plugin/item/tail.c            |  3 +--
 fs/reiser4/safe_link.c                   |  4 +---
 6 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/fs/reiser4/plugin/file/file.c b/fs/reiser4/plugin/file/file.c
index 94029cd..b0bfa86 100644
--- a/fs/reiser4/plugin/file/file.c
+++ b/fs/reiser4/plugin/file/file.c
@@ -335,11 +335,10 @@ static int find_file_state(struct inode *inode, struct unix_file_info *uf_info)
  */
 static int reserve_partial_page(reiser4_tree * tree)
 {
-	grab_space_enable();
 	return reiser4_grab_reserved(reiser4_get_current_sb(),
 				     1 +
 				     2 * estimate_one_insert_into_item(tree),
-				     BA_CAN_COMMIT);
+				     BA_CAN_COMMIT | BA_FORCE);
 }
 
 /* estimate and reserve space needed to cut one item and update one stat data */
@@ -350,11 +349,10 @@ static int reserve_cut_iteration(reiser4_tree * tree)
 
 	assert("nikita-3172", lock_stack_isclean(get_current_lock_stack()));
 
-	grab_space_enable();
 	/* We need to double our estimate now that we can delete more than one
 	   node. */
 	return reiser4_grab_reserved(reiser4_get_current_sb(), estimate * 2,
-				     BA_CAN_COMMIT);
+				     BA_CAN_COMMIT | BA_FORCE);
 }
 
 int reiser4_update_file_size(struct inode *inode, loff_t new_size,
@@ -875,10 +873,9 @@ static int capture_page_and_create_extent(struct page *page)
 	/* page capture may require extent creation (if it does not exist yet)
 	   and stat data's update (number of blocks changes on extent
 	   creation) */
-	grab_space_enable();
 	result = reiser4_grab_space(2 * estimate_one_insert_into_item
 				    (reiser4_tree_by_inode(inode)),
-				    BA_CAN_COMMIT);
+				    BA_CAN_COMMIT | BA_FORCE);
 	if (likely(!result))
 		result = find_or_create_extent(page);
 
@@ -2421,9 +2418,8 @@ static int unpack(struct file *filp, struct inode *inode, int forever)
 
 		set_file_notail(inode);
 
-		grab_space_enable();
 		tograb = inode_file_plugin(inode)->estimate.update(inode);
-		result = reiser4_grab_space(tograb, BA_CAN_COMMIT);
+		result = reiser4_grab_space(tograb, BA_CAN_COMMIT | BA_FORCE);
 		result = reiser4_update_sd(inode);
 	}
 
diff --git a/fs/reiser4/plugin/file/file_conversion.c b/fs/reiser4/plugin/file/file_conversion.c
index 3ce4898..e01720f 100644
--- a/fs/reiser4/plugin/file/file_conversion.c
+++ b/fs/reiser4/plugin/file/file_conversion.c
@@ -388,14 +388,13 @@ static int reserve_cryptcompress2unixfile(struct inode *inode)
 	 *     5. possible update of stat-data
 	 *
 	 */
-	grab_space_enable();
 	return reiser4_grab_space
 		(2 * tree->height +
 		 unformatted_nodes  +
 		 unformatted_nodes * estimate_one_insert_into_item(tree) +
 		 1 + estimate_one_insert_item(tree) +
 		 inode_file_plugin(inode)->estimate.update(inode),
-		 BA_CAN_COMMIT);
+		 BA_CAN_COMMIT | BA_FORCE);
 }
 
 /**
diff --git a/fs/reiser4/plugin/file/tail_conversion.c b/fs/reiser4/plugin/file/tail_conversion.c
index 1763cb1..22f30fd 100644
--- a/fs/reiser4/plugin/file/tail_conversion.c
+++ b/fs/reiser4/plugin/file/tail_conversion.c
@@ -221,13 +221,13 @@ static int reserve_tail2extent_iteration(struct inode *inode)
 	 *     5. possible update of stat-data
 	 *
 	 */
-	grab_space_enable();
 	return reiser4_grab_space
 	    (2 * tree->height +
 	     TAIL2EXTENT_PAGE_NUM +
 	     TAIL2EXTENT_PAGE_NUM * estimate_one_insert_into_item(tree) +
 	     1 + estimate_one_insert_item(tree) +
-	     inode_file_plugin(inode)->estimate.update(inode), BA_CAN_COMMIT);
+	     inode_file_plugin(inode)->estimate.update(inode),
+	     BA_CAN_COMMIT | BA_FORCE);
 }
 
 /* clear stat data's flag indicating that conversion is being converted */
@@ -235,10 +235,9 @@ static int complete_conversion(struct inode *inode)
 {
 	int result;
 
-	grab_space_enable();
 	result =
 	    reiser4_grab_space(inode_file_plugin(inode)->estimate.update(inode),
-			       BA_CAN_COMMIT);
+			       BA_CAN_COMMIT | BA_FORCE);
 	if (result == 0) {
 		reiser4_inode_clr_flag(inode, REISER4_PART_MIXED);
 		result = reiser4_update_sd(inode);
@@ -552,12 +551,12 @@ static int reserve_extent2tail_iteration(struct inode *inode)
 	 *
 	 *     4. possible update of stat-data
 	 */
-	grab_space_enable();
 	return reiser4_grab_space
 	    (estimate_one_item_removal(tree) +
 	     estimate_insert_flow(tree->height) +
 	     1 + estimate_one_insert_item(tree) +
-	     inode_file_plugin(inode)->estimate.update(inode), BA_CAN_COMMIT);
+	     inode_file_plugin(inode)->estimate.update(inode),
+	     BA_CAN_COMMIT | BA_FORCE);
 }
 
 /* for every page of file: read page, cut part of extent pointing to this page,
diff --git a/fs/reiser4/plugin/item/extent_file_ops.c b/fs/reiser4/plugin/item/extent_file_ops.c
index cb57379..8086b5f 100644
--- a/fs/reiser4/plugin/item/extent_file_ops.c
+++ b/fs/reiser4/plugin/item/extent_file_ops.c
@@ -933,8 +933,7 @@ static int write_extent_reserve_space(struct inode *inode)
 	count = estimate_one_insert_item(tree) +
 		WRITE_GRANULARITY * (1 + estimate_one_insert_into_item(tree)) +
 		estimate_one_insert_item(tree);
-	grab_space_enable();
-	return reiser4_grab_space(count, 0 /* flags */);
+	return reiser4_grab_space(count, BA_FORCE);
 }
 
 /*
diff --git a/fs/reiser4/plugin/item/tail.c b/fs/reiser4/plugin/item/tail.c
index 7f4a101..67cb4ba 100644
--- a/fs/reiser4/plugin/item/tail.c
+++ b/fs/reiser4/plugin/item/tail.c
@@ -603,8 +603,7 @@ static int write_extent_reserve_space(struct inode *inode)
 	count = estimate_one_insert_item(tree) +
 		estimate_insert_flow(tree->height) +
 		estimate_one_insert_item(tree);
-	grab_space_enable();
-	return reiser4_grab_space(count, 0 /* flags */);
+	return reiser4_grab_space(count, BA_FORCE);
 }
 
 #define PAGE_PER_FLOW 4
diff --git a/fs/reiser4/safe_link.c b/fs/reiser4/safe_link.c
index d59f6f0..f4cff69 100644
--- a/fs/reiser4/safe_link.c
+++ b/fs/reiser4/safe_link.c
@@ -130,13 +130,11 @@ int safe_link_grab(reiser4_tree * tree, reiser4_ba_flags_t flags)
 {
 	int result;
 
-	grab_space_enable();
 	/* The sbinfo->delete_mutex can be taken here.
 	 * safe_link_release() should be called before leaving reiser4
 	 * context. */
 	result =
-	    reiser4_grab_reserved(tree->super, safe_link_tograb(tree), flags);
-	grab_space_enable();
+	    reiser4_grab_reserved(tree->super, safe_link_tograb(tree), flags | BA_FORCE);
 	return result;
 }
 
-- 
2.1.2

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Ivan Shapovalov]

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

On Thursday 23 October 2014 at 11:18:02, Ivan Shapovalov wrote:	
> [...]
> 
> Ivan Shapovalov (3):
>   reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
>   reiser4: block_alloc: sanitize grab_enabled modifications.
>   reiser4: do not mess with grab_enabled; instead, use BA_FORCE.
> 
> [...]

This is a diff between v1 and v2, for ease of reviewing.

diff --git a/fs/reiser4/block_alloc.c b/fs/reiser4/block_alloc.c
index a30f7b9..9339de7 100644
--- a/fs/reiser4/block_alloc.c
+++ b/fs/reiser4/block_alloc.c
@@ -329,7 +329,7 @@ int reiser4_grab_space(__u64 count, reiser4_ba_flags_t flags)
 		}
 	}
 
-	if (!(flags & BA_FORCE) && (ret == 0)) {
+	if (ret == 0) {
 		/* disable grab space in current context */
 		ctx->grab_enabled = 0;
 	}

-- 
Ivan Shapovalov / intelfx /

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

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Ivan Shapovalov]

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

On Thursday 23 October 2014 at 11:18:02, Ivan Shapovalov wrote:	
> These are mostly equivalent transformations (aside from the first patch)
> I've done when I was experimenting with rwsems at zero-th iteration of
> batch discard support.
> 
> v2: - disable grab in current context unconditionally, not only if !BA_FORCE
> 
> Ivan Shapovalov (3):
>   reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
>   reiser4: block_alloc: sanitize grab_enabled modifications.
>   reiser4: do not mess with grab_enabled; instead, use BA_FORCE.
> 
>  fs/reiser4/block_alloc.c                 | 57 +++++++++++++++++---------------
>  fs/reiser4/plugin/file/file.c            | 12 +++----
>  fs/reiser4/plugin/file/file_conversion.c |  3 +-
>  fs/reiser4/plugin/file/tail_conversion.c | 11 +++---
>  fs/reiser4/plugin/item/extent_file_ops.c |  3 +-
>  fs/reiser4/plugin/item/tail.c            |  3 +-
>  fs/reiser4/safe_link.c                   |  4 +--
>  7 files changed, 44 insertions(+), 49 deletions(-)

ping, what's the status?

/*
I'll send another round of discard patches shortly... That is, once I figure
out how to explain in comments the sheer amount of happily coinciding subtle
details, whose coincidence makes everything actually work. :/

BTW, could you please check that I've rebased your "precise discard" patch
correctly?
https://github.com/intelfx/linux/commit/47f27446d5ae7b796a842735811c48cc07615dd6
*/

-- 
Ivan Shapovalov / intelfx /

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

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Edward Shishkin]

On 12/10/2014 01:27 PM, Ivan Shapovalov wrote:
> On Thursday 23 October 2014 at 11:18:02, Ivan Shapovalov wrote:	
>> These are mostly equivalent transformations (aside from the first patch)
>> I've done when I was experimenting with rwsems at zero-th iteration of
>> batch discard support.
>>
>> v2: - disable grab in current context unconditionally, not only if !BA_FORCE
>>
>> Ivan Shapovalov (3):
>>    reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
>>    reiser4: block_alloc: sanitize grab_enabled modifications.
>>    reiser4: do not mess with grab_enabled; instead, use BA_FORCE.
>>
>>   fs/reiser4/block_alloc.c                 | 57 +++++++++++++++++---------------
>>   fs/reiser4/plugin/file/file.c            | 12 +++----
>>   fs/reiser4/plugin/file/file_conversion.c |  3 +-
>>   fs/reiser4/plugin/file/tail_conversion.c | 11 +++---
>>   fs/reiser4/plugin/item/extent_file_ops.c |  3 +-
>>   fs/reiser4/plugin/item/tail.c            |  3 +-
>>   fs/reiser4/safe_link.c                   |  4 +--
>>   7 files changed, 44 insertions(+), 49 deletions(-)
> ping, what's the status?


This is in "upstream" already.


>
> /*
> I'll send another round of discard patches shortly... That is, once I figure
> out how to explain in comments the sheer amount of happily coinciding subtle
> details, whose coincidence makes everything actually work. :/


Note, that basic discard stuff is also in "upstream".
So, please, make sure that your patches are "re-based"


>
> BTW, could you please check that I've rebased your "precise discard" patch
> correctly?
> https://github.com/intelfx/linux/commit/47f27446d5ae7b796a842735811c48cc07615dd6
> */
>


I suggest to start with implementing the bitmap primitives that
I talked about. Just to avoid extra-work..

Thanks,
Edward.

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Ivan Shapovalov]

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

On Wednesday 10 December 2014 at 13:52:08, Edward Shishkin wrote:	
> 
> On 12/10/2014 01:27 PM, Ivan Shapovalov wrote:
> > On Thursday 23 October 2014 at 11:18:02, Ivan Shapovalov wrote:	
> >> These are mostly equivalent transformations (aside from the first patch)
> >> I've done when I was experimenting with rwsems at zero-th iteration of
> >> batch discard support.
> >>
> >> v2: - disable grab in current context unconditionally, not only if !BA_FORCE
> >>
> >> Ivan Shapovalov (3):
> >>    reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
> >>    reiser4: block_alloc: sanitize grab_enabled modifications.
> >>    reiser4: do not mess with grab_enabled; instead, use BA_FORCE.
> >>
> >>   fs/reiser4/block_alloc.c                 | 57 +++++++++++++++++---------------
> >>   fs/reiser4/plugin/file/file.c            | 12 +++----
> >>   fs/reiser4/plugin/file/file_conversion.c |  3 +-
> >>   fs/reiser4/plugin/file/tail_conversion.c | 11 +++---
> >>   fs/reiser4/plugin/item/extent_file_ops.c |  3 +-
> >>   fs/reiser4/plugin/item/tail.c            |  3 +-
> >>   fs/reiser4/safe_link.c                   |  4 +--
> >>   7 files changed, 44 insertions(+), 49 deletions(-)
> > ping, what's the status?
> 
> 
> This is in "upstream" already.

Hm.. I didn't see it..

Well, then a follow-up fix should be applied, as described in one of previous
messages, or this should be reverted.

> > /*
> > I'll send another round of discard patches shortly... That is, once I figure
> > out how to explain in comments the sheer amount of happily coinciding subtle
> > details, whose coincidence makes everything actually work. :/
> 
> 
> Note, that basic discard stuff is also in "upstream".
> So, please, make sure that your patches are "re-based"

Yes, I'll ensure that everything can be applied cleanly.

> > BTW, could you please check that I've rebased your "precise discard" patch
> > correctly?
> > https://github.com/intelfx/linux/commit/47f27446d5ae7b796a842735811c48cc07615dd6
> > */
> >
> 
> 
> I suggest to start with implementing the bitmap primitives that
> I talked about. Just to avoid extra-work..

If I understand you correctly, then I've already done that.

But let's first deal with this (current) patchset; it's broken. Sorry for
creating more problems than solving.

Thanks,
-- 
Ivan Shapovalov / intelfx /

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

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Edward Shishkin]

On 12/10/2014 02:30 PM, Ivan Shapovalov wrote:
> On Wednesday 10 December 2014 at 13:52:08, Edward Shishkin wrote:	
>> On 12/10/2014 01:27 PM, Ivan Shapovalov wrote:
>>> On Thursday 23 October 2014 at 11:18:02, Ivan Shapovalov wrote:	
>>>> These are mostly equivalent transformations (aside from the first patch)
>>>> I've done when I was experimenting with rwsems at zero-th iteration of
>>>> batch discard support.
>>>>
>>>> v2: - disable grab in current context unconditionally, not only if !BA_FORCE
>>>>
>>>> Ivan Shapovalov (3):
>>>>     reiser4: block_alloc: improve error handling in reiser4_grab_reserved().
>>>>     reiser4: block_alloc: sanitize grab_enabled modifications.
>>>>     reiser4: do not mess with grab_enabled; instead, use BA_FORCE.
>>>>
>>>>    fs/reiser4/block_alloc.c                 | 57 +++++++++++++++++---------------
>>>>    fs/reiser4/plugin/file/file.c            | 12 +++----
>>>>    fs/reiser4/plugin/file/file_conversion.c |  3 +-
>>>>    fs/reiser4/plugin/file/tail_conversion.c | 11 +++---
>>>>    fs/reiser4/plugin/item/extent_file_ops.c |  3 +-
>>>>    fs/reiser4/plugin/item/tail.c            |  3 +-
>>>>    fs/reiser4/safe_link.c                   |  4 +--
>>>>    7 files changed, 44 insertions(+), 49 deletions(-)
>>> ping, what's the status?
>>
>> This is in "upstream" already.
> Hm.. I didn't see it..
>
> Well, then a follow-up fix should be applied, as described in one of previous
> messages, or this should be reverted.


With that patch things looked better and you promised
"equivalent transform". OK, I'll roll it back then.

Thanks,
Edward.


>
>>> /*
>>> I'll send another round of discard patches shortly... That is, once I figure
>>> out how to explain in comments the sheer amount of happily coinciding subtle
>>> details, whose coincidence makes everything actually work. :/
>>
>> Note, that basic discard stuff is also in "upstream".
>> So, please, make sure that your patches are "re-based"
> Yes, I'll ensure that everything can be applied cleanly.
>
>>> BTW, could you please check that I've rebased your "precise discard" patch
>>> correctly?
>>> https://github.com/intelfx/linux/commit/47f27446d5ae7b796a842735811c48cc07615dd6
>>> */
>>>
>>
>> I suggest to start with implementing the bitmap primitives that
>> I talked about. Just to avoid extra-work..
> If I understand you correctly, then I've already done that.
>
> But let's first deal with this (current) patchset; it's broken. Sorry for
> creating more problems than solving.
>
> Thanks,

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Ivan Shapovalov]

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

On Wednesday 10 December 2014 at 22:51:41, Edward Shishkin wrote:	
> 
> On 12/10/2014 02:30 PM, Ivan Shapovalov wrote:
> > On Wednesday 10 December 2014 at 13:52:08, Edward Shishkin wrote:	
> >> On 12/10/2014 01:27 PM, Ivan Shapovalov wrote:
> >>> [...]
> >>> ping, what's the status?
> >>
> >> This is in "upstream" already.
> > Hm.. I didn't see it..
> >
> > Well, then a follow-up fix should be applied, as described in one of previous
> > messages, or this should be reverted.
> 
> 
> With that patch things looked better and you promised
> "equivalent transform". OK, I'll roll it back then.

Yes, that was a mistake. It's actually not an equivalent transformation.
And I need to think what to do with this, because current code looks messy,
but things are apparently so fragile...

Are there any documentation on the "space grabbing" subsystem? For example,
there's some flush code which grabs reserved space _without_ taking the mutex..

Thanks,
--
Ivan Shapovalov / intelfx /

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

Re: [PATCHv2 0/3] reiser4: space grabbing fixes.

[Edward Shishkin]

On 12/12/2014 11:19 PM, Ivan Shapovalov wrote:
> On Wednesday 10 December 2014 at 22:51:41, Edward Shishkin wrote:	
>> On 12/10/2014 02:30 PM, Ivan Shapovalov wrote:
>>> On Wednesday 10 December 2014 at 13:52:08, Edward Shishkin wrote:	
>>>> On 12/10/2014 01:27 PM, Ivan Shapovalov wrote:
>>>>> [...]
>>>>> ping, what's the status?
>>>> This is in "upstream" already.
>>> Hm.. I didn't see it..
>>>
>>> Well, then a follow-up fix should be applied, as described in one of previous
>>> messages, or this should be reverted.
>>
>> With that patch things looked better and you promised
>> "equivalent transform". OK, I'll roll it back then.
> Yes, that was a mistake. It's actually not an equivalent transformation.
> And I need to think what to do with this, because current code looks messy,
> but things are apparently so fragile...
>
> Are there any documentation on the "space grabbing" subsystem?


Nop.
I think that you grasped the basic idea of the reservation
subsystem perfectly.


>   For example,
> there's some flush code which grabs reserved space _without_ taking the mutex..


Most likely it is not needed for some subtle reasons,
which nobody remembers.

Edward.