Re: [PATCH] btrfs: allocate btrfs_inode::file_extent_tree only without NO_HOLES

[Anand Jain <anand.jain@oracle.com>]

On 29/11/2023 05:23, David Sterba wrote:
> The file_extent_tree was added in 41a2ee75aab0 ("btrfs: introduce
> per-inode file extent tree") so we have an explicit mapping of the file
> extents to know where it is safe to update i_size. When the feature
> NO_HOLES is enabled, and it's been a mkfs default since 5.15, the tree
> is not necessary.
> 
> To save some space in the inode, allocate the tree only when necessary.

   I don't see how the free is taken care? Does it memleak?

Thanks, Anand

> This reduces size by 16 bytes from 1096 to 1080 on a x86_64 release
> config.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/btrfs_inode.h    |  6 ++++--
>   fs/btrfs/extent-io-tree.c |  2 ++
>   fs/btrfs/file-item.c      |  6 +++---
>   fs/btrfs/inode.c          | 23 ++++++++++++++++++-----
>   4 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 5572ae52444e..bfbd0d896fcf 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -107,9 +107,11 @@ struct btrfs_inode {
>   
>   	/*
>   	 * Keep track of where the inode has extent items mapped in order to
> -	 * make sure the i_size adjustments are accurate
> +	 * make sure the i_size adjustments are accurate. Not required when the
> +	 * filesystem is NO_HOLES, the status can't be set while mounted as
> +	 * it's a mkfs-time feature.
>   	 */
> -	struct extent_io_tree file_extent_tree;
> +	struct extent_io_tree *file_extent_tree;
>   
>   	/* held while logging the inode in tree-log.c */
>   	struct mutex log_mutex;
> diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
> index dbd201a99693..e3ee5449cc4a 100644
> --- a/fs/btrfs/extent-io-tree.c
> +++ b/fs/btrfs/extent-io-tree.c
> @@ -962,6 +962,8 @@ int find_contiguous_extent_bit(struct extent_io_tree *tree, u64 start,
>   	struct extent_state *state;
>   	int ret = 1;
>   
> +	ASSERT(!btrfs_fs_incompat(extent_io_tree_to_fs_info(tree), NO_HOLES));
> +
>   	spin_lock(&tree->lock);
>   	state = find_first_extent_bit_state(tree, start, bits);
>   	if (state) {
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 45cae356e89b..1f0110f48353 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -59,7 +59,7 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
>   		goto out_unlock;
>   	}
>   
> -	ret = find_contiguous_extent_bit(&inode->file_extent_tree, 0, &start,
> +	ret = find_contiguous_extent_bit(inode->file_extent_tree, 0, &start,
>   					 &end, EXTENT_DIRTY);
>   	if (!ret && start == 0)
>   		i_size = min(i_size, end + 1);
> @@ -94,7 +94,7 @@ int btrfs_inode_set_file_extent_range(struct btrfs_inode *inode, u64 start,
>   
>   	if (btrfs_fs_incompat(inode->root->fs_info, NO_HOLES))
>   		return 0;
> -	return set_extent_bit(&inode->file_extent_tree, start, start + len - 1,
> +	return set_extent_bit(inode->file_extent_tree, start, start + len - 1,
>   			      EXTENT_DIRTY, NULL);
>   }
>   
> @@ -123,7 +123,7 @@ int btrfs_inode_clear_file_extent_range(struct btrfs_inode *inode, u64 start,
>   
>   	if (btrfs_fs_incompat(inode->root->fs_info, NO_HOLES))
>   		return 0;
> -	return clear_extent_bit(&inode->file_extent_tree, start,
> +	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 096b3004a19f..04bbcb6bf34b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8472,10 +8472,20 @@ 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)
> +	if (!ei) {
> +		kfree(file_extent_tree);
>   		return NULL;
> +	}
>   
>   	ei->root = NULL;
>   	ei->generation = 0;
> @@ -8516,10 +8526,13 @@ 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;
>   
> -	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 = 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);
> +	}
>   	mutex_init(&ei->log_mutex);
>   	spin_lock_init(&ei->ordered_tree_lock);
>   	ei->ordered_tree = RB_ROOT;