Re: [PATCH 1/2] btrfs: support remount of ro fs with free space tree

[Josef Bacik <josef@toxicpanda.com>]

On 9/3/20 4:33 PM, Boris Burkov wrote:
> When a user attempts to remount a btrfs filesystem with
> 'mount -o remount,space_cache=v2', that operation succeeds.
> Unfortunately, this is misleading, because the remount does not create
> the free space tree. /proc/mounts will incorrectly show space_cache=v2,
> but on the next mount, the file system will revert to the old
> space_cache.
> 
> For now, we handle only the easier case, where the existing mount is
> read only. In that case, we can create the free space tree without
> contending with the block groups changing as we go. If it is not read
> only, we fail more explicitly so the user knows that the remount was not
> successful, and we don't end up in a state where /proc/mounts is giving
> misleading information.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/super.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 3ebe7240c63d..cbb2cdb0b488 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -47,6 +47,7 @@
>   #include "tests/btrfs-tests.h"
>   #include "block-group.h"
>   #include "discard.h"
> +#include "free-space-tree.h"
>   
>   #include "qgroup.h"
>   #define CREATE_TRACE_POINTS
> @@ -1862,6 +1863,22 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>   	btrfs_resize_thread_pool(fs_info,
>   		fs_info->thread_pool_size, old_thread_pool_size);
>   
> +	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE) &&
> +	    !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
> +		if (!sb_rdonly(sb)) {
> +			btrfs_warn(fs_info, "cannot create free space tree on writeable mount");
> +			ret = -EINVAL;
> +			goto restore;
> +		}
> +		btrfs_info(fs_info, "creating free space tree");
> +		ret = btrfs_create_free_space_tree(fs_info);
> +		if (ret) {
> +			btrfs_warn(fs_info,
> +				   "failed to create free space tree: %d", ret);
> +			goto restore;
> +		}
> +	}
> +

This whole chunk actually needs to be moved down into the

} else {
	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {

bit, and after all the checks to make sure it's ok to flip RW, but _before_ we 
do btrfs_cleanup_roots.

Also put a comment there saying something like

/*
  * Don't do anything that writes above this, otherwise bad things will happen.
  */

So that nobody accidentally messes this up in the future.  Thanks,

Josef