From: Alexey Lyashkov <[email protected]>
e2fsck don't print a message if 'p' option used and error can be fixed without
user assistance, but 'v' option asks to be more verbose, so user expect to
see any output. But not.
Fix this, by avoid message suppress with verbose option used.
Change-Id: I358e9b04e44dd33fdb124c5663b2df0bf54ce370
Signed-off-by: Alexey Lyashkov <[email protected]>
Cray-bug-id: LUS-6890
---
e2fsck/e2fsck.h | 1 +
e2fsck/problem.c | 3 ++-
e2fsck/unix.c | 15 ++++++++-------
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index 1c7a67cb..64140fc3 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -174,6 +174,7 @@ struct resource_track {
#define E2F_OPT_NOOPT_EXTENTS 0x10000 /* don't optimize extents */
#define E2F_OPT_ICOUNT_FULLMAP 0x20000 /* use an array for inode counts */
#define E2F_OPT_UNSHARE_BLOCKS 0x40000
+#define E2F_OPT_VERBOSE 0x80000 /* be verbose and PREEN_NOMSG ignore */
/*
* E2fsck flags
diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index 0e6bacec..2576c9bb 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -2259,7 +2259,8 @@ int fix_problem(e2fsck_t ctx, problem_t code, struct problem_context *pctx)
if (ldesc->flags & PRL_SUPPRESS)
suppress++;
}
- if ((ptr->flags & PR_PREEN_NOMSG) &&
+ if (((ctx->options & E2F_OPT_VERBOSE) == 0) &&
+ (ptr->flags & PR_PREEN_NOMSG) &&
(ctx->options & E2F_OPT_PREEN))
suppress++;
if ((ptr->flags & PR_NO_NOMSG) &&
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 5b3552ec..b073615e 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -60,7 +60,6 @@ extern int optind;
/* Command line options */
static int cflag; /* check disk */
static int show_version_only;
-static int verbose;
static int replace_bad_blocks;
static int keep_bad_blocks;
@@ -133,7 +132,7 @@ static void show_stats(e2fsck_t ctx)
frag_percent_total = (frag_percent_total + 5) / 10;
}
- if (!verbose) {
+ if ((ctx->options & E2F_OPT_VERBOSE) == 0) {
log_out(ctx, _("%s: %u/%u files (%0d.%d%% non-contiguous), "
"%llu/%llu blocks\n"),
ctx->device_name, inodes_used, inodes,
@@ -143,7 +142,7 @@ static void show_stats(e2fsck_t ctx)
}
profile_get_boolean(ctx->profile, "options", "report_features", 0, 0,
&i);
- if (verbose && i) {
+ if ((ctx->options & E2F_OPT_VERBOSE) && i) {
log_out(ctx, "\nFilesystem features:");
mask = &ctx->fs->super->s_feature_compat;
for (i = 0; i < 3; i++, mask++) {
@@ -807,6 +806,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
char *jbd_debug;
#endif
unsigned long long phys_mem_kb;
+ int verbose = 0;
retval = e2fsck_allocate_context(&ctx);
if (retval)
@@ -1037,8 +1037,8 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
ctx->options |= E2F_OPT_TIME | E2F_OPT_TIME2;
profile_get_boolean(ctx->profile, "options", "report_verbose", 0, 0,
&c);
- if (c)
- verbose = 1;
+ if (c || verbose)
+ ctx->options |= E2F_OPT_VERBOSE;
profile_get_boolean(ctx->profile, "options", "no_optimize_extents",
0, 0, &c);
@@ -1231,8 +1231,9 @@ static errcode_t e2fsck_check_mmp(ext2_filsys fs, e2fsck_t ctx)
if (retval)
goto check_error;
- /* Print warning if e2fsck will wait for more than 20 secs. */
- if (verbose || wait_time > EXT4_MMP_MIN_CHECK_INTERVAL * 4) {
+ /* Print warning if e2fck will wait for more than 20 secs. */
+ if ((ctx->options & E2F_OPT_VERBOSE) ||
+ wait_time > EXT4_MMP_MIN_CHECK_INTERVAL * 4) {
log_out(ctx, _("MMP interval is %u seconds and total wait "
"time is %u seconds. Please wait...\n"),
mmp_check_interval, wait_time * 2);
--
2.14.3
On Fri, Apr 26, 2019 at 04:09:13PM +0300, Artem Blagodarenko wrote:
> From: Alexey Lyashkov <[email protected]>
>
> e2fsck don't print a message if 'p' option used and error can be fixed without
> user assistance, but 'v' option asks to be more verbose, so user expect to
> see any output. But not.
> Fix this, by avoid message suppress with verbose option used.
>
> Change-Id: I358e9b04e44dd33fdb124c5663b2df0bf54ce370
> Signed-off-by: Alexey Lyashkov <[email protected]>
> Cray-bug-id: LUS-6890
I need to understand the use case of what you are trying to do here.
The preen and verbose options were never intended to be mixed and this
patch changes what the verbose flag does at a fairly fundamental
level. I'm not sure the results will be correct and they will almost
certainly be surprising.
So (a) what is the user trying to do, and (b) what does the user want
to be trying to do? Preen was intended to be used as part of the boot
process, when multiple e2fsck's would be running in parallel, and so
you don't *want* much in the way of verbosity.
- Ted
Theodore,
Usecase is simple. User use a -p with -v flag,
in this case, -p block any messages on console in case it successfully fixed.
It’s OK _without_ -v flag, situation is different with -v flag.
In this case, user expect to see mode debug info about check/fix process,
and «no messages» in this mode confuse a user, as he think «no messages» == «no bugs fixed»,
but it’s not a true in common way.
From other side, -p print a messages about fix process, but not for bitmaps, it’s source of additional
confuse for the user, as he lack an info about FS changes during e2fsck run.
> 29 апр. 2019 г., в 2:38, Theodore Ts'o <[email protected]> написал(а):
>
> On Fri, Apr 26, 2019 at 04:09:13PM +0300, Artem Blagodarenko wrote:
>> From: Alexey Lyashkov <[email protected]>
>>
>> e2fsck don't print a message if 'p' option used and error can be fixed without
>> user assistance, but 'v' option asks to be more verbose, so user expect to
>> see any output. But not.
>> Fix this, by avoid message suppress with verbose option used.
>>
>> Change-Id: I358e9b04e44dd33fdb124c5663b2df0bf54ce370
>> Signed-off-by: Alexey Lyashkov <[email protected]>
>> Cray-bug-id: LUS-6890
>
> I need to understand the use case of what you are trying to do here.
> The preen and verbose options were never intended to be mixed and this
> patch changes what the verbose flag does at a fairly fundamental
> level. I'm not sure the results will be correct and they will almost
> certainly be surprising.
>
> So (a) what is the user trying to do, and (b) what does the user want
> to be trying to do? Preen was intended to be used as part of the boot
> process, when multiple e2fsck's would be running in parallel, and so
> you don't *want* much in the way of verbosity.
>
> - Ted
On Mon, Apr 29, 2019 at 07:16:36AM +0300, Alexey Lyashkov wrote:
> Theodore,
>
> Usecase is simple. User use a -p with -v flag,
> in this case, -p block any messages on console in case it successfully fixed.
> It’s OK _without_ -v flag, situation is different with -v flag.
> In this case, user expect to see mode debug info about check/fix process,
> and «no messages» in this mode confuse a user, as he think «no messages» == «no bugs fixed»,
> but it’s not a true in common way.
> From other side, -p print a messages about fix process, but not for bitmaps, it’s source of additional
> confuse for the user, as he lack an info about FS changes during e2fsck run.
That's not a use case. *Why* is the user using -p?
The -p option is only intended to be used when called from boot
scripts, where e2fsck is run in parallel. This usage, "preen mode"
dates back to BSD 4.3. What it does is pretty clearly described in
the man page.
The user seems to be very confused with their expectation, and it's
not at all clear it's a correct one. Why does the user have this
expectation, and under what circumstances would they want e2fsck to
abort for some fixes, and automatically fix others, *and* want a full
set of messages of what was fixed?
If you are running things interactively (e.g., not at boot time),
having e2fsck abort for certain sets of error may end up wasting a lot
of time, since then you'll have to restart the e2fsck run.
Essentially, I'm asking for a complete justification for why this is a
general thing that many users will want, and why it makes sense for
them to want it. The fact that *some* user had some twisted
expectation, and filed a Lustre bug, doesn't mean that you
automatically get to have that expectation satisified in the upstream
e2fsprogs sources. You need to justify why other users would want it,
and how this is optimal for that particular way that people would want
to run e2fsck, for some particular set of circumstances.
Otherwise, some other user will have some *other* set if expectations,
and file another bug with Debian or Red Hat, demanding some other
random change, and this way lies madness.
- Ted
> 5 мая 2019 г., в 0:49, Theodore Ts'o <[email protected]> написал(а):
>
> On Mon, Apr 29, 2019 at 07:16:36AM +0300, Alexey Lyashkov wrote:
>> Theodore,
>>
>> Usecase is simple. User use a -p with -v flag,
>> in this case, -p block any messages on console in case it successfully fixed.
>> It’s OK _without_ -v flag, situation is different with -v flag.
>> In this case, user expect to see mode debug info about check/fix process,
>> and «no messages» in this mode confuse a user, as he think «no messages» == «no bugs fixed»,
>> but it’s not a true in common way.
>> From other side, -p print a messages about fix process, but not for bitmaps, it’s source of additional
>> confuse for the user, as he lack an info about FS changes during e2fsck run.
>
> That's not a use case. *Why* is the user using -p?
>
> The -p option is only intended to be used when called from boot
> scripts, where e2fsck is run in parallel
It’s not a true. -p option is good enough in run to run automatic fixes, for minor bugs, without relation to boot scripts.
-y option too soft, -n option - too strict, -p is good enough in common case for initial fix.
But anyway, is it run from boot scripts command output is logged and can be analyzed by automatic tools,
if it run by hand it will analyzed by user.
It’s very strange to have some output is in console, some output is omitted.
A specially in case user request a -v option.
---
Alex
On Sun, May 05, 2019 at 01:04:00AM +0300, Alexey Lyashkov wrote:
> >
> > The -p option is only intended to be used when called from boot
> > scripts, where e2fsck is run in parallel
>
> It’s not a true.
It *is* true. That was the original intent. You may be wanting to
use -p for something else, but that is *not* how -p was originally
intended to be used. As the author, I'm entitled to tell you how I
originally intended the option to be used. :-)
> -p option is good enough in run to run automatic fixes, for minor
> bugs, without relation to boot scripts. -y option too soft,
> -n option - too strict, -p is good enough in common case for initial fix.
You or your user want to use it for something else, but we should
discuss whether -p is really the right approach. It sounds like the
user doesn't want to answer all of the questions by hand. That's a
valid desire, but using -p means that after first problem which e2fsck
finds which it can't safely fix, it will abort.
As I understand things, the Lustre folks are interested in using
super, super big ext4 file systems. Which is fine, but that means the
e2fsck run might take a long time. To have it get half-way through
the run, only to have it abort, and then forcing the user to restart
the e2fsck might not be the user-friendly way to go, hmmm?
So *perhaps* the right answer is to add a new option which
automatically answers "yes" to all PR_PREEN_OK problems, but for
problems that are not PR_PREEN_OK, e2fsck should not abort, but stop
and ask the user for a yes/no confirmation about how to proceed.
Another potential answer would be to add two new "always" and "never"
answers, which gives e2fsck permission to proceed to fix (or skip
fixing) all future instances of that particular problem. This isn't
as automatic, but it gives much finer-grain control to the user.
(These two proposals are not mutually exclusive, by the way; we might
want to do both.)
Using -p -v is a hack, and it's in my opinion not really the best
answer.
Finally, it's clear that this patch was never properly tested. It
doesn't work right for PR_PREEN_NOMSG problems which previously had
been suppressed now get printed:
<tytso@lambda> {/build/e2fsprogs-maint/e2fsck}
1077% gunzip < /usr/src/e2fsprogs/tests/f_bad_bbitmap/image.gz > /tmp/foo.img
<tytso@lambda> {/build/e2fsprogs-maint/e2fsck}
1078% ./e2fsck /tmp/foo.img
e2fsck 1.45.0 (6-Mar-2019)
One or more block group descriptor checksums are invalid. Fix<y>? yes
Group descriptor 0 checksum is 0x49ff, should be 0x4972. FIXED.
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Block bitmap differences: -(8--10) -(12--17) -(19--31)
Fix<y>? no
Free blocks count wrong for group #0 (494, counted=472).
Fix<y>? no
Free blocks count wrong (494, counted=472).
Fix<y>? no
Block bitmap differences: Group 0 block bitmap does not match checksum.
FIXED.
test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
test_filesys: ********** WARNING: Filesystem still has errors **********
test_filesys: 11/128 files (0.0% non-contiguous), 18/512 blocks
<tytso@lambda> {/build/e2fsprogs-maint/e2fsck}
1079% ./e2fsck -pv /tmp/foo.img
test_filesys was not cleanly unmounted, check forced.
test_filesys: Block bitmap differences: test_filesys: -(8--10)test_filesys: -(12--17)test_filesys: -(19--31)test_filesys:
Note how badly the "Bad bitmap differences" message was mangled with
the patch and -p -v. That's because the PR_PREEN_NOMSG messages were
never intended to be printed in preen mode. By definition. :-)
So even if "e2fsck -p -v" is the best solution for this particular use
case (and I don't think it is); the patch as proposed is *definitely*
is not the best implementation of the design, and so it's not suitable
for upstream adoption.
Regards,
- Ted