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 <rgoldwyn@xxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
