| Message ID | 20161128185153.14908-1-kilobyte@angband.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 28, 2016 at 07:51:53PM +0100, Adam Borowski wrote: > People who don't frequent IRC nor the mailing list tend to believe RAID 5/6 > are stable; this leads to data loss. Thus, let's do warn them. > > At this point, I think fiery letters that won't be missed are warranted. > > Kernel 4.9 and its -progs will be a part of LTS of multiple distributions, > so leaving experimental features without a warning is inappropriate. I'm ok with adding the warning about raid56 feature, but I have some comments to how it's implemented. Special case warning for the raid56 is ok, as it corresponds to the 'mkfs_features' table where the missing value for 'safe' should lead to a similar warning. This is planned to be more generic, so I just want to make sure we can adjust it later without problems. The warning should go last, after the final summary (and respect verbosity level). If the message were not colored, I'd completely miss the warning. This also means the warning should not be printed from a helper function and not during the option parsing phase. The colors seem a bit too much to me, red text or just emphasize 'warning' would IMHO suffice. -- 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
On Wed, Nov 30, 2016 at 04:12:46PM +0100, David Sterba wrote: > On Mon, Nov 28, 2016 at 07:51:53PM +0100, Adam Borowski wrote: > > People who don't frequent IRC nor the mailing list tend to believe RAID 5/6 > > are stable; this leads to data loss. Thus, let's do warn them. > > > > At this point, I think fiery letters that won't be missed are warranted. > > > > Kernel 4.9 and its -progs will be a part of LTS of multiple distributions, > > so leaving experimental features without a warning is inappropriate. > > I'm ok with adding the warning about raid56 feature, but I have some > comments to how it's implemented. > > Special case warning for the raid56 is ok, as it corresponds to the > 'mkfs_features' table where the missing value for 'safe' should lead to > a similar warning. This is planned to be more generic, so I just want to > make sure we can adjust it later without problems. There are at least two tools that can make a raid56: mkfs and balance convert. mkfs_features is used for the former but not for the latter. It doesn't appear to be machine-parseable as well (but that's a matter of editing the #define). Of course, making this generic is doable, I just don't know how do you plan to do that. Should I try to use mkfs_features somehow, or do it as a variant of the current patch for now? > The warning should go last, after the final summary (and respect verbosity > level). and for balance convert, before the output. > The colors seem a bit too much to me, red text or just emphasize > 'warning' would IMHO suffice. (in the paragraph before:) > If the message were not colored, I'd completely miss the warning. ... which means the color works! It _is_ intended to be offensive, so people can't possibly miss it. XFS uses something far more tame: black text on red background (\e[0;30;41m) but I want something that conveys the message appropriately. Obviously I'm not married to a particular color for this bikeshed, I just want a color that makes the neighbours complain :p English lacks a proper word for this, something akin to https://en.wiktionary.org/wiki/oczojebny
diff --git a/cmds-balance.c b/cmds-balance.c index f17345e..0967162 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -48,8 +48,10 @@ static int parse_one_profile(const char *profile, u64 *flags) } else if (!strcmp(profile, "raid10")) { *flags |= BTRFS_BLOCK_GROUP_RAID10; } else if (!strcmp(profile, "raid5")) { + print_raid56_warning(); *flags |= BTRFS_BLOCK_GROUP_RAID5; } else if (!strcmp(profile, "raid6")) { + print_raid56_warning(); *flags |= BTRFS_BLOCK_GROUP_RAID6; } else if (!strcmp(profile, "dup")) { *flags |= BTRFS_BLOCK_GROUP_DUP; diff --git a/mkfs.c b/mkfs.c index e501a93..8d460b7 100644 --- a/mkfs.c +++ b/mkfs.c @@ -378,8 +378,10 @@ static u64 parse_profile(const char *s) } else if (strcasecmp(s, "raid1") == 0) { return BTRFS_BLOCK_GROUP_RAID1; } else if (strcasecmp(s, "raid5") == 0) { + print_raid56_warning(); return BTRFS_BLOCK_GROUP_RAID5; } else if (strcasecmp(s, "raid6") == 0) { + print_raid56_warning(); return BTRFS_BLOCK_GROUP_RAID6; } else if (strcasecmp(s, "raid10") == 0) { return BTRFS_BLOCK_GROUP_RAID10; diff --git a/utils.c b/utils.c index 69b580a..b9e44c2 100644 --- a/utils.c +++ b/utils.c @@ -4204,6 +4204,52 @@ out: return ret; } +#define NFL 5 +#define CYCLE (NFL * 2 - 2) +/* force text-on-black even if reversed */ +#define BG "\e[0;40;" +static const char *fire_levels[NFL] = +{ BG"1;30m", BG"31m", BG"1;31m", BG"1;33m", BG"1;37m", }; +#undef BG + +/* If this ever gets localized, we'll need to split by characters not bytes. */ +static void text_on_fire(const char *str) +{ + int s = 0, sl, chunks, len = strlen(str); + + /* no colors if our output is redirected */ + if (!isatty(fileno(stderr))) + return (void)fputs(str, stderr); + + /* repeat the scheme if too long, try 2-3 letters per level */ + chunks = (len / CYCLE / 3 + 1) * CYCLE + 1; + for (int ch = 0; ch < chunks; ++ch) { + int lev = ch % CYCLE; + fputs(fire_levels[(lev < NFL) ? lev : CYCLE - lev], + stderr); + sl = len * (ch + 1) / chunks; + for (; s < sl; ++s) + fputc(*str++, stderr); + } + fputs("\e[0m", stderr); /* reset color */ +} +#undef NFL +#undef CYCLE + +void print_raid56_warning(void) +{ + static int given = 0; + if (given) + return; + given = 1; + + fprintf(stderr, +"WARNING: btrfs RAID5 and 6 are still experimental, suffering from serious\n" +"problems. Thus, you are advised not to continue unless you know what you're\n" +"doing. In all cases, do keep up-to-date backups!\n"); + text_on_fire("Don't use RAID5/6 for anything important!\n"); +} + void init_rand_seed(u64 seed) { int i; diff --git a/utils.h b/utils.h index 366ca29..00a253a 100644 --- a/utils.h +++ b/utils.h @@ -423,6 +423,8 @@ static inline int __error_on(int condition, const char *fmt, ...) return 1; } +void print_raid56_warning(void); + /* Pseudo random number generator wrappers */ u32 rand_u32(void);
People who don't frequent IRC nor the mailing list tend to believe RAID 5/6 are stable; this leads to data loss. Thus, let's do warn them. At this point, I think fiery letters that won't be missed are warranted. Kernel 4.9 and its -progs will be a part of LTS of multiple distributions, so leaving experimental features without a warning is inappropriate. Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- cmds-balance.c | 2 ++ mkfs.c | 2 ++ utils.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ utils.h | 2 ++ 4 files changed, 52 insertions(+)