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

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

On Jan 14, 2022 / 04:03, Shinichiro Kawasaki wrote:
> 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

I have just posted v2 patch based on this approach. Review comments will be
appreciated.

> > > again. Still, moving QUOTA_ENABLED check in rescan_should_stop would be
> > > useful as well.

I saw Nikolay posted this part as a separated patch, so it is not included in
the v2 patch.

https://lore.kernel.org/linux-btrfs/20220113151618.2149736-1-nborisov@suse.com/T/#u

-- 
Best Regards,
Shin'ichiro Kawasaki