| Message ID | 20171220045731.19343-3-suy.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20.12.2017 06:57, Su Yue wrote: > In lowmem mode with '--repair', check_chunks_and_extents_v2() > will fix accounting in block groups and clear the error > bit BG_ACCOUNTING_ERROR. > However, return value of check_btrfs_root() is 0 either 1 instead of > error bits. > > If extent tree is on error, lowmem repair always prints error and > returns nonzero value even the filesystem is fine after repair. > > So let @err contains bits after walk_down_tree_v2(). > > Introduce FATAL_ERROR for lowmem mode to represents negative return > values since negative and positive can't not be mixed in bits operations. > > Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> > --- > cmds-check.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/cmds-check.c b/cmds-check.c > index 309ac9553b3a..ebede26cef01 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -134,6 +134,7 @@ struct data_backref { > #define DIR_INDEX_MISMATCH (1<<19) /* INODE_INDEX found but not match */ > #define DIR_COUNT_AGAIN (1<<20) /* DIR isize should be recalculated */ > #define BG_ACCOUNTING_ERROR (1<<21) /* Block group accounting error */ > +#define FATAL_ERROR (1<<22) /* fatal bit for errno */ > > static inline struct data_backref* to_data_backref(struct extent_backref *back) > { > @@ -6556,7 +6557,7 @@ static struct data_backref *find_data_backref(struct extent_record *rec, > * otherwise means check fs tree(s) items relationship and > * @root MUST be a fs tree root. > * Returns 0 represents OK. > - * Returns not 0 represents error. > + * Returns > 0 represents error bits. > */ What about the code in 'if (!check_all)' branch, check_fs_first_inode can return a negative value, hence check_btrfs_root can return a negative value. A negative value can also be returned from btrfs_search_slot. Clearly this patch needs to be thought out better > static int check_btrfs_root(struct btrfs_trans_handle *trans, > struct btrfs_root *root, unsigned int ext_ref, > @@ -6607,12 +6608,12 @@ static int check_btrfs_root(struct btrfs_trans_handle *trans, > while (1) { > ret = walk_down_tree_v2(trans, root, &path, &level, &nrefs, > ext_ref, check_all); > - > - err |= !!ret; > + if (ret > 0) > + err |= ret; > > /* if ret is negative, walk shall stop */ > if (ret < 0) { > - ret = err; > + ret = err | FATAL_ERROR; > break; > } > > @@ -6636,12 +6637,12 @@ out: > * @ext_ref: the EXTENDED_IREF feature > * > * Return 0 if no error found. > - * Return <0 for error. > + * Return not 0 for error. > */ > static int check_fs_root_v2(struct btrfs_root *root, unsigned int ext_ref) > { > reset_cached_block_groups(root->fs_info); > - return check_btrfs_root(NULL, root, ext_ref, 0); > + return !!check_btrfs_root(NULL, root, ext_ref, 0); > } You make the function effectively boolean, make this explicit by changing its return value to bool. Also the name and the boolean return makes the function REALLY confusing. I.e when should we return true or false? As it stands it return "false" on success and "true" otherwise, this is a mess... > > /* > -- 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 12/29/2017 07:17 PM, Nikolay Borisov wrote: > > > On 20.12.2017 06:57, Su Yue wrote: >> In lowmem mode with '--repair', check_chunks_and_extents_v2() >> will fix accounting in block groups and clear the error >> bit BG_ACCOUNTING_ERROR. >> However, return value of check_btrfs_root() is 0 either 1 instead of >> error bits. >> >> If extent tree is on error, lowmem repair always prints error and >> returns nonzero value even the filesystem is fine after repair. >> >> So let @err contains bits after walk_down_tree_v2(). >> >> Introduce FATAL_ERROR for lowmem mode to represents negative return >> values since negative and positive can't not be mixed in bits operations. >> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> >> --- >> cmds-check.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/cmds-check.c b/cmds-check.c >> index 309ac9553b3a..ebede26cef01 100644 >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -134,6 +134,7 @@ struct data_backref { >> #define DIR_INDEX_MISMATCH (1<<19) /* INODE_INDEX found but not match */ >> #define DIR_COUNT_AGAIN (1<<20) /* DIR isize should be recalculated */ >> #define BG_ACCOUNTING_ERROR (1<<21) /* Block group accounting error */ >> +#define FATAL_ERROR (1<<22) /* fatal bit for errno */ >> >> static inline struct data_backref* to_data_backref(struct extent_backref *back) >> { >> @@ -6556,7 +6557,7 @@ static struct data_backref *find_data_backref(struct extent_record *rec, >> * otherwise means check fs tree(s) items relationship and >> * @root MUST be a fs tree root. >> * Returns 0 represents OK. >> - * Returns not 0 represents error. >> + * Returns > 0 represents error bits. >> */ > > What about the code in 'if (!check_all)' branch, check_fs_first_inode > can return a negative value, hence check_btrfs_root can return a > negative value. A negative value can also be returned from > btrfs_search_slot. > > Clearly this patch needs to be thought out better > >> static int check_btrfs_root(struct btrfs_trans_handle *trans, >> struct btrfs_root *root, unsigned int ext_ref, >> @@ -6607,12 +6608,12 @@ static int check_btrfs_root(struct btrfs_trans_handle *trans, >> while (1) { >> ret = walk_down_tree_v2(trans, root, &path, &level, &nrefs, >> ext_ref, check_all); >> - >> - err |= !!ret; >> + if (ret > 0) >> + err |= ret; >> >> /* if ret is negative, walk shall stop */ >> if (ret < 0) { >> - ret = err; >> + ret = err | FATAL_ERROR; >> break; >> } >> >> @@ -6636,12 +6637,12 @@ out: >> * @ext_ref: the EXTENDED_IREF feature >> * >> * Return 0 if no error found. >> - * Return <0 for error. >> + * Return not 0 for error. >> */ >> static int check_fs_root_v2(struct btrfs_root *root, unsigned int ext_ref) >> { >> reset_cached_block_groups(root->fs_info); >> - return check_btrfs_root(NULL, root, ext_ref, 0); >> + return !!check_btrfs_root(NULL, root, ext_ref, 0); >> } > > You make the function effectively boolean, make this explicit by > changing its return value to bool. Also the name and the boolean return > makes the function REALLY confusing. I.e when should we return true or In the past and present, check_fs_root_v2() always returns boolean. So the old annotation "Return <0 for error." is wrong. Here check_btrfs_root() returns error bits instead of boolean, so I just make check_fs_root_v2() return boolean explictly. > false? As it stands it return "false" on success and "true" otherwise, > this is a mess... > Although it returns 1 or 0, IMHO, let it return inteager is good enough. Thanks, Su > >> >> /* >> > > -- 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 12/29/2017 07:17 PM, Nikolay Borisov wrote: > > > On 20.12.2017 06:57, Su Yue wrote: >> In lowmem mode with '--repair', check_chunks_and_extents_v2() >> will fix accounting in block groups and clear the error >> bit BG_ACCOUNTING_ERROR. >> However, return value of check_btrfs_root() is 0 either 1 instead of >> error bits. >> >> If extent tree is on error, lowmem repair always prints error and >> returns nonzero value even the filesystem is fine after repair. >> >> So let @err contains bits after walk_down_tree_v2(). >> >> Introduce FATAL_ERROR for lowmem mode to represents negative return >> values since negative and positive can't not be mixed in bits operations. >> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> >> --- >> cmds-check.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/cmds-check.c b/cmds-check.c >> index 309ac9553b3a..ebede26cef01 100644 >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -134,6 +134,7 @@ struct data_backref { >> #define DIR_INDEX_MISMATCH (1<<19) /* INODE_INDEX found but not match */ >> #define DIR_COUNT_AGAIN (1<<20) /* DIR isize should be recalculated */ >> #define BG_ACCOUNTING_ERROR (1<<21) /* Block group accounting error */ >> +#define FATAL_ERROR (1<<22) /* fatal bit for errno */ >> >> static inline struct data_backref* to_data_backref(struct extent_backref *back) >> { >> @@ -6556,7 +6557,7 @@ static struct data_backref *find_data_backref(struct extent_record *rec, >> * otherwise means check fs tree(s) items relationship and >> * @root MUST be a fs tree root. >> * Returns 0 represents OK. >> - * Returns not 0 represents error. >> + * Returns > 0 represents error bits. >> */ > > What about the code in 'if (!check_all)' branch, check_fs_first_inode > can return a negative value, hence check_btrfs_root can return a > negative value. A negative value can also be returned from > btrfs_search_slot. > > Clearly this patch needs to be thought out better > OK, I will update it. Thanks for review. >> static int check_btrfs_root(struct btrfs_trans_handle *trans, >> struct btrfs_root *root, unsigned int ext_ref, >> @@ -6607,12 +6608,12 @@ static int check_btrfs_root(struct btrfs_trans_handle *trans, >> while (1) { >> ret = walk_down_tree_v2(trans, root, &path, &level, &nrefs, >> ext_ref, check_all); >> - >> - err |= !!ret; >> + if (ret > 0) >> + err |= ret; >> >> /* if ret is negative, walk shall stop */ >> if (ret < 0) { >> - ret = err; >> + ret = err | FATAL_ERROR; >> break; >> } >> >> @@ -6636,12 +6637,12 @@ out: >> * @ext_ref: the EXTENDED_IREF feature >> * >> * Return 0 if no error found. >> - * Return <0 for error. >> + * Return not 0 for error. >> */ >> static int check_fs_root_v2(struct btrfs_root *root, unsigned int ext_ref) >> { >> reset_cached_block_groups(root->fs_info); >> - return check_btrfs_root(NULL, root, ext_ref, 0); >> + return !!check_btrfs_root(NULL, root, ext_ref, 0); >> } > > You make the function effectively boolean, make this explicit by > changing its return value to bool. Also the name and the boolean return > makes the function REALLY confusing. I.e when should we return true or > false? As it stands it return "false" on success and "true" otherwise, > this is a mess... > > >> >> /* >> > > -- 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/cmds-check.c b/cmds-check.c index 309ac9553b3a..ebede26cef01 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -134,6 +134,7 @@ struct data_backref { #define DIR_INDEX_MISMATCH (1<<19) /* INODE_INDEX found but not match */ #define DIR_COUNT_AGAIN (1<<20) /* DIR isize should be recalculated */ #define BG_ACCOUNTING_ERROR (1<<21) /* Block group accounting error */ +#define FATAL_ERROR (1<<22) /* fatal bit for errno */ static inline struct data_backref* to_data_backref(struct extent_backref *back) { @@ -6556,7 +6557,7 @@ static struct data_backref *find_data_backref(struct extent_record *rec, * otherwise means check fs tree(s) items relationship and * @root MUST be a fs tree root. * Returns 0 represents OK. - * Returns not 0 represents error. + * Returns > 0 represents error bits. */ static int check_btrfs_root(struct btrfs_trans_handle *trans, struct btrfs_root *root, unsigned int ext_ref, @@ -6607,12 +6608,12 @@ static int check_btrfs_root(struct btrfs_trans_handle *trans, while (1) { ret = walk_down_tree_v2(trans, root, &path, &level, &nrefs, ext_ref, check_all); - - err |= !!ret; + if (ret > 0) + err |= ret; /* if ret is negative, walk shall stop */ if (ret < 0) { - ret = err; + ret = err | FATAL_ERROR; break; } @@ -6636,12 +6637,12 @@ out: * @ext_ref: the EXTENDED_IREF feature * * Return 0 if no error found. - * Return <0 for error. + * Return not 0 for error. */ static int check_fs_root_v2(struct btrfs_root *root, unsigned int ext_ref) { reset_cached_block_groups(root->fs_info); - return check_btrfs_root(NULL, root, ext_ref, 0); + return !!check_btrfs_root(NULL, root, ext_ref, 0); } /*
In lowmem mode with '--repair', check_chunks_and_extents_v2() will fix accounting in block groups and clear the error bit BG_ACCOUNTING_ERROR. However, return value of check_btrfs_root() is 0 either 1 instead of error bits. If extent tree is on error, lowmem repair always prints error and returns nonzero value even the filesystem is fine after repair. So let @err contains bits after walk_down_tree_v2(). Introduce FATAL_ERROR for lowmem mode to represents negative return values since negative and positive can't not be mixed in bits operations. Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com> --- cmds-check.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)