Re: [PATCH] btrfs-progs: Fix extents after finding all errors

[David Sterba]

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

Re: [PATCH] btrfs-progs: Fix extents after finding all errors

[David Sterba]

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();
>   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();
> - 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