Re: [PATCH] btrfs: drop logs when we've aborted a transaction

[Nikolay Borisov <nborisov@suse.com>]

On 24.03.20 г. 16:47 ч., Josef Bacik wrote:
> Dave reported a problem where we were panicing with generic/475 with
> misc-5.7.  This is because we were doing IO after we had stopped all of

This doesn't seem correct. Before, the log tree would be freed in
btrfs_free_fs_roots which is called before btrfs_stop_all_workers and
only with transaction_kthread and cleaner_kthread stopped. I'd expect
that now btrfs_cleanup_transaction will be called from
btrfs_error_commit_super. Perhaps it's not "after we had stopped all of
the worker threads" but simply "We have stopped transaction/cleaner
kthreads" But then again I don't see how those 2 make any difference.

Could it be that this is not a full fix for the issue, and it can still
crash in case it's called from btrfs_error_commit_super ?

> the worker threads, because we do the log tree cleanup on roots at drop
> time.  Cleaning up the log tree may need to do reads if we happened to
> have evicted the blocks from memory.
> 
> Because of this simply add a helper to btrfs_cleanup_transaction() that
> will go through and drop all of the log roots.  This gets run before we
> do the close_ctree() work, and thus we are allowed to do any reads that
> we would need.  I ran this through many iterations of generic/475 with
> constrained memory and I did not see the issue.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> ---
>  fs/btrfs/disk-io.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a6cb5cbbdb9f..d10c7be10f3b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2036,9 +2036,6 @@ void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
>  		for (i = 0; i < ret; i++)
>  			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
>  	}
> -
> -	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> -		btrfs_free_log_root_tree(NULL, fs_info);
>  }
>  
>  static void btrfs_init_scrub(struct btrfs_fs_info *fs_info)
> @@ -3888,7 +3885,7 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>  	spin_unlock(&fs_info->fs_roots_radix_lock);
>  
>  	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
> -		btrfs_free_log(NULL, root);
> +		ASSERT(root->log_root == NULL);
>  		if (root->reloc_root) {
>  			btrfs_put_root(root->reloc_root);
>  			root->reloc_root = NULL;
> @@ -4211,6 +4208,36 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info)
>  	up_write(&fs_info->cleanup_work_sem);
>  }
>  
> +static void btrfs_drop_all_logs(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_root *gang[8];
> +	u64 root_objectid = 0;
> +	int ret;
> +
> +	spin_lock(&fs_info->fs_roots_radix_lock);
> +	while ((ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> +					     (void **)gang, root_objectid,
> +					     ARRAY_SIZE(gang))) != 0) {
> +		int i;
> +
> +		for (i = 0; i < ret; i++)
> +			gang[i] = btrfs_grab_root(gang[i]);
> +		spin_unlock(&fs_info->fs_roots_radix_lock);
> +
> +		for (i = 0; i < ret; i++) {
> +			if (!gang[i])
> +				continue;
> +			root_objectid = gang[i]->root_key.objectid;
> +			btrfs_free_log(NULL, gang[i]);
> +			btrfs_put_root(gang[i]);
> +		}
> +		root_objectid++;
> +		spin_lock(&fs_info->fs_roots_radix_lock);
> +	}
> +	spin_unlock(&fs_info->fs_roots_radix_lock);
> +	btrfs_free_log_root_tree(NULL, fs_info);
> +}
> +
>  static void btrfs_destroy_ordered_extents(struct btrfs_root *root)
>  {
>  	struct btrfs_ordered_extent *ordered;
> @@ -4603,6 +4630,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
>  	btrfs_destroy_delayed_inodes(fs_info);
>  	btrfs_assert_delayed_root_empty(fs_info);
>  	btrfs_destroy_all_delalloc_inodes(fs_info);
> +	btrfs_drop_all_logs(fs_info);
>  	mutex_unlock(&fs_info->transaction_kthread_mutex);
>  
>  	return 0;
>