From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Shishkin Subject: Re: [PATCHv2 2/3] reiser4: block_alloc: sanitize grab_enabled modifications. Date: Sun, 16 Nov 2014 12:49:45 +0100 Message-ID: <54688F59.9020102@gmail.com> References: <1414048685-4224-1-git-send-email-intelfx100@gmail.com> <1414048685-4224-3-git-send-email-intelfx100@gmail.com> <2228941.DWoZ5v2hiQ@intelfx-laptop> <546873FF.6040804@gmail.com> <54688047.2020508@gmail.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; bh=BgqSlWLlGb0+CuASSxgD/gIjYudkhJBDmvuAbivGzME=; b=tKvvYrAPLO33X+LW0o9S9255e7ThZuXCAxlq0PzEPc2WG9aoPNGl6vociycyXg+bzr sNYHZL07S3g3chcOmyo/KTQPI2QLNaGBzIdTmWhe3Hmg/5d83u7KFuQDwVov/hqWveeY Hf7IW9GoxjjjVWIIdHIfsW/NuhoIzEzLvwjfItD3h+m7umJWQTq3FTOjGNZCLWd+7KOK aR59r2VsRmXzJEJ8MElhdhQT1sSQ7dL0RBxkSfN2YXIjNMmU0iuJ8ySnxB+MCzlDYSQN kASGWkuMmHY7qGwfzHW/lgi1DniSEqgL3pQE7VW2cc0y2H4F080iYo2iOi3xlFO7Ek2M +Stw== In-Reply-To: <54688047.2020508@gmail.com> Sender: reiserfs-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Ivan Shapovalov , reiserfs-devel@vger.kernel.org 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 >>>> --- >>>> 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.