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

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

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.