| Message ID | 20190817231434.1034-1-git@vladimir.panteleev.md (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: balance: check for full-balance before background fork | expand |
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.
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/.
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.
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
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(-)