On Wed, Dec 06, 2023 at 08:28:00AM +1100, Dave Chinner wrote: > On Mon, Dec 04, 2023 at 08:35:08PM -0800, Christoph Hellwig wrote: > > On Mon, Dec 04, 2023 at 12:39:10PM +0800, Long Li wrote: > > > Look at the perag insertion into the radix tree, protected by > > > mp->m_perag_lock. When the file system is unmounted, the perag is > > > removed from the radix tree, also protected by mp->m_perag_lock. > > > Therefore, mp->m_perag_lock is also added when removing a perag > > > from the radix tree in error path in xfs_initialize_perag(). > > > > There really can't be anything we are racing with at this point. > > I'm pretty sure that there can be racing operations. Lookups are > fine - they are RCU protected so already deal with the tree changing > shape underneath the lookup - but tagging operations require the > tree to be stable while the tags are propagated back up to the root. > > Right now there's nothing stopping radix tree tagging from > operating while a growfs operation is progress and adding/removing > new entries into the radix tree. > > Hence we can have traversals that require a stable tree occurring at > the same time we are removing unused entries from the radix tree > which causes the shape of the tree to change... > > Likely this hasn't caused a problem in the past because we are only > doing append addition and removal so the active AG part of the tree > is not changing shape, but that doesn't mean it is safe... > > > That beeing said I think clearing locking rules are always a good > > thing. So maybe reword the above as: > > > > "Take mp->m_perag_lock for deletions from the perag radix tree in > > xfs_initialize_perag to be consistent with additions to it even > > if there can't be concurrent modifications to it at this point" > > I don't think it needs even that - just making the radix tree > modifications serialise against each other is obviously correct... > If my understanding is correct, it makes sense to add lock protection when modifying the radix tree. So I will update the commit message in the next version. Thanks, Long Li
