| Message ID | 20180904124128.22367-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: Continue checking even we found something wrong in free space cache | expand |
On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote: > No need to abort checking, especially for RO check free space cache is > meaningless, the errors in fs/extent tree is more interesting for > developers. > > So continue checking even something in free space cache is wrong. > > Reported-by: Etienne Champetier <champetier.etienne@gmail.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > check/main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/check/main.c b/check/main.c > index b361cd7e26a0..4f720163221e 100644 > --- a/check/main.c > +++ b/check/main.c > @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv) > error("errors found in free space tree"); > else > error("errors found in free space cache"); Please make it a warning and update the message so it says that it will continue despite the errors found. I noticed that error handling in check_space_cache is a bit fishy, some errors are stored to 'ret' but ignored at the end of the function.
On 2018/9/11 下午10:48, David Sterba wrote: > On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote: >> No need to abort checking, especially for RO check free space cache is >> meaningless, the errors in fs/extent tree is more interesting for >> developers. >> >> So continue checking even something in free space cache is wrong. >> >> Reported-by: Etienne Champetier <champetier.etienne@gmail.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> check/main.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/check/main.c b/check/main.c >> index b361cd7e26a0..4f720163221e 100644 >> --- a/check/main.c >> +++ b/check/main.c >> @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv) >> error("errors found in free space tree"); >> else >> error("errors found in free space cache"); > > Please make it a warning and update the message so it says that it will > continue despite the errors found. I'm fine to add warning, but isn't it the expected behavior? In fact I'm quite surprised that when we found something wrong we just abort checking. It's never the case for file/extent tree check. We always report all errors we found, not only the first error and exit. Thanks, Qu > > I noticed that error handling in check_space_cache is a bit fishy, some > errors are stored to 'ret' but ignored at the end of the function. >
On Wed, Sep 12, 2018 at 07:43:27AM +0800, Qu Wenruo wrote: > > > On 2018/9/11 下午10:48, David Sterba wrote: > > On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote: > >> No need to abort checking, especially for RO check free space cache is > >> meaningless, the errors in fs/extent tree is more interesting for > >> developers. > >> > >> So continue checking even something in free space cache is wrong. > >> > >> Reported-by: Etienne Champetier <champetier.etienne@gmail.com> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> check/main.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/check/main.c b/check/main.c > >> index b361cd7e26a0..4f720163221e 100644 > >> --- a/check/main.c > >> +++ b/check/main.c > >> @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv) > >> error("errors found in free space tree"); > >> else > >> error("errors found in free space cache"); > > > > Please make it a warning and update the message so it says that it will > > continue despite the errors found. > > I'm fine to add warning, but isn't it the expected behavior? > > In fact I'm quite surprised that when we found something wrong we just > abort checking. > > It's never the case for file/extent tree check. We always report all > errors we found, not only the first error and exit. You're right, in context of 'check' this is the expected behaviour. So let's keep error().
diff --git a/check/main.c b/check/main.c index b361cd7e26a0..4f720163221e 100644 --- a/check/main.c +++ b/check/main.c @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv) error("errors found in free space tree"); else error("errors found in free space cache"); - goto out; } /*
No need to abort checking, especially for RO check free space cache is meaningless, the errors in fs/extent tree is more interesting for developers. So continue checking even something in free space cache is wrong. Reported-by: Etienne Champetier <champetier.etienne@gmail.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- check/main.c | 1 - 1 file changed, 1 deletion(-)