| Message ID | ca7009f0e52c804f85e96f75b54a408a2de8b260.1599686801.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free space tree mounting fixes | expand |
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
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); > - } > Dave also noticed we're leaking the inode with these patches, that's because here you do the remove_free_space_inode thing instead, which is good, but we've already done our own lookup, and we're no longer dropping our reference here. I would probalby make btrfs_remove_free_space_inode() take the inode itself so you don't have to do two lookups, and then add the lookup to create_free_space_tree in a helper that deal's with this. Then btrfs_remove_free_space_inode() _only_ drops the ref for ->iref, and then the caller has to drop their own ref from their lookup. Thanks, Josef
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; + 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) { diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h index e3d5e0ad8f8e..606eca93e43f 100644 --- a/fs/btrfs/free-space-cache.h +++ b/fs/btrfs/free-space-cache.h @@ -84,6 +84,8 @@ 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 btrfs_block_group *block_group); int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info, struct btrfs_block_rsv *rsv); diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c index 6b9faf3b0e96..7b6be89c1862 100644 --- a/fs/btrfs/free-space-tree.c +++ b/fs/btrfs/free-space-tree.c @@ -1165,6 +1165,9 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info) block_group = rb_entry(node, struct btrfs_block_group, cache_node); ret = populate_free_space_tree(trans, block_group); + if (ret) + goto abort; + ret = btrfs_remove_free_space_inode(trans, block_group); if (ret) goto abort; node = rb_next(node);
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(-)