Re: [PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[Long Li <leo.lilong@huawei.com>]

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