On 2020/5/8 上午4:51, David Sterba wrote: > On Thu, May 07, 2020 at 06:48:24AM +0800, Qu Wenruo wrote: >>>>> @@ -2809,6 +2821,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, >>>>> out: >>>>> if (!committing) >>>>> mutex_unlock(&fs_info->qgroup_ioctl_lock); >>>>> + if (need_rescan) >>>>> + fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT; >>> >>> This got me curious, a non-atomic change to qgroup_flags and without >>> any protection. The function is not running in a safe context (like >>> quota enable or disable) so lack of synchronization seems suspicious. I >>> grepped for other changes to the qgroup_flags and it's very >>> inconsistent. Sometimes it's the fs_info::qgroup_lock, no lokcing at all >>> or no obvious lock but likely fs_info::qgroup_ioctl_lock or >>> qgroup_rescan_lock. >>> >>> I was considering using atomic bit updates but that would be another >>> patch. >>> >> Isn't the context safe because we're at the commit transaction critical >> section? > > I don't see why, the qgroup_flags could be changed from ioctls, eg. > adding a group relation and other places. Without protection the update > can race and some of the changes lost. > Oh, you're right. Do I need another patch to address all the inconsistence? Thanks, Qu
Attachment:
signature.asc
Description: OpenPGP digital signature
