* A question for kernel code in btrfs_run_qgroups() @ 2022-01-31 15:40 Sidong Yang 2022-02-01 0:21 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Sidong Yang @ 2022-01-31 15:40 UTC (permalink / raw) To: linux-btrfs 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A question for kernel code in btrfs_run_qgroups() 2022-01-31 15:40 A question for kernel code in btrfs_run_qgroups() Sidong Yang @ 2022-02-01 0:21 ` Qu Wenruo 2022-02-01 4:05 ` Sidong Yang 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-02-01 0:21 UTC (permalink / raw) To: Sidong Yang, linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A question for kernel code in btrfs_run_qgroups() 2022-02-01 0:21 ` Qu Wenruo @ 2022-02-01 4:05 ` Sidong Yang 2022-02-01 7:31 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Sidong Yang @ 2022-02-01 4:05 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A question for kernel code in btrfs_run_qgroups() 2022-02-01 4:05 ` Sidong Yang @ 2022-02-01 7:31 ` Qu Wenruo 2022-02-01 9:24 ` Sidong Yang 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2022-02-01 7:31 UTC (permalink / raw) To: Sidong Yang; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A question for kernel code in btrfs_run_qgroups() 2022-02-01 7:31 ` Qu Wenruo @ 2022-02-01 9:24 ` Sidong Yang 2022-02-01 11:35 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Sidong Yang @ 2022-02-01 9:24 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: A question for kernel code in btrfs_run_qgroups() 2022-02-01 9:24 ` Sidong Yang @ 2022-02-01 11:35 ` Qu Wenruo 0 siblings, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2022-02-01 11:35 UTC (permalink / raw) To: Sidong Yang; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-01 11:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-31 15:40 A question for kernel code in btrfs_run_qgroups() Sidong Yang 2022-02-01 0:21 ` Qu Wenruo 2022-02-01 4:05 ` Sidong Yang 2022-02-01 7:31 ` Qu Wenruo 2022-02-01 9:24 ` Sidong Yang 2022-02-01 11:35 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox