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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> >>> ---
> >>>   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: signature.asc
Description: This is a digitally signed message part.


[Index of Archives]     [Linux File System Development]     [Linux BTRFS]     [Linux NFS]     [Linux Filesystems]     [Ext4 Filesystem]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Resources]

  Powered by Linux