[PATCH] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[PATCH] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to hold the root's ordered_extent_lock for so long at
btrfs_split_ordered_extent(). We don't need to hold it in order to modify
the inode's rb tree of ordered extents, to modify the trimmed ordered
extent and move checksums between the trimmed and the new ordered extent.
That's only increasing contention of the root's ordered_extent_lock for
other writes that are starting or completing.

So lock the root's ordered_extent_lock only before we add the new ordered
extent to the root's ordered list and increment the root's counter for the
number of ordered extents.

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

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index c5bdd674f55c..a6c2b4e5edf1 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -1181,7 +1181,6 @@ struct btrfs_ordered_extent *btrfs_split_ordered_extent(
 	/* One ref for the tree. */
 	refcount_inc(&new->refs);
 
-	spin_lock_irq(&root->ordered_extent_lock);
 	spin_lock(&inode->ordered_tree_lock);
 	/* Remove from tree once */
 	node = &ordered->rb_node;
@@ -1234,6 +1233,7 @@ struct btrfs_ordered_extent *btrfs_split_ordered_extent(
 			new->file_offset);
 	spin_unlock(&inode->ordered_tree_lock);
 
+	spin_lock_irq(&root->ordered_extent_lock);
 	list_add_tail(&new->root_extent_list, &root->ordered_extents);
 	root->nr_ordered_extents++;
 	spin_unlock_irq(&root->ordered_extent_lock);
-- 
2.43.0

[PATCH v2] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to hold the root's ordered_extent_lock for so long at
btrfs_split_ordered_extent(). We don't need to hold it in order to modify
the inode's rb tree of ordered extents, to modify the trimmed ordered
extent and move checksums between the trimmed and the new ordered extent.
That's only increasing contention of the root's ordered_extent_lock for
other writes that are starting or completing.

So lock the root's ordered_extent_lock only before we add the new ordered
extent to the root's ordered list and increment the root's counter for the
number of ordered extents.

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

V2: Fix a bug where we woudn't disable irqs when taking the inode's
    ordered tree lock. Often triggered with generic/208.

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index c5bdd674f55c..e6025d9645f5 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -1181,8 +1181,7 @@ struct btrfs_ordered_extent *btrfs_split_ordered_extent(
 	/* One ref for the tree. */
 	refcount_inc(&new->refs);
 
-	spin_lock_irq(&root->ordered_extent_lock);
-	spin_lock(&inode->ordered_tree_lock);
+	spin_lock_irq(&inode->ordered_tree_lock);
 	/* Remove from tree once */
 	node = &ordered->rb_node;
 	rb_erase(node, &inode->ordered_tree);
@@ -1232,8 +1231,9 @@ struct btrfs_ordered_extent *btrfs_split_ordered_extent(
 		btrfs_panic(fs_info, -EEXIST,
 			"zoned: inconsistency in ordered tree at offset %llu",
 			new->file_offset);
-	spin_unlock(&inode->ordered_tree_lock);
+	spin_unlock_irq(&inode->ordered_tree_lock);
 
+	spin_lock_irq(&root->ordered_extent_lock);
 	list_add_tail(&new->root_extent_list, &root->ordered_extents);
 	root->nr_ordered_extents++;
 	spin_unlock_irq(&root->ordered_extent_lock);
-- 
2.43.0

Re: [PATCH v2] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[David Sterba]

On Tue, May 14, 2024 at 06:54:18PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's no need to hold the root's ordered_extent_lock for so long at
> btrfs_split_ordered_extent(). We don't need to hold it in order to modify
> the inode's rb tree of ordered extents, to modify the trimmed ordered
> extent and move checksums between the trimmed and the new ordered extent.
> That's only increasing contention of the root's ordered_extent_lock for
> other writes that are starting or completing.
> 
> So lock the root's ordered_extent_lock only before we add the new ordered
> extent to the root's ordered list and increment the root's counter for the
> number of ordered extents.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[Filipe Manana]

On Tue, May 28, 2024 at 11:22 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Tue, May 14, 2024 at 06:54:18PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > There's no need to hold the root's ordered_extent_lock for so long at
> > btrfs_split_ordered_extent(). We don't need to hold it in order to modify
> > the inode's rb tree of ordered extents, to modify the trimmed ordered
> > extent and move checksums between the trimmed and the new ordered extent.
> > That's only increasing contention of the root's ordered_extent_lock for
> > other writes that are starting or completing.
> >
> > So lock the root's ordered_extent_lock only before we add the new ordered
> > extent to the root's ordered list and increment the root's counter for the
> > number of ordered extents.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Reviewed-by: David Sterba <dsterba@suse.com>

This needs to be dropped for now as there are some problems there.
I'll address those in a patch set.

Thanks.

Re: [PATCH v2] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent()

[David Sterba]

On Wed, May 29, 2024 at 12:51:13PM +0100, Filipe Manana wrote:
> On Tue, May 28, 2024 at 11:22 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Tue, May 14, 2024 at 06:54:18PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > There's no need to hold the root's ordered_extent_lock for so long at
> > > btrfs_split_ordered_extent(). We don't need to hold it in order to modify
> > > the inode's rb tree of ordered extents, to modify the trimmed ordered
> > > extent and move checksums between the trimmed and the new ordered extent.
> > > That's only increasing contention of the root's ordered_extent_lock for
> > > other writes that are starting or completing.
> > >
> > > So lock the root's ordered_extent_lock only before we add the new ordered
> > > extent to the root's ordered list and increment the root's counter for the
> > > number of ordered extents.
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >
> > Reviewed-by: David Sterba <dsterba@suse.com>
> 
> This needs to be dropped for now as there are some problems there.
> I'll address those in a patch set.

I had the patch in my testing branch and did not notice anything that
would point to this patch but I did not do any target testing.

Moving the IRQ context was fixed in v2, the pattern of independent
ranges of ordered_tree_lock and ordered_extent_lock changes the
semantics, another case is in insert_ordered_extent() but it could be
fine there.