diff mbox

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

Message ID 20180702130726.21a3257d@samba.org (mailing list archive)
State New, archived
Headers show

Commit Message

David Disseldorp July 2, 2018, 11:07 a.m. UTC
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.

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

Comments

Su Yue July 2, 2018, 1:57 p.m. UTC | #1
---
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
> 
--
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
diff mbox

Patch

--- 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>",