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

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

On Jan 13, 2022 / 14:53, Nikolay Borisov wrote:
> 
> 
> 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.

Nikolay, thank you for the comment.

The reason to introduce the new flag is that qgroup_ioctl_lock, which guards
QUOTA_ENABLED set by quota enable ioctl, is unlocked during the quota disable
ioctl process to start the transaction. Quote from btrfs_quota_disable():

	...
	mutex_unlock(&fs_info->qgroup_ioctl_lock);

	/*
	 * 1 For the root item
	 *
	 * We should also reserve enough items for the quota tree deletion in
	 * btrfs_clean_quota_tree but this is not done.
	 *
	 * Also, we must always start a transaction without holding the mutex
	 * qgroup_ioctl_lock, see btrfs_quota_enable().
	 */
	trans = btrfs_start_transaction(fs_info->tree_root, 1);

	mutex_lock(&fs_info->qgroup_ioctl_lock);
	...

Then even if we would clear QUOTA_ENABLED and wait for qgroup worker completion
before the transaction start, QUOTA_ENABLED can be set again by quota enable
ioctl and another qgroup worker can start at the point of the transaction start.
Even though this scenario is very rare and unlikely, it can happen. So IMO, we
can not rely on QUOTA_ENABLED to wait for qgroup worker completion.

> 
> >  	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,
> > 

-- 
Best Regards,
Shin'ichiro Kawasaki