Skip to contentSkip to conversation

Btrfs: avoid unnecessarily locking inode when clearing a range

PATCH In review [email protected] 3 participants 4 messages Jul 27, 2017 – Aug 3, 2017 Btrfs file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 Btrfs: avoid unnecessarily locking inode when clearing a range

From: Filipe Manana <[email protected]>

If the range being cleared was not marked for defrag and we are not about to clear the range from the defrag status, we don't need to lock and unlock the inode.

Signed-off-by Filipe Manana <[email protected]>
---
 fs/btrfs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/btrfs/inode.c +4 -3
index eb495e956d53..51c45c0a8553 100644
@@ -1797,10 +1797,11 @@ static void btrfs_clear_bit_hook(void *private_data,
 	u64 len = state->end + 1 - state->start;
 	u32 num_extents = count_max_extents(len);
 
-	spin_lock(&inode->lock);
-	if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG))
+	if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG)) {
+		spin_lock(&inode->lock);
 		inode->defrag_bytes -= len;
-	spin_unlock(&inode->lock);
+		spin_unlock(&inode->lock);
+	}
 
 	/*
 	 * set_bit and clear bit hooks normally require _irqsave/restore
Signature · 1 line
2.11.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · On 07/27/2017 02:52 PM, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the range being cleared was not marked for defrag and we are not about to clear the range from the defrag status, we don't need to lock and unlock the inode.

Signed-off-by Filipe Manana <[email protected]>

Thanks Filipe, looks like it goes all the way back to:

commit 47059d930f0e002ff851beea87d738146804726d

Author: Wang Shilong <[email protected]>
Date:   Thu Jul 3 18:22:07 2014 +0800

Btrfs: make defragment work with nodatacow option

I can't see how the inode lock is required here.

Reviewed-by Chris Mason <[email protected]>

-chris

Quoted 24 lines
---
  fs/btrfs/inode.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
fs/btrfs/inode.c +4 -3
index eb495e956d53..51c45c0a8553 100644
@@ -1797,10 +1797,11 @@ static void btrfs_clear_bit_hook(void *private_data,
  	u64 len = state->end + 1 - state->start;
  	u32 num_extents = count_max_extents(len);
 
-	spin_lock(&inode->lock);
-	if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG))
+	if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG)) {
+		spin_lock(&inode->lock);
  		inode->defrag_bytes -= len;
-	spin_unlock(&inode->lock);
+		spin_unlock(&inode->lock);
+	}
 
  	/*
  	 * set_bit and clear bit hooks normally require _irqsave/restore
Reviewed-by
#2 lore raw
Quoted 15 lines · Chris Mason
Quoted 8 lines · On 07/27/2017 02:52 PM, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the range being cleared was not marked for defrag and we are not about to clear the range from the defrag status, we don't need to lock and unlock the inode.

Signed-off-by Filipe Manana <[email protected]>

Thanks Filipe, looks like it goes all the way back to:

Quoted 7 lines

commit 47059d930f0e002ff851beea87d738146804726d

Author: Wang Shilong <[email protected]>
Date:   Thu Jul 3 18:22:07 2014 +0800

Btrfs: make defragment work with nodatacow option

I can't see how the inode lock is required here.

This blames to me, thanks for fixing it.

Reviewed-by Wang Shilong <[email protected]>
Quoted 32 lines
Reviewed-by Chris Mason <[email protected]>

-chris

Quoted 13 lines
---
  fs/btrfs/inode.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
fs/btrfs/inode.c +0 -0
index eb495e956d53..51c45c0a8553 100644
@@ -1797,10 +1797,11 @@ static void btrfs_clear_bit_hook(void
 *private_data,
        u64 len = state->end + 1 - state->start;
        u32 num_extents = count_max_extents(len);

fs/btrfs/inode.c static void btrfs_clear_bit_hook(void · line 1807

Quoted 10 lines
fs/btrfs/inode.c +4 -2
  -     spin_lock(&inode->lock);
-       if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG))
+       if ((state->state & EXTENT_DEFRAG) && (*bits & EXTENT_DEFRAG)) {
+               spin_lock(&inode->lock);
                inode->defrag_bytes -= len;
-       spin_unlock(&inode->lock);
+               spin_unlock(&inode->lock);
+       }
        /*
         * set_bit and clear bit hooks normally require _irqsave/restore

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 20 lines · Wang Shilong
Quoted 19 lines · Chris Mason
Quoted 8 lines · On 07/27/2017 02:52 PM, [email protected] wrote:

From: Filipe Manana <[email protected]>

If the range being cleared was not marked for defrag and we are not about to clear the range from the defrag status, we don't need to lock and unlock the inode.

Signed-off-by Filipe Manana <[email protected]>

Thanks Filipe, looks like it goes all the way back to:

commit 47059d930f0e002ff851beea87d738146804726d

Author: Wang Shilong <[email protected]>
Date:   Thu Jul 3 18:22:07 2014 +0800
Quoted 5 lines
Quoted 3 lines

Btrfs: make defragment work with nodatacow option

I can't see how the inode lock is required here.

This blames to me, thanks for fixing it.

No blame ;) I'll take code that works any day.

-chris