| Message ID | 9bca6fcf34bffc73c42323c9f79f5c1a9e6ef131.1599686801.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free space tree mounting fixes | expand |
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 > + > 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
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
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; + } + } + if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb)) goto out; @@ -1924,6 +1937,22 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) goto restore; } + /* + * NOTE: when remounting with a change that does writes, don't + * put it anywhere above this point, as we are not sure to be + * safe to write until we pass the above checks. + */ + if (create_fst) { + 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; + } + } + + ret = btrfs_cleanup_fs_roots(fs_info); if (ret) goto restore;
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(+)