[PATCH 0/9] btrfs: cleanups and minor performance change to setting/clearing delalloc

[PATCH 0/9] btrfs: cleanups and minor performance change to setting/clearing delalloc

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some cleanups and a minor performance improvement around setting and
clearing delalloc ranges. More details in the changelogs.

Filipe Manana (9):
  btrfs: stop passing root argument to btrfs_add_delalloc_inodes()
  btrfs: stop passing root argument to __btrfs_del_delalloc_inode()
  btrfs: assert root delalloc lock is held at __btrfs_del_delalloc_inode()
  btrfs: rename btrfs_add_delalloc_inodes() to singular form
  btrfs: reduce inode lock critical section when setting and clearing delalloc
  btrfs: add lockdep assertion to remaining delalloc callbacks
  btrfs: use assertion instead of BUG_ON when adding/removing to delalloc list
  btrfs: remove do_list variable at btrfs_set_delalloc_extent()
  btrfs: remove do_list variable at btrfs_clear_delalloc_extent()

 fs/btrfs/btrfs_inode.h |  3 +-
 fs/btrfs/disk-io.c     |  2 +-
 fs/btrfs/inode.c       | 93 ++++++++++++++++++++++++++----------------
 3 files changed, 60 insertions(+), 38 deletions(-)

-- 
2.40.1

[PATCH 1/9] btrfs: stop passing root argument to btrfs_add_delalloc_inodes()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to pass a root argument to btrfs_add_delalloc_inodes(), we
can just pass the inode since the root is always the root associated to
the inode in the context it's called. So remove it and have the single
caller pass only the inode.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2d16bb08e905..e3d12d8cf088 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2385,10 +2385,10 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
 	spin_unlock(&inode->lock);
 }
 
-static void btrfs_add_delalloc_inodes(struct btrfs_root *root,
-				      struct btrfs_inode *inode)
+static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
 {
-	struct btrfs_fs_info *fs_info = inode->root->fs_info;
+	struct btrfs_root *root = inode->root;
+	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	spin_lock(&root->delalloc_lock);
 	if (list_empty(&inode->delalloc_inodes)) {
@@ -2451,7 +2451,6 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 	 * bit, which is only set or cleared with irqs on
 	 */
 	if (!(state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
-		struct btrfs_root *root = inode->root;
 		u64 len = state->end + 1 - state->start;
 		u32 num_extents = count_max_extents(fs_info, len);
 		bool do_list = !btrfs_is_free_space_inode(inode);
@@ -2472,7 +2471,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 			inode->defrag_bytes += len;
 		if (do_list && !test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
 					 &inode->runtime_flags))
-			btrfs_add_delalloc_inodes(root, inode);
+			btrfs_add_delalloc_inodes(inode);
 		spin_unlock(&inode->lock);
 	}
 
-- 
2.40.1

Re: [PATCH 1/9] btrfs: stop passing root argument to btrfs_add_delalloc_inodes()

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> There's no need to pass a root argument to btrfs_add_delalloc_inodes(), we
> can just pass the inode since the root is always the root associated to
> the inode in the context it's called. So remove it and have the single
> caller pass only the inode.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/inode.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2d16bb08e905..e3d12d8cf088 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2385,10 +2385,10 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
>   	spin_unlock(&inode->lock);
>   }
>
> -static void btrfs_add_delalloc_inodes(struct btrfs_root *root,
> -				      struct btrfs_inode *inode)
> +static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
>   {
> -	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	struct btrfs_root *root = inode->root;
> +	struct btrfs_fs_info *fs_info = root->fs_info;
>
>   	spin_lock(&root->delalloc_lock);
>   	if (list_empty(&inode->delalloc_inodes)) {
> @@ -2451,7 +2451,6 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
>   	 * bit, which is only set or cleared with irqs on
>   	 */
>   	if (!(state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
> -		struct btrfs_root *root = inode->root;
>   		u64 len = state->end + 1 - state->start;
>   		u32 num_extents = count_max_extents(fs_info, len);
>   		bool do_list = !btrfs_is_free_space_inode(inode);
> @@ -2472,7 +2471,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
>   			inode->defrag_bytes += len;
>   		if (do_list && !test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
>   					 &inode->runtime_flags))
> -			btrfs_add_delalloc_inodes(root, inode);
> +			btrfs_add_delalloc_inodes(inode);
>   		spin_unlock(&inode->lock);
>   	}
>

[PATCH 2/9] btrfs: stop passing root argument to __btrfs_del_delalloc_inode()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to pass a root argument to __btrfs_del_delalloc_inode()
and btrfs_del_delalloc_inode(), we can just pass the inode since the root
is always the root associated to that inode. Some remove the root argument
from these functions.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/btrfs_inode.h |  2 +-
 fs/btrfs/disk-io.c     |  2 +-
 fs/btrfs/inode.c       | 15 +++++++--------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 83d78a6f3aa2..32c59c5bd94d 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -428,7 +428,7 @@ noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 			      u64 *orig_start, u64 *orig_block_len,
 			      u64 *ram_bytes, bool nowait, bool strict);
 
-void __btrfs_del_delalloc_inode(struct btrfs_root *root, struct btrfs_inode *inode);
+void __btrfs_del_delalloc_inode(struct btrfs_inode *inode);
 struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
 int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index);
 int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4280f8e23461..8ab185182c30 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4629,7 +4629,7 @@ static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root)
 		struct inode *inode = NULL;
 		btrfs_inode = list_first_entry(&splice, struct btrfs_inode,
 					       delalloc_inodes);
-		__btrfs_del_delalloc_inode(root, btrfs_inode);
+		__btrfs_del_delalloc_inode(btrfs_inode);
 		spin_unlock(&root->delalloc_lock);
 
 		/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e3d12d8cf088..ec8af7d0f166 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2406,9 +2406,9 @@ static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
 	spin_unlock(&root->delalloc_lock);
 }
 
-void __btrfs_del_delalloc_inode(struct btrfs_root *root,
-				struct btrfs_inode *inode)
+void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
 {
+	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	if (!list_empty(&inode->delalloc_inodes)) {
@@ -2426,12 +2426,11 @@ void __btrfs_del_delalloc_inode(struct btrfs_root *root,
 	}
 }
 
-static void btrfs_del_delalloc_inode(struct btrfs_root *root,
-				     struct btrfs_inode *inode)
+static void btrfs_del_delalloc_inode(struct btrfs_inode *inode)
 {
-	spin_lock(&root->delalloc_lock);
-	__btrfs_del_delalloc_inode(root, inode);
-	spin_unlock(&root->delalloc_lock);
+	spin_lock(&inode->root->delalloc_lock);
+	__btrfs_del_delalloc_inode(inode);
+	spin_unlock(&inode->root->delalloc_lock);
 }
 
 /*
@@ -2538,7 +2537,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 		if (do_list && inode->delalloc_bytes == 0 &&
 		    test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
 					&inode->runtime_flags))
-			btrfs_del_delalloc_inode(root, inode);
+			btrfs_del_delalloc_inode(inode);
 		spin_unlock(&inode->lock);
 	}
 
-- 
2.40.1

Re: [PATCH 2/9] btrfs: stop passing root argument to __btrfs_del_delalloc_inode()

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> There's no need to pass a root argument to __btrfs_del_delalloc_inode()
> and btrfs_del_delalloc_inode(), we can just pass the inode since the root
> is always the root associated to that inode. Some remove the root argument
> from these functions.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/btrfs_inode.h |  2 +-
>   fs/btrfs/disk-io.c     |  2 +-
>   fs/btrfs/inode.c       | 15 +++++++--------
>   3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 83d78a6f3aa2..32c59c5bd94d 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -428,7 +428,7 @@ noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>   			      u64 *orig_start, u64 *orig_block_len,
>   			      u64 *ram_bytes, bool nowait, bool strict);
>
> -void __btrfs_del_delalloc_inode(struct btrfs_root *root, struct btrfs_inode *inode);
> +void __btrfs_del_delalloc_inode(struct btrfs_inode *inode);
>   struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
>   int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index);
>   int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 4280f8e23461..8ab185182c30 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4629,7 +4629,7 @@ static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root)
>   		struct inode *inode = NULL;
>   		btrfs_inode = list_first_entry(&splice, struct btrfs_inode,
>   					       delalloc_inodes);
> -		__btrfs_del_delalloc_inode(root, btrfs_inode);
> +		__btrfs_del_delalloc_inode(btrfs_inode);
>   		spin_unlock(&root->delalloc_lock);
>
>   		/*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e3d12d8cf088..ec8af7d0f166 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2406,9 +2406,9 @@ static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
>   	spin_unlock(&root->delalloc_lock);
>   }
>
> -void __btrfs_del_delalloc_inode(struct btrfs_root *root,
> -				struct btrfs_inode *inode)
> +void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
>   {
> +	struct btrfs_root *root = inode->root;
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>
>   	if (!list_empty(&inode->delalloc_inodes)) {
> @@ -2426,12 +2426,11 @@ void __btrfs_del_delalloc_inode(struct btrfs_root *root,
>   	}
>   }
>
> -static void btrfs_del_delalloc_inode(struct btrfs_root *root,
> -				     struct btrfs_inode *inode)
> +static void btrfs_del_delalloc_inode(struct btrfs_inode *inode)
>   {
> -	spin_lock(&root->delalloc_lock);
> -	__btrfs_del_delalloc_inode(root, inode);
> -	spin_unlock(&root->delalloc_lock);
> +	spin_lock(&inode->root->delalloc_lock);
> +	__btrfs_del_delalloc_inode(inode);
> +	spin_unlock(&inode->root->delalloc_lock);
>   }
>
>   /*
> @@ -2538,7 +2537,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>   		if (do_list && inode->delalloc_bytes == 0 &&
>   		    test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
>   					&inode->runtime_flags))
> -			btrfs_del_delalloc_inode(root, inode);
> +			btrfs_del_delalloc_inode(inode);
>   		spin_unlock(&inode->lock);
>   	}
>

[PATCH 3/9] btrfs: assert root delalloc lock is held at __btrfs_del_delalloc_inode()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

This function requires the delalloc lock of the inode's root to be held,
so assert it's held.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ec8af7d0f166..3a19e30676e8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2411,6 +2411,8 @@ void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
+	lockdep_assert_held(&root->delalloc_lock);
+
 	if (!list_empty(&inode->delalloc_inodes)) {
 		list_del_init(&inode->delalloc_inodes);
 		clear_bit(BTRFS_INODE_IN_DELALLOC_LIST,
-- 
2.40.1

Re: [PATCH 3/9] btrfs: assert root delalloc lock is held at __btrfs_del_delalloc_inode()

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> This function requires the delalloc lock of the inode's root to be held,
> so assert it's held.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ec8af7d0f166..3a19e30676e8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2411,6 +2411,8 @@ void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
>   	struct btrfs_root *root = inode->root;
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>
> +	lockdep_assert_held(&root->delalloc_lock);
> +
>   	if (!list_empty(&inode->delalloc_inodes)) {
>   		list_del_init(&inode->delalloc_inodes);
>   		clear_bit(BTRFS_INODE_IN_DELALLOC_LIST,

[PATCH 4/9] btrfs: rename btrfs_add_delalloc_inodes() to singular form

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The function btrfs_add_delalloc_inodes() adds a single inode its root's
list of delalloc inodes, so it doesn't make any sense at all for the
function's name to be plural. Rename it to the singular form
btrfs_add_delalloc_inode() to avoid any confusion.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3a19e30676e8..b273fdbd63cd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2385,7 +2385,7 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
 	spin_unlock(&inode->lock);
 }
 
-static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
+static void btrfs_add_delalloc_inode(struct btrfs_inode *inode)
 {
 	struct btrfs_root *root = inode->root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -2472,7 +2472,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 			inode->defrag_bytes += len;
 		if (do_list && !test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
 					 &inode->runtime_flags))
-			btrfs_add_delalloc_inodes(inode);
+			btrfs_add_delalloc_inode(inode);
 		spin_unlock(&inode->lock);
 	}
 
-- 
2.40.1

Re: [PATCH 4/9] btrfs: rename btrfs_add_delalloc_inodes() to singular form

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The function btrfs_add_delalloc_inodes() adds a single inode its root's
> list of delalloc inodes, so it doesn't make any sense at all for the
> function's name to be plural. Rename it to the singular form
> btrfs_add_delalloc_inode() to avoid any confusion.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3a19e30676e8..b273fdbd63cd 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2385,7 +2385,7 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
>   	spin_unlock(&inode->lock);
>   }
>
> -static void btrfs_add_delalloc_inodes(struct btrfs_inode *inode)
> +static void btrfs_add_delalloc_inode(struct btrfs_inode *inode)
>   {
>   	struct btrfs_root *root = inode->root;
>   	struct btrfs_fs_info *fs_info = root->fs_info;
> @@ -2472,7 +2472,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
>   			inode->defrag_bytes += len;
>   		if (do_list && !test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
>   					 &inode->runtime_flags))
> -			btrfs_add_delalloc_inodes(inode);
> +			btrfs_add_delalloc_inode(inode);
>   		spin_unlock(&inode->lock);
>   	}
>

[PATCH 5/9] btrfs: reduce inode lock critical section when setting and clearing delalloc

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When setting and clearing a delalloc range, at btrfs_set_delalloc_extent()
and btrfs_clear_delalloc_extent(), we are adding/removing the inode
to/from the root's list of delalloc inodes while under the protection of
the inode's lock. This however is not needed, we can add and remove the
inode to the root's list without holding the inode's lock because here
we are under the protection of the io tree's lock, reducing the size of
the critical section delimited by the inode's lock. The inode's lock is
used in many other places such as when finishing an ordered extent (when
calling btrfs_update_inode_bytes() or btrfs_delalloc_release_metadata(),
or decreasing the number of outstanding extents) or when reserving space
when doing a buffered or direct IO write (calls to functions from
delalloc-space.c).

So move the inode add/remove operations to the root's list of delalloc
inodes to outside the critical section delimited by the inode's lock.
This also allows us to get rid of the BTRFS_INODE_IN_DELALLOC_LIST flag
since we can rely on the inode's delalloc bytes counter to determine if
the inode is or is not in the list.

The following fio based test, that exercises IO to multiple files in the
same subvolume, was used to test:

   $ cat test.sh
   #!/bin/bash

   DEV=/dev/nullb0
   MNT=/mnt/nullb0
   MOUNT_OPTIONS="-o ssd"

   mkfs.btrfs -f $DEV &> /dev/null
   mount $MOUNT_OPTIONS $DEV $MNT

   fio --direct=0 --ioengine=sync --thread --directory=$MNT \
       --invalidate=1 --group_reporting=1 \
       --new_group --rw=randwrite --size=50m --numjobs=200 \
       --bs=4k --fsync_on_close=0 --fallocate=none --end_fsync=0 \
       --name=foo --filename_format=FioWorkloads.\$jobnum

   umount $MNT

The test was run on a non-debug kernel (Debian's default kernel config)
against a 16G null block device.

Result before this patch:

   WRITE: bw=81.9MiB/s (85.9MB/s), 81.9MiB/s-81.9MiB/s (85.9MB/s-85.9MB/s), io=9.77GiB (10.5GB), run=122136-122136msec

Result after this patch:

   WRITE: bw=86.8MiB/s (91.0MB/s), 86.8MiB/s-86.8MiB/s (91.0MB/s-91.0MB/s), io=9.77GiB (10.5GB), run=115180-115180msec

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/btrfs_inode.h |  1 -
 fs/btrfs/inode.c       | 60 ++++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 32c59c5bd94d..7799d00cc5d6 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -41,7 +41,6 @@ enum {
 	  */
 	BTRFS_INODE_NEEDS_FULL_SYNC,
 	BTRFS_INODE_COPY_EVERYTHING,
-	BTRFS_INODE_IN_DELALLOC_LIST,
 	BTRFS_INODE_HAS_PROPS,
 	BTRFS_INODE_SNAPSHOT_FLUSH,
 	/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b273fdbd63cd..778bb6754e00 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2391,17 +2391,14 @@ static void btrfs_add_delalloc_inode(struct btrfs_inode *inode)
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	spin_lock(&root->delalloc_lock);
-	if (list_empty(&inode->delalloc_inodes)) {
-		list_add_tail(&inode->delalloc_inodes, &root->delalloc_inodes);
-		set_bit(BTRFS_INODE_IN_DELALLOC_LIST, &inode->runtime_flags);
-		root->nr_delalloc_inodes++;
-		if (root->nr_delalloc_inodes == 1) {
-			spin_lock(&fs_info->delalloc_root_lock);
-			BUG_ON(!list_empty(&root->delalloc_root));
-			list_add_tail(&root->delalloc_root,
-				      &fs_info->delalloc_roots);
-			spin_unlock(&fs_info->delalloc_root_lock);
-		}
+	ASSERT(list_empty(&inode->delalloc_inodes));
+	list_add_tail(&inode->delalloc_inodes, &root->delalloc_inodes);
+	root->nr_delalloc_inodes++;
+	if (root->nr_delalloc_inodes == 1) {
+		spin_lock(&fs_info->delalloc_root_lock);
+		BUG_ON(!list_empty(&root->delalloc_root));
+		list_add_tail(&root->delalloc_root, &fs_info->delalloc_roots);
+		spin_unlock(&fs_info->delalloc_root_lock);
 	}
 	spin_unlock(&root->delalloc_lock);
 }
@@ -2413,10 +2410,14 @@ void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
 
 	lockdep_assert_held(&root->delalloc_lock);
 
+	/*
+	 * We may be called after the inode was already deleted from the list,
+	 * namely in the transaction abort path btrfs_destroy_delalloc_inodes(),
+	 * and then later through btrfs_clear_delalloc_extent() while the inode
+	 * still has ->delalloc_bytes > 0.
+	 */
 	if (!list_empty(&inode->delalloc_inodes)) {
 		list_del_init(&inode->delalloc_inodes);
-		clear_bit(BTRFS_INODE_IN_DELALLOC_LIST,
-			  &inode->runtime_flags);
 		root->nr_delalloc_inodes--;
 		if (!root->nr_delalloc_inodes) {
 			ASSERT(list_empty(&root->delalloc_inodes));
@@ -2444,6 +2445,8 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 
+	lockdep_assert_held(&inode->io_tree.lock);
+
 	if ((bits & EXTENT_DEFRAG) && !(bits & EXTENT_DELALLOC))
 		WARN_ON(1);
 	/*
@@ -2453,6 +2456,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 	 */
 	if (!(state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
 		u64 len = state->end + 1 - state->start;
+		u64 prev_delalloc_bytes;
 		u32 num_extents = count_max_extents(fs_info, len);
 		bool do_list = !btrfs_is_free_space_inode(inode);
 
@@ -2467,13 +2471,20 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 		percpu_counter_add_batch(&fs_info->delalloc_bytes, len,
 					 fs_info->delalloc_batch);
 		spin_lock(&inode->lock);
+		prev_delalloc_bytes = inode->delalloc_bytes;
 		inode->delalloc_bytes += len;
 		if (bits & EXTENT_DEFRAG)
 			inode->defrag_bytes += len;
-		if (do_list && !test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
-					 &inode->runtime_flags))
-			btrfs_add_delalloc_inode(inode);
 		spin_unlock(&inode->lock);
+
+		/*
+		 * We don't need to be under the protection of the inode's lock,
+		 * because we are called while holding the inode's io_tree lock
+		 * and are therefore protected against concurrent calls of this
+		 * function and btrfs_clear_delalloc_extent().
+		 */
+		if (do_list && prev_delalloc_bytes == 0)
+			btrfs_add_delalloc_inode(inode);
 	}
 
 	if (!(state->state & EXTENT_DELALLOC_NEW) &&
@@ -2495,6 +2506,8 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 	u64 len = state->end + 1 - state->start;
 	u32 num_extents = count_max_extents(fs_info, len);
 
+	lockdep_assert_held(&inode->io_tree.lock);
+
 	if ((state->state & EXTENT_DEFRAG) && (bits & EXTENT_DEFRAG)) {
 		spin_lock(&inode->lock);
 		inode->defrag_bytes -= len;
@@ -2508,6 +2521,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 	 */
 	if ((state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
 		struct btrfs_root *root = inode->root;
+		u64 new_delalloc_bytes;
 		bool do_list = !btrfs_is_free_space_inode(inode);
 
 		spin_lock(&inode->lock);
@@ -2536,11 +2550,17 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 					 fs_info->delalloc_batch);
 		spin_lock(&inode->lock);
 		inode->delalloc_bytes -= len;
-		if (do_list && inode->delalloc_bytes == 0 &&
-		    test_bit(BTRFS_INODE_IN_DELALLOC_LIST,
-					&inode->runtime_flags))
-			btrfs_del_delalloc_inode(inode);
+		new_delalloc_bytes = inode->delalloc_bytes;
 		spin_unlock(&inode->lock);
+
+		/*
+		 * We don't need to be under the protection of the inode's lock,
+		 * because we are called while holding the inode's io_tree lock
+		 * and are therefore protected against concurrent calls of this
+		 * function and btrfs_set_delalloc_extent().
+		 */
+		if (do_list && new_delalloc_bytes == 0)
+			btrfs_del_delalloc_inode(inode);
 	}
 
 	if ((state->state & EXTENT_DELALLOC_NEW) &&
-- 
2.40.1

[PATCH 6/9] btrfs: add lockdep assertion to remaining delalloc callbacks

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The merge and split callbacks for an inode's io tree are supposed to be
called while the io tree's spinlock is being held, so that the given
extent_state records are stable, not modified or freed while the callbacks
are using them. So add lockdep assertions in the callbacks.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 778bb6754e00..c7a5fb1f8b3e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2300,6 +2300,8 @@ void btrfs_split_delalloc_extent(struct btrfs_inode *inode,
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	u64 size;
 
+	lockdep_assert_held(&inode->io_tree.lock);
+
 	/* not delalloc, ignore it */
 	if (!(orig->state & EXTENT_DELALLOC))
 		return;
@@ -2338,6 +2340,8 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
 	u64 new_size, old_size;
 	u32 num_extents;
 
+	lockdep_assert_held(&inode->io_tree.lock);
+
 	/* not delalloc, ignore it */
 	if (!(other->state & EXTENT_DELALLOC))
 		return;
-- 
2.40.1

Re: [PATCH 6/9] btrfs: add lockdep assertion to remaining delalloc callbacks

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The merge and split callbacks for an inode's io tree are supposed to be
> called while the io tree's spinlock is being held, so that the given
> extent_state records are stable, not modified or freed while the callbacks
> are using them. So add lockdep assertions in the callbacks.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 778bb6754e00..c7a5fb1f8b3e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2300,6 +2300,8 @@ void btrfs_split_delalloc_extent(struct btrfs_inode *inode,
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	u64 size;
>
> +	lockdep_assert_held(&inode->io_tree.lock);
> +
>   	/* not delalloc, ignore it */
>   	if (!(orig->state & EXTENT_DELALLOC))
>   		return;
> @@ -2338,6 +2340,8 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
>   	u64 new_size, old_size;
>   	u32 num_extents;
>
> +	lockdep_assert_held(&inode->io_tree.lock);
> +
>   	/* not delalloc, ignore it */
>   	if (!(other->state & EXTENT_DELALLOC))
>   		return;

[PATCH 7/9] btrfs: use assertion instead of BUG_ON when adding/removing to delalloc list

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When adding or removing and inode to/from the root's delalloc list,
instead of using a BUG_ON() to validate list emptiness, use ASSERT()
since this is to check logic errors rather than real errors.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c7a5fb1f8b3e..fe962a6045fd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2400,7 +2400,7 @@ static void btrfs_add_delalloc_inode(struct btrfs_inode *inode)
 	root->nr_delalloc_inodes++;
 	if (root->nr_delalloc_inodes == 1) {
 		spin_lock(&fs_info->delalloc_root_lock);
-		BUG_ON(!list_empty(&root->delalloc_root));
+		ASSERT(list_empty(&root->delalloc_root));
 		list_add_tail(&root->delalloc_root, &fs_info->delalloc_roots);
 		spin_unlock(&fs_info->delalloc_root_lock);
 	}
@@ -2426,7 +2426,7 @@ void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
 		if (!root->nr_delalloc_inodes) {
 			ASSERT(list_empty(&root->delalloc_inodes));
 			spin_lock(&fs_info->delalloc_root_lock);
-			BUG_ON(list_empty(&root->delalloc_root));
+			ASSERT(!list_empty(&root->delalloc_root));
 			list_del_init(&root->delalloc_root);
 			spin_unlock(&fs_info->delalloc_root_lock);
 		}
-- 
2.40.1

Re: [PATCH 7/9] btrfs: use assertion instead of BUG_ON when adding/removing to delalloc list

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When adding or removing and inode to/from the root's delalloc list,
> instead of using a BUG_ON() to validate list emptiness, use ASSERT()
> since this is to check logic errors rather than real errors.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c7a5fb1f8b3e..fe962a6045fd 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2400,7 +2400,7 @@ static void btrfs_add_delalloc_inode(struct btrfs_inode *inode)
>   	root->nr_delalloc_inodes++;
>   	if (root->nr_delalloc_inodes == 1) {
>   		spin_lock(&fs_info->delalloc_root_lock);
> -		BUG_ON(!list_empty(&root->delalloc_root));
> +		ASSERT(list_empty(&root->delalloc_root));
>   		list_add_tail(&root->delalloc_root, &fs_info->delalloc_roots);
>   		spin_unlock(&fs_info->delalloc_root_lock);
>   	}
> @@ -2426,7 +2426,7 @@ void __btrfs_del_delalloc_inode(struct btrfs_inode *inode)
>   		if (!root->nr_delalloc_inodes) {
>   			ASSERT(list_empty(&root->delalloc_inodes));
>   			spin_lock(&fs_info->delalloc_root_lock);
> -			BUG_ON(list_empty(&root->delalloc_root));
> +			ASSERT(!list_empty(&root->delalloc_root));
>   			list_del_init(&root->delalloc_root);
>   			spin_unlock(&fs_info->delalloc_root_lock);
>   		}

[PATCH 8/9] btrfs: remove do_list variable at btrfs_set_delalloc_extent()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The "do_list" variable is only used once, plus its name/meaning is a bit
confusing, so remove it and directory use btrfs_is_free_space_inode().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fe962a6045fd..17b6ab71584a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2462,7 +2462,6 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 		u64 len = state->end + 1 - state->start;
 		u64 prev_delalloc_bytes;
 		u32 num_extents = count_max_extents(fs_info, len);
-		bool do_list = !btrfs_is_free_space_inode(inode);
 
 		spin_lock(&inode->lock);
 		btrfs_mod_outstanding_extents(inode, num_extents);
@@ -2487,7 +2486,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
 		 * and are therefore protected against concurrent calls of this
 		 * function and btrfs_clear_delalloc_extent().
 		 */
-		if (do_list && prev_delalloc_bytes == 0)
+		if (!btrfs_is_free_space_inode(inode) && prev_delalloc_bytes == 0)
 			btrfs_add_delalloc_inode(inode);
 	}
 
-- 
2.40.1

Re: [PATCH 8/9] btrfs: remove do_list variable at btrfs_set_delalloc_extent()

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The "do_list" variable is only used once, plus its name/meaning is a bit
> confusing, so remove it and directory use btrfs_is_free_space_inode().
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fe962a6045fd..17b6ab71584a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2462,7 +2462,6 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
>   		u64 len = state->end + 1 - state->start;
>   		u64 prev_delalloc_bytes;
>   		u32 num_extents = count_max_extents(fs_info, len);
> -		bool do_list = !btrfs_is_free_space_inode(inode);
>
>   		spin_lock(&inode->lock);
>   		btrfs_mod_outstanding_extents(inode, num_extents);
> @@ -2487,7 +2486,7 @@ void btrfs_set_delalloc_extent(struct btrfs_inode *inode, struct extent_state *s
>   		 * and are therefore protected against concurrent calls of this
>   		 * function and btrfs_clear_delalloc_extent().
>   		 */
> -		if (do_list && prev_delalloc_bytes == 0)
> +		if (!btrfs_is_free_space_inode(inode) && prev_delalloc_bytes == 0)
>   			btrfs_add_delalloc_inode(inode);
>   	}
>

[PATCH 9/9] btrfs: remove do_list variable at btrfs_clear_delalloc_extent()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The "do_list" variable has a rather confusing name, so remove it and
directly use btrfs_is_free_space_inode() instead.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 17b6ab71584a..5bb61d4aa2cb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2525,7 +2525,6 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 	if ((state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
 		struct btrfs_root *root = inode->root;
 		u64 new_delalloc_bytes;
-		bool do_list = !btrfs_is_free_space_inode(inode);
 
 		spin_lock(&inode->lock);
 		btrfs_mod_outstanding_extents(inode, -num_extents);
@@ -2545,7 +2544,8 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 			return;
 
 		if (!btrfs_is_data_reloc_root(root) &&
-		    do_list && !(state->state & EXTENT_NORESERVE) &&
+		    !btrfs_is_free_space_inode(inode) &&
+		    !(state->state & EXTENT_NORESERVE) &&
 		    (bits & EXTENT_CLEAR_DATA_RESV))
 			btrfs_free_reserved_data_space_noquota(fs_info, len);
 
@@ -2562,7 +2562,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
 		 * and are therefore protected against concurrent calls of this
 		 * function and btrfs_set_delalloc_extent().
 		 */
-		if (do_list && new_delalloc_bytes == 0)
+		if (!btrfs_is_free_space_inode(inode) && new_delalloc_bytes == 0)
 			btrfs_del_delalloc_inode(inode);
 	}
 
-- 
2.40.1

Re: [PATCH 9/9] btrfs: remove do_list variable at btrfs_clear_delalloc_extent()

[Qu Wenruo]

On 2024/2/10 04:30, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The "do_list" variable has a rather confusing name, so remove it and
> directly use btrfs_is_free_space_inode() instead.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/inode.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 17b6ab71584a..5bb61d4aa2cb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2525,7 +2525,6 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>   	if ((state->state & EXTENT_DELALLOC) && (bits & EXTENT_DELALLOC)) {
>   		struct btrfs_root *root = inode->root;
>   		u64 new_delalloc_bytes;
> -		bool do_list = !btrfs_is_free_space_inode(inode);
>
>   		spin_lock(&inode->lock);
>   		btrfs_mod_outstanding_extents(inode, -num_extents);
> @@ -2545,7 +2544,8 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>   			return;
>
>   		if (!btrfs_is_data_reloc_root(root) &&
> -		    do_list && !(state->state & EXTENT_NORESERVE) &&
> +		    !btrfs_is_free_space_inode(inode) &&
> +		    !(state->state & EXTENT_NORESERVE) &&
>   		    (bits & EXTENT_CLEAR_DATA_RESV))
>   			btrfs_free_reserved_data_space_noquota(fs_info, len);
>
> @@ -2562,7 +2562,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
>   		 * and are therefore protected against concurrent calls of this
>   		 * function and btrfs_set_delalloc_extent().
>   		 */
> -		if (do_list && new_delalloc_bytes == 0)
> +		if (!btrfs_is_free_space_inode(inode) && new_delalloc_bytes == 0)
>   			btrfs_del_delalloc_inode(inode);
>   	}
>

Re: [PATCH 0/9] btrfs: cleanups and minor performance change to setting/clearing delalloc

[Boris Burkov]

On Fri, Feb 09, 2024 at 06:00:42PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Some cleanups and a minor performance improvement around setting and
> clearing delalloc ranges. More details in the changelogs.

These all LGTM.

If you want to hunt more wins to claim for the inode lock improvement,
you can also try running fsperf.

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> Filipe Manana (9):
>   btrfs: stop passing root argument to btrfs_add_delalloc_inodes()
>   btrfs: stop passing root argument to __btrfs_del_delalloc_inode()
>   btrfs: assert root delalloc lock is held at __btrfs_del_delalloc_inode()
>   btrfs: rename btrfs_add_delalloc_inodes() to singular form
>   btrfs: reduce inode lock critical section when setting and clearing delalloc
>   btrfs: add lockdep assertion to remaining delalloc callbacks
>   btrfs: use assertion instead of BUG_ON when adding/removing to delalloc list
>   btrfs: remove do_list variable at btrfs_set_delalloc_extent()
>   btrfs: remove do_list variable at btrfs_clear_delalloc_extent()
> 
>  fs/btrfs/btrfs_inode.h |  3 +-
>  fs/btrfs/disk-io.c     |  2 +-
>  fs/btrfs/inode.c       | 93 ++++++++++++++++++++++++++----------------
>  3 files changed, 60 insertions(+), 38 deletions(-)
> 
> -- 
> 2.40.1
>