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

[Boris Burkov <boris@bur.io>]

On Thu, Sep 03, 2020 at 05:40:39PM -0400, Josef Bacik wrote:
> 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

This makes sense, since we need to be able to write to create the tree.
My mistake. With that said, do you think I should keep the logic that
makes remount explicitly fail when -o space_cache=v2 won't actually be
honored?

e.g.:
- fs is rw
- fs is ro, remount is ro

I think that loudly failing in these cases makes the user experience
quite a bit better, but it's not as simple as just throwing the create
code in the appropriate block for the ro->rw transition case.

Thanks for the review,
Boris