Re: [PATCH v3.1 3/7] btrfs: relocation: Delay reloc tree deletion after merge_reloc_roots()

[David Sterba <dsterba@suse.cz>]

On Mon, Jan 07, 2019 at 01:41:47PM +0800, Qu Wenruo wrote:
> +static int insert_dirty_root(struct btrfs_trans_handle *trans,
> +			     struct reloc_control *rc,
> +			     struct btrfs_root *root)
> +{
> +	struct rb_node **p = &rc->dirty_roots.rb_node;
> +	struct rb_node *parent = NULL;
> +	struct dirty_source_root *entry;
> +	struct btrfs_root *reloc_root = root->reloc_root;
> +	struct btrfs_root_item *reloc_root_item;
> +	u64 root_objectid = root->root_key.objectid;
> +
> +	/* @root must be a file tree root*/
> +	ASSERT(root_objectid != BTRFS_TREE_RELOC_OBJECTID);
> +	ASSERT(reloc_root);
> +
> +	reloc_root_item = &reloc_root->root_item;
> +	memset(&reloc_root_item->drop_progress, 0,
> +		sizeof(reloc_root_item->drop_progress));
> +	reloc_root_item->drop_level = 0;
> +	btrfs_set_root_refs(reloc_root_item, 0);
> +	btrfs_update_reloc_root(trans, root);
> +
> +	/* We're at relocation route, not writeback route, GFP_KERNEL is OK */
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);

The open transaction also mandates GFP_NOFS, so that's another thing
to the locks and writeback constraints.

> +	if (!entry)
> +		return -ENOMEM;
> +	btrfs_grab_fs_root(root);
> +	entry->root = root;
> +	while (*p) {
> +		struct dirty_source_root *cur_entry;
> +
> +		parent = *p;
> +		cur_entry = rb_entry(parent, struct dirty_source_root, node);
> +
> +		if (root_objectid < cur_entry->root->root_key.objectid)
> +			p = &(*p)->rb_left;
> +		else if (root_objectid > cur_entry->root->root_key.objectid)
> +			p = &(*p)->rb_right;

Please stick to the coding style, { } around single statement if/else in
case they're chained and there's one multi statement block.

> +		else {
> +			/* This root is already dirtied */
> +			btrfs_put_fs_root(root);
> +			kfree(entry);
> +			return 0;
> +		}
> +	}
> +	rb_link_node(&entry->node, parent, p);
> +	rb_insert_color(&entry->node, &rc->dirty_roots);
> +	return 0;
> +}