| Message ID | 20180705073731.18459-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo > Sent: Thursday, July 05, 2018 3:37 PM > To: linux-btrfs@vger.kernel.org > Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks > > We have function btrfs_fsck_reinit_root() to reinit csum or extent tree. Nit： or fs/file tree > However this function allows us to let it overwrite existing tree blocks > using @overwrite parameter. > > Such behavior is pretty dangerous while no caller is using this feature > explicitly. > > So just remove @overwrite parameter and allow btrfs_fsck_reinit_root() > to error out when it fails to allocate tree block. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > check/main.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/check/main.c b/check/main.c > index 8db300abb825..c8c347236543 100644 > --- a/check/main.c > +++ b/check/main.c > @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info) > } > > static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, > - struct btrfs_root *root, int overwrite) > + struct btrfs_root *root) > { > struct extent_buffer *c; > struct extent_buffer *old = root->node; > @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, > > level = 0; > > - if (overwrite) { > - c = old; > - extent_buffer_get(c); > - goto init; > - } > c = btrfs_alloc_free_block(trans, root, > root->fs_info->nodesize, > root->root_key.objectid, > &disk_key, level, 0, 0); > - if (IS_ERR(c)) { > - c = old; > - extent_buffer_get(c); > - overwrite = 1; > - } > -init: > + if (IS_ERR(c)) > + return PTR_ERR(c); > + > memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header)); > btrfs_set_header_level(c, level); > btrfs_set_header_bytenr(c, c->start); > @@ -8422,9 +8414,7 @@ init: > /* > * this case can happen in the following case: > * > - * 1.overwrite previous root. > - * > - * 2.reinit reloc data root, this is because we skip pin > + * reinit reloc data root, this is because we skip pin > * down reloc data tree before which means we can allocate > * same block bytenr here. > */ > @@ -8609,7 +8599,7 @@ reinit_data_reloc: > goto out; > } > record_root_in_trans(trans, root); > - ret = btrfs_fsck_reinit_root(trans, root, 0); > + ret = btrfs_fsck_reinit_root(trans, root); > if (ret) > goto out; > ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID); > @@ -8675,7 +8665,7 @@ again: > } > > /* Ok we can allocate now, reinit the extent root */ > - ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0); > + ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root); > if (ret) { > fprintf(stderr, "extent root initialization failed\n"); > /* > @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv) > > if (init_csum_tree) { > printf("Reinitialize checksum tree\n"); > - ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0); > + ret = btrfs_fsck_reinit_root(trans, info->csum_root); > if (ret) { > error("checksum tree initialization failed: %d", > ret); > -- Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com> > 2.18.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 2018年07月05日 16:50, Gu, Jinxiang wrote: > > >> -----Original Message----- >> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo >> Sent: Thursday, July 05, 2018 3:37 PM >> To: linux-btrfs@vger.kernel.org >> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks >> >> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree. > > Nit： > or fs/file tree Nope, btrfs check is only capable of reinit csum or extent tree. Thanks, Qu > >> However this function allows us to let it overwrite existing tree blocks >> using @overwrite parameter. >> >> Such behavior is pretty dangerous while no caller is using this feature >> explicitly. >> >> So just remove @overwrite parameter and allow btrfs_fsck_reinit_root() >> to error out when it fails to allocate tree block. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> check/main.c | 26 ++++++++------------------ >> 1 file changed, 8 insertions(+), 18 deletions(-) >> >> diff --git a/check/main.c b/check/main.c >> index 8db300abb825..c8c347236543 100644 >> --- a/check/main.c >> +++ b/check/main.c >> @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info) >> } >> >> static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, >> - struct btrfs_root *root, int overwrite) >> + struct btrfs_root *root) >> { >> struct extent_buffer *c; >> struct extent_buffer *old = root->node; >> @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, >> >> level = 0; >> >> - if (overwrite) { >> - c = old; >> - extent_buffer_get(c); >> - goto init; >> - } >> c = btrfs_alloc_free_block(trans, root, >> root->fs_info->nodesize, >> root->root_key.objectid, >> &disk_key, level, 0, 0); >> - if (IS_ERR(c)) { >> - c = old; >> - extent_buffer_get(c); >> - overwrite = 1; >> - } >> -init: >> + if (IS_ERR(c)) >> + return PTR_ERR(c); >> + >> memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header)); >> btrfs_set_header_level(c, level); >> btrfs_set_header_bytenr(c, c->start); >> @@ -8422,9 +8414,7 @@ init: >> /* >> * this case can happen in the following case: >> * >> - * 1.overwrite previous root. >> - * >> - * 2.reinit reloc data root, this is because we skip pin >> + * reinit reloc data root, this is because we skip pin >> * down reloc data tree before which means we can allocate >> * same block bytenr here. >> */ >> @@ -8609,7 +8599,7 @@ reinit_data_reloc: >> goto out; >> } >> record_root_in_trans(trans, root); >> - ret = btrfs_fsck_reinit_root(trans, root, 0); >> + ret = btrfs_fsck_reinit_root(trans, root); >> if (ret) >> goto out; >> ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID); >> @@ -8675,7 +8665,7 @@ again: >> } >> >> /* Ok we can allocate now, reinit the extent root */ >> - ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0); >> + ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root); >> if (ret) { >> fprintf(stderr, "extent root initialization failed\n"); >> /* >> @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv) >> >> if (init_csum_tree) { >> printf("Reinitialize checksum tree\n"); >> - ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0); >> + ret = btrfs_fsck_reinit_root(trans, info->csum_root); >> if (ret) { >> error("checksum tree initialization failed: %d", >> ret); >> -- > > Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com> > > >> 2.18.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > > N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w* jg� 秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤 瓽珴閔�鎗:+v墾妛鑶佶 >
> -----Original Message----- > From: Qu Wenruo [mailto:quwenruo.btrfs@gmx.com] > Sent: Thursday, July 05, 2018 5:41 PM > To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; Qu Wenruo <wqu@suse.com>; linux-btrfs@vger.kernel.org > Subject: Re: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks > > > > On 2018年07月05日 16:50, Gu, Jinxiang wrote: > > > > > >> -----Original Message----- > >> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo > >> Sent: Thursday, July 05, 2018 3:37 PM > >> To: linux-btrfs@vger.kernel.org > >> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks > >> > >> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree. > > > > Nit： > > or fs/file tree > > Nope, btrfs check is only capable of reinit csum or extent tree. Sorry, not fs/file tree. But DATA RELOC TREE. Call stack is reset_balance -> btrfs_fsck_reinit_root, and the root is get by btrfs_read_fs_root(fs_info, &key), which key is (BTRFS_DATA_RELOC_TREE_OBJECTID, BTRFS_ROOT_ITEM_KEY , (u64)-1) > > Thanks, > Qu > > > > >> However this function allows us to let it overwrite existing tree blocks > >> using @overwrite parameter. > >> > >> Such behavior is pretty dangerous while no caller is using this feature > >> explicitly. > >> > >> So just remove @overwrite parameter and allow btrfs_fsck_reinit_root() > >> to error out when it fails to allocate tree block. > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> check/main.c | 26 ++++++++------------------ > >> 1 file changed, 8 insertions(+), 18 deletions(-) > >> > >> diff --git a/check/main.c b/check/main.c > >> index 8db300abb825..c8c347236543 100644 > >> --- a/check/main.c > >> +++ b/check/main.c > >> @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info) > >> } > >> > >> static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, > >> - struct btrfs_root *root, int overwrite) > >> + struct btrfs_root *root) > >> { > >> struct extent_buffer *c; > >> struct extent_buffer *old = root->node; > >> @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, > >> > >> level = 0; > >> > >> - if (overwrite) { > >> - c = old; > >> - extent_buffer_get(c); > >> - goto init; > >> - } > >> c = btrfs_alloc_free_block(trans, root, > >> root->fs_info->nodesize, > >> root->root_key.objectid, > >> &disk_key, level, 0, 0); > >> - if (IS_ERR(c)) { > >> - c = old; > >> - extent_buffer_get(c); > >> - overwrite = 1; > >> - } > >> -init: > >> + if (IS_ERR(c)) > >> + return PTR_ERR(c); > >> + > >> memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header)); > >> btrfs_set_header_level(c, level); > >> btrfs_set_header_bytenr(c, c->start); > >> @@ -8422,9 +8414,7 @@ init: > >> /* > >> * this case can happen in the following case: > >> * > >> - * 1.overwrite previous root. > >> - * > >> - * 2.reinit reloc data root, this is because we skip pin > >> + * reinit reloc data root, this is because we skip pin > >> * down reloc data tree before which means we can allocate > >> * same block bytenr here. > >> */ > >> @@ -8609,7 +8599,7 @@ reinit_data_reloc: > >> goto out; > >> } > >> record_root_in_trans(trans, root); > >> - ret = btrfs_fsck_reinit_root(trans, root, 0); > >> + ret = btrfs_fsck_reinit_root(trans, root); > >> if (ret) > >> goto out; > >> ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID); > >> @@ -8675,7 +8665,7 @@ again: > >> } > >> > >> /* Ok we can allocate now, reinit the extent root */ > >> - ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0); > >> + ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root); > >> if (ret) { > >> fprintf(stderr, "extent root initialization failed\n"); > >> /* > >> @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv) > >> > >> if (init_csum_tree) { > >> printf("Reinitialize checksum tree\n"); > >> - ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0); > >> + ret = btrfs_fsck_reinit_root(trans, info->csum_root); > >> if (ret) { > >> error("checksum tree initialization failed: %d", > >> ret); > >> -- > > > > Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com> > > > > > >> 2.18.0 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > > > > > > > > N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w*jg� 秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤 瓽珴閔�鎗:+v墾 > 妛鑶佶 > >
On Fri, Jul 06, 2018 at 01:53:24AM +0000, Gu, Jinxiang wrote: > > >> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks > > >> > > >> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree. > > > > > > Nit： > > > or fs/file tree > > > > Nope, btrfs check is only capable of reinit csum or extent tree. > Sorry, not fs/file tree. But DATA RELOC TREE. > Call stack is reset_balance -> btrfs_fsck_reinit_root, and the root is get by > btrfs_read_fs_root(fs_info, &key), which key is > (BTRFS_DATA_RELOC_TREE_OBJECTID, BTRFS_ROOT_ITEM_KEY , (u64)-1) So, it's correct that the data reloc tree is reinitialized, but this is part of extent tree reset and there's no separate commandline option for it. I understand the changelog as a reference to --init-csum-tree and --init-extent-tree . -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/check/main.c b/check/main.c index 8db300abb825..c8c347236543 100644 --- a/check/main.c +++ b/check/main.c @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info) } static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, - struct btrfs_root *root, int overwrite) + struct btrfs_root *root) { struct extent_buffer *c; struct extent_buffer *old = root->node; @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans, level = 0; - if (overwrite) { - c = old; - extent_buffer_get(c); - goto init; - } c = btrfs_alloc_free_block(trans, root, root->fs_info->nodesize, root->root_key.objectid, &disk_key, level, 0, 0); - if (IS_ERR(c)) { - c = old; - extent_buffer_get(c); - overwrite = 1; - } -init: + if (IS_ERR(c)) + return PTR_ERR(c); + memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header)); btrfs_set_header_level(c, level); btrfs_set_header_bytenr(c, c->start); @@ -8422,9 +8414,7 @@ init: /* * this case can happen in the following case: * - * 1.overwrite previous root. - * - * 2.reinit reloc data root, this is because we skip pin + * reinit reloc data root, this is because we skip pin * down reloc data tree before which means we can allocate * same block bytenr here. */ @@ -8609,7 +8599,7 @@ reinit_data_reloc: goto out; } record_root_in_trans(trans, root); - ret = btrfs_fsck_reinit_root(trans, root, 0); + ret = btrfs_fsck_reinit_root(trans, root); if (ret) goto out; ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID); @@ -8675,7 +8665,7 @@ again: } /* Ok we can allocate now, reinit the extent root */ - ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0); + ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root); if (ret) { fprintf(stderr, "extent root initialization failed\n"); /* @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv) if (init_csum_tree) { printf("Reinitialize checksum tree\n"); - ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0); + ret = btrfs_fsck_reinit_root(trans, info->csum_root); if (ret) { error("checksum tree initialization failed: %d", ret);
We have function btrfs_fsck_reinit_root() to reinit csum or extent tree. However this function allows us to let it overwrite existing tree blocks using @overwrite parameter. Such behavior is pretty dangerous while no caller is using this feature explicitly. So just remove @overwrite parameter and allow btrfs_fsck_reinit_root() to error out when it fails to allocate tree block. Signed-off-by: Qu Wenruo <wqu@suse.com> --- check/main.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)