| Message ID | dea5fb2c9131b0b1c274f011596e5798fdc1971d.1599164377.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free space tree mounting fixes | expand |
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
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
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; + } + } + if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb)) goto out;
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(+)