[PATCH] e2fsck: skip extent optimization by default

[PATCH] e2fsck: skip extent optimization by default

[adilger]

From: Andreas Dilger <adilger@whamcloud.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.

Signed-off-by: Andreas Dilger <adilger@dilger.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

Re: [PATCH] e2fsck: skip extent optimization by default

[Lukas Czerner]

On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@whamcloud.com wrote:
> From: Andreas Dilger <adilger@whamcloud.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.

Hi Andreas,

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 ?

-Lukas

> 
> Signed-off-by: Andreas Dilger <adilger@dilger.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
> 

Re: [PATCH] e2fsck: skip extent optimization by default

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4408 bytes --]

On Sep 22, 2020, at 4:26 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@whamcloud.com wrote:
>> From: Andreas Dilger <adilger@whamcloud.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.

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.

Cheers, Andreas

>> 
>> Signed-off-by: Andreas Dilger <adilger@dilger.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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] e2fsck: skip extent optimization by default

[Lukas Czerner]

On Tue, Sep 22, 2020 at 11:42:40AM -0600, Andreas Dilger wrote:
> On Sep 22, 2020, at 4:26 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> > 
> > On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@whamcloud.com wrote:
> >> From: Andreas Dilger <adilger@whamcloud.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@redhat.com>

Thanks!
-Lukas

> 
> Cheers, Andreas
> 
> >> 
> >> Signed-off-by: Andreas Dilger <adilger@dilger.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
> 
> 
> 
> 
> 

Re: [PATCH] e2fsck: skip extent optimization by default

[Theodore Y. Ts'o]

On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@whamcloud.com wrote:
> From: Andreas Dilger <adilger@whamcloud.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.
> 
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>

Applying this patch causes a whole bunch of tests fail:

348 tests succeeded 9 tests failed
Tests failed: d_punch_bigalloc d_punch f_collapse_extent_tree
      f_compress_extent_tree_level f_extent_bad_node f_extent_int_bad_magic
      f_extent_leaf_bad_magic f_extent_oobounds f_quota_extent_opt


> @@ -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;
> +

We already have a no_optimize_extents option supported in e2fsck.conf.
So if we want to change the default, a simpler way to do this might be
to edit e2fsck.conf.5.in to simply add "no_optimize_extents=true" to
the default version of e2fsck.conf defined by default.

As a reminder, for future changes, when we add a new tunable to
e2fsck.conf or mke2fs.conf, the man page should be edited.  And please
do run the regression test suites before sending out a patch.   Thanks!!

       	   	      	   	  	 	 - Ted

Re: [PATCH] e2fsck: skip extent optimization by default

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

On Oct 1, 2020, at 12:03 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
> On Mon, Sep 21, 2020 at 04:16:02PM -0600, adilger@whamcloud.com wrote:
>> From: Andreas Dilger <adilger@whamcloud.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.
>> 
>> Signed-off-by: Andreas Dilger <adilger@dilger.ca>
> 
> Applying this patch causes a whole bunch of tests fail:
> 
> 348 tests succeeded 9 tests failed
> Tests failed: d_punch_bigalloc d_punch f_collapse_extent_tree
>      f_compress_extent_tree_level f_extent_bad_node f_extent_int_bad_magic
>      f_extent_leaf_bad_magic f_extent_oobounds f_quota_extent_opt

Sorry about that, I usually *do* run the tests after every patch, I'm not
sure why I didn't for this patch.

>> @@ -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;
>> +
> 
> We already have a no_optimize_extents option supported in e2fsck.conf.
> So if we want to change the default, a simpler way to do this might be
> to edit e2fsck.conf.5.in to simply add "no_optimize_extents=true" to
> the default version of e2fsck.conf defined by default.

Does that mean you *don't* want a refresh of this patch that fixes the
test cases?  Lukas had also been discussing how to change e2fsck so it
still fixed the inodes, but didn't print a message for each one, though
it wasn't clear to me that there is much benefit to this at all.

> As a reminder, for future changes, when we add a new tunable to
> e2fsck.conf or mke2fs.conf, the man page should be edited.

Yes, I did edit the e2fsck.8.in man page to describe the change in
default behavior.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] e2fsck: skip extent optimization by default

[Theodore Y. Ts'o]

On Thu, Oct 01, 2020 at 08:11:06PM -0600, Andreas Dilger wrote:
> > We already have a no_optimize_extents option supported in e2fsck.conf.
> > So if we want to change the default, a simpler way to do this might be
> > to edit e2fsck.conf.5.in to simply add "no_optimize_extents=true" to
> > the default version of e2fsck.conf defined by default.
> 
> Does that mean you *don't* want a refresh of this patch that fixes the
> test cases?  Lukas had also been discussing how to change e2fsck so it
> still fixed the inodes, but didn't print a message for each one, though
> it wasn't clear to me that there is much benefit to this at all.

I think that if e2fsck is going to make a change, we need to print
something --- otherwise people will get confused since e2fsck will say
"file system modified", and without any kind of messages, people will
get confused in a different way.  It also makes it hard to debug if
e2fsck doesn't print anything at all.

Yet another approach would be change the messaging so that it's more
clear that e2fsck is optimizing the extent tree.

In the long run, the really right way this fix is to have the kernel
optimize the extent tree at runtime, so we don't need to let e2fsck do
things.  So it may be that simply changing the default e2fsck.conf
might be a better approach.  At least, we should consider that
alternative, which is why I suggested.
> 
> > As a reminder, for future changes, when we add a new tunable to
> > e2fsck.conf or mke2fs.conf, the man page should be edited.
> 
> Yes, I did edit the e2fsck.8.in man page to describe the change in
> default behavior.

I was referring to the e2fsck.conf.5.in man page.  If we're going to
add a new tunable to e2fsck.conf, it really needs to be documented in
the e2fsck.conf(5) man page.

Cheers,

					- Ted