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

[Qu Wenruo <wqu@suse.com>]

在 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?

Thanks,
Qu

> 
> The cleanup operation is kmem_cache_free() which will likely take the
> fast path for most objects.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/defrag.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 41d67065d02b..bd1769dd134c 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -212,19 +212,18 @@ static struct inode_defrag *btrfs_pick_defrag_inode(
>   
>   void btrfs_cleanup_defrag_inodes(struct btrfs_fs_info *fs_info)
>   {
> -	struct inode_defrag *defrag;
> -	struct rb_node *node;
> +	struct inode_defrag *defrag, *next;
>   
>   	spin_lock(&fs_info->defrag_inodes_lock);
> -	node = rb_first(&fs_info->defrag_inodes);
> -	while (node) {
> -		rb_erase(node, &fs_info->defrag_inodes);
> -		defrag = rb_entry(node, struct inode_defrag, rb_node);
> +again:
> +	rbtree_postorder_for_each_entry_safe(defrag, next, &fs_info->defrag_inodes,
> +					     rb_node) {
>   		kmem_cache_free(btrfs_inode_defrag_cachep, defrag);
>   
> -		cond_resched_lock(&fs_info->defrag_inodes_lock);
> -
> -		node = rb_first(&fs_info->defrag_inodes);
> +		if (cond_resched_lock(&fs_info->defrag_inodes_lock)) {
> +			/* Rescheduled and unlocked. */
> +			goto again;
> +		}
>   	}
>   	spin_unlock(&fs_info->defrag_inodes_lock);
>   }