[PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[Daniel Xu]

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(+)

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;
 
-- 
2.27.0

Re: [PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[Josef Bacik]

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

Re: [PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[David Sterba]

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.

Re: [PATCH] btrfs-progs: --init-extent-tree if extent tree is unreadable

[David Sterba]

On Wed, Aug 12, 2020 at 07:14:44PM +0200, David Sterba wrote:
> 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.

To answer that, favoring safety over convenience here, so the option
needs to be specified manually if needed.