From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99A2CC433E2 for ; Thu, 3 Sep 2020 23:34:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6971D2083B for ; Thu, 3 Sep 2020 23:34:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=bur.io header.i=@bur.io header.b="MMc5kvrx"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="NQxhi8tF" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729353AbgICXe0 (ORCPT ); Thu, 3 Sep 2020 19:34:26 -0400 Received: from wout5-smtp.messagingengine.com ([64.147.123.21]:53223 "EHLO wout5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729306AbgICXeY (ORCPT ); Thu, 3 Sep 2020 19:34:24 -0400 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.west.internal (Postfix) with ESMTP id 1A9C6872; Thu, 3 Sep 2020 19:34:23 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Thu, 03 Sep 2020 19:34:23 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bur.io; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm1; bh=ATPi494MZGRrHWITOJQ9+HkH4nx 4DpMkTFwxzTGVf6E=; b=MMc5kvrx4WZPsv6Cb2HwG2PtRTE9g4EaJOFloo7ZVRg zzpIi9cOQybDHaQHR96DJDVi8aucHc4PoWCRmy3YK3Oc07fWQM1pd39aQU+5ujJT lLKQtVTq7S4QQamy0NgGDjidq7j4lAeQGTlLtIV+BIr/xRDp4BXrYXTW0/jjr1NQ 3o1cq9QRTwXE+x20U6PF+dnLOTzRiHSa0uAKHSKHyh+9Ersm/dKXBLqIv9UuEKfd ObJasJ33VXBJv0w+AmfmF5HWBypu1BvCd+b/qrPMbA1Ae67P89kKGvx3E33+cG92 rV73GkYsVr45ipvFSuZcwVJN2dhxf/rUFI2GalllFsw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=ATPi49 4MZGRrHWITOJQ9+HkH4nx4DpMkTFwxzTGVf6E=; b=NQxhi8tF8PP1Qk/hav7DL0 Lv53J4h1DkZtVpweIbd7hwmbS702UE+XDz4L4I+fEiQfyGkI8lKcJxPJ49mCujH/ lkaKSVSfWd5TYLPRWtO6k8N0omqC/EgXTrqSJf4GPqHYqBGdegPCQ+iqpKDYD2l+ cKjZS4MetBl5qMDlqjO+Nfl6hPjtBOz9PbHWl8XYhMLOypyZZgOUfNH7FkLQZTgv JegMux8xompfxLv79SIjF8Ei74KiHl6sFcceeOqP8mmLML8A0pgbQO5Vmb0k+uAo qyaRTmyXduWfwNI8Z2+dDYwz6TDDeJkswwFFO4hcuwXfA0xzoDILWNjFCes7JvSQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedrudegvddgvdefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjfgesthdtredttdervdenucfhrhhomhepuehorhhi shcuuehurhhkohhvuceosghorhhishessghurhdrihhoqeenucggtffrrghtthgvrhhnpe eugefhvdffgedvuefgffeufeegkedtuefhtddtveevgfekleelkeeiueekleelteenucff ohhmrghinhepghhithhhuhgsrdgtohhmnecukfhppeduieefrdduudegrddufedvrdefne cuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepsghorhhi shessghurhdrihho X-ME-Proxy: Received: from localhost (unknown [163.114.132.3]) by mail.messagingengine.com (Postfix) with ESMTPA id 01E393060060; Thu, 3 Sep 2020 19:34:22 -0400 (EDT) Date: Thu, 3 Sep 2020 16:34:18 -0700 From: Boris Burkov To: Josef Bacik Cc: Dave Sterba , Chris Mason , linux-btrfs@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH 1/2] btrfs: support remount of ro fs with free space tree Message-ID: <20200903233418.GB486887@devvm842.ftw2.facebook.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org 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 > >--- > > 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