diff mbox series

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

Message ID 6bd1663678f119791c1e2b6071f4973f35dcf049.1715708811.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series [v2] btrfs: reduce ordered_extent_lock section at btrfs_split_ordered_extent() | expand

Commit Message

Filipe Manana May 14, 2024, 5:54 p.m. UTC 
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.

Comments

David Sterba May 28, 2024, 10:22 p.m. UTC | #1 
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>
Filipe Manana May 29, 2024, 11:51 a.m. UTC | #2 
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.
David Sterba May 29, 2024, 3:16 p.m. UTC | #3 
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.
diff mbox series

Patch

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);