Date:   Wed, 23 Sep 2020 13:00:57 +0200
From:   Lukas Czerner <lczerner@...hat.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] e2fsck: skip extent optimization by default

On Tue, Sep 22, 2020 at 11:42:40AM -0600, Andreas Dilger wrote:
> On Sep 22, 2020, at 4:26 AM, Lukas Czerner <lczerner@...hat.com> wrote:
> > 
> > On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@...mcloud.com wrote:
> >> From: Andreas Dilger <adilger@...mcloud.com>
> >> 
> >> The e2fsck error message:
> >> 
> >>    inode nnn extent tree (at level 1) could be narrower. Optimize<y>?
> >> 
> >> can be fairly verbose at times, and leads users to think that there
> >> may be something wrong with the filesystem.  Basically, almost any
> >> message printed by e2fsck makes users nervous when they are facing
> >> other corruption, and a few thousand of these printed may hide other
> >> errors.  It also isn't clear that saving a few blocks optimizing the
> >> extent tree noticeably improves performance.
> >> 
> >> This message has previously been annoying enough for Ted to add the
> >> "-E no_optimize_extents" option to disable it.  Just enable this
> >> option by default, similar to the "-D" directory optimization option.
> > 
> > it seem counterproductive to me that we would disable usefull (even if
> > just a little) optimization just because the way it is presented to the
> > user is inconvenient. I agree that messages during e2fsck often raise
> > alarms, as they should, but perfeps instead of disabling the feature we
> > can figure out a way to make the messaging better ?
> > 
> > Can we just not print the every message if the answer is going to be yes
> > anyway, either because of -y, -p, <a> or whatever when the user is not
> > involved in the decision anymore ? Maybe a log file can be created
> > for the purpose of storing the full log of changes. Or perhaps we can
> > print out a summary for each type of the problem and how many of the
> > instaces of a particular problem have been optimized/fixed after the
> > e2fsck is done pointing to that full log for details ?
> 
> I think the standard way to handle this in e2fsck is with a "latch question",
> so that after the first or second 'y' with "answer 'y' to all questions".
> This will quiet most of the messages without disabling the optimization.

This will still print the message right ? Or am I mistaken ?

> 
> The other question is whether the "optimization" is worthwhile or not?
> Since e2fsck is rarely run, a number of unoptimized files will exist in
> the filesystem all the time.  In our case at least, files have a turnover
> rate, so optimizing the current set of inodes doesn't help much, since
> they would likely be deleted in a few weeks and new files will replace them.

I rarely see this and fundamentally I feel that any optimization we can
make in the rare opportunity the e2fsck is run, is worth it. But you
have a good point that it might not be all that helpful in some
situations. I trust your judgement on which trade-off is ultimately
better.

If you decide to do it this way, you can add

Reviewed-by: Lukas Czerner <lczerner@...hat.com>

Thanks!
-Lukas

> 
> Cheers, Andreas
> 
> >> 
> >> Signed-off-by: Andreas Dilger <adilger@...ger.ca>
> >> ---
> >> e2fsck/e2fsck.8.in | 4 ++--
> >> e2fsck/unix.c      | 7 +++++++
> >> 2 files changed, 9 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
> >> index 4e3890b..4f5086a 100644
> >> --- a/e2fsck/e2fsck.8.in
> >> +++ b/e2fsck/e2fsck.8.in
> >> @@ -228,12 +228,12 @@ exactly the opposite of discard option. This is set as default.
> >> .TP
> >> .BI no_optimize_extents
> >> Do not offer to optimize the extent tree by eliminating unnecessary
> >> -width or depth.  This can also be enabled in the options section of
> >> +width or depth.  This is the default unless otherwise specified in
> >> .BR /etc/e2fsck.conf .
> >> .TP
> >> .BI optimize_extents
> >> Offer to optimize the extent tree by eliminating unnecessary
> >> -width or depth.  This is the default unless otherwise specified in
> >> +width or depth.  This can also be enabled in the options section of
> >> .BR /etc/e2fsck.conf .
> >> .TP
> >> .BI inode_count_fullmap
> >> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> >> index 1b7ccea..445f806 100644
> >> --- a/e2fsck/unix.c
> >> +++ b/e2fsck/unix.c
> >> @@ -840,6 +840,8 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
> >> 	else
> >> 		ctx->program_name = "e2fsck";
> >> 
> >> +	ctx->options |= E2F_OPT_NOOPT_EXTENTS;
> >> +
> >> 	phys_mem_kb = get_memory_size() / 1024;
> >> 	ctx->readahead_kb = ~0ULL;
> >> 	while ((c = getopt(argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDkz:")) != EOF)
> >> @@ -1051,6 +1053,11 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
> >> 	if (c)
> >> 		ctx->options |= E2F_OPT_NOOPT_EXTENTS;
> >> 
> >> +	profile_get_boolean(ctx->profile, "options", "optimize_extents",
> >> +			    0, 0, &c);
> >> +	if (c)
> >> +		ctx->options &= ~E2F_OPT_NOOPT_EXTENTS;
> >> +
> >> 	profile_get_boolean(ctx->profile, "options", "inode_count_fullmap",
> >> 			    0, 0, &c);
> >> 	if (c)
> >> --
> >> 1.7.12.4
> >> 
> > 
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

Powered by blists - more mailing lists