| Message ID | 20200728021224.148671-1-dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: --init-extent-tree if extent tree is unreadable | expand |
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
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; > >
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.
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.
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;
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(+)