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

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

On Jan 14, 2022 / 00:52, Shinichiro Kawasaki wrote:
> 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.

I relooked at btrfs_quota_enable() and btrfs_quota_disable() and found that my
response above is wrong. btrfs_quota_enable() checks fs_info->quota_root to see
if quota is enabled or not. So while QUOTA_ENABLED is cleared in quota disabling
process, quota enable does not run, regardless of the qgroup_ioctl_lock unlock
in btrfs_quota_disable().

With this, Nikolay's suggestion looks the good idea. Will rework the patch.

-- 
Best Regards,
Shin'ichiro Kawasaki