[PATCH 0/3] Refactor prealloc_file_extent_cluster

[PATCH 0/3] Refactor prealloc_file_extent_cluster

[Nikolay Borisov]

Those 3 minor patches refactor prealloc_file_extent_cluster by:

1. Removing useless check
2. Re-order code around to make heavyweight operations outside of inode lock,
(not that it matters for performance) and also get rid of the out label
3. Switch a while to a for loop to make the loop intentino more explicit.


Nikolay Borisov (3):
  btrfs: Remove hole  check in prealloc_file_extent_cluster
  btrfs: Perform data management operations outside of inode lock
  btrfs: Use for loop in prealloc_file_extent_cluster

 fs/btrfs/relocation.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

--
2.17.1

[PATCH 1/3] btrfs: Remove hole check in prealloc_file_extent_cluster

[Nikolay Borisov]

Extents in the extent cluster are guaranteed to be contiguous as such
the hole check inside the loop can never trigger. In fact this check
was never functional since it was added in 18513091af948 which came
after the commit introducing clustered/contiguous extents: 0257bb82d21b.

Let's just remove it as it adds noise to the source.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/relocation.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 11d156995446..348985c8a559 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2604,9 +2604,6 @@ int prealloc_file_extent_cluster(struct inode *inode,
 
 		lock_extent(&BTRFS_I(inode)->io_tree, start, end);
 		num_bytes = end + 1 - start;
-		if (cur_offset < start)
-			btrfs_free_reserved_data_space_noquota(inode,
-						       start - cur_offset);
 		ret = btrfs_prealloc_file_range(inode, 0, start,
 						num_bytes, num_bytes,
 						end + 1, &alloc_hint);
-- 
2.17.1

Re: [PATCH 1/3] btrfs: Remove hole check in prealloc_file_extent_cluster

[Johannes Thumshirn]

Looks ok,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/3] btrfs: Remove hole check in prealloc_file_extent_cluster

[David Sterba]

On Wed, Jun 17, 2020 at 12:10:42PM +0300, Nikolay Borisov wrote:
> Extents in the extent cluster are guaranteed to be contiguous as such
> the hole check inside the loop can never trigger. In fact this check
> was never functional since it was added in 18513091af948 which came
> after the commit introducing clustered/contiguous extents: 0257bb82d21b.

Please use full commit references like 18513091af94 ("btrfs: update
btrfs_space_info's bytes_may_use timely").

[PATCH 2/3] btrfs: Perform data management operations outside of inode lock

[Nikolay Borisov]

btrfs_alloc_data_chunk_ondemand and btrfs_free_reserved_data_space_noquota
don't really use the guts of the inodes being passed to them. This
implies it's not required to call them under extent lock. Move code
around in prealloc_file_extent_cluster to do the heavy, data
alloc/free operations outside of the lock. This also makes the 'out'
label unnecessary, so remove it.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/relocation.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 348985c8a559..020d04035be1 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2584,17 +2584,15 @@ int prealloc_file_extent_cluster(struct inode *inode,
 	int ret = 0;
 	u64 prealloc_start = cluster->start - offset;
 	u64 prealloc_end = cluster->end - offset;
-	u64 cur_offset;
+	u64 cur_offset = prealloc_start;
 
 	BUG_ON(cluster->start != cluster->boundary[0]);
-	inode_lock(inode);
-
 	ret = btrfs_alloc_data_chunk_ondemand(BTRFS_I(inode),
 					      prealloc_end + 1 - prealloc_start);
 	if (ret)
-		goto out;
+		return ret;
 
-	cur_offset = prealloc_start;
+	inode_lock(inode);
 	while (nr < cluster->nr) {
 		start = cluster->boundary[nr] - offset;
 		if (nr + 1 < cluster->nr)
@@ -2613,11 +2611,11 @@ int prealloc_file_extent_cluster(struct inode *inode,
 			break;
 		nr++;
 	}
+	inode_unlock(inode);
+
 	if (cur_offset < prealloc_end)
 		btrfs_free_reserved_data_space_noquota(inode,
 					       prealloc_end + 1 - cur_offset);
-out:
-	inode_unlock(inode);
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH 2/3] btrfs: Perform data management operations outside of inode lock

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/3] btrfs: Use for loop in prealloc_file_extent_cluster

[Nikolay Borisov]

This function iterates all extents in the extent cluster, make this
intention obvious by using a for loop. No functional chanes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/relocation.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 020d04035be1..8972aa574a05 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2580,7 +2580,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
 	u64 end;
 	u64 offset = BTRFS_I(inode)->index_cnt;
 	u64 num_bytes;
-	int nr = 0;
+	int nr;
 	int ret = 0;
 	u64 prealloc_start = cluster->start - offset;
 	u64 prealloc_end = cluster->end - offset;
@@ -2593,7 +2593,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
 		return ret;
 
 	inode_lock(inode);
-	while (nr < cluster->nr) {
+	for (nr = 0; nr < cluster->nr; nr++) {
 		start = cluster->boundary[nr] - offset;
 		if (nr + 1 < cluster->nr)
 			end = cluster->boundary[nr + 1] - 1 - offset;
@@ -2609,7 +2609,6 @@ int prealloc_file_extent_cluster(struct inode *inode,
 		unlock_extent(&BTRFS_I(inode)->io_tree, start, end);
 		if (ret)
 			break;
-		nr++;
 	}
 	inode_unlock(inode);
 
-- 
2.17.1

Re: [PATCH 3/3] btrfs: Use for loop in prealloc_file_extent_cluster

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

I've you have to re-post another potential candidate in this 
function would be:
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 9235c671bef8..3ebf64578a32 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2578,7 +2578,8 @@ int prealloc_file_extent_cluster(struct inode *inode,
        u64 alloc_hint = 0;
        u64 start;
        u64 end;
-       u64 offset = BTRFS_I(inode)->index_cnt;
+       struct btrfs_inode *btrfs_inode = BTRFS_I(inode);
+       u64 offset = btrfs_inode->index_cnt;
        u64 num_bytes;
        int nr = 0;
        int ret = 0;
@@ -2589,7 +2590,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
        BUG_ON(cluster->start != cluster->boundary[0]);
        inode_lock(inode);
 
-       ret = btrfs_alloc_data_chunk_ondemand(BTRFS_I(inode),
+       ret = btrfs_alloc_data_chunk_ondemand(btrfs_inode,
                                              prealloc_end + 1 - prealloc_start);
        if (ret)
                goto out;
@@ -2611,7 +2612,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
                                                num_bytes, num_bytes,
                                                end + 1, &alloc_hint);
                cur_offset = end + 1;
-               unlock_extent(&BTRFS_I(inode)->io_tree, start, end);
+               unlock_extent(&btrfs_inode->io_tree, start, end);
                if (ret)
                        break;
                nr++;


This would save 3 BTRFS_I() calls, but not sure how much of a difference it
makes in the end.

Re: [PATCH 3/3] btrfs: Use for loop in prealloc_file_extent_cluster

[Nikolay Borisov]

On 17.06.20 г. 16:02 ч., Johannes Thumshirn wrote:
> Looks good,
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> I've you have to re-post another potential candidate in this 
> function would be:
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 9235c671bef8..3ebf64578a32 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2578,7 +2578,8 @@ int prealloc_file_extent_cluster(struct inode *inode,
>         u64 alloc_hint = 0;
>         u64 start;
>         u64 end;
> -       u64 offset = BTRFS_I(inode)->index_cnt;
> +       struct btrfs_inode *btrfs_inode = BTRFS_I(inode);
> +       u64 offset = btrfs_inode->index_cnt;
>         u64 num_bytes;
>         int nr = 0;
>         int ret = 0;
> @@ -2589,7 +2590,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
>         BUG_ON(cluster->start != cluster->boundary[0]);
>         inode_lock(inode);
>  
> -       ret = btrfs_alloc_data_chunk_ondemand(BTRFS_I(inode),
> +       ret = btrfs_alloc_data_chunk_ondemand(btrfs_inode,
>                                               prealloc_end + 1 - prealloc_start);
>         if (ret)
>                 goto out;
> @@ -2611,7 +2612,7 @@ int prealloc_file_extent_cluster(struct inode *inode,
>                                                 num_bytes, num_bytes,
>                                                 end + 1, &alloc_hint);
>                 cur_offset = end + 1;
> -               unlock_extent(&BTRFS_I(inode)->io_tree, start, end);
> +               unlock_extent(&btrfs_inode->io_tree, start, end);
>                 if (ret)
>                         break;
>                 nr++;
> 
> 
> This would save 3 BTRFS_I() calls, but not sure how much of a difference it
> makes in the end.
> 

As a matter of fact I've already done this in my monstrous in patch 44/46.

Re: [PATCH 0/3] Refactor prealloc_file_extent_cluster

[David Sterba]

On Wed, Jun 17, 2020 at 12:10:41PM +0300, Nikolay Borisov wrote:
> Those 3 minor patches refactor prealloc_file_extent_cluster by:
> 
> 1. Removing useless check
> 2. Re-order code around to make heavyweight operations outside of inode lock,
> (not that it matters for performance) and also get rid of the out label
> 3. Switch a while to a for loop to make the loop intentino more explicit.
> 
> 
> Nikolay Borisov (3):
>   btrfs: Remove hole  check in prealloc_file_extent_cluster
>   btrfs: Perform data management operations outside of inode lock
>   btrfs: Use for loop in prealloc_file_extent_cluster

Added to misc-next, thanks.