From: Wang Yugui <wangyugui@e16-tech.com>
To: Boris Burkov <boris@bur.io>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v5 09/10] btrfs: remove free space items when disabling space cache v1
Date: Thu, 29 Oct 2020 19:07:53 +0800 [thread overview]
Message-ID: <20201029190748.563D.409509F4@e16-tech.com> (raw)
In-Reply-To: <750174b66139e105150b988b35705a1a708b2eb5.1603828718.git.boris@bur.io>
Hi,
'mkfs.btrfs -R free-space-tree' still create 'space cache v1'?
#mkfs.btrfs-5.9 -R free-space-tree /dev/sdb1 -f
1st time mount
#mount /dev/sdb1 /btrfs/
#dmesg
BTRFS info (device sdb1): cleaning free space cache v1
#umount
2nd time mount
#mount /dev/sdb1 /btrfs/
#dmesg
NO BTRFS info (device sdb1): cleaning free space cache v1
#umount
Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2020/10/29
> When the file system transitions from space cache v1 to v2 or to
> nospace_cache, it removes the old cached data, but does not remove
> the FREE_SPACE items nor the free space inodes they point to. This
> doesn't cause any issues besides being a bit inefficient, since these
> items no longer do anything useful.
>
> To fix it, when we are mounting, and plan to disable the space cache,
> destroy each block group's free space item and free space inode.
> The code to remove the items is lifted from the existing use case of
> removing the block group, with a light adaptation to handle whether or
> not we have already looked up the free space inode.
>
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/block-group.c | 39 +-------------
> fs/btrfs/free-space-cache.c | 101 ++++++++++++++++++++++++++++++++++--
> fs/btrfs/free-space-cache.h | 3 ++
> 3 files changed, 102 insertions(+), 41 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index c0f1d6818df7..8938b11a3339 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -892,8 +892,6 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> struct btrfs_path *path;
> struct btrfs_block_group *block_group;
> struct btrfs_free_cluster *cluster;
> - struct btrfs_root *tree_root = fs_info->tree_root;
> - struct btrfs_key key;
> struct inode *inode;
> struct kobject *kobj = NULL;
> int ret;
> @@ -971,42 +969,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> spin_unlock(&trans->transaction->dirty_bgs_lock);
> mutex_unlock(&trans->transaction->cache_write_mutex);
>
> - if (!IS_ERR(inode)) {
> - ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> - if (ret) {
> - btrfs_add_delayed_iput(inode);
> - goto out;
> - }
> - clear_nlink(inode);
> - /* One for the block groups ref */
> - spin_lock(&block_group->lock);
> - if (block_group->iref) {
> - block_group->iref = 0;
> - block_group->inode = NULL;
> - spin_unlock(&block_group->lock);
> - iput(inode);
> - } else {
> - spin_unlock(&block_group->lock);
> - }
> - /* One for our lookup ref */
> - btrfs_add_delayed_iput(inode);
> - }
> -
> - key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> - key.type = 0;
> - key.offset = block_group->start;
> -
> - ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> - if (ret < 0)
> + ret = btrfs_remove_free_space_inode(trans, inode, block_group);
> + if (ret)
> goto out;
> - if (ret > 0)
> - btrfs_release_path(path);
> - if (ret == 0) {
> - ret = btrfs_del_item(trans, tree_root, path);
> - if (ret)
> - goto out;
> - btrfs_release_path(path);
> - }
>
> spin_lock(&fs_info->block_group_cache_lock);
> rb_erase(&block_group->cache_node,
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 3acf935536ea..5d357786c9da 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -207,6 +207,65 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
> ino, block_group->start);
> }
>
> +/*
> + * inode is an optional sink: if it is NULL, btrfs_remove_free_space_inode
> + * handles lookup, otherwise it takes ownership and iputs the inode.
> + * Don't reuse an inode pointer after passing it into this function.
> + */
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> + struct inode *inode,
> + struct btrfs_block_group *block_group)
> +{
> + struct btrfs_path *path;
> + struct btrfs_key key;
> + int ret = 0;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + if (!inode)
> + inode = lookup_free_space_inode(block_group, path);
> + if (IS_ERR(inode)) {
> + if (PTR_ERR(inode) != -ENOENT)
> + ret = PTR_ERR(inode);
> + goto out;
> + }
> + ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> + if (ret) {
> + btrfs_add_delayed_iput(inode);
> + goto out;
> + }
> + clear_nlink(inode);
> + /* One for the block groups ref */
> + spin_lock(&block_group->lock);
> + if (block_group->iref) {
> + block_group->iref = 0;
> + block_group->inode = NULL;
> + spin_unlock(&block_group->lock);
> + iput(inode);
> + } else {
> + spin_unlock(&block_group->lock);
> + }
> + /* One for the lookup ref */
> + btrfs_add_delayed_iput(inode);
> +
> + key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> + key.type = 0;
> + key.offset = block_group->start;
> + ret = btrfs_search_slot(trans, trans->fs_info->tree_root, &key, path,
> + -1, 1);
> + if (ret) {
> + if (ret > 0)
> + ret = 0;
> + goto out;
> + }
> + ret = btrfs_del_item(trans, trans->fs_info->tree_root, path);
> +out:
> + btrfs_free_path(path);
> + return ret;
> +}
> +
> int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
> struct btrfs_block_rsv *rsv)
> {
> @@ -3997,6 +4056,28 @@ bool btrfs_free_space_cache_v1_active(struct btrfs_fs_info *fs_info)
> return btrfs_super_cache_generation(fs_info->super_copy);
> }
>
> +static int cleanup_free_space_cache_v1(struct btrfs_fs_info *fs_info,
> + struct btrfs_trans_handle *trans)
> +{
> + struct btrfs_block_group *block_group;
> + struct rb_node *node;
> + int ret;
> +
> + btrfs_info(fs_info, "cleaning free space cache v1");
> +
> + node = rb_first(&fs_info->block_group_cache_tree);
> + while (node) {
> + block_group = rb_entry(node, struct btrfs_block_group,
> + cache_node);
> + ret = btrfs_remove_free_space_inode(trans, NULL, block_group);
> + if (ret)
> + goto out;
> + node = rb_next(node);
> + }
> +out:
> + return ret;
> +}
> +
> int btrfs_set_free_space_cache_v1_active(struct btrfs_fs_info *fs_info,
> bool active)
> {
> @@ -4004,18 +4085,30 @@ int btrfs_set_free_space_cache_v1_active(struct btrfs_fs_info *fs_info,
> int ret;
>
> /*
> - * update_super_roots will appropriately set
> - * fs_info->super_copy->cache_generation based on the SPACE_CACHE
> - * option, so all we have to do is trigger a transaction commit.
> + * update_super_roots will appropriately set or unset
> + * fs_info->super_copy->cache_generation based on SPACE_CACHE and
> + * BTRFS_FS_CLEANUP_SPACE_CACHE_V1. For this reason, we need a
> + * transaction commit whether we are enabling space cache v1 and don't
> + * have any other work to do, or are disabling it and removing free
> + * space inodes.
> */
> trans = btrfs_start_transaction(fs_info->tree_root, 0);
> if (IS_ERR(trans))
> return PTR_ERR(trans);
>
> - if (!active)
> + if (!active) {
> set_bit(BTRFS_FS_CLEANUP_SPACE_CACHE_V1, &fs_info->flags);
> + ret = cleanup_free_space_cache_v1(fs_info, trans);
> + if (ret)
> + goto abort;
> + }
>
> ret = btrfs_commit_transaction(trans);
> + goto out;
> +abort:
> + btrfs_abort_transaction(trans, ret);
> + btrfs_end_transaction(trans);
> +out:
> clear_bit(BTRFS_FS_CLEANUP_SPACE_CACHE_V1, &fs_info->flags);
> return ret;
> }
> diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h
> index 5c546898ded9..8df4b2925eca 100644
> --- a/fs/btrfs/free-space-cache.h
> +++ b/fs/btrfs/free-space-cache.h
> @@ -84,6 +84,9 @@ struct inode *lookup_free_space_inode(struct btrfs_block_group *block_group,
> int create_free_space_inode(struct btrfs_trans_handle *trans,
> struct btrfs_block_group *block_group,
> struct btrfs_path *path);
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> + struct inode *inode,
> + struct btrfs_block_group *block_group);
>
> int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
> struct btrfs_block_rsv *rsv);
> --
> 2.24.1
next prev parent reply other threads:[~2020-10-29 11:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-27 21:07 Boris Burkov
2020-10-27 21:07 ` [PATCH v5 01/10] btrfs: lift rw mount setup from mount and remount Boris Burkov
2020-10-28 14:08 ` Josef Bacik
2020-10-27 21:07 ` [PATCH v5 02/10] btrfs: cleanup all orphan inodes on ro->rw remount Boris Burkov
2020-10-28 13:52 ` Josef Bacik
2020-10-27 21:07 ` [PATCH v5 03/10] btrfs: create free space tree " Boris Burkov
2020-10-27 21:07 ` [PATCH v5 04/10] btrfs: clear oneshot options on mount and remount Boris Burkov
2020-10-28 13:58 ` Josef Bacik
2020-10-27 21:07 ` [PATCH v5 05/10] btrfs: clear free space tree on ro->rw remount Boris Burkov
2020-10-27 21:08 ` [PATCH v5 06/10] btrfs: keep sb cache_generation consistent with space_cache Boris Burkov
2020-10-27 21:08 ` [PATCH v5 07/10] btrfs: use sb state to print space_cache mount option Boris Burkov
2020-10-27 21:08 ` [PATCH v5 08/10] btrfs: warn when remount will not change the free space tree Boris Burkov
2020-10-27 21:08 ` [PATCH v5 09/10] btrfs: remove free space items when disabling space cache v1 Boris Burkov
2020-10-29 11:07 ` Wang Yugui [this message]
2020-10-29 12:25 ` Wang Yugui
2020-10-27 21:08 ` [PATCH v5 10/10] btrfs: skip space_cache v1 setup when not using it Boris Burkov
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=20201029190748.563D.409509F4@e16-tech.com \
--to=wangyugui@e16-tech.com \
--cc=boris@bur.io \
--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