Re: [PATCH] btrfs-progs: Fix extents after finding all errors
On Thu, Dec 01, 2016 at 11:28:08AM -0600, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues > > Simplifying the logic of fixing. > > Calling fixup_extent_ref() after encountering every error causes > more error messages after the extent is fixed. In case of multiple errors, > this is confusing because the error message is displayed after the fix > message and it works on stale data. It is best to show all errors and > then fix the extents. > > Set a variable and call fixup_extent_ref() if it is set. err is not used, > so cleared it. > > Changes since v1: > + assign fix from ret for a correct repair_abort code path > > Signed-off-by: Goldwyn Rodrigues Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] btrfs-progs: Fix extents after finding all errors
From: Goldwyn Rodrigues
Simplifying the logic of fixing.
Calling fixup_extent_ref() after encountering every error causes
more error messages after the extent is fixed. In case of multiple errors,
this is confusing because the error message is displayed after the fix
message and it works on stale data. It is best to show all errors and
then fix the extents.
Set a variable and call fixup_extent_ref() if it is set. err is not used,
so cleared it.
Changes since v1:
+ assign fix from ret for a correct repair_abort code path
Signed-off-by: Goldwyn Rodrigues
---
cmds-check.c | 72 +++-
1 file changed, 23 insertions(+), 49 deletions(-)
diff --git a/cmds-check.c b/cmds-check.c
index 30eabb2..85eaa63 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8998,6 +8998,9 @@ out:
ret = err;
}
+ if (!ret)
+ fprintf(stderr, "Repaired extent references for %llu\n",
(unsigned long long)rec->start);
+
btrfs_release_path(&path);
return ret;
}
@@ -9055,7 +9058,11 @@ static int fixup_extent_flags(struct btrfs_fs_info
*fs_info,
btrfs_set_extent_flags(path.nodes[0], ei, flags);
btrfs_mark_buffer_dirty(path.nodes[0]);
btrfs_release_path(&path);
- return btrfs_commit_transaction(trans, root);
+ ret = btrfs_commit_transaction(trans, root);
+ if (!ret)
+ fprintf(stderr, "Repaired extent flags for %llu\n", (unsigned
long long)rec->start);
+
+ return ret;
}
/* right now we only prune from the extent allocation tree */
@@ -9182,11 +9189,8 @@ static int check_extent_refs(struct btrfs_root *root,
{
struct extent_record *rec;
struct cache_extent *cache;
- int err = 0;
int ret = 0;
- int fixed = 0;
int had_dups = 0;
- int recorded = 0;
if (repair) {
/*
@@ -9255,9 +9259,8 @@ static int check_extent_refs(struct btrfs_root *root,
while(1) {
int cur_err = 0;
+ int fix = 0;
- fixed = 0;
- recorded = 0;
cache = search_cache_extent(extent_cache, 0);
if (!cache)
break;
@@ -9265,7 +9268,6 @@ static int check_extent_refs(struct btrfs_root *root,
if (rec->num_duplicates) {
fprintf(stderr, "extent item %llu has multiple extent "
"items\n", (unsigned long long)rec->start);
- err = 1;
cur_err = 1;
}
@@ -9279,54 +9281,31 @@ static int check_extent_refs(struct btrfs_root *root,
ret = record_orphan_data_extents(root->fs_info, rec);
if (ret < 0)
goto repair_abort;
- if (ret == 0) {
- recorded = 1;
- } else {
- /*
-* we can't use the extent to repair file
-* extent, let the fallback method handle it.
-*/
- if (!fixed && repair) {
- ret = fixup_extent_refs(
- root->fs_info,
- extent_cache, rec);
- if (ret)
- goto repair_abort;
- fixed = 1;
- }
- }
- err = 1;
+ fix = ret;
cur_err = 1;
}
if (all_backpointers_checked(rec, 1)) {
fprintf(stderr, "backpointer mismatch on [%llu %llu]\n",
(unsigned long long)rec->start,
(unsigned long long)rec->nr);
-
- if (!fixed && !recorded && repair) {
- ret = fixup_extent_refs(root->fs_info,
- extent_cache, rec);
- if (ret)
- goto repair_abort;
- fixed = 1;
- }
+ fix = 1;
cur_err = 1;
- err = 1;
}
if (!rec->owner_ref_checked) {
fprintf(stderr, "owner ref check failed [%llu %llu]\n",
(unsigned long long)rec->start,
(unsigned long long)rec->nr);
- if (!fixed && !recorded && repair) {
- ret = fixup_extent_refs(root->fs_info,
-
Re: [PATCH] btrfs-progs: Fix extents after finding all errors
On Thu, Nov 10, 2016 at 09:01:47AM -0600, Goldwyn Rodrigues wrote:
> Simplifying the logic of fixing.
>
> Calling fixup_extent_ref() after encountering every error causes
> more error messages after the extent is fixed. In case of multiple errors,
> this is confusing because the error message is displayed after the fix
> message and it works on stale data. It is best to show all errors and
> then fix the extents.
>
> Set a variable and call fixup_extent_ref() if it is set. err is not used,
> so cleared it.
Sounds ok, more comments below.
> Signed-off-by: Goldwyn Rodrigues
> ---
> cmds-check.c | 75
> +++-
> 1 file changed, 24 insertions(+), 51 deletions(-)
>
> diff --git a/cmds-check.c b/cmds-check.c
> index 779870a..8fa0b38 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -8994,6 +8994,9 @@ out:
> ret = err;
> }
>
> + if (!ret)
> + fprintf(stderr, "Repaired extent references for %llu\n",
> (unsigned long long)rec->start);
Line too long, please stick to ~80 chars, here it's easy to break line
after string.
> +
> btrfs_release_path(&path);
> return ret;
> }
> @@ -9051,7 +9054,11 @@ static int fixup_extent_flags(struct btrfs_fs_info
> *fs_info,
> btrfs_set_extent_flags(path.nodes[0], ei, flags);
> btrfs_mark_buffer_dirty(path.nodes[0]);
> btrfs_release_path(&path);
> - return btrfs_commit_transaction(trans, root);
> + ret = btrfs_commit_transaction(trans, root);
> + if (!ret)
> + fprintf(stderr, "Repaired extent flags for %llu\n", (unsigned
> long long)rec->start);
> +
> + return ret;
> }
>
> /* right now we only prune from the extent allocation tree */
> @@ -9178,11 +9185,8 @@ static int check_extent_refs(struct btrfs_root *root,
> {
> struct extent_record *rec;
> struct cache_extent *cache;
> - int err = 0;
> int ret = 0;
> - int fixed = 0;
> int had_dups = 0;
> - int recorded = 0;
>
> if (repair) {
> /*
> @@ -9251,9 +9255,8 @@ static int check_extent_refs(struct btrfs_root *root,
>
> while(1) {
> int cur_err = 0;
> + int fix = 0;
>
> - fixed = 0;
> - recorded = 0;
> cache = search_cache_extent(extent_cache, 0);
> if (!cache)
> break;
> @@ -9261,7 +9264,6 @@ static int check_extent_refs(struct btrfs_root *root,
> if (rec->num_duplicates) {
> fprintf(stderr, "extent item %llu has multiple extent "
> "items\n", (unsigned long long)rec->start);
> - err = 1;
> cur_err = 1;
> }
>
> @@ -9272,57 +9274,33 @@ static int check_extent_refs(struct btrfs_root *root,
> fprintf(stderr, "extent item %llu, found %llu\n",
> (unsigned long long)rec->extent_item_refs,
> (unsigned long long)rec->refs);
> - ret = record_orphan_data_extents(root->fs_info, rec);
> - if (ret < 0)
> + fix = record_orphan_data_extents(root->fs_info, rec);
> + if (fix < 0)
> goto repair_abort;
I think ret has to be set to fix here as well (in some way, eg. not
using fix for a return value), otherwise the repair_abort label will not
thake the same code path as before.
> - if (ret == 0) {
> - recorded = 1;
> - } else {
> - /*
> - * we can't use the extent to repair file
> - * extent, let the fallback method handle it.
> - */
> - if (!fixed && repair) {
> - ret = fixup_extent_refs(
> - root->fs_info,
> - extent_cache, rec);
> - if (ret)
> - goto repair_abort;
> - fixed = 1;
> - }
> - }
> - err = 1;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] btrfs-progs: Fix extents after finding all errors
Simplifying the logic of fixing.
Calling fixup_extent_ref() after encountering every error causes
more error messages after the extent is fixed. In case of multiple errors,
this is confusing because the error message is displayed after the fix
message and it works on stale data. It is best to show all errors and
then fix the extents.
Set a variable and call fixup_extent_ref() if it is set. err is not used,
so cleared it.
Signed-off-by: Goldwyn Rodrigues
---
cmds-check.c | 75 +++-
1 file changed, 24 insertions(+), 51 deletions(-)
diff --git a/cmds-check.c b/cmds-check.c
index 779870a..8fa0b38 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8994,6 +8994,9 @@ out:
ret = err;
}
+ if (!ret)
+ fprintf(stderr, "Repaired extent references for %llu\n",
(unsigned long long)rec->start);
+
btrfs_release_path(&path);
return ret;
}
@@ -9051,7 +9054,11 @@ static int fixup_extent_flags(struct btrfs_fs_info
*fs_info,
btrfs_set_extent_flags(path.nodes[0], ei, flags);
btrfs_mark_buffer_dirty(path.nodes[0]);
btrfs_release_path(&path);
- return btrfs_commit_transaction(trans, root);
+ ret = btrfs_commit_transaction(trans, root);
+ if (!ret)
+ fprintf(stderr, "Repaired extent flags for %llu\n", (unsigned
long long)rec->start);
+
+ return ret;
}
/* right now we only prune from the extent allocation tree */
@@ -9178,11 +9185,8 @@ static int check_extent_refs(struct btrfs_root *root,
{
struct extent_record *rec;
struct cache_extent *cache;
- int err = 0;
int ret = 0;
- int fixed = 0;
int had_dups = 0;
- int recorded = 0;
if (repair) {
/*
@@ -9251,9 +9255,8 @@ static int check_extent_refs(struct btrfs_root *root,
while(1) {
int cur_err = 0;
+ int fix = 0;
- fixed = 0;
- recorded = 0;
cache = search_cache_extent(extent_cache, 0);
if (!cache)
break;
@@ -9261,7 +9264,6 @@ static int check_extent_refs(struct btrfs_root *root,
if (rec->num_duplicates) {
fprintf(stderr, "extent item %llu has multiple extent "
"items\n", (unsigned long long)rec->start);
- err = 1;
cur_err = 1;
}
@@ -9272,57 +9274,33 @@ static int check_extent_refs(struct btrfs_root *root,
fprintf(stderr, "extent item %llu, found %llu\n",
(unsigned long long)rec->extent_item_refs,
(unsigned long long)rec->refs);
- ret = record_orphan_data_extents(root->fs_info, rec);
- if (ret < 0)
+ fix = record_orphan_data_extents(root->fs_info, rec);
+ if (fix < 0)
goto repair_abort;
- if (ret == 0) {
- recorded = 1;
- } else {
- /*
-* we can't use the extent to repair file
-* extent, let the fallback method handle it.
-*/
- if (!fixed && repair) {
- ret = fixup_extent_refs(
- root->fs_info,
- extent_cache, rec);
- if (ret)
- goto repair_abort;
- fixed = 1;
- }
- }
- err = 1;
cur_err = 1;
}
if (all_backpointers_checked(rec, 1)) {
fprintf(stderr, "backpointer mismatch on [%llu %llu]\n",
(unsigned long long)rec->start,
(unsigned long long)rec->nr);
-
- if (!fixed && !recorded && repair) {
- ret = fixup_extent_refs(root->fs_info,
- extent_cache, rec);
- if (ret)
- goto repair_abort;
- fixed = 1;
- }
+ fix = 1;
cur_err = 1;
- err = 1;
}
if (!rec->owner_ref_checked) {
fprintf(stderr, "owner ref check failed [%llu %llu]\n",
(unsigned long long)rec->start,
