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 20:25:09 +0800 [thread overview]
Message-ID: <20201029202508.01AD.409509F4@e16-tech.com> (raw)
In-Reply-To: <20201029190748.563D.409509F4@e16-tech.com>
Hi,
After mkfs.btrfs with/without -R free-space-tree,
the result of 'btrfs inspect-internal dump-super' is
cache_generation 18446744073709551615 => -1LL
uuid_tree_generation 0
No needs of 'cleaning free space cache v1' when
cache_generation is -1LL?
Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2020/10/29
> 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 12:25 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
2020-10-29 12:25 ` Wang Yugui [this message]
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=20201029202508.01AD.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;
as well as URLs for NNTP newsgroup(s).