Re: A question for kernel code in btrfs_run_qgroups()

[Sidong Yang <realwakka@gmail.com>]

On Tue, Feb 01, 2022 at 08:21:22AM +0800, Qu Wenruo wrote:

Hi Qu,

Thanks for answering

> 
> 
> On 2022/1/31 23:40, Sidong Yang wrote:
> > Hi, I'm reading btrfs code for contributing.
> > I have a question about code in btrfs_run_qgroups().
> > 
> > It seems that it updates dirty qgroups modified in transaction.
> 
> Yep.
> 
> > And it iterates dirty qgroups with locking and unlocking qgroup_lock for
> > protecting dirty_qgroups list. According to code, It locks before enter
> > the loop and pick a entry and unlock. At the end of loop, It locks again
> > for next loop. And after loop, it set some flags and unlock.
> > 
> > I wonder that it should be locked with setting STATUS_FLAG_ON? if not,
> > is there unnecessary locking in last loop?
> 
> From a quick look, it indeed looks like we don't need to hole
> qgroup_lock to modify qgroup_flags.
> In fact, just lines below, we update qgroup_flags without any lock for
> INCONSISTENT bit.

Apparently it is, but I don't know surely that it really don't need to
hold lock while modifying qgroup_flags.
It has FLAG_ON that it indicates that quota turned on. I think it should
be modified carefully. Or it can be protected by other locks like
qgroup_lock or ioctl_lock?

> 
> 
> Unfortunately we indeed have inconsistent locking for qgroups_flags.

I agree. there is a lot of codes that modify qgroup_flags without lock
or with lock.

> 
> So it's completely possible that we may not need to do modify the
> qgroup_flags under qgroup_lock.
> 
> In fact there are tons of call sites that we don't hold locks for
> qgroups_flags update.
> 
> So if you're interested in contributing to btrfs, starting from sorting
> the qgroup_lock usage would be a excellent start.

Yeah, I really interested in this. but I don't know good way to handle
this problem. is it really good way to remove the code holding lock
while modifying flags?

Thanks,
Sidong

> 
> Thanks,
> Qu