A question for kernel code in btrfs_run_qgroups()

A question for kernel code in btrfs_run_qgroups()

[Sidong Yang]

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.
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?

Re: A question for kernel code in btrfs_run_qgroups()

[Qu Wenruo]

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.


Unfortunately we indeed have inconsistent locking for qgroups_flags.

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.

Thanks,
Qu

Re: A question for kernel code in btrfs_run_qgroups()

[Sidong Yang]

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

Re: A question for kernel code in btrfs_run_qgroups()

[Qu Wenruo]

On 2022/2/1 12:05, Sidong Yang wrote:
> 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?

In fact, for qgroup_flags, FLAG_ON is rarely changed.
Only qgroup enable/disable would change that.

And qgroup enable/disable have to acquire qgroup_ioctl_lock, thus AFAIK
FLAG_ON is less a concern.

To me, the concern is around INCONSISTENT flag.

As it can happen at almost any time.

As btrfs_qgroup_trace_extent()/trace_extent_post() can fail and cause
qgroup to be inconsistent.
Another location is in the future to make snapshot creation/snapshot
drop to mark qgroup INCONSISTENT to avoid super expensive subtree rescan.

>
>>
>>
>> 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 far we use it just as bit operations, thus it may be convert to
set/clear/test_bit() to be atomic, and then get rid of the chaos of locks.

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

Maybe you can start by converting it to bit ops and move all the flags
operation out of critical sections?

Thanks,
Qu

>
> Thanks,
> Sidong
>
>>
>> Thanks,
>> Qu

Re: A question for kernel code in btrfs_run_qgroups()

[Sidong Yang]

On Tue, Feb 01, 2022 at 03:31:03PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/2/1 12:05, Sidong Yang wrote:
> > 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?
> 
> In fact, for qgroup_flags, FLAG_ON is rarely changed.
> Only qgroup enable/disable would change that.
> 
> And qgroup enable/disable have to acquire qgroup_ioctl_lock, thus AFAIK
> FLAG_ON is less a concern.
> 
> To me, the concern is around INCONSISTENT flag.
> 
> As it can happen at almost any time.
> 
> As btrfs_qgroup_trace_extent()/trace_extent_post() can fail and cause
> qgroup to be inconsistent.
> Another location is in the future to make snapshot creation/snapshot
> drop to mark qgroup INCONSISTENT to avoid super expensive subtree rescan.

I agree. INCONSISTENT flag could be set anywhere. And it could make more
bigger problem.
> 
> > 
> > > 
> > > 
> > > 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 far we use it just as bit operations, thus it may be convert to
> set/clear/test_bit() to be atomic, and then get rid of the chaos of locks.
> 
> > 
> > > 
> > > 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?
> 
> Maybe you can start by converting it to bit ops and move all the flags
> operation out of critical sections?

Thanks. I would write a patch that converts the code to bitops first.

Thanks,
Sidong
> 
> Thanks,
> Qu
> 
> > 
> > Thanks,
> > Sidong
> > 
> > > 
> > > Thanks,
> > > Qu

Re: A question for kernel code in btrfs_run_qgroups()

[Qu Wenruo]

On 2022/2/1 17:24, Sidong Yang wrote:
> On Tue, Feb 01, 2022 at 03:31:03PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2022/2/1 12:05, Sidong Yang wrote:
>>> 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?
>>
>> In fact, for qgroup_flags, FLAG_ON is rarely changed.
>> Only qgroup enable/disable would change that.
>>
>> And qgroup enable/disable have to acquire qgroup_ioctl_lock, thus AFAIK
>> FLAG_ON is less a concern.
>>
>> To me, the concern is around INCONSISTENT flag.
>>
>> As it can happen at almost any time.
>>
>> As btrfs_qgroup_trace_extent()/trace_extent_post() can fail and cause
>> qgroup to be inconsistent.
>> Another location is in the future to make snapshot creation/snapshot
>> drop to mark qgroup INCONSISTENT to avoid super expensive subtree rescan.
>
> I agree. INCONSISTENT flag could be set anywhere. And it could make more
> bigger problem.
>>
>>>
>>>>
>>>>
>>>> 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 far we use it just as bit operations, thus it may be convert to
>> set/clear/test_bit() to be atomic, and then get rid of the chaos of locks.
>>
>>>
>>>>
>>>> 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?
>>
>> Maybe you can start by converting it to bit ops and move all the flags
>> operation out of critical sections?
>
> Thanks. I would write a patch that converts the code to bitops first.

One thing to mention is, if you're converting the flags operations to
bit ops, then please considering protecting it under qgroup_lock.

For now, it's fine as under most cases we just set/clear/test one bit once.

But for future patches, like this one:
https://patchwork.kernel.org/project/linux-btrfs/patch/20211207011655.21579-4-wqu@suse.com/

Which will introduce a new flag beyond the existing three flags.

And we need to set that flag along with the existing INCONSITENT flag,
thus it's no longer atomic, and may need to use spin lock to protect them.

Thanks,
Qu
>
> Thanks,
> Sidong
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks,
>>> Sidong
>>>
>>>>
>>>> Thanks,
>>>> Qu