Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[David Sterba <dsterba@suse.cz>]

On Mon, Jul 27, 2020 at 10:33:32AM -0400, Josef Bacik wrote:
> >> @@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >>   	btrfs_put_block_group_cache(fs_info);
> >>   
> >>   fail_tree_roots:
> >> +	btrfs_free_fs_roots(fs_info);
> >>   	free_root_pointers(fs_info, true);
> > 
> > The data reloc tree is freed inside free_root_pointers, that it's also
> > in the radix tree is for convenience so I'd rather fix it inside
> > free_root_pointers and not reorder btrfs_free_fs_roots.
> 
> We can't do that, because free_root_pointers() is called to drop the extent 
> buffers when we're looping through the backup roots.  btrfs_free_fs_roots() is 
> the correct thing to do here, it goes through anything that ended up in the 
> radix tree.  Thanks,

I see, free_root_pointers is used elsewhere. I'm concerned about the
reordeing because there are several functions that are now called after
the roots are freed.

(before)	btrfs_free_fs_roots(fs_info);

		kthread_stop(fs_info->cleaner_kthread);
		filemap_write_and_wait(fs_info->btree_inode->i_mapping);
		btrfs_sysfs_remove_mounted(fs_info);
		btrfs_sysfs_remove_fsid(fs_info->fs_devices);
		btrfs_put_block_group_cache(fs_info);

(after)		btrfs_free_fs_roots(fs_info);

If something called by btrfs_free_fs_roots depends on structures
removed/cleaned by the other functions, eg. btrfs_put_block_group_cache
ti could be a problem.

I haven't spotted anything so far, the functions are called after
failure still during mount, this is easier than shutdown sequence after
a full mount.