Re: [PATCH 6/8] btrfs: don't allocate file extent tree for non regular files

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/5/8 21:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> When not using the NO_HOLES feature we always allocate an io tree for an
> inode's file_extent_tree.

I'm wondering can we discard non-NO_HOLES support directly so that we
can get rid of the file_extent_tree completely?

Initially I'm wondering why NO_HOLES makes a difference, especially as
NO_HOLES can be set halfway, thus we can still have explicit hole extents.

But it turns out that the whole file_extent_tree() is only to handle
non-NO_HOLES case so that we always have a hole for the whole file range
until i_size.

Considering NO_HOLES is considered safe at 4.0 kernel, can we start
deprecating non-NO_HOLES?

> This is wasteful because that io tree is only
> used for regular files, so we allocate more memory than needed for inodes
> that represent directories or symlinks for example, or for inodes that
> correspond to free space inodes.
>
> So improve on this by allocating the io tree only for inodes of regular
> files that are not free space inodes.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Otherwise looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/file-item.c | 13 ++++++-----
>   fs/btrfs/inode.c     | 53 +++++++++++++++++++++++++++++---------------
>   2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index bce95f871750..f3ed78e21fa4 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -45,13 +45,12 @@
>    */
>   void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_size)
>   {
> -	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	u64 start, end, i_size;
>   	int ret;
>
>   	spin_lock(&inode->lock);
>   	i_size = new_i_size ?: i_size_read(&inode->vfs_inode);
> -	if (btrfs_fs_incompat(fs_info, NO_HOLES)) {
> +	if (!inode->file_extent_tree) {
>   		inode->disk_i_size = i_size;
>   		goto out_unlock;
>   	}
> @@ -84,13 +83,14 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
>   int btrfs_inode_set_file_extent_range(struct btrfs_inode *inode, u64 start,
>   				      u64 len)
>   {
> +	if (!inode->file_extent_tree)
> +		return 0;
> +
>   	if (len == 0)
>   		return 0;
>
>   	ASSERT(IS_ALIGNED(start + len, inode->root->fs_info->sectorsize));
>
> -	if (btrfs_fs_incompat(inode->root->fs_info, NO_HOLES))
> -		return 0;
>   	return set_extent_bit(inode->file_extent_tree, start, start + len - 1,
>   			      EXTENT_DIRTY, NULL);
>   }
> @@ -112,14 +112,15 @@ int btrfs_inode_set_file_extent_range(struct btrfs_inode *inode, u64 start,
>   int btrfs_inode_clear_file_extent_range(struct btrfs_inode *inode, u64 start,
>   					u64 len)
>   {
> +	if (!inode->file_extent_tree)
> +		return 0;
> +
>   	if (len == 0)
>   		return 0;
>
>   	ASSERT(IS_ALIGNED(start + len, inode->root->fs_info->sectorsize) ||
>   	       len == (u64)-1);
>
> -	if (btrfs_fs_incompat(inode->root->fs_info, NO_HOLES))
> -		return 0;
>   	return clear_extent_bit(inode->file_extent_tree, start,
>   				start + len - 1, EXTENT_DIRTY, NULL);
>   }
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 9b98aa65cc63..175fd007f0ef 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3781,6 +3781,30 @@ static noinline int acls_after_inode_item(struct extent_buffer *leaf,
>   	return 1;
>   }
>
> +static int btrfs_init_file_extent_tree(struct btrfs_inode *inode)
> +{
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +
> +	if (WARN_ON_ONCE(inode->file_extent_tree))
> +		return 0;
> +	if (btrfs_fs_incompat(fs_info, NO_HOLES))
> +		return 0;
> +	if (!S_ISREG(inode->vfs_inode.i_mode))
> +		return 0;
> +	if (btrfs_is_free_space_inode(inode))
> +		return 0;
> +
> +	inode->file_extent_tree = kmalloc(sizeof(struct extent_io_tree), GFP_KERNEL);
> +	if (!inode->file_extent_tree)
> +		return -ENOMEM;
> +
> +	extent_io_tree_init(fs_info, inode->file_extent_tree, IO_TREE_INODE_FILE_EXTENT);
> +	/* Lockdep class is set only for the file extent tree. */
> +	lockdep_set_class(&inode->file_extent_tree->lock, &file_extent_tree_class);
> +
> +	return 0;
> +}
> +
>   /*
>    * read an inode from the btree into the in-memory inode
>    */
> @@ -3800,6 +3824,10 @@ static int btrfs_read_locked_inode(struct inode *inode,
>   	bool filled = false;
>   	int first_xattr_slot;
>
> +	ret = btrfs_init_file_extent_tree(BTRFS_I(inode));
> +	if (ret)
> +		return ret;
> +
>   	ret = btrfs_fill_inode(inode, &rdev);
>   	if (!ret)
>   		filled = true;
> @@ -6247,6 +6275,10 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
>   		BTRFS_I(inode)->root = btrfs_grab_root(BTRFS_I(dir)->root);
>   	root = BTRFS_I(inode)->root;
>
> +	ret = btrfs_init_file_extent_tree(BTRFS_I(inode));
> +	if (ret)
> +		goto out;
> +
>   	ret = btrfs_get_free_objectid(root, &objectid);
>   	if (ret)
>   		goto out;
> @@ -8413,20 +8445,10 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
>   	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
>   	struct btrfs_inode *ei;
>   	struct inode *inode;
> -	struct extent_io_tree *file_extent_tree = NULL;
> -
> -	/* Self tests may pass a NULL fs_info. */
> -	if (fs_info && !btrfs_fs_incompat(fs_info, NO_HOLES)) {
> -		file_extent_tree = kmalloc(sizeof(struct extent_io_tree), GFP_KERNEL);
> -		if (!file_extent_tree)
> -			return NULL;
> -	}
>
>   	ei = alloc_inode_sb(sb, btrfs_inode_cachep, GFP_KERNEL);
> -	if (!ei) {
> -		kfree(file_extent_tree);
> +	if (!ei)
>   		return NULL;
> -	}
>
>   	ei->root = NULL;
>   	ei->generation = 0;
> @@ -8471,13 +8493,8 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
>   	extent_io_tree_init(fs_info, &ei->io_tree, IO_TREE_INODE_IO);
>   	ei->io_tree.inode = ei;
>
> -	ei->file_extent_tree = file_extent_tree;
> -	if (file_extent_tree) {
> -		extent_io_tree_init(fs_info, ei->file_extent_tree,
> -				    IO_TREE_INODE_FILE_EXTENT);
> -		/* Lockdep class is set only for the file extent tree. */
> -		lockdep_set_class(&ei->file_extent_tree->lock, &file_extent_tree_class);
> -	}
> +	ei->file_extent_tree = NULL;
> +
>   	mutex_init(&ei->log_mutex);
>   	spin_lock_init(&ei->ordered_tree_lock);
>   	ei->ordered_tree = RB_ROOT;