btrfs-progs: --init-extent-tree if extent tree is unreadable
diff mbox series

Message ID 20200728021224.148671-1-dxu@dxuuu.xyz
State New
Headers show
Series
  • btrfs-progs: --init-extent-tree if extent tree is unreadable
Related show

Commit Message

Daniel Xu July 28, 2020, 2:12 a.m. UTC
This change can save the user an extra step of running `btrfs check
--init-extent-tree ...` if the user was already trying to repair the
filesystem.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 check/main.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Josef Bacik Aug. 11, 2020, 7 p.m. UTC | #1
On 7/27/20 10:12 PM, Daniel Xu wrote:
> This change can save the user an extra step of running `btrfs check
> --init-extent-tree ...` if the user was already trying to repair the
> filesystem.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
Qu Wenruo Aug. 12, 2020, 1:29 a.m. UTC | #2
On 2020/7/28 上午10:12, Daniel Xu wrote:
> This change can save the user an extra step of running `btrfs check
> --init-extent-tree ...` if the user was already trying to repair the
> filesystem.

This looks too aggressive to me.

Extent tree repair, not only --init-extent-tree, is not considered safe
overall.

In fact, we could hit cases with things like completely sane fs trees,
but corrupted extent and csum trees.

In that case, I'm not sure --init-extent-tree would solve or just worse
the situation.

Thus --init-extent-tree should only be triggered by users who know what
they are doing.
(In that case, I would call them developers other than users)

Thanks,
Qu

> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  check/main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/check/main.c b/check/main.c
> index f93bd7d4..4da17253 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -10243,6 +10243,10 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>  		goto close_out;
>  	}
>  
> +        /* Fallback to --init-extent-tree if extent tree is unreadable */
> +        if (!extent_buffer_uptodate(info->extent_root->node) && repair)
> +		init_extent_tree = true;
> +
>  	if (init_extent_tree || init_csum_tree) {
>  		struct btrfs_trans_handle *trans;
>  
>
David Sterba Aug. 12, 2020, 5:14 p.m. UTC | #3
On Wed, Aug 12, 2020 at 09:29:18AM +0800, Qu Wenruo wrote:
> On 2020/7/28 上午10:12, Daniel Xu wrote:
> > This change can save the user an extra step of running `btrfs check
> > --init-extent-tree ...` if the user was already trying to repair the
> > filesystem.
> 
> This looks too aggressive to me.
> 
> Extent tree repair, not only --init-extent-tree, is not considered safe
> overall.
> 
> In fact, we could hit cases with things like completely sane fs trees,
> but corrupted extent and csum trees.
> 
> In that case, I'm not sure --init-extent-tree would solve or just worse
> the situation.
> 
> Thus --init-extent-tree should only be triggered by users who know what
> they are doing.
> (In that case, I would call them developers other than users)

I have basically the same answer, just did not get to writing it.  I'll
have another look after the merge window is over.

This touches on the higher level topic what shoud check do, we can't
trade convenience for safety. The extra step to specify the option on
the command line can be actually the difference between repairing and
not repairing.

Patch
diff mbox series

diff --git a/check/main.c b/check/main.c
index f93bd7d4..4da17253 100644
--- a/check/main.c
+++ b/check/main.c
@@ -10243,6 +10243,10 @@  static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 		goto close_out;
 	}
 
+        /* Fallback to --init-extent-tree if extent tree is unreadable */
+        if (!extent_buffer_uptodate(info->extent_root->node) && repair)
+		init_extent_tree = true;
+
 	if (init_extent_tree || init_csum_tree) {
 		struct btrfs_trans_handle *trans;