diff mbox series

[5/9] btrfs: rework btrfs_calc_reclaim_metadata_size

Message ID bc6b0eeceacb2b444acf1ff74673471e2dfd2bb9.1601495426.git.josef@toxicpanda.com
State New, archived
Headers show
Series Improve preemptive ENOSPC flushing | expand

Commit Message

Josef Bacik Sept. 30, 2020, 8:01 p.m. UTC
Currently btrfs_calc_reclaim_metadata_size does two things, it returns
the space currently required for flushing by the tickets, and if there
are no tickets it calculates a value for the preemptive flushing.

However for the normal ticketed flushing we really only care about the
space required for tickets.  We will accidentally come in and flush one
time, but as soon as we see there are no tickets we bail out of our
flushing.

Fix this by making btrfs_calc_reclaim_metadata_size really only tell us
what is required for flushing if we have people waiting on space.  Then
move the preemptive flushing logic into need_preemptive_reclaim().  We
ignore btrfs_calc_reclaim_metadata_size() in need_preemptive_reclaim()
because if we are in this path then we made our reservation and there
are not pending tickets currently, so we do not need to check it, simply
do the fuzzy logic to check if we're getting low on space.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 44 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

Comments

Nikolay Borisov Oct. 1, 2020, 1:59 p.m. UTC | #1
On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
<snip>
>  
> @@ -800,6 +777,7 @@ static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>  					  u64 used)
>  {
>  	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
> +	u64 to_reclaim, expected;
>  
>  	/* If we're just plain full then async reclaim just slows us down. */
>  	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
> @@ -812,7 +790,25 @@ static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>  	if (space_info->reclaim_size)
>  		return 0;
>  
> -	if (!btrfs_calc_reclaim_metadata_size(fs_info, space_info))
> +	to_reclaim = min_t(u64, num_online_cpus() * SZ_1M, SZ_16M);
> +	if (btrfs_can_overcommit(fs_info, space_info, to_reclaim,
> +				 BTRFS_RESERVE_FLUSH_ALL))
> +		return 0;
> +
> +	used = btrfs_space_info_used(space_info, true);
> +	if (btrfs_can_overcommit(fs_info, space_info, SZ_1M,
> +				 BTRFS_RESERVE_FLUSH_ALL))
> +		expected = div_factor_fine(space_info->total_bytes, 95);
> +	else
> +		expected = div_factor_fine(space_info->total_bytes, 90);

I think this should be just:

expected = div_factor_fine(space_info->total_bytes, 90);

Because before this check we tried to overcommit between 1 and 16m
(depending on the online CPU's) and we failed. So there is no reason to
think that :

btrfs_can_overcommit(fs_info, space_info, SZ_1M, BTRFS_RESERVE_FLUSH_ALL)

would succeed. So you can simplify the logic by eliminating the 2nd
check for btrfs_can_overcommit

> +
> +	if (used > expected)
> +		to_reclaim = used - expected;
> +	else
> +		to_reclaim = 0;
> +	to_reclaim = min(to_reclaim, space_info->bytes_may_use +
> +				     space_info->bytes_reserved);
> +	if (!to_reclaim)
>  		return 0;
>  
>  	return (used >= thresh && !btrfs_fs_closing(fs_info) &&
> 

nit: Not directly related to your patch but since you are moving the
code does it make sense to keep the fs_closing and STATE_REMOUNTING
checks around?
Josef Bacik Oct. 1, 2020, 9:38 p.m. UTC | #2
On 10/1/20 9:59 AM, Nikolay Borisov wrote:
> 
> 
> On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
> <snip>
>>   
>> @@ -800,6 +777,7 @@ static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>>   					  u64 used)
>>   {
>>   	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
>> +	u64 to_reclaim, expected;
>>   
>>   	/* If we're just plain full then async reclaim just slows us down. */
>>   	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
>> @@ -812,7 +790,25 @@ static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
>>   	if (space_info->reclaim_size)
>>   		return 0;
>>   
>> -	if (!btrfs_calc_reclaim_metadata_size(fs_info, space_info))
>> +	to_reclaim = min_t(u64, num_online_cpus() * SZ_1M, SZ_16M);
>> +	if (btrfs_can_overcommit(fs_info, space_info, to_reclaim,
>> +				 BTRFS_RESERVE_FLUSH_ALL))
>> +		return 0;
>> +
>> +	used = btrfs_space_info_used(space_info, true);
>> +	if (btrfs_can_overcommit(fs_info, space_info, SZ_1M,
>> +				 BTRFS_RESERVE_FLUSH_ALL))
>> +		expected = div_factor_fine(space_info->total_bytes, 95);
>> +	else
>> +		expected = div_factor_fine(space_info->total_bytes, 90);
> 
> I think this should be just:
> 
> expected = div_factor_fine(space_info->total_bytes, 90);
> 
> Because before this check we tried to overcommit between 1 and 16m
> (depending on the online CPU's) and we failed. So there is no reason to
> think that :
> 
> btrfs_can_overcommit(fs_info, space_info, SZ_1M, BTRFS_RESERVE_FLUSH_ALL)
> 
> would succeed. So you can simplify the logic by eliminating the 2nd
> check for btrfs_can_overcommit

I remove all this code in a later patch, I'm just moving it here to simplify 
btrfs_calc_reclaim_metadata_size, and then changing this logic later so the 
changes are discrete.

> 
>> +
>> +	if (used > expected)
>> +		to_reclaim = used - expected;
>> +	else
>> +		to_reclaim = 0;
>> +	to_reclaim = min(to_reclaim, space_info->bytes_may_use +
>> +				     space_info->bytes_reserved);
>> +	if (!to_reclaim)
>>   		return 0;
>>   
>>   	return (used >= thresh && !btrfs_fs_closing(fs_info) &&
>>
> 
> nit: Not directly related to your patch but since you are moving the
> code does it make sense to keep the fs_closing and STATE_REMOUNTING
> checks around?
> 

It does because we use this as the breaking condition for the preemptive flusher 
thread.  Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 518749093bc5..37eb5a11a875 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -752,7 +752,6 @@  btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 {
 	u64 used;
 	u64 avail;
-	u64 expected;
 	u64 to_reclaim = space_info->reclaim_size;
 
 	lockdep_assert_held(&space_info->lock);
@@ -770,28 +769,6 @@  btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 	if (space_info->total_bytes + avail < used)
 		to_reclaim += used - (space_info->total_bytes + avail);
 
-	if (to_reclaim)
-		return to_reclaim;
-
-	to_reclaim = min_t(u64, num_online_cpus() * SZ_1M, SZ_16M);
-	if (btrfs_can_overcommit(fs_info, space_info, to_reclaim,
-				 BTRFS_RESERVE_FLUSH_ALL))
-		return 0;
-
-	used = btrfs_space_info_used(space_info, true);
-
-	if (btrfs_can_overcommit(fs_info, space_info, SZ_1M,
-				 BTRFS_RESERVE_FLUSH_ALL))
-		expected = div_factor_fine(space_info->total_bytes, 95);
-	else
-		expected = div_factor_fine(space_info->total_bytes, 90);
-
-	if (used > expected)
-		to_reclaim = used - expected;
-	else
-		to_reclaim = 0;
-	to_reclaim = min(to_reclaim, space_info->bytes_may_use +
-				     space_info->bytes_reserved);
 	return to_reclaim;
 }
 
@@ -800,6 +777,7 @@  static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 					  u64 used)
 {
 	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
+	u64 to_reclaim, expected;
 
 	/* If we're just plain full then async reclaim just slows us down. */
 	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
@@ -812,7 +790,25 @@  static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 	if (space_info->reclaim_size)
 		return 0;
 
-	if (!btrfs_calc_reclaim_metadata_size(fs_info, space_info))
+	to_reclaim = min_t(u64, num_online_cpus() * SZ_1M, SZ_16M);
+	if (btrfs_can_overcommit(fs_info, space_info, to_reclaim,
+				 BTRFS_RESERVE_FLUSH_ALL))
+		return 0;
+
+	used = btrfs_space_info_used(space_info, true);
+	if (btrfs_can_overcommit(fs_info, space_info, SZ_1M,
+				 BTRFS_RESERVE_FLUSH_ALL))
+		expected = div_factor_fine(space_info->total_bytes, 95);
+	else
+		expected = div_factor_fine(space_info->total_bytes, 90);
+
+	if (used > expected)
+		to_reclaim = used - expected;
+	else
+		to_reclaim = 0;
+	to_reclaim = min(to_reclaim, space_info->bytes_may_use +
+				     space_info->bytes_reserved);
+	if (!to_reclaim)
 		return 0;
 
 	return (used >= thresh && !btrfs_fs_closing(fs_info) &&