Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation

[Nikolay Borisov <nborisov@suse.com>]

On 19.11.18 г. 11:48 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If the quota enable and snapshot creation ioctls are called concurrently
> we can get into a deadlock where the task enabling quotas will deadlock
> on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it
> twice. The following time diagram shows how this happens.
> 
>            CPU 0                                    CPU 1
> 
>  btrfs_ioctl()
>   btrfs_ioctl_quota_ctl()
>    btrfs_quota_enable()
>     mutex_lock(fs_info->qgroup_ioctl_lock)
>     btrfs_start_transaction()
> 
>                                              btrfs_ioctl()
>                                               btrfs_ioctl_snap_create_v2
>                                                create_snapshot()
>                                                 --> adds snapshot to the
>                                                     list pending_snapshots
>                                                     of the current
>                                                     transaction
> 
>     btrfs_commit_transaction()
>      create_pending_snapshots()
>        create_pending_snapshot()
>         qgroup_account_snapshot()
>          btrfs_qgroup_inherit()
> 	   mutex_lock(fs_info->qgroup_ioctl_lock)
> 	    --> deadlock, mutex already locked
> 	        by this task at
> 		btrfs_quota_enable()
> 
> So fix this by adding a flag to the transaction handle that signals if the
> transaction is being used for enabling quotas (only seen by the task doing
> it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit()
> if the transaction handle corresponds to the one being used to enable the
> quotas.
> 
> Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/qgroup.c      | 10 ++++++++--
>  fs/btrfs/transaction.h |  1 +
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index d4917c0cddf5..3aec3bfa3d70 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>  		trans = NULL;
>  		goto out;
>  	}
> +	trans->enabling_quotas = true;
>  
>  	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
>  	if (!fs_info->qgroup_ulist) {
> @@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  	u32 level_size = 0;
>  	u64 nums;
>  
> -	mutex_lock(&fs_info->qgroup_ioctl_lock);
> +	if (trans->enabling_quotas)
> +		lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
> +	else
> +		mutex_lock(&fs_info->qgroup_ioctl_lock);
> +

nit: That's a bit ugly for my taste, but I don't think the 
alternative is any better: 

ASSERT((trans->enabling_quotas && !lockdep_assert_held(&fs_info->qgroup_ioctl_lock)) || !trans->enabling_quotas)

if (!trans->enabling_quotas)
	mutex_lock(...)


>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
>  		goto out;
>  
> @@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  unlock:
>  	spin_unlock(&fs_info->qgroup_lock);
>  out:
> -	mutex_unlock(&fs_info->qgroup_ioctl_lock);
> +	if (!trans->enabling_quotas)
> +		mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  	return ret;
>  }
>  
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 703d5116a2fc..a5553a1dee30 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -122,6 +122,7 @@ struct btrfs_trans_handle {
>  	bool reloc_reserved;
>  	bool sync;
>  	bool dirty;
> +	bool enabling_quotas;
>  	struct btrfs_root *root;
>  	struct btrfs_fs_info *fs_info;
>  	struct list_head new_bgs;
>