From: Josef Bacik <josef@toxicpanda.com>
To: Boris Burkov <boris@bur.io>, Dave Sterba <dsterba@suse.com>,
Chris Mason <clm@fb.com>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v2 2/3] btrfs: remove free space items when creating free space tree
Date: Thu, 10 Sep 2020 10:07:32 -0400 [thread overview]
Message-ID: <acff3e36-059d-a167-91d4-11ae9933a96d@toxicpanda.com> (raw)
In-Reply-To: <ca7009f0e52c804f85e96f75b54a408a2de8b260.1599686801.git.boris@bur.io>
On 9/9/20 5:45 PM, Boris Burkov wrote:
> When the file system transitions from space cache v1 to v2 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, as part of populating the free space tree, destroy each block
> group's free space item and free space inode. This code is lifted from
> the existing code for removing them when removing the block group.
>
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> v2:
> - remove_free_space_inode -> btrfs_remove_free_space_inode
> - undo sinful whitespace change
>
> fs/btrfs/block-group.c | 39 ++----------------------------
> fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
> fs/btrfs/free-space-cache.h | 2 ++
> fs/btrfs/free-space-tree.c | 3 +++
> 4 files changed, 55 insertions(+), 37 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 01e8ba1da1d3..e5e5baad88d8 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, 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 8759f5a1d6a0..da6d436c58fa 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -207,6 +207,54 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
> ino, block_group->start);
> }
>
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> + struct btrfs_block_group *block_group)
> +{
> + struct btrfs_path *path;
> + struct inode *inode;
> + struct btrfs_key key;
> + int ret = 0;
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + 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 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, trans->fs_info->tree_root, &key, path, -1, 1);
> + if (ret != 0)
> + goto out;
This leaks the ret == 1 if we don't have a free space cache objectid. This
needs to be
if (ret) {
if (ret > 0)
ret = 0;
goto out;
}
or something like that. Thanks,
Josef
next prev parent reply other threads:[~2020-09-10 14:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-09 21:45 [PATCH v2 0/3] btrfs: free space tree mounting fixes Boris Burkov
2020-09-09 21:45 ` [PATCH v2 1/3] btrfs: support remount of ro fs with free space tree Boris Burkov
2020-09-10 14:05 ` Josef Bacik
2020-09-10 16:47 ` Boris Burkov
2020-09-09 21:45 ` [PATCH v2 2/3] btrfs: remove free space items when creating " Boris Burkov
2020-09-10 14:07 ` Josef Bacik [this message]
2020-09-10 14:18 ` Josef Bacik
2020-09-09 21:45 ` [PATCH 3/3] btrfs: skip space_cache v1 setup when not using it Boris Burkov
2020-09-10 14:07 ` 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=acff3e36-059d-a167-91d4-11ae9933a96d@toxicpanda.com \
--to=josef@toxicpanda.com \
--cc=boris@bur.io \
--cc=clm@fb.com \
--cc=dsterba@suse.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).