Re: [PATCH] btrfs: fix deadlock between quota disable and qgroup rescan worker

[Nikolay Borisov <nborisov@suse.com>]

On 13.01.22 г. 12:40, Shin'ichiro Kawasaki wrote:
> Quota disable ioctl starts a transaction before waiting for the qgroup
> rescan worker completes. However, this wait can be infinite and results
> in deadlock because of circular dependency among the quota disable
> ioctl, the qgroup rescan worker and the other task with transaction such
> as block group relocation task.

<snip>

> Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  fs/btrfs/ctree.h  |  2 ++
>  fs/btrfs/qgroup.c | 20 ++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index b4a9b1c58d22..fe275697e3eb 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -145,6 +145,8 @@ enum {
>  	BTRFS_FS_STATE_DUMMY_FS_INFO,
>  
>  	BTRFS_FS_STATE_NO_CSUMS,
> +	/* Quota is in disabling process */
> +	BTRFS_FS_STATE_QUOTA_DISABLING,
>  };
>  
>  #define BTRFS_BACKREF_REV_MAX		256
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8928275823a1..6f94da4896bc 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1188,6 +1188,17 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root)
>  		goto out;
> +	/*
> +	 * Request qgroup rescan worker to complete and wait for it. This wait
> +	 * must be done before transaction start for quota disable since it may
> +	 * deadlock with transaction by the qgroup rescan worker.
> +	 */
> +	if (test_and_set_bit(BTRFS_FS_STATE_QUOTA_DISABLING,
> +			     &fs_info->fs_state)) {
> +		ret = -EBUSY;
> +		goto busy;
> +	}
> +	btrfs_qgroup_wait_for_completion(fs_info, false);

Actually you don't need to introduce a separate flag to have this logic,
simply clear QUOTA_ENABLED, do the wait, start the transaction - in case
of failure i.e not able to get a trans handle just set QUOTA_ENABLED
again. Still, moving QUOTA_ENABLED check in rescan_should_stop would be
useful as well.

>  	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  
>  	/*
> @@ -1212,7 +1223,6 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  		goto out;
>  
>  	clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> -	btrfs_qgroup_wait_for_completion(fs_info, false);
>  	spin_lock(&fs_info->qgroup_lock);
>  	quota_root = fs_info->quota_root;
>  	fs_info->quota_root = NULL;
> @@ -1244,6 +1254,8 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  	btrfs_put_root(quota_root);
>  
>  out:
> +	clear_bit(BTRFS_FS_STATE_QUOTA_DISABLING, &fs_info->fs_state);
> +busy:
>  	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  	if (ret && trans)
>  		btrfs_end_transaction(trans);
> @@ -3277,7 +3289,8 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
>  			err = PTR_ERR(trans);
>  			break;
>  		}
> -		if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
> +		if (test_bit(BTRFS_FS_STATE_QUOTA_DISABLING,
> +			     &fs_info->fs_state)) {
>  			err = -EINTR;
>  		} else {
>  			err = qgroup_rescan_leaf(trans, path);
> @@ -3378,6 +3391,9 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
>  			btrfs_warn(fs_info,
>  				   "qgroup rescan is already in progress");
>  			ret = -EINPROGRESS;
> +		} else if (test_bit(BTRFS_FS_STATE_QUOTA_DISABLING,
> +				    &fs_info->fs_state)) {
> +			ret = -EBUSY;
>  		} else if (!(fs_info->qgroup_flags &
>  			     BTRFS_QGROUP_STATUS_FLAG_ON)) {
>  			btrfs_warn(fs_info,
>