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