From: Boris Burkov <boris@bur.io>
To: Josef Bacik <josef@toxicpanda.com>
Cc: Dave Sterba <dsterba@suse.com>, Chris Mason <clm@fb.com>,
linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 1/2] btrfs: support remount of ro fs with free space tree
Date: Thu, 3 Sep 2020 16:34:18 -0700 [thread overview]
Message-ID: <20200903233418.GB486887@devvm842.ftw2.facebook.com> (raw)
In-Reply-To: <d21b0694-2ff6-dba0-2c93-ceaef0c0bed8@toxicpanda.com>
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
next prev parent reply other threads:[~2020-09-03 23:34 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-03 20:33 [PATCH 0/2] btrfs: free space tree mounting fixes Boris Burkov
2020-09-03 20:33 ` [PATCH 1/2] btrfs: support remount of ro fs with free space tree Boris Burkov
2020-09-03 21:40 ` Josef Bacik
2020-09-03 23:34 ` Boris Burkov [this message]
2020-09-03 20:33 ` [PATCH 2/2] btrfs: remove free space items when creating " Boris Burkov
2020-09-03 21:43 ` Josef Bacik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200903233418.GB486887@devvm842.ftw2.facebook.com \
--to=boris@bur.io \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=josef@toxicpanda.com \
--cc=kernel-team@fb.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).