Re: [PATCH] btrfs: take the cleaner_mutex earlier in qgroup disable

[David Sterba <dsterba@suse.cz>]

On Mon, Apr 22, 2024 at 04:27:26PM +0100, Filipe Manana wrote:
> >         case BTRFS_QUOTA_CTL_DISABLE:
> > +               /*
> > +                * Lock the cleaner mutex to prevent races with concurrent
> > +                * relocation, because relocation may be building backrefs for
> > +                * blocks of the quota root while we are deleting the root. This
> > +                * is like dropping fs roots of deleted snapshots/subvolumes, we
> > +                * need the same protection.
> > +                *
> > +                * This also prevents races between concurrent tasks trying to
> > +                * disable quotas, because we will unlock and relock
> > +                * qgroup_ioctl_lock across BTRFS_FS_QUOTA_ENABLED changes.
> > +                */
> > +               mutex_lock(&fs_info->cleaner_mutex);
> > +               down_write(&fs_info->subvol_sem);
> 
> Everything is correct and makes sense.
> 
> I'm afraid in the future someone looking into this code after the
> patch is merged, will wonder why we have this duplicated locking of
> the
> subvol_sem in each case of the switch statement and then decide "ah
> let's get rid of this duplicated code and make the locking before the
> switch and the unlocking after".

I think we'd catch that at review time, moving locks around must be done
carefully. We could improve the lock documentation either as
lockdep_assert_held() or comments, I would not mind adding the comment
here but it also does not seem necessary.