[PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[Su Yue]

Since lowmem repair is dangerous, it should remind user more obviously.
The patchset add 10 seconds delay like btrfs balance and add am option
'--force-repair-lowmem' to skip the delay.

---
I don't whether it's a good idea to add delay and the option only for
lowmem repair is acceptable, so make it RFC.

Su Yue (3):
  btrfs-progs: lowmem: delay before lowmem repair starts
  btrfs-progs: lowmem: force to start without delay with option
    '--force-repair-lowmem'
  btrfs-progs: tests: append '--force-repair-lowmem' if lowmem repair is
    enabled

 check/main.c       | 43 ++++++++++++++++++++++++++++++++++++-------
 tests/common.local |  1 +
 2 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH RFC 1/3] btrfs-progs: lowmem: delay before lowmem repair starts

[Su Yue]

Since lowmem mode repair is so dangerous, delay 10 seconds before
start.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 check/main.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/check/main.c b/check/main.c
index 3190b5d4f293..b9997460162f 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9553,12 +9553,6 @@ int cmd_check(int argc, char **argv)
 		exit(1);
 	}
 
-	/*
-	 * experimental and dangerous
-	 */
-	if (repair && check_mode == CHECK_MODE_LOWMEM)
-		warning("low-memory mode repair support is only partial");
-
 	radix_tree_init();
 	cache_tree_init(&root_cache);
 
@@ -9600,6 +9594,28 @@ int cmd_check(int argc, char **argv)
 	if (repair)
 		ctree_flags |= OPEN_CTREE_PARTIAL;
 
+	/*
+	 * experimental and dangerous
+	 */
+	if (repair && check_mode == CHECK_MODE_LOWMEM) {
+		int delay = 10;
+
+		printf("WARNING:\n\n");
+		printf("\tLow-memory mode repair support is only partial.\n");
+		printf("\tIt's experimental and very dangerous.\n");
+		printf("\tIt may run slow or crash unexpectedly.\n");
+		printf("\tPlease backup device before running low-memory mode repair.\n");
+		printf("\tThe 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 to check and repair filesystem.\n");
+	}
+
 	info = open_ctree_fs_info(argv[optind], bytenr, tree_root_bytenr,
 				  chunk_root_bytenr, ctree_flags);
 	if (!info) {
-- 
2.17.1

[PATCH RFC 2/3] btrfs-progs: lowmem: force to start without delay with option '--force-repair-lowmem'

[Su Yue]

Add an option '--force-repair-lowmem' to start check without any delay.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 check/main.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/check/main.c b/check/main.c
index b9997460162f..3c9dc242f3db 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9377,6 +9377,7 @@ const char * const cmd_check_usage[] = {
 	"-s|--super <superblock>     use this superblock copy",
 	"-b|--backup                 use the first valid backup root copy",
 	"--force                     skip mount checks, repair is not possible",
+	"--force-repair-lowmem       start to check and repair in lowmem mode without delay",
 	"--repair                    try to repair the filesystem",
 	"--readonly                  run in read-only mode (default)",
 	"--init-csum-tree            create a new CRC tree",
@@ -9419,6 +9420,7 @@ int cmd_check(int argc, char **argv)
 	int qgroup_report_ret;
 	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE;
 	int force = 0;
+	int force_repair_lowmem = 0;
 
 	while(1) {
 		int c;
@@ -9426,7 +9428,7 @@ int cmd_check(int argc, char **argv)
 			GETOPT_VAL_INIT_EXTENT, GETOPT_VAL_CHECK_CSUM,
 			GETOPT_VAL_READONLY, GETOPT_VAL_CHUNK_TREE,
 			GETOPT_VAL_MODE, GETOPT_VAL_CLEAR_SPACE_CACHE,
-			GETOPT_VAL_FORCE };
+			GETOPT_VAL_FORCE, GETOPT_VAL_FORCE_REPAIR_LOWMEM };
 		static const struct option long_options[] = {
 			{ "super", required_argument, NULL, 's' },
 			{ "repair", no_argument, NULL, GETOPT_VAL_REPAIR },
@@ -9449,6 +9451,8 @@ int cmd_check(int argc, char **argv)
 			{ "clear-space-cache", required_argument, NULL,
 				GETOPT_VAL_CLEAR_SPACE_CACHE},
 			{ "force", no_argument, NULL, GETOPT_VAL_FORCE },
+			{"force-repair-lowmem", no_argument, NULL,
+				GETOPT_VAL_FORCE_REPAIR_LOWMEM},
 			{ NULL, 0, NULL, 0}
 		};
 
@@ -9536,6 +9540,9 @@ int cmd_check(int argc, char **argv)
 			case GETOPT_VAL_FORCE:
 				force = 1;
 				break;
+			case GETOPT_VAL_FORCE_REPAIR_LOWMEM:
+				force_repair_lowmem = 1;
+				break;
 		}
 	}
 
@@ -9552,6 +9559,11 @@ int cmd_check(int argc, char **argv)
 		error("repair options are not compatible with --readonly");
 		exit(1);
 	}
+	if (force_repair_lowmem &&
+	    (!repair || check_mode != CHECK_MODE_LOWMEM)) {
+		error("--force-repair-lowmem only works with --mode=lowmem and --repair");
+		exit(1);
+	}
 
 	radix_tree_init();
 	cache_tree_init(&root_cache);
@@ -9597,7 +9609,7 @@ int cmd_check(int argc, char **argv)
 	/*
 	 * experimental and dangerous
 	 */
-	if (repair && check_mode == CHECK_MODE_LOWMEM) {
+	if (repair && check_mode == CHECK_MODE_LOWMEM && !force_repair_lowmem) {
 		int delay = 10;
 
 		printf("WARNING:\n\n");
@@ -9605,6 +9617,7 @@ int cmd_check(int argc, char **argv)
 		printf("\tIt's experimental and very dangerous.\n");
 		printf("\tIt may run slow or crash unexpectedly.\n");
 		printf("\tPlease backup device before running low-memory mode repair.\n");
+		printf("\tUse option'--force-repair-lowmem' option to skip this warning.\n");
 		printf("\tThe operation will start in %d seconds.\n", delay);
 		printf("\tUse Ctrl-C to stop it.\n");
 
-- 
2.17.1

[PATCH RFC 3/3] btrfs-progs: tests: append '--force-repair-lowmem' if lowmem repair is enabled

[Su Yue]

Since commit 62785de7c107 ("btrfs-progs: lowmem: force to start
without delay with option '--force-repair-lowmem'") delays repair in
lowmem mode.
For tests which repair in lowmem mode, append '--force-repair-lowmem'
to force them to start without delay.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 tests/common.local | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/common.local b/tests/common.local
index f5e96f5b73ac..4facc5482ef7 100644
--- a/tests/common.local
+++ b/tests/common.local
@@ -29,6 +29,7 @@ _skip_spec()
 	   echo "$@" | grep -q -- '--repair'; then
 		dir="$(dirname ${@: -1})"
 		if [ -f ${dir}/${beacon} ]; then
+			TEST_ARGS_CHECK+=" --force-repair-lowmem"
 			return 1;
 		fi
 		return 0;
-- 
2.17.1

Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[Nikolay Borisov]

On  2.07.2018 12:28, Su Yue wrote:
> Since lowmem repair is dangerous, it should remind user more obviously.
> The patchset add 10 seconds delay like btrfs balance and add am option
> '--force-repair-lowmem' to skip the delay.

IMO this is the wrong way to approach a dangerous option. If it's so
dangerous it needs to be written in the documentation explicitly this is
so. If someone wants to use lowmem then they should explicitly set
--mode lowmem. So I'm inclined to NACK this patch.

> 
> ---
> I don't whether it's a good idea to add delay and the option only for
> lowmem repair is acceptable, so make it RFC.
> 
> Su Yue (3):
>   btrfs-progs: lowmem: delay before lowmem repair starts
>   btrfs-progs: lowmem: force to start without delay with option
>     '--force-repair-lowmem'
>   btrfs-progs: tests: append '--force-repair-lowmem' if lowmem repair is
>     enabled
> 
>  check/main.c       | 43 ++++++++++++++++++++++++++++++++++++-------
>  tests/common.local |  1 +
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 

Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[David Disseldorp]

On Mon, 2 Jul 2018 12:43:20 +0300, Nikolay Borisov wrote:

> On  2.07.2018 12:28, Su Yue wrote:
> > Since lowmem repair is dangerous, it should remind user more obviously.
> > The patchset add 10 seconds delay like btrfs balance and add am option
> > '--force-repair-lowmem' to skip the delay.  
> 
> IMO this is the wrong way to approach a dangerous option. If it's so
> dangerous it needs to be written in the documentation explicitly this is
> so. If someone wants to use lowmem then they should explicitly set
> --mode lowmem. So I'm inclined to NACK this patch.

AFAICT it's already documented as "experimental" in the manpage, but the
usage flag appears to have been dropped as part of refactoring for
87c1bd13c1fca430c3dbf0da62e9aa33bde609c8 . If nobody's working on a fix,
and lowmem removal isn't an option, then please consider adding the usage
flag back, e.g.
--- a/check/main.c
+++ b/check/main.c
@@ -9386,7 +9386,7 @@ const char * const cmd_check_usage[] = {
        "                            original - read inodes and extents to memory (requires",
        "                                       more memory, does less IO)",
        "                            lowmem   - try to use less memory but read blocks again",
-       "                                       when needed",
+       "                                       when needed (experimental)",
        "--check-data-csum           verify checksums of data blocks",
        "-Q|--qgroup-report          print a report on qgroup consistency",
        "-E|--subvol-extents <subvolid>",

Cheers, David

Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[Nikolay Borisov]

On  2.07.2018 14:07, David Disseldorp wrote:
> On Mon, 2 Jul 2018 12:43:20 +0300, Nikolay Borisov wrote:
> 
>> On  2.07.2018 12:28, Su Yue wrote:
>>> Since lowmem repair is dangerous, it should remind user more obviously.
>>> The patchset add 10 seconds delay like btrfs balance and add am option
>>> '--force-repair-lowmem' to skip the delay.  
>>
>> IMO this is the wrong way to approach a dangerous option. If it's so
>> dangerous it needs to be written in the documentation explicitly this is
>> so. If someone wants to use lowmem then they should explicitly set
>> --mode lowmem. So I'm inclined to NACK this patch.
> 
> AFAICT it's already documented as "experimental" in the manpage, but the
> usage flag appears to have been dropped as part of refactoring for
> 87c1bd13c1fca430c3dbf0da62e9aa33bde609c8 . If nobody's working on a fix,
> and lowmem removal isn't an option, then please consider adding the usage
> flag back, e.g.

lowmem seems to be here to stay and it seems to be more useful than
original mode. So your patch is acceptable.

> --- a/check/main.c
> +++ b/check/main.c
> @@ -9386,7 +9386,7 @@ const char * const cmd_check_usage[] = {
>         "                            original - read inodes and extents to memory (requires",
>         "                                       more memory, does less IO)",
>         "                            lowmem   - try to use less memory but read blocks again",
> -       "                                       when needed",
> +       "                                       when needed (experimental)",
>         "--check-data-csum           verify checksums of data blocks",
>         "-Q|--qgroup-report          print a report on qgroup consistency",
>         "-E|--subvol-extents <subvolid>",
> 
> Cheers, David
> 

Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[Su Yue]

---
Su


> Sent: Monday, July 02, 2018 at 7:09 PM
> From: "Nikolay Borisov" <nborisov@suse.com>
> To: "David Disseldorp" <ddiss@samba.org>
> Cc: "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair
>
> 
> 
> On  2.07.2018 14:07, David Disseldorp wrote:
> > On Mon, 2 Jul 2018 12:43:20 +0300, Nikolay Borisov wrote:
> > 
> >> On  2.07.2018 12:28, Su Yue wrote:
> >>> Since lowmem repair is dangerous, it should remind user more obviously.
> >>> The patchset add 10 seconds delay like btrfs balance and add am option
> >>> '--force-repair-lowmem' to skip the delay.  
> >>
> >> IMO this is the wrong way to approach a dangerous option. If it's so
> >> dangerous it needs to be written in the documentation explicitly this is
> >> so. If someone wants to use lowmem then they should explicitly set
> >> --mode lowmem. So I'm inclined to NACK this patch.
> > 
> > AFAICT it's already documented as "experimental" in the manpage, but the
> > usage flag appears to have been dropped as part of refactoring for
> > 87c1bd13c1fca430c3dbf0da62e9aa33bde609c8 . If nobody's working on a fix,
> > and lowmem removal isn't an option, then please consider adding the usage
> > flag back, e.g.
> 
> lowmem seems to be here to stay and it seems to be more useful than
> original mode. So your patch is acceptable.
> 
Agreed.

Su
> > --- a/check/main.c
> > +++ b/check/main.c
> > @@ -9386,7 +9386,7 @@ const char * const cmd_check_usage[] = {
> >         "                            original - read inodes and extents to memory (requires",
> >         "                                       more memory, does less IO)",
> >         "                            lowmem   - try to use less memory but read blocks again",
> > -       "                                       when needed",
> > +       "                                       when needed (experimental)",
> >         "--check-data-csum           verify checksums of data blocks",
> >         "-Q|--qgroup-report          print a report on qgroup consistency",
> >         "-E|--subvol-extents <subvolid>",
> > 
> > Cheers, David
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair

[Su Yue]

> Sent: Monday, July 02, 2018 at 5:43 PM
> From: "Nikolay Borisov" <nborisov@suse.com>
> To: "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH RFC 0/3] btrfs-progs: lowmem: delay before lowmem repair
>
> 
> 
> On  2.07.2018 12:28, Su Yue wrote:
> > Since lowmem repair is dangerous, it should remind user more obviously.
> > The patchset add 10 seconds delay like btrfs balance and add am option
> > '--force-repair-lowmem' to skip the delay.
> 
> IMO this is the wrong way to approach a dangerous option. If it's so
> dangerous it needs to be written in the documentation explicitly this is
> so. If someone wants to use lowmem then they should explicitly set
> --mode lowmem. So I'm inclined to NACK this patch.
> 
OK. After some considerations, I thinks the patchset is indeed appropriate.
Droping it is fine.

Thanks,
Su
> > 
> > ---
> > I don't whether it's a good idea to add delay and the option only for
> > lowmem repair is acceptable, so make it RFC.
> > 
> > Su Yue (3):
> >   btrfs-progs: lowmem: delay before lowmem repair starts
> >   btrfs-progs: lowmem: force to start without delay with option
> >     '--force-repair-lowmem'
> >   btrfs-progs: tests: append '--force-repair-lowmem' if lowmem repair is
> >     enabled
> > 
> >  check/main.c       | 43 ++++++++++++++++++++++++++++++++++++-------
> >  tests/common.local |  1 +
> >  2 files changed, 37 insertions(+), 7 deletions(-)
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>