diff mbox series

[6/9] btrfs: simplify the logic in need_preemptive_flushing

Message ID 8f5cc79f377c0358c3ad40188bf5917b4bc07924.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
A lot of this was added all in one go with no explanation, and is a bit
unwieldy and confusing.  Simplify the logic to start preemptive flushing
if we've reserved more than half of our available free space.

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

Comments

Nikolay Borisov Oct. 1, 2020, 2:09 p.m. UTC | #1
On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
> A lot of this was added all in one go with no explanation, and is a bit
> unwieldy and confusing.  Simplify the logic to start preemptive flushing
> if we've reserved more than half of our available free space.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

<snip>

> +	 * If we have over half of the free space occupied by reservations or
> +	 * pinned then we want to start flushing.
> +	 *
> +	 * We do not do the traditional thing here, which is to say
> +	 *
> +	 *   if (used >= ((total_bytes + avail) >> 1))
> +	 *     return 1;
> +	 *
> +	 * because this doesn't quite work how we want.  If we had more than 50%
> +	 * of the space_info used by bytes_used and we had 0 available we'd just
> +	 * constantly run the background flusher.  Instead we want it to kick in
> +	 * if our reclaimable space exceeds 50% of our available free space.
> +	 */
> +	thresh = calc_available_free_space(fs_info, space_info,
> +					   BTRFS_RESERVE_FLUSH_ALL);
> +	thresh += (space_info->total_bytes - space_info->bytes_used -
> +		   space_info->bytes_reserved - space_info->bytes_readonly);

Isn't the freespace in space_info calculated by subtracting every
bytes_* from total_bytes ? I.e why aren't you subtracting bytes_may_use
and bytes_pinned ? Shouldn't this be

thresh += space_info->total_bytes - btrfs_space_info_used(space_info, true)



> +	thresh >>= 1;
>  
> -	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;
> +	used = space_info->bytes_may_use + space_info->bytes_pinned;
>  
>  	return (used >= thresh && !btrfs_fs_closing(fs_info) &&
>  		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));

<snip>
Josef Bacik Oct. 1, 2020, 9:40 p.m. UTC | #2
On 10/1/20 10:09 AM, Nikolay Borisov wrote:
> 
> 
> On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
>> A lot of this was added all in one go with no explanation, and is a bit
>> unwieldy and confusing.  Simplify the logic to start preemptive flushing
>> if we've reserved more than half of our available free space.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> <snip>
> 
>> +	 * If we have over half of the free space occupied by reservations or
>> +	 * pinned then we want to start flushing.
>> +	 *
>> +	 * We do not do the traditional thing here, which is to say
>> +	 *
>> +	 *   if (used >= ((total_bytes + avail) >> 1))
>> +	 *     return 1;
>> +	 *
>> +	 * because this doesn't quite work how we want.  If we had more than 50%
>> +	 * of the space_info used by bytes_used and we had 0 available we'd just
>> +	 * constantly run the background flusher.  Instead we want it to kick in
>> +	 * if our reclaimable space exceeds 50% of our available free space.
>> +	 */
>> +	thresh = calc_available_free_space(fs_info, space_info,
>> +					   BTRFS_RESERVE_FLUSH_ALL);
>> +	thresh += (space_info->total_bytes - space_info->bytes_used -
>> +		   space_info->bytes_reserved - space_info->bytes_readonly);
> 
> Isn't the freespace in space_info calculated by subtracting every
> bytes_* from total_bytes ? I.e why aren't you subtracting bytes_may_use
> and bytes_pinned ? Shouldn't this be
> 
> thresh += space_info->total_bytes - btrfs_space_info_used(space_info, true)
> 

Because I'm using the reservation in `used` below.  What I want is to see how my 
free space we have for all of our reservations, and then use that as the 
threshold for when to start preemptive flushing.  Then below we use 
bytes_may_use and bytes_pinned as the used.  If I subtracted it from here it 
would appear that we had less free space when I then went and did

return (used >= thresh);

Thanks,

Josef
Nikolay Borisov Oct. 2, 2020, 7:13 a.m. UTC | #3
On 30.09.20 г. 23:01 ч., Josef Bacik wrote:
> A lot of this was added all in one go with no explanation, and is a bit
> unwieldy and confusing.  Simplify the logic to start preemptive flushing
> if we've reserved more than half of our available free space.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---

<snip>

> +	/*
> +	 * If we have over half of the free space occupied by reservations or
> +	 * pinned then we want to start flushing.
> +	 *
> +	 * We do not do the traditional thing here, which is to say
> +	 *
> +	 *   if (used >= ((total_bytes + avail) >> 1))
> +	 *     return 1;
> +	 *
> +	 * because this doesn't quite work how we want.  If we had more than 50%
> +	 * of the space_info used by bytes_used and we had 0 available we'd just
> +	 * constantly run the background flusher.  Instead we want it to kick in
> +	 * if our reclaimable space exceeds 50% of our available free space.
> +	 */
> +	thresh = calc_available_free_space(fs_info, space_info,
> +					   BTRFS_RESERVE_FLUSH_ALL);
> +	thresh += (space_info->total_bytes - space_info->bytes_used -
> +		   space_info->bytes_reserved - space_info->bytes_readonly);
> +	thresh >>= 1;

Ok, a fresh read illuminates the logic, thresh contains the freespace in
space_info + bytes_may_use + bytes_pinned so the check below says if
byte_may_use + bytes-Pinned are 50% or more than what's potentially
available - flush .


<snip>
diff mbox series

Patch

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 37eb5a11a875..a41cf358f438 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -773,11 +773,10 @@  btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 }
 
 static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
-					  struct btrfs_space_info *space_info,
-					  u64 used)
+					  struct btrfs_space_info *space_info)
 {
 	u64 thresh = div_factor_fine(space_info->total_bytes, 98);
-	u64 to_reclaim, expected;
+	u64 used;
 
 	/* If we're just plain full then async reclaim just slows us down. */
 	if ((space_info->bytes_used + space_info->bytes_reserved) >= thresh)
@@ -790,26 +789,27 @@  static inline int need_preemptive_reclaim(struct btrfs_fs_info *fs_info,
 	if (space_info->reclaim_size)
 		return 0;
 
-	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 we have over half of the free space occupied by reservations or
+	 * pinned then we want to start flushing.
+	 *
+	 * We do not do the traditional thing here, which is to say
+	 *
+	 *   if (used >= ((total_bytes + avail) >> 1))
+	 *     return 1;
+	 *
+	 * because this doesn't quite work how we want.  If we had more than 50%
+	 * of the space_info used by bytes_used and we had 0 available we'd just
+	 * constantly run the background flusher.  Instead we want it to kick in
+	 * if our reclaimable space exceeds 50% of our available free space.
+	 */
+	thresh = calc_available_free_space(fs_info, space_info,
+					   BTRFS_RESERVE_FLUSH_ALL);
+	thresh += (space_info->total_bytes - space_info->bytes_used -
+		   space_info->bytes_reserved - space_info->bytes_readonly);
+	thresh >>= 1;
 
-	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;
+	used = space_info->bytes_may_use + space_info->bytes_pinned;
 
 	return (used >= thresh && !btrfs_fs_closing(fs_info) &&
 		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
@@ -1006,7 +1006,6 @@  static void btrfs_preempt_reclaim_metadata_space(struct work_struct *work)
 	struct btrfs_block_rsv *delayed_refs_rsv;
 	struct btrfs_block_rsv *global_rsv;
 	struct btrfs_block_rsv *trans_rsv;
-	u64 used;
 
 	fs_info = container_of(work, struct btrfs_fs_info,
 			       preempt_reclaim_work);
@@ -1017,8 +1016,7 @@  static void btrfs_preempt_reclaim_metadata_space(struct work_struct *work)
 	trans_rsv = &fs_info->trans_block_rsv;
 
 	spin_lock(&space_info->lock);
-	used = btrfs_space_info_used(space_info, true);
-	while (need_preemptive_reclaim(fs_info, space_info, used)) {
+	while (need_preemptive_reclaim(fs_info, space_info)) {
 		enum btrfs_reserve_flush_enum flush;
 		u64 delalloc_size = 0;
 		u64 to_reclaim, block_rsv_size;
@@ -1101,7 +1099,6 @@  static void btrfs_preempt_reclaim_metadata_space(struct work_struct *work)
 next:
 		cond_resched();
 		spin_lock(&space_info->lock);
-		used = btrfs_space_info_used(space_info, true);
 	}
 	spin_unlock(&space_info->lock);
 }
@@ -1512,7 +1509,7 @@  static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 		 * the async reclaim as we will panic.
 		 */
 		if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags) &&
-		    need_preemptive_reclaim(fs_info, space_info, used) &&
+		    need_preemptive_reclaim(fs_info, space_info) &&
 		    !work_busy(&fs_info->preempt_reclaim_work)) {
 			trace_btrfs_trigger_flush(fs_info, space_info->flags,
 						  orig_bytes, flush, "preempt");