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.