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

[Boris Burkov <boris@bur.io>]

On Thu, Sep 10, 2020 at 10:05:40AM -0400, Josef Bacik wrote:
> On 9/9/20 5:45 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. We also fail if the remount is read-only, since
> >we would not be able to create the free space tree in that case.
> >
> >References: https://github.com/btrfs/btrfs-todo/issues/5
> >Signed-off-by: Boris Burkov <boris@bur.io>
> >---
> >v2:
> >- move creation down to ro->rw case
> >- error on all other remount cases
> >- add a comment to help future remount modifiers
> >
> >  fs/btrfs/super.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> >diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> >index 3ebe7240c63d..0a1b5f554c27 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
> >@@ -1838,6 +1839,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
> >  	u64 old_max_inline = fs_info->max_inline;
> >  	u32 old_thread_pool_size = fs_info->thread_pool_size;
> >  	u32 old_metadata_ratio = fs_info->metadata_ratio;
> >+	bool create_fst = false;
> >  	int ret;
> >  	sync_filesystem(sb);
> >@@ -1862,6 +1864,17 @@ 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)) {
> >+		create_fst = true;
> >+		if(!sb_rdonly(sb) || *flags & SB_RDONLY) {
> >+			btrfs_warn(fs_info,
> >+				   "Remounting with free space tree only allowed from read-only to read-write");
> >+			ret = -EINVAL;
> >+			goto restore;
> >+		}
> >+	}
> 
> This will bite us if we remount -o ro,noatime but had previous
> mounted with -o ro,space_cache=v2.  These checks need to be under
> 

I wanted this case to fail because I was thinking of this patch as
creating the invariant: "if remount -o space_cache=v2 succeeds, we
actually created the free space tree". However, that behavior is
inconsistent with the fact that 'mount -o ro,space_cache=v2' does
succeed while failing to create the tree.

I guess my question is would we rather have a ro mount that tries to
create the free space tree fail silently or noisily in both cases?

If we want to allow them to silently fail, I will also send a patch to
change the mount option reporting logic.

Thanks for the review!

> >+
> >  	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
> >  		goto out;
> 
> This part right here.  Put the check for remounting RO with
> space_cache=v2 in that part, and the check if we need to create the
> fst at all down where you create it.  Thanks,
> 
> Josef