Re: [PATCH v2 09/17] btrfs-progs: lowmem check: start to remove parameters @trans in lowmem

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3370 bytes --]



On 2017年12月20日 12:57, Su Yue wrote:
> Since extents can be avoid overwrite by excluding or new chunk
> allocation. It's unnessesary to do all repairs in one transaction.
> 
> This patch removes parameter @trans of repair_extent_data_item().
> repair_extent_data_item() calls try_avoid_extents_overwrite()
> and starts a transaction by itself.
> 
> Note: This patch and next patches cause error in lowmem repair like:
> "Error: Commit_root already set when starting transaction".
> This error will disappear after removing @trans finished.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  cmds-check.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index bc405d0f3fc0..20bf3d230946 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -12064,18 +12064,19 @@ out:
>  	return err;
>  }
>  
> +static int try_avoid_extents_overwrite(struct btrfs_fs_info *fs_info);
>  /*
>   * If @err contains BACKREF_MISSING then add extent of the
>   * file_extent_data_item.
>   *
>   * Returns error bits after reapir.
>   */
> -static int repair_extent_data_item(struct btrfs_trans_handle *trans,
> -				   struct btrfs_root *root,
> +static int repair_extent_data_item(struct btrfs_root *root,
>  				   struct btrfs_path *pathp,
>  				   struct node_refs *nrefs,
>  				   int err)
>  {
> +	struct btrfs_trans_handle *trans = NULL;
>  	struct btrfs_file_extent_item *fi;
>  	struct btrfs_key fi_key;
>  	struct btrfs_key key;
> @@ -12092,6 +12093,7 @@ static int repair_extent_data_item(struct btrfs_trans_handle *trans,
>  	u64 file_offset;
>  	int generation;
>  	int slot;
> +	int need_insert = 0;
>  	int ret = 0;
>  
>  	eb = pathp->nodes[0];
> @@ -12130,9 +12132,20 @@ static int repair_extent_data_item(struct btrfs_trans_handle *trans,
>  		ret = -EIO;
>  		goto out;
>  	}
> +	need_insert = ret;
>  
> +	ret = try_avoid_extents_overwrite(root->fs_info);

Even it's much cheaper to pin extent tree using chunk unit, I don't
think it's a good idea to do it every time you are going to fix some
extent tree problem.

Why not just do it at the beginning of extent tree check?

Thanks,
Qu

> +	if (ret)
> +		goto out;
> +	trans = btrfs_start_transaction(root, 1);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		trans = NULL;
> +		error("fail to start transaction %s", strerror(-ret));
> +		goto out;
> +	}
>  	/* insert an extent item */
> -	if (ret > 0) {
> +	if (need_insert) {
>  		key.objectid = disk_bytenr;
>  		key.type = BTRFS_EXTENT_ITEM_KEY;
>  		key.offset = num_bytes;
> @@ -12172,6 +12185,8 @@ static int repair_extent_data_item(struct btrfs_trans_handle *trans,
>  
>  	err &= ~BACKREF_MISSING;
>  out:
> +	if (trans)
> +		btrfs_commit_transaction(trans, root);
>  	btrfs_release_path(&path);
>  	if (ret)
>  		error("can't repair root %llu extent data item[%llu %llu]",
> @@ -13445,8 +13460,7 @@ again:
>  	case BTRFS_EXTENT_DATA_KEY:
>  		ret = check_extent_data_item(root, path, nrefs, account_bytes);
>  		if (repair && ret)
> -			ret = repair_extent_data_item(trans, root, path, nrefs,
> -						      ret);
> +			ret = repair_extent_data_item(root, path, nrefs, ret);
>  		err |= ret;
>  		break;
>  	case BTRFS_BLOCK_GROUP_ITEM_KEY:
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]