| Message ID | 8f5cc79f377c0358c3ad40188bf5917b4bc07924.1601495426.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve preemptive ENOSPC flushing | expand |
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>
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
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 --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");
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(-)