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

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

[David Sterba]

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.
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;
-- 
2.42.1

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

[Anand Jain]

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;

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

[David Sterba]

On Wed, Nov 29, 2023 at 05:21:44PM +0800, Anand Jain wrote:
> 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?

Yes, there's kfree call missing in btrfs_free_inode(), thanks.