Re: [PATCH v3] btrfs: qgroup: fix deadlock between rescan worker and remove qgroup

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

Thanks for this v3 patch. I found some more points to improve in the commit
message.

On Feb 26, 2022 / 16:03, Sidong Yang wrote:
> The commit e804861bd4e6 ("btrfs: fix deadlock between quota disable and
> qgroup rescan worker") by Kawasaki resolves deadlock between quota
> disable and qgroup rescan worker. but also there is a deadlock case like

nit: s/but also/But also/

> it. It's about enabling or disabling quota and creating or removing
> qgroup. It can be reproduced in simple script below.
> 
> for i in {1..100}
> do
>     btrfs quota enable /mnt &
>     btrfs qgroup create 1/0 /mnt &
>     btrfs qgroup destroy 1/0 /mnt &
>     btrfs quota disable /mnt &
> done
> 
> Here's why the deadlock happens:
> 
> 1) The quota rescan task is running.
> 
> 2) Task A calls btrfs_quota_disable(), locks the qgroup_ioctl_lock
>    mutex, and then calls btrfs_qgroup_wait_for_completion(), to wait for
>    the quota rescan task to complete.
> 
> 3) Task B calls btrfs_remove_qgroup() and it blocks when trying to lock
>    the qgroup_ioctl_lock mutex, because it's being held by task A. At that
>    To  resolve this issue, The thread disabling quota should unlock
>    int task B is holding a transaction handle for the current transaction.

I think you made a mistake in the two lines above, when you copied the text that
Filipe suggested.

> 
> 4) The quota rescan task calls btrfs_commit_transaction(). This results
>    in it waiting for all other tasks to release their handles on the
>    transaction, but task B is blocked on the qgroup_ioctl_lock mutex
>    while holding a handle on the transaction, and that mutex is being held
>    by task A, which is waiting for the quota rescan task to complete,
>    resulting in a deadlock between these 3 tasks.
> 
> To resolve this issue, The thread disabling quota should unlock

nit: s/The thread/the thread/

> qgroup_ioctl_lock before waiting rescan completion. This patch moves
> btrfs_qgroup_wait_for_completion() after qgroup_ioctl_lock().

Do you mean 'unlock of qgroup_ioctl_lock' instead of 'qgroup_ioclt_lock()?

Also, according to Documentation/process/submitting-patches.rst, imperative
mood is recommended for commit messages. Then the sentence above can be:

  Move btrfs_qgroup_wait_for_completion() after unlock of qgroup_ioctl_lock.

-- 
Best Regards,
Shin'ichiro Kawasaki

> 
> Fixes: e804861bd4e6 ("btrfs: fix deadlock between quota disable and
> qgroup rescan worker")
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
> v3: fix comments, typos, changelog.
> v2: add comments, move locking before clear_bit.
> ---
>  fs/btrfs/qgroup.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 2c0dd6b8a80c..1866b1f0da01 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1213,6 +1213,14 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  	if (!fs_info->quota_root)
>  		goto out;
>  
> +	/*
> +	 * Unlock the qgroup_ioctl_lock mutex before waiting for the rescan worker to
> +	 * complete. Otherwise we can deadlock because btrfs_remove_qgroup() needs
> +	 * to lock that mutex while holding a transaction handle and the rescan
> +	 * worker needs to commit a transaction.
> +	 */
> +	mutex_unlock(&fs_info->qgroup_ioctl_lock);
> +
>  	/*
>  	 * Request qgroup rescan worker to complete and wait for it. This wait
>  	 * must be done before transaction start for quota disable since it may
> @@ -1220,7 +1228,6 @@ int btrfs_quota_disable(struct btrfs_fs_info *fs_info)
>  	 */
>  	clear_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>  	btrfs_qgroup_wait_for_completion(fs_info, false);
> -	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>  
>  	/*
>  	 * 1 For the root item
> -- 
> 2.25.1
>