[PATCH] btrfs-progs: balance: check for full-balance before background fork

[PATCH] btrfs-progs: balance: check for full-balance before background fork

[Vladimir Panteleev]

Move the full-balance warning to before the fork, so that the user can
see and react to it.

Notes on test:

- Don't use grep -q, as it causes a SIGPIPE during the countdown, and
  the balance thus doesn't start.

- The "balance cancel" is superfluous as the last command, but it
  provides some idempotence and allows adding more tests below it.

Fixes: https://github.com/kdave/btrfs-progs/issues/168

Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>
---
 cmds/balance.c                                | 36 +++++++++----------
 .../002-balance-full-no-filters/test.sh       |  5 +++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/cmds/balance.c b/cmds/balance.c
index 6f2d4803..32830002 100644
--- a/cmds/balance.c
+++ b/cmds/balance.c
@@ -437,24 +437,6 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 	if (fd < 0)
 		return 1;
 
-	if (!(flags & BALANCE_START_FILTERS) && !(flags & BALANCE_START_NOWARN)) {
-		int delay = 10;
-
-		printf("WARNING:\n\n");
-		printf("\tFull balance without filters requested. This operation is very\n");
-		printf("\tintense and takes potentially very long. It is recommended to\n");
-		printf("\tuse the balance filters to narrow down the scope of balance.\n");
-		printf("\tUse 'btrfs balance start --full-balance' option to skip this\n");
-		printf("\twarning. The operation will start in %d seconds.\n", delay);
-		printf("\tUse Ctrl-C to stop it.\n");
-		while (delay) {
-			printf("%2d", delay--);
-			fflush(stdout);
-			sleep(1);
-		}
-		printf("\nStarting balance without any filters.\n");
-	}
-
 	ret = ioctl(fd, BTRFS_IOC_BALANCE_V2, args);
 	if (ret < 0) {
 		/*
@@ -634,6 +616,24 @@ static int cmd_balance_start(const struct cmd_struct *cmd,
 		}
 	}
 
+	if (!(start_flags & BALANCE_START_FILTERS) && !(start_flags & BALANCE_START_NOWARN)) {
+		int delay = 10;
+
+		printf("WARNING:\n\n");
+		printf("\tFull balance without filters requested. This operation is very\n");
+		printf("\tintense and takes potentially very long. It is recommended to\n");
+		printf("\tuse the balance filters to narrow down the scope of balance.\n");
+		printf("\tUse 'btrfs balance start --full-balance' option to skip this\n");
+		printf("\twarning. The operation will start in %d seconds.\n", delay);
+		printf("\tUse Ctrl-C to stop it.\n");
+		while (delay) {
+			printf("%2d", delay--);
+			fflush(stdout);
+			sleep(1);
+		}
+		printf("\nStarting balance without any filters.\n");
+	}
+
 	if (force)
 		args.flags |= BTRFS_BALANCE_FORCE;
 	if (verbose)
diff --git a/tests/cli-tests/002-balance-full-no-filters/test.sh b/tests/cli-tests/002-balance-full-no-filters/test.sh
index 9c31dd6f..daadcc44 100755
--- a/tests/cli-tests/002-balance-full-no-filters/test.sh
+++ b/tests/cli-tests/002-balance-full-no-filters/test.sh
@@ -18,4 +18,9 @@ run_check $SUDO_HELPER "$TOP/btrfs" balance start "$TEST_MNT"
 run_check $SUDO_HELPER "$TOP/btrfs" balance --full-balance "$TEST_MNT"
 run_check $SUDO_HELPER "$TOP/btrfs" balance "$TEST_MNT"
 
+run_check_stdout $SUDO_HELPER "$TOP/btrfs" balance start --background "$TEST_MNT" |
+	grep -F "Full balance without filters requested." ||
+	_fail "full balance warning not in the output"
+run_mayfail $SUDO_HELPER "$TOP/btrfs" balance cancel "$TEST_MNT"
+
 run_check_umount_test_dev
-- 
2.22.0

Re: [PATCH] btrfs-progs: balance: check for full-balance before background fork

[David Sterba]

On Sat, Aug 17, 2019 at 11:14:34PM +0000, Vladimir Panteleev wrote:
> Move the full-balance warning to before the fork, so that the user can
> see and react to it.
> 
> Notes on test:
> 
> - Don't use grep -q, as it causes a SIGPIPE during the countdown, and
>   the balance thus doesn't start.
> 
> - The "balance cancel" is superfluous as the last command, but it
>   provides some idempotence and allows adding more tests below it.
> 
> Fixes: https://github.com/kdave/btrfs-progs/issues/168
> 
> Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>

Applied, thanks. The issues can be referenced as

Issue: #168

> --- a/tests/cli-tests/002-balance-full-no-filters/test.sh
> +++ b/tests/cli-tests/002-balance-full-no-filters/test.sh
> @@ -18,4 +18,9 @@ run_check $SUDO_HELPER "$TOP/btrfs" balance start "$TEST_MNT"
>  run_check $SUDO_HELPER "$TOP/btrfs" balance --full-balance "$TEST_MNT"
>  run_check $SUDO_HELPER "$TOP/btrfs" balance "$TEST_MNT"
>  
> +run_check_stdout $SUDO_HELPER "$TOP/btrfs" balance start --background "$TEST_MNT" |
> +	grep -F "Full balance without filters requested." ||

This needs -q, otherwise the text appears in the output of make. Fixed.

Re: [PATCH] btrfs-progs: balance: check for full-balance before background fork

[Vladimir Panteleev]

Hi David,

On 20/08/2019 14.32, David Sterba wrote:
> On Sat, Aug 17, 2019 at 11:14:34PM +0000, Vladimir Panteleev wrote:
>> - Don't use grep -q, as it causes a SIGPIPE during the countdown, and
>>    the balance thus doesn't start.
>>
> This needs -q, otherwise the text appears in the output of make. Fixed.

What of the SIGPIPE problem mentioned in the commit message?

If using -q is preferred despite of that, then probably the note about 
it should be removed from the commit message, and the "cancel" 
afterwards should probably be removed as well (along with its note in 
the commit message too), as the SIGPIPE will prevent the balance from 
ever starting.

Perhaps redirecting the output of grep to /dev/null is a better option.

 > Applied, thanks.

Not a big issue but for some reason my email address was mangled 
(@panteleev.md instead of @vladimir.panteleev.md). Looks fine when I 
look at https://patchwork.kernel.org/patch/11099359/mbox/.

-- 
Best regards,
  Vladimir

Re: [PATCH] btrfs-progs: balance: check for full-balance before background fork

[David Sterba]

On Tue, Aug 20, 2019 at 02:46:49PM +0000, Vladimir Panteleev wrote:
> Hi David,
> 
> On 20/08/2019 14.32, David Sterba wrote:
> > On Sat, Aug 17, 2019 at 11:14:34PM +0000, Vladimir Panteleev wrote:
> >> - Don't use grep -q, as it causes a SIGPIPE during the countdown, and
> >>    the balance thus doesn't start.
> >>
> > This needs -q, otherwise the text appears in the output of make. Fixed.
> 
> What of the SIGPIPE problem mentioned in the commit message?
> 
> If using -q is preferred despite of that, then probably the note about 
> it should be removed from the commit message, and the "cancel" 
> afterwards should probably be removed as well (along with its note in 
> the commit message too), as the SIGPIPE will prevent the balance from 
> ever starting.
> 
> Perhaps redirecting the output of grep to /dev/null is a better option.

Agreed, I've updated the patch to remove -q and add the redirection.

>  > Applied, thanks.
> 
> Not a big issue but for some reason my email address was mangled 
> (@panteleev.md instead of @vladimir.panteleev.md). Looks fine when I 
> look at https://patchwork.kernel.org/patch/11099359/mbox/.

Hm, strange. I'll fix the patches from you so they match your
expectations. The mangled domain name does not appear anywhere in the
mail headers, I wonder what's going on here.