Re: [PATCH 09/12] btrfs: clear defragmented inodes using postorder in btrfs_cleanup_defrag_inodes()

[David Sterba <dsterba@suse.cz>]

On Wed, Aug 28, 2024 at 08:29:23AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2024/8/28 07:25, David Sterba 写道:
> > btrfs_cleanup_defrag_inodes() is not called frequently, only in remount
> > or unmount, but the way it frees the inodes in fs_info->defrag_inodes
> > is inefficient. Each time it needs to locate first node, remove it,
> > potentially rebalance tree until it's done. This allows to do a
> > conditional reschedule.
> > 
> > For cleanups the rbtree_postorder_for_each_entry_safe() iterator is
> > convenient but if the reschedule happens and unlocks fs_info->defrag_inodes_lock
> > we can't be sure that the tree is in the same state. If that happens,
> > restart the iteration from the beginning.
> 
> In that case, isn't the rbtree itself in an inconsistent state, and 
> restarting will only cause invalid memory access?
> 
> So in this particular case, since we can be interrupted, the full tree 
> balance looks like the only safe way we can go?

You're right, the nodes get freed so even if the iteration is restarted
it would touch freed memory, IOW rbtree_postorder_for_each_entry_safe()
can't be interrupted. I can drop the reschedule, with the same argument
that it should be relatively fast even for thousands of entries, this
should not hurt for remouunt/umount context.