[41/43] btrfs: make the init of static elements in fs_info separate
diff mbox series

Message ID 20200117212602.6737-42-josef@toxicpanda.com
State New
Headers show
Series
  • Cleanup how we handle root refs, part 1
Related show

Commit Message

Josef Bacik Jan. 17, 2020, 9:26 p.m. UTC
In adding things like eb leak checking and root leak checking there were
a lot of weird corner cases that come from the fact that

1) We do not init the fs_info until we get to open_ctree time in the
normal case and

2) The test infrastructure half-init's the fs_info for things that it
needs.

This makes it really annoying to make changes because you have to add
init in two different places, have special cases for testing fs_info's
that may not have certain things init'ed, and cases for fs_info's that
didn't make it to open_ctree and thus are not fully init'ed.

Fix this by extracting out the non-allocating init of the fs info into
it's own public function and use that to make sure we're all getting
consistent views of an allocated fs_info.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c           | 19 +++++++++++--------
 fs/btrfs/disk-io.h           |  1 +
 fs/btrfs/super.c             |  1 +
 fs/btrfs/tests/btrfs-tests.c | 28 ++++------------------------
 4 files changed, 17 insertions(+), 32 deletions(-)

Comments

Nikolay Borisov Jan. 23, 2020, 12:30 p.m. UTC | #1
On 17.01.20 г. 23:26 ч., Josef Bacik wrote:
> In adding things like eb leak checking and root leak checking there were
> a lot of weird corner cases that come from the fact that
> 
> 1) We do not init the fs_info until we get to open_ctree time in the
> normal case and
> 
> 2) The test infrastructure half-init's the fs_info for things that it
> needs.
> 
> This makes it really annoying to make changes because you have to add
> init in two different places, have special cases for testing fs_info's
> that may not have certain things init'ed, and cases for fs_info's that
> didn't make it to open_ctree and thus are not fully init'ed.
> 
> Fix this by extracting out the non-allocating init of the fs info into
> it's own public function and use that to make sure we're all getting
> consistent views of an allocated fs_info.
> 


Imo it will be better if btrfs_init_fs_info is called from
init_mount_fs_info. And then called explicitly from the test code. That
way we keep the initialization close. Otherwise having it in
btrfs_mount_root is just confusing and not very obvious what's going on.
Nikolay Borisov Jan. 23, 2020, 12:34 p.m. UTC | #2
On 23.01.20 г. 14:30 ч., Nikolay Borisov wrote:
> 
> 
> On 17.01.20 г. 23:26 ч., Josef Bacik wrote:
>> In adding things like eb leak checking and root leak checking there were
>> a lot of weird corner cases that come from the fact that
>>
>> 1) We do not init the fs_info until we get to open_ctree time in the
>> normal case and
>>
>> 2) The test infrastructure half-init's the fs_info for things that it
>> needs.
>>
>> This makes it really annoying to make changes because you have to add
>> init in two different places, have special cases for testing fs_info's
>> that may not have certain things init'ed, and cases for fs_info's that
>> didn't make it to open_ctree and thus are not fully init'ed.
>>
>> Fix this by extracting out the non-allocating init of the fs info into
>> it's own public function and use that to make sure we're all getting
>> consistent views of an allocated fs_info.
>>
> 
> 
> Imo it will be better if btrfs_init_fs_info is called from
> init_mount_fs_info. And then called explicitly from the test code. That
> way we keep the initialization close. Otherwise having it in
> btrfs_mount_root is just confusing and not very obvious what's going on.
> 


Actually now that I think more about it IMO btrfs_init_fs_info should be
exported iff btrfs tests are enabled i.e using EXPORT_FOR_TESTS defines
Josef Bacik Jan. 24, 2020, 2:25 p.m. UTC | #3
On 1/23/20 7:30 AM, Nikolay Borisov wrote:
> 
> 
> On 17.01.20 г. 23:26 ч., Josef Bacik wrote:
>> In adding things like eb leak checking and root leak checking there were
>> a lot of weird corner cases that come from the fact that
>>
>> 1) We do not init the fs_info until we get to open_ctree time in the
>> normal case and
>>
>> 2) The test infrastructure half-init's the fs_info for things that it
>> needs.
>>
>> This makes it really annoying to make changes because you have to add
>> init in two different places, have special cases for testing fs_info's
>> that may not have certain things init'ed, and cases for fs_info's that
>> didn't make it to open_ctree and thus are not fully init'ed.
>>
>> Fix this by extracting out the non-allocating init of the fs info into
>> it's own public function and use that to make sure we're all getting
>> consistent views of an allocated fs_info.
>>
> 
> 
> Imo it will be better if btrfs_init_fs_info is called from
> init_mount_fs_info. And then called explicitly from the test code. That
> way we keep the initialization close. Otherwise having it in
> btrfs_mount_root is just confusing and not very obvious what's going on.
> 

I'm not sure what's confusing about it, we're allocating the structure and then 
initializing it.  And as I point out in the changelog, this is necessary because 
the error handling is bonkers because we can fail at multiple places _before_ 
open_ctree, and having random garbage in the fs_info makes proper cleanup a 
pain.  Thus it _has_ to be exported and the fs_info _has_ to have it's static 
elements properly initialized in order to make cleanup in the error cases clean 
and sane.  Thanks,

Josef

Patch
diff mbox series

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b7e4313bdc6f..87bad959b1a5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2649,10 +2649,8 @@  static int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 	return ret;
 }
 
-static int init_fs_info(struct btrfs_fs_info *fs_info, struct super_block *sb)
+void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 {
-	int ret;
-
 	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
 	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
 	INIT_LIST_HEAD(&fs_info->trans_list);
@@ -2696,7 +2694,6 @@  static int init_fs_info(struct btrfs_fs_info *fs_info, struct super_block *sb)
 	atomic_set(&fs_info->reada_works_cnt, 0);
 	atomic_set(&fs_info->nr_delayed_iputs, 0);
 	atomic64_set(&fs_info->tree_mod_seq, 0);
-	fs_info->sb = sb;
 	fs_info->max_inline = BTRFS_DEFAULT_MAX_INLINE;
 	fs_info->metadata_ratio = 0;
 	fs_info->defrag_inodes = RB_ROOT;
@@ -2722,9 +2719,6 @@  static int init_fs_info(struct btrfs_fs_info *fs_info, struct super_block *sb)
 	btrfs_init_balance(fs_info);
 	btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work);
 
-	sb->s_blocksize = BTRFS_BDEV_BLOCKSIZE;
-	sb->s_blocksize_bits = blksize_bits(BTRFS_BDEV_BLOCKSIZE);
-
 	spin_lock_init(&fs_info->block_group_cache_lock);
 	fs_info->block_group_cache_tree = RB_ROOT;
 	fs_info->first_logical_byte = (u64)-1;
@@ -2769,6 +2763,15 @@  static int init_fs_info(struct btrfs_fs_info *fs_info, struct super_block *sb)
 	fs_info->swapfile_pins = RB_ROOT;
 
 	fs_info->send_in_progress = 0;
+}
+
+static int init_mount_fs_info(struct btrfs_fs_info *fs_info, struct super_block *sb)
+{
+	int ret;
+
+	fs_info->sb = sb;
+	sb->s_blocksize = BTRFS_BDEV_BLOCKSIZE;
+	sb->s_blocksize_bits = blksize_bits(BTRFS_BDEV_BLOCKSIZE);
 
 	ret = init_srcu_struct(&fs_info->subvol_srcu);
 	if (ret)
@@ -2833,7 +2836,7 @@  int __cold open_ctree(struct super_block *sb,
 	int clear_free_space_tree = 0;
 	int level;
 
-	ret = init_fs_info(fs_info, sb);
+	ret = init_mount_fs_info(fs_info, sb);
 	if (ret) {
 		err = ret;
 		goto fail;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 97e7ac474a52..2414d572bc9a 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -39,6 +39,7 @@  static inline u64 btrfs_sb_offset(int mirror)
 struct btrfs_device;
 struct btrfs_fs_devices;
 
+void btrfs_init_fs_info(struct btrfs_fs_info *fs_info);
 int btrfs_verify_level_key(struct extent_buffer *eb, int level,
 			   struct btrfs_key *first_key, u64 parent_transid);
 struct extent_buffer *read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8ce292a47634..8d5bb8dfed0d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1531,6 +1531,7 @@  static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		error = -ENOMEM;
 		goto error_sec_opts;
 	}
+	btrfs_init_fs_info(fs_info);
 
 	fs_info->super_copy = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
 	fs_info->super_for_commit = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index 27f5b662d2cb..683381a692bc 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -120,6 +120,8 @@  struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 		kfree(fs_info);
 		return NULL;
 	}
+	INIT_LIST_HEAD(&fs_info->fs_devices->devices);
+
 	fs_info->super_copy = kzalloc(sizeof(struct btrfs_super_block),
 				      GFP_KERNEL);
 	if (!fs_info->super_copy) {
@@ -128,6 +130,8 @@  struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 		return NULL;
 	}
 
+	btrfs_init_fs_info(fs_info);
+
 	fs_info->nodesize = nodesize;
 	fs_info->sectorsize = sectorsize;
 
@@ -138,30 +142,6 @@  struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize)
 		return NULL;
 	}
 
-	spin_lock_init(&fs_info->buffer_lock);
-	spin_lock_init(&fs_info->qgroup_lock);
-	spin_lock_init(&fs_info->super_lock);
-	spin_lock_init(&fs_info->fs_roots_radix_lock);
-	spin_lock_init(&fs_info->tree_mod_seq_lock);
-	mutex_init(&fs_info->qgroup_ioctl_lock);
-	mutex_init(&fs_info->qgroup_rescan_lock);
-	rwlock_init(&fs_info->tree_mod_log_lock);
-	fs_info->running_transaction = NULL;
-	fs_info->qgroup_tree = RB_ROOT;
-	fs_info->qgroup_ulist = NULL;
-	atomic64_set(&fs_info->tree_mod_seq, 0);
-	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
-	INIT_LIST_HEAD(&fs_info->dead_roots);
-	INIT_LIST_HEAD(&fs_info->tree_mod_seq_list);
-	INIT_LIST_HEAD(&fs_info->fs_devices->devices);
-	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
-	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[0],
-			    IO_TREE_FS_INFO_FREED_EXTENTS0, NULL);
-	extent_io_tree_init(fs_info, &fs_info->freed_extents[1],
-			    IO_TREE_FS_INFO_FREED_EXTENTS1, NULL);
-	extent_map_tree_init(&fs_info->mapping_tree);
-	fs_info->pinned_extents = &fs_info->freed_extents[0];
 	set_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
 
 	test_mnt->mnt_sb->s_fs_info = fs_info;