| Message ID | a1d52aa93f0e9e36fcf89979664a55bccf1a0459.1570479299.git.dennis@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: async discard support | expand |
On Mon, Oct 07, 2019 at 04:17:37PM -0400, Dennis Zhou wrote: > block_group removal is a little tricky. It can race with the extent > allocator, the cleaner thread, and balancing. The current path is for a > block_group to be added to the unused_bgs list. Then, when the cleaner > thread comes around, it starts a transaction and then proceeds with > removing the block_group. Extents that are pinned are subsequently > removed from the pinned trees and then eventually a discard is issued > for the entire block_group. > > Async discard introduces another player into the game, the discard > workqueue. While it has none of the racing issues, the new problem is > ensuring we don't leave free space untrimmed prior to forgetting the > block_group. This is handled by placing fully free block_groups on a > separate discard queue. This is necessary to maintain discarding order > as in the future we will slowly trim even fully free block_groups. The > ordering helps us make progress on the same block_group rather than say > the last fully freed block_group or needing to search through the fully > freed block groups at the beginning of a list and insert after. > > The new order of events is a fully freed block group gets placed on the > discard queue first. Once it's processed, it will be placed on the > unusued_bgs list and then the original sequence of events will happen, > just without the final whole block_group discard. > > The mount flags can change when processing unused_bgs, so when flipping > from DISCARD to DISCARD_ASYNC, the unused_bgs must be punted to the > discard_list to be trimmed. If we flip off DISCARD_ASYNC, we punt > free block groups on the discard_list to the unused_bg queue which will > do the final discard for us. > > Signed-off-by: Dennis Zhou <dennis@kernel.org> > --- > fs/btrfs/block-group.c | 39 ++++++++++++++++++--- > fs/btrfs/ctree.h | 2 +- > fs/btrfs/discard.c | 68 ++++++++++++++++++++++++++++++++++++- > fs/btrfs/discard.h | 11 +++++- > fs/btrfs/free-space-cache.c | 33 ++++++++++++++++++ > fs/btrfs/free-space-cache.h | 1 + > fs/btrfs/scrub.c | 7 +++- > 7 files changed, 153 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > index 8bbbe7488328..73e5a9384491 100644 > --- a/fs/btrfs/block-group.c > +++ b/fs/btrfs/block-group.c > @@ -1251,6 +1251,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > struct btrfs_block_group_cache *block_group; > struct btrfs_space_info *space_info; > struct btrfs_trans_handle *trans; > + bool async_trim_enabled = btrfs_test_opt(fs_info, DISCARD_ASYNC); > int ret = 0; > > if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags)) > @@ -1260,6 +1261,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > while (!list_empty(&fs_info->unused_bgs)) { > u64 start, end; > int trimming; > + bool async_trimmed; > > block_group = list_first_entry(&fs_info->unused_bgs, > struct btrfs_block_group_cache, > @@ -1281,10 +1283,20 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > /* Don't want to race with allocators so take the groups_sem */ > down_write(&space_info->groups_sem); > spin_lock(&block_group->lock); > + > + /* async discard requires block groups to be fully trimmed */ > + async_trimmed = (!btrfs_test_opt(fs_info, DISCARD_ASYNC) || > + btrfs_is_free_space_trimmed(block_group)); > + > if (block_group->reserved || block_group->pinned || > btrfs_block_group_used(&block_group->item) || > block_group->ro || > - list_is_singular(&block_group->list)) { > + list_is_singular(&block_group->list) || > + !async_trimmed) { > + /* requeue if we failed because of async discard */ > + if (!async_trimmed) > + btrfs_discard_queue_work(&fs_info->discard_ctl, > + block_group); > /* > * We want to bail if we made new allocations or have > * outstanding allocations in this block group. We do > @@ -1367,6 +1379,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > spin_unlock(&block_group->lock); > spin_unlock(&space_info->lock); > > + if (!async_trim_enabled && > + btrfs_test_opt(fs_info, DISCARD_ASYNC)) > + goto flip_async; > + This took me a minute to grok, please add a comment indicating that this is meant to catch the case that we flipped from no async to async and thus need to kick off the async trim work now before removing the unused bg. > /* DISCARD can flip during remount */ > trimming = btrfs_test_opt(fs_info, DISCARD_SYNC); > > @@ -1411,6 +1427,13 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > spin_lock(&fs_info->unused_bgs_lock); > } > spin_unlock(&fs_info->unused_bgs_lock); > + return; > + > +flip_async: > + btrfs_end_transaction(trans); > + mutex_unlock(&fs_info->delete_unused_bgs_mutex); > + btrfs_put_block_group(block_group); > + btrfs_discard_punt_unused_bgs_list(fs_info); > } > > void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg) > @@ -1618,6 +1641,8 @@ static struct btrfs_block_group_cache *btrfs_create_block_group_cache( > cache->full_stripe_len = btrfs_full_stripe_len(fs_info, start); > set_free_space_tree_thresholds(cache); > > + cache->discard_index = 1; > + > atomic_set(&cache->count, 1); > spin_lock_init(&cache->lock); > init_rwsem(&cache->data_rwsem); > @@ -1829,7 +1854,11 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info) > inc_block_group_ro(cache, 1); > } else if (btrfs_block_group_used(&cache->item) == 0) { > ASSERT(list_empty(&cache->bg_list)); > - btrfs_mark_bg_unused(cache); > + if (btrfs_test_opt(info, DISCARD_ASYNC)) > + btrfs_add_to_discard_free_list( > + &info->discard_ctl, cache); > + else > + btrfs_mark_bg_unused(cache); > } > } > > @@ -2724,8 +2753,10 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans, > * dirty list to avoid races between cleaner kthread and space > * cache writeout. > */ > - if (!alloc && old_val == 0) > - btrfs_mark_bg_unused(cache); > + if (!alloc && old_val == 0) { > + if (!btrfs_test_opt(info, DISCARD_ASYNC)) > + btrfs_mark_bg_unused(cache); > + } > > btrfs_put_block_group(cache); > total -= num_bytes; > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 419445868909..c328d2e85e4d 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -439,7 +439,7 @@ struct btrfs_full_stripe_locks_tree { > }; > > /* discard control */ > -#define BTRFS_NR_DISCARD_LISTS 1 > +#define BTRFS_NR_DISCARD_LISTS 2 > > struct btrfs_discard_ctl { > struct workqueue_struct *discard_workers; > diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c > index 6df124639e55..fb92b888774d 100644 > --- a/fs/btrfs/discard.c > +++ b/fs/btrfs/discard.c > @@ -29,8 +29,11 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > > spin_lock(&discard_ctl->lock); > > - if (list_empty(&cache->discard_list)) > + if (list_empty(&cache->discard_list) || !cache->discard_index) { > + if (!cache->discard_index) > + cache->discard_index = 1; Need a #define for this so it's clear what our intention is, I hate magic numbers. > cache->discard_delay = now + BTRFS_DISCARD_DELAY; > + } > > list_move_tail(&cache->discard_list, > btrfs_get_discard_list(discard_ctl, cache)); > @@ -38,6 +41,23 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > spin_unlock(&discard_ctl->lock); > } > > +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, > + struct btrfs_block_group_cache *cache) > +{ > + u64 now = ktime_get_ns(); > + > + spin_lock(&discard_ctl->lock); > + > + if (!list_empty(&cache->discard_list)) > + list_del_init(&cache->discard_list); > + > + cache->discard_index = 0; > + cache->discard_delay = now; > + list_add_tail(&cache->discard_list, &discard_ctl->discard_list[0]); > + > + spin_unlock(&discard_ctl->lock); > +} > + > static bool remove_from_discard_list(struct btrfs_discard_ctl *discard_ctl, > struct btrfs_block_group_cache *cache) > { > @@ -161,10 +181,52 @@ static void btrfs_discard_workfn(struct work_struct *work) > btrfs_block_group_end(cache), 0); > > remove_from_discard_list(discard_ctl, cache); > + if (btrfs_is_free_space_trimmed(cache)) > + btrfs_mark_bg_unused(cache); > + else if (cache->free_space_ctl->free_space == cache->key.offset) > + btrfs_add_to_discard_free_list(discard_ctl, cache); This needs to be else if (btrfs_block_group_used(cache) == 0) because we just exclude super mirrors from the free space cache, so completely empty free_space != cache->key.offset. > > btrfs_discard_schedule_work(discard_ctl, false); > } > > +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info) > +{ > + struct btrfs_block_group_cache *cache, *next; > + > + /* we enabled async discard, so punt all to the queue */ > + spin_lock(&fs_info->unused_bgs_lock); > + > + list_for_each_entry_safe(cache, next, &fs_info->unused_bgs, bg_list) { > + list_del_init(&cache->bg_list); > + btrfs_add_to_discard_free_list(&fs_info->discard_ctl, cache); > + } > + > + spin_unlock(&fs_info->unused_bgs_lock); > +} > + > +static void btrfs_discard_purge_list(struct btrfs_discard_ctl *discard_ctl) > +{ > + struct btrfs_block_group_cache *cache, *next; > + int i; > + > + spin_lock(&discard_ctl->lock); > + > + for (i = 0; i < BTRFS_NR_DISCARD_LISTS; i++) { > + list_for_each_entry_safe(cache, next, > + &discard_ctl->discard_list[i], > + discard_list) { > + list_del_init(&cache->discard_list); > + spin_unlock(&discard_ctl->lock); > + if (cache->free_space_ctl->free_space == > + cache->key.offset) > + btrfs_mark_bg_unused(cache); > + spin_lock(&discard_ctl->lock); > + } > + } > + > + spin_unlock(&discard_ctl->lock); > +} > + > void btrfs_discard_resume(struct btrfs_fs_info *fs_info) > { > if (!btrfs_test_opt(fs_info, DISCARD_ASYNC)) { > @@ -172,6 +234,8 @@ void btrfs_discard_resume(struct btrfs_fs_info *fs_info) > return; > } > > + btrfs_discard_punt_unused_bgs_list(fs_info); > + > set_bit(BTRFS_FS_DISCARD_RUNNING, &fs_info->flags); > } > > @@ -197,4 +261,6 @@ void btrfs_discard_cleanup(struct btrfs_fs_info *fs_info) > { > btrfs_discard_stop(fs_info); > cancel_delayed_work_sync(&fs_info->discard_ctl.work); > + > + btrfs_discard_purge_list(&fs_info->discard_ctl); > } > diff --git a/fs/btrfs/discard.h b/fs/btrfs/discard.h > index 6d7805bb0eb7..55f79b624943 100644 > --- a/fs/btrfs/discard.h > +++ b/fs/btrfs/discard.h > @@ -10,9 +10,14 @@ > #include <linux/workqueue.h> > > #include "ctree.h" > +#include "block-group.h" > +#include "free-space-cache.h" > > void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > struct btrfs_block_group_cache *cache); > +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, > + struct btrfs_block_group_cache *cache); > +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info); > > void btrfs_discard_cancel_work(struct btrfs_discard_ctl *discard_ctl, > struct btrfs_block_group_cache *cache); > @@ -41,7 +46,11 @@ void btrfs_discard_queue_work(struct btrfs_discard_ctl *discard_ctl, > if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC)) > return; > > - btrfs_add_to_discard_list(discard_ctl, cache); > + if (cache->free_space_ctl->free_space == cache->key.offset) > + btrfs_add_to_discard_free_list(discard_ctl, cache); Same here. > + else > + btrfs_add_to_discard_list(discard_ctl, cache); > + > if (!delayed_work_pending(&discard_ctl->work)) > btrfs_discard_schedule_work(discard_ctl, false); > } > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > index 54ff1bc97777..ed0e7ee4c78d 100644 > --- a/fs/btrfs/free-space-cache.c > +++ b/fs/btrfs/free-space-cache.c > @@ -2653,6 +2653,31 @@ void btrfs_remove_free_space_cache(struct btrfs_block_group_cache *block_group) > > } > > +bool btrfs_is_free_space_trimmed(struct btrfs_block_group_cache *cache) > +{ > + struct btrfs_free_space_ctl *ctl = cache->free_space_ctl; > + struct btrfs_free_space *info; > + struct rb_node *node; > + bool ret = true; > + > + spin_lock(&ctl->tree_lock); > + node = rb_first(&ctl->free_space_offset); > + > + while (node) { > + info = rb_entry(node, struct btrfs_free_space, offset_index); > + > + if (!btrfs_free_space_trimmed(info)) { > + ret = false; > + break; > + } > + > + node = rb_next(node); > + } > + > + spin_unlock(&ctl->tree_lock); > + return ret; > +} > + > u64 btrfs_find_space_for_alloc(struct btrfs_block_group_cache *block_group, > u64 offset, u64 bytes, u64 empty_size, > u64 *max_extent_size) > @@ -2739,6 +2764,9 @@ int btrfs_return_cluster_to_free_space( > ret = __btrfs_return_cluster_to_free_space(block_group, cluster); > spin_unlock(&ctl->tree_lock); > > + btrfs_discard_queue_work(&block_group->fs_info->discard_ctl, > + block_group); > + > /* finally drop our ref */ > btrfs_put_block_group(block_group); > return ret; > @@ -3097,6 +3125,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > u64 min_bytes; > u64 cont1_bytes; > int ret; > + bool found_cluster = false; > > /* > * Choose the minimum extent size we'll require for this > @@ -3149,6 +3178,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > list_del_init(&entry->list); > > if (!ret) { > + found_cluster = true; > atomic_inc(&block_group->count); > list_add_tail(&cluster->block_group_list, > &block_group->cluster_list); > @@ -3160,6 +3190,9 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > spin_unlock(&cluster->lock); > spin_unlock(&ctl->tree_lock); > > + if (found_cluster) > + btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); > + > return ret; > } This bit here seems unrelated to the rest of the change. Why do we want to cancel the discard work if we find a cluster? And what does it have to do with unused bgs? If it's the allocation part that makes you want to cancel then that sort of makes sense in the unused bg context, but this is happening no matter what. It should probably be in its own patch with an explanation, or at least in a different patch. Thanks, Josef
On Thu, Oct 10, 2019 at 11:00:42AM -0400, Josef Bacik wrote: > On Mon, Oct 07, 2019 at 04:17:37PM -0400, Dennis Zhou wrote: > > block_group removal is a little tricky. It can race with the extent > > allocator, the cleaner thread, and balancing. The current path is for a > > block_group to be added to the unused_bgs list. Then, when the cleaner > > thread comes around, it starts a transaction and then proceeds with > > removing the block_group. Extents that are pinned are subsequently > > removed from the pinned trees and then eventually a discard is issued > > for the entire block_group. > > > > Async discard introduces another player into the game, the discard > > workqueue. While it has none of the racing issues, the new problem is > > ensuring we don't leave free space untrimmed prior to forgetting the > > block_group. This is handled by placing fully free block_groups on a > > separate discard queue. This is necessary to maintain discarding order > > as in the future we will slowly trim even fully free block_groups. The > > ordering helps us make progress on the same block_group rather than say > > the last fully freed block_group or needing to search through the fully > > freed block groups at the beginning of a list and insert after. > > > > The new order of events is a fully freed block group gets placed on the > > discard queue first. Once it's processed, it will be placed on the > > unusued_bgs list and then the original sequence of events will happen, > > just without the final whole block_group discard. > > > > The mount flags can change when processing unused_bgs, so when flipping > > from DISCARD to DISCARD_ASYNC, the unused_bgs must be punted to the > > discard_list to be trimmed. If we flip off DISCARD_ASYNC, we punt > > free block groups on the discard_list to the unused_bg queue which will > > do the final discard for us. > > > > Signed-off-by: Dennis Zhou <dennis@kernel.org> > > --- > > fs/btrfs/block-group.c | 39 ++++++++++++++++++--- > > fs/btrfs/ctree.h | 2 +- > > fs/btrfs/discard.c | 68 ++++++++++++++++++++++++++++++++++++- > > fs/btrfs/discard.h | 11 +++++- > > fs/btrfs/free-space-cache.c | 33 ++++++++++++++++++ > > fs/btrfs/free-space-cache.h | 1 + > > fs/btrfs/scrub.c | 7 +++- > > 7 files changed, 153 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > > index 8bbbe7488328..73e5a9384491 100644 > > --- a/fs/btrfs/block-group.c > > +++ b/fs/btrfs/block-group.c > > @@ -1251,6 +1251,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > > struct btrfs_block_group_cache *block_group; > > struct btrfs_space_info *space_info; > > struct btrfs_trans_handle *trans; > > + bool async_trim_enabled = btrfs_test_opt(fs_info, DISCARD_ASYNC); > > int ret = 0; > > > > if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags)) > > @@ -1260,6 +1261,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > > while (!list_empty(&fs_info->unused_bgs)) { > > u64 start, end; > > int trimming; > > + bool async_trimmed; > > > > block_group = list_first_entry(&fs_info->unused_bgs, > > struct btrfs_block_group_cache, > > @@ -1281,10 +1283,20 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > > /* Don't want to race with allocators so take the groups_sem */ > > down_write(&space_info->groups_sem); > > spin_lock(&block_group->lock); > > + > > + /* async discard requires block groups to be fully trimmed */ > > + async_trimmed = (!btrfs_test_opt(fs_info, DISCARD_ASYNC) || > > + btrfs_is_free_space_trimmed(block_group)); > > + > > if (block_group->reserved || block_group->pinned || > > btrfs_block_group_used(&block_group->item) || > > block_group->ro || > > - list_is_singular(&block_group->list)) { > > + list_is_singular(&block_group->list) || > > + !async_trimmed) { > > + /* requeue if we failed because of async discard */ > > + if (!async_trimmed) > > + btrfs_discard_queue_work(&fs_info->discard_ctl, > > + block_group); > > /* > > * We want to bail if we made new allocations or have > > * outstanding allocations in this block group. We do > > @@ -1367,6 +1379,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > > spin_unlock(&block_group->lock); > > spin_unlock(&space_info->lock); > > > > + if (!async_trim_enabled && > > + btrfs_test_opt(fs_info, DISCARD_ASYNC)) > > + goto flip_async; > > + > > This took me a minute to grok, please add a comment indicating that this is > meant to catch the case that we flipped from no async to async and thus need to > kick off the async trim work now before removing the unused bg. > Added a comment explaining what's going on here. > > /* DISCARD can flip during remount */ > > trimming = btrfs_test_opt(fs_info, DISCARD_SYNC); > > > > @@ -1411,6 +1427,13 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) > > spin_lock(&fs_info->unused_bgs_lock); > > } > > spin_unlock(&fs_info->unused_bgs_lock); > > + return; > > + > > +flip_async: > > + btrfs_end_transaction(trans); > > + mutex_unlock(&fs_info->delete_unused_bgs_mutex); > > + btrfs_put_block_group(block_group); > > + btrfs_discard_punt_unused_bgs_list(fs_info); > > } > > > > void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg) > > @@ -1618,6 +1641,8 @@ static struct btrfs_block_group_cache *btrfs_create_block_group_cache( > > cache->full_stripe_len = btrfs_full_stripe_len(fs_info, start); > > set_free_space_tree_thresholds(cache); > > > > + cache->discard_index = 1; > > + > > atomic_set(&cache->count, 1); > > spin_lock_init(&cache->lock); > > init_rwsem(&cache->data_rwsem); > > @@ -1829,7 +1854,11 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info) > > inc_block_group_ro(cache, 1); > > } else if (btrfs_block_group_used(&cache->item) == 0) { > > ASSERT(list_empty(&cache->bg_list)); > > - btrfs_mark_bg_unused(cache); > > + if (btrfs_test_opt(info, DISCARD_ASYNC)) > > + btrfs_add_to_discard_free_list( > > + &info->discard_ctl, cache); > > + else > > + btrfs_mark_bg_unused(cache); > > } > > } > > > > @@ -2724,8 +2753,10 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans, > > * dirty list to avoid races between cleaner kthread and space > > * cache writeout. > > */ > > - if (!alloc && old_val == 0) > > - btrfs_mark_bg_unused(cache); > > + if (!alloc && old_val == 0) { > > + if (!btrfs_test_opt(info, DISCARD_ASYNC)) > > + btrfs_mark_bg_unused(cache); > > + } > > > > btrfs_put_block_group(cache); > > total -= num_bytes; > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index 419445868909..c328d2e85e4d 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -439,7 +439,7 @@ struct btrfs_full_stripe_locks_tree { > > }; > > > > /* discard control */ > > -#define BTRFS_NR_DISCARD_LISTS 1 > > +#define BTRFS_NR_DISCARD_LISTS 2 > > > > struct btrfs_discard_ctl { > > struct workqueue_struct *discard_workers; > > diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c > > index 6df124639e55..fb92b888774d 100644 > > --- a/fs/btrfs/discard.c > > +++ b/fs/btrfs/discard.c > > @@ -29,8 +29,11 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > > > > spin_lock(&discard_ctl->lock); > > > > - if (list_empty(&cache->discard_list)) > > + if (list_empty(&cache->discard_list) || !cache->discard_index) { > > + if (!cache->discard_index) > > + cache->discard_index = 1; > > Need a #define for this so it's clear what our intention is, I hate magic > numbers. > Sounds good, done. > > cache->discard_delay = now + BTRFS_DISCARD_DELAY; > > + } > > > > list_move_tail(&cache->discard_list, > > btrfs_get_discard_list(discard_ctl, cache)); > > @@ -38,6 +41,23 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > > spin_unlock(&discard_ctl->lock); > > } > > > > +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, > > + struct btrfs_block_group_cache *cache) > > +{ > > + u64 now = ktime_get_ns(); > > + > > + spin_lock(&discard_ctl->lock); > > + > > + if (!list_empty(&cache->discard_list)) > > + list_del_init(&cache->discard_list); > > + > > + cache->discard_index = 0; > > + cache->discard_delay = now; > > + list_add_tail(&cache->discard_list, &discard_ctl->discard_list[0]); > > + > > + spin_unlock(&discard_ctl->lock); > > +} > > + > > static bool remove_from_discard_list(struct btrfs_discard_ctl *discard_ctl, > > struct btrfs_block_group_cache *cache) > > { > > @@ -161,10 +181,52 @@ static void btrfs_discard_workfn(struct work_struct *work) > > btrfs_block_group_end(cache), 0); > > > > remove_from_discard_list(discard_ctl, cache); > > + if (btrfs_is_free_space_trimmed(cache)) > > + btrfs_mark_bg_unused(cache); > > + else if (cache->free_space_ctl->free_space == cache->key.offset) > > + btrfs_add_to_discard_free_list(discard_ctl, cache); > > This needs to be > > else if (btrfs_block_group_used(cache) == 0) > > because we just exclude super mirrors from the free space cache, so completely > empty free_space != cache->key.offset. > Ah cool I didn't know that. This is fixed now. > > > > btrfs_discard_schedule_work(discard_ctl, false); > > } > > > > +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info) > > +{ > > + struct btrfs_block_group_cache *cache, *next; > > + > > + /* we enabled async discard, so punt all to the queue */ > > + spin_lock(&fs_info->unused_bgs_lock); > > + > > + list_for_each_entry_safe(cache, next, &fs_info->unused_bgs, bg_list) { > > + list_del_init(&cache->bg_list); > > + btrfs_add_to_discard_free_list(&fs_info->discard_ctl, cache); > > + } > > + > > + spin_unlock(&fs_info->unused_bgs_lock); > > +} > > + > > +static void btrfs_discard_purge_list(struct btrfs_discard_ctl *discard_ctl) > > +{ > > + struct btrfs_block_group_cache *cache, *next; > > + int i; > > + > > + spin_lock(&discard_ctl->lock); > > + > > + for (i = 0; i < BTRFS_NR_DISCARD_LISTS; i++) { > > + list_for_each_entry_safe(cache, next, > > + &discard_ctl->discard_list[i], > > + discard_list) { > > + list_del_init(&cache->discard_list); > > + spin_unlock(&discard_ctl->lock); > > + if (cache->free_space_ctl->free_space == > > + cache->key.offset) > > + btrfs_mark_bg_unused(cache); > > + spin_lock(&discard_ctl->lock); > > + } > > + } > > + > > + spin_unlock(&discard_ctl->lock); > > +} > > + > > void btrfs_discard_resume(struct btrfs_fs_info *fs_info) > > { > > if (!btrfs_test_opt(fs_info, DISCARD_ASYNC)) { > > @@ -172,6 +234,8 @@ void btrfs_discard_resume(struct btrfs_fs_info *fs_info) > > return; > > } > > > > + btrfs_discard_punt_unused_bgs_list(fs_info); > > + > > set_bit(BTRFS_FS_DISCARD_RUNNING, &fs_info->flags); > > } > > > > @@ -197,4 +261,6 @@ void btrfs_discard_cleanup(struct btrfs_fs_info *fs_info) > > { > > btrfs_discard_stop(fs_info); > > cancel_delayed_work_sync(&fs_info->discard_ctl.work); > > + > > + btrfs_discard_purge_list(&fs_info->discard_ctl); > > } > > diff --git a/fs/btrfs/discard.h b/fs/btrfs/discard.h > > index 6d7805bb0eb7..55f79b624943 100644 > > --- a/fs/btrfs/discard.h > > +++ b/fs/btrfs/discard.h > > @@ -10,9 +10,14 @@ > > #include <linux/workqueue.h> > > > > #include "ctree.h" > > +#include "block-group.h" > > +#include "free-space-cache.h" > > > > void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, > > struct btrfs_block_group_cache *cache); > > +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, > > + struct btrfs_block_group_cache *cache); > > +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info); > > > > void btrfs_discard_cancel_work(struct btrfs_discard_ctl *discard_ctl, > > struct btrfs_block_group_cache *cache); > > @@ -41,7 +46,11 @@ void btrfs_discard_queue_work(struct btrfs_discard_ctl *discard_ctl, > > if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC)) > > return; > > > > - btrfs_add_to_discard_list(discard_ctl, cache); > > + if (cache->free_space_ctl->free_space == cache->key.offset) > > + btrfs_add_to_discard_free_list(discard_ctl, cache); > > Same here. > > > + else > > + btrfs_add_to_discard_list(discard_ctl, cache); > > + > > if (!delayed_work_pending(&discard_ctl->work)) > > btrfs_discard_schedule_work(discard_ctl, false); > > } > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index 54ff1bc97777..ed0e7ee4c78d 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -2653,6 +2653,31 @@ void btrfs_remove_free_space_cache(struct btrfs_block_group_cache *block_group) > > > > } > > > > +bool btrfs_is_free_space_trimmed(struct btrfs_block_group_cache *cache) > > +{ > > + struct btrfs_free_space_ctl *ctl = cache->free_space_ctl; > > + struct btrfs_free_space *info; > > + struct rb_node *node; > > + bool ret = true; > > + > > + spin_lock(&ctl->tree_lock); > > + node = rb_first(&ctl->free_space_offset); > > + > > + while (node) { > > + info = rb_entry(node, struct btrfs_free_space, offset_index); > > + > > + if (!btrfs_free_space_trimmed(info)) { > > + ret = false; > > + break; > > + } > > + > > + node = rb_next(node); > > + } > > + > > + spin_unlock(&ctl->tree_lock); > > + return ret; > > +} > > + > > u64 btrfs_find_space_for_alloc(struct btrfs_block_group_cache *block_group, > > u64 offset, u64 bytes, u64 empty_size, > > u64 *max_extent_size) > > @@ -2739,6 +2764,9 @@ int btrfs_return_cluster_to_free_space( > > ret = __btrfs_return_cluster_to_free_space(block_group, cluster); > > spin_unlock(&ctl->tree_lock); > > > > + btrfs_discard_queue_work(&block_group->fs_info->discard_ctl, > > + block_group); > > + > > /* finally drop our ref */ > > btrfs_put_block_group(block_group); > > return ret; > > @@ -3097,6 +3125,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > > u64 min_bytes; > > u64 cont1_bytes; > > int ret; > > + bool found_cluster = false; > > > > /* > > * Choose the minimum extent size we'll require for this > > @@ -3149,6 +3178,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > > list_del_init(&entry->list); > > > > if (!ret) { > > + found_cluster = true; > > atomic_inc(&block_group->count); > > list_add_tail(&cluster->block_group_list, > > &block_group->cluster_list); > > @@ -3160,6 +3190,9 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, > > spin_unlock(&cluster->lock); > > spin_unlock(&ctl->tree_lock); > > > > + if (found_cluster) > > + btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); > > + > > return ret; > > } > > This bit here seems unrelated to the rest of the change. Why do we want to > cancel the discard work if we find a cluster? And what does it have to do with > unused bgs? If it's the allocation part that makes you want to cancel then that > sort of makes sense in the unused bg context, but this is happening no matter > what. It should probably be in its own patch with an explanation, or at least > in a different patch. Thanks, > > Josef I think this got lost in my rebasing over time. I'll move this or axe it, I'm not sure yet. The idea is later on we don't care about metadata block groups and they are the only ones that use clustering. In that case don't worry about fully trimming as the only way it'll start trimming is if it's a fully free metadata block group. Thanks, Dennis
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index 8bbbe7488328..73e5a9384491 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -1251,6 +1251,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) struct btrfs_block_group_cache *block_group; struct btrfs_space_info *space_info; struct btrfs_trans_handle *trans; + bool async_trim_enabled = btrfs_test_opt(fs_info, DISCARD_ASYNC); int ret = 0; if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags)) @@ -1260,6 +1261,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) while (!list_empty(&fs_info->unused_bgs)) { u64 start, end; int trimming; + bool async_trimmed; block_group = list_first_entry(&fs_info->unused_bgs, struct btrfs_block_group_cache, @@ -1281,10 +1283,20 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) /* Don't want to race with allocators so take the groups_sem */ down_write(&space_info->groups_sem); spin_lock(&block_group->lock); + + /* async discard requires block groups to be fully trimmed */ + async_trimmed = (!btrfs_test_opt(fs_info, DISCARD_ASYNC) || + btrfs_is_free_space_trimmed(block_group)); + if (block_group->reserved || block_group->pinned || btrfs_block_group_used(&block_group->item) || block_group->ro || - list_is_singular(&block_group->list)) { + list_is_singular(&block_group->list) || + !async_trimmed) { + /* requeue if we failed because of async discard */ + if (!async_trimmed) + btrfs_discard_queue_work(&fs_info->discard_ctl, + block_group); /* * We want to bail if we made new allocations or have * outstanding allocations in this block group. We do @@ -1367,6 +1379,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) spin_unlock(&block_group->lock); spin_unlock(&space_info->lock); + if (!async_trim_enabled && + btrfs_test_opt(fs_info, DISCARD_ASYNC)) + goto flip_async; + /* DISCARD can flip during remount */ trimming = btrfs_test_opt(fs_info, DISCARD_SYNC); @@ -1411,6 +1427,13 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info) spin_lock(&fs_info->unused_bgs_lock); } spin_unlock(&fs_info->unused_bgs_lock); + return; + +flip_async: + btrfs_end_transaction(trans); + mutex_unlock(&fs_info->delete_unused_bgs_mutex); + btrfs_put_block_group(block_group); + btrfs_discard_punt_unused_bgs_list(fs_info); } void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg) @@ -1618,6 +1641,8 @@ static struct btrfs_block_group_cache *btrfs_create_block_group_cache( cache->full_stripe_len = btrfs_full_stripe_len(fs_info, start); set_free_space_tree_thresholds(cache); + cache->discard_index = 1; + atomic_set(&cache->count, 1); spin_lock_init(&cache->lock); init_rwsem(&cache->data_rwsem); @@ -1829,7 +1854,11 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info) inc_block_group_ro(cache, 1); } else if (btrfs_block_group_used(&cache->item) == 0) { ASSERT(list_empty(&cache->bg_list)); - btrfs_mark_bg_unused(cache); + if (btrfs_test_opt(info, DISCARD_ASYNC)) + btrfs_add_to_discard_free_list( + &info->discard_ctl, cache); + else + btrfs_mark_bg_unused(cache); } } @@ -2724,8 +2753,10 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans, * dirty list to avoid races between cleaner kthread and space * cache writeout. */ - if (!alloc && old_val == 0) - btrfs_mark_bg_unused(cache); + if (!alloc && old_val == 0) { + if (!btrfs_test_opt(info, DISCARD_ASYNC)) + btrfs_mark_bg_unused(cache); + } btrfs_put_block_group(cache); total -= num_bytes; diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 419445868909..c328d2e85e4d 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -439,7 +439,7 @@ struct btrfs_full_stripe_locks_tree { }; /* discard control */ -#define BTRFS_NR_DISCARD_LISTS 1 +#define BTRFS_NR_DISCARD_LISTS 2 struct btrfs_discard_ctl { struct workqueue_struct *discard_workers; diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c index 6df124639e55..fb92b888774d 100644 --- a/fs/btrfs/discard.c +++ b/fs/btrfs/discard.c @@ -29,8 +29,11 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, spin_lock(&discard_ctl->lock); - if (list_empty(&cache->discard_list)) + if (list_empty(&cache->discard_list) || !cache->discard_index) { + if (!cache->discard_index) + cache->discard_index = 1; cache->discard_delay = now + BTRFS_DISCARD_DELAY; + } list_move_tail(&cache->discard_list, btrfs_get_discard_list(discard_ctl, cache)); @@ -38,6 +41,23 @@ void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, spin_unlock(&discard_ctl->lock); } +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, + struct btrfs_block_group_cache *cache) +{ + u64 now = ktime_get_ns(); + + spin_lock(&discard_ctl->lock); + + if (!list_empty(&cache->discard_list)) + list_del_init(&cache->discard_list); + + cache->discard_index = 0; + cache->discard_delay = now; + list_add_tail(&cache->discard_list, &discard_ctl->discard_list[0]); + + spin_unlock(&discard_ctl->lock); +} + static bool remove_from_discard_list(struct btrfs_discard_ctl *discard_ctl, struct btrfs_block_group_cache *cache) { @@ -161,10 +181,52 @@ static void btrfs_discard_workfn(struct work_struct *work) btrfs_block_group_end(cache), 0); remove_from_discard_list(discard_ctl, cache); + if (btrfs_is_free_space_trimmed(cache)) + btrfs_mark_bg_unused(cache); + else if (cache->free_space_ctl->free_space == cache->key.offset) + btrfs_add_to_discard_free_list(discard_ctl, cache); btrfs_discard_schedule_work(discard_ctl, false); } +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info) +{ + struct btrfs_block_group_cache *cache, *next; + + /* we enabled async discard, so punt all to the queue */ + spin_lock(&fs_info->unused_bgs_lock); + + list_for_each_entry_safe(cache, next, &fs_info->unused_bgs, bg_list) { + list_del_init(&cache->bg_list); + btrfs_add_to_discard_free_list(&fs_info->discard_ctl, cache); + } + + spin_unlock(&fs_info->unused_bgs_lock); +} + +static void btrfs_discard_purge_list(struct btrfs_discard_ctl *discard_ctl) +{ + struct btrfs_block_group_cache *cache, *next; + int i; + + spin_lock(&discard_ctl->lock); + + for (i = 0; i < BTRFS_NR_DISCARD_LISTS; i++) { + list_for_each_entry_safe(cache, next, + &discard_ctl->discard_list[i], + discard_list) { + list_del_init(&cache->discard_list); + spin_unlock(&discard_ctl->lock); + if (cache->free_space_ctl->free_space == + cache->key.offset) + btrfs_mark_bg_unused(cache); + spin_lock(&discard_ctl->lock); + } + } + + spin_unlock(&discard_ctl->lock); +} + void btrfs_discard_resume(struct btrfs_fs_info *fs_info) { if (!btrfs_test_opt(fs_info, DISCARD_ASYNC)) { @@ -172,6 +234,8 @@ void btrfs_discard_resume(struct btrfs_fs_info *fs_info) return; } + btrfs_discard_punt_unused_bgs_list(fs_info); + set_bit(BTRFS_FS_DISCARD_RUNNING, &fs_info->flags); } @@ -197,4 +261,6 @@ void btrfs_discard_cleanup(struct btrfs_fs_info *fs_info) { btrfs_discard_stop(fs_info); cancel_delayed_work_sync(&fs_info->discard_ctl.work); + + btrfs_discard_purge_list(&fs_info->discard_ctl); } diff --git a/fs/btrfs/discard.h b/fs/btrfs/discard.h index 6d7805bb0eb7..55f79b624943 100644 --- a/fs/btrfs/discard.h +++ b/fs/btrfs/discard.h @@ -10,9 +10,14 @@ #include <linux/workqueue.h> #include "ctree.h" +#include "block-group.h" +#include "free-space-cache.h" void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, struct btrfs_block_group_cache *cache); +void btrfs_add_to_discard_free_list(struct btrfs_discard_ctl *discard_ctl, + struct btrfs_block_group_cache *cache); +void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info); void btrfs_discard_cancel_work(struct btrfs_discard_ctl *discard_ctl, struct btrfs_block_group_cache *cache); @@ -41,7 +46,11 @@ void btrfs_discard_queue_work(struct btrfs_discard_ctl *discard_ctl, if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC)) return; - btrfs_add_to_discard_list(discard_ctl, cache); + if (cache->free_space_ctl->free_space == cache->key.offset) + btrfs_add_to_discard_free_list(discard_ctl, cache); + else + btrfs_add_to_discard_list(discard_ctl, cache); + if (!delayed_work_pending(&discard_ctl->work)) btrfs_discard_schedule_work(discard_ctl, false); } diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index 54ff1bc97777..ed0e7ee4c78d 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -2653,6 +2653,31 @@ void btrfs_remove_free_space_cache(struct btrfs_block_group_cache *block_group) } +bool btrfs_is_free_space_trimmed(struct btrfs_block_group_cache *cache) +{ + struct btrfs_free_space_ctl *ctl = cache->free_space_ctl; + struct btrfs_free_space *info; + struct rb_node *node; + bool ret = true; + + spin_lock(&ctl->tree_lock); + node = rb_first(&ctl->free_space_offset); + + while (node) { + info = rb_entry(node, struct btrfs_free_space, offset_index); + + if (!btrfs_free_space_trimmed(info)) { + ret = false; + break; + } + + node = rb_next(node); + } + + spin_unlock(&ctl->tree_lock); + return ret; +} + u64 btrfs_find_space_for_alloc(struct btrfs_block_group_cache *block_group, u64 offset, u64 bytes, u64 empty_size, u64 *max_extent_size) @@ -2739,6 +2764,9 @@ int btrfs_return_cluster_to_free_space( ret = __btrfs_return_cluster_to_free_space(block_group, cluster); spin_unlock(&ctl->tree_lock); + btrfs_discard_queue_work(&block_group->fs_info->discard_ctl, + block_group); + /* finally drop our ref */ btrfs_put_block_group(block_group); return ret; @@ -3097,6 +3125,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, u64 min_bytes; u64 cont1_bytes; int ret; + bool found_cluster = false; /* * Choose the minimum extent size we'll require for this @@ -3149,6 +3178,7 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, list_del_init(&entry->list); if (!ret) { + found_cluster = true; atomic_inc(&block_group->count); list_add_tail(&cluster->block_group_list, &block_group->cluster_list); @@ -3160,6 +3190,9 @@ int btrfs_find_space_cluster(struct btrfs_block_group_cache *block_group, spin_unlock(&cluster->lock); spin_unlock(&ctl->tree_lock); + if (found_cluster) + btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); + return ret; } diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h index dc73ec8d34bb..b688e70a7512 100644 --- a/fs/btrfs/free-space-cache.h +++ b/fs/btrfs/free-space-cache.h @@ -107,6 +107,7 @@ int btrfs_remove_free_space(struct btrfs_block_group_cache *block_group, void __btrfs_remove_free_space_cache(struct btrfs_free_space_ctl *ctl); void btrfs_remove_free_space_cache(struct btrfs_block_group_cache *block_group); +bool btrfs_is_free_space_trimmed(struct btrfs_block_group_cache *cache); u64 btrfs_find_space_for_alloc(struct btrfs_block_group_cache *block_group, u64 offset, u64 bytes, u64 empty_size, u64 *max_extent_size); diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index f7d4e03f4c5d..49927a642b5a 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -8,6 +8,7 @@ #include <linux/sched/mm.h> #include <crypto/hash.h> #include "ctree.h" +#include "discard.h" #include "volumes.h" #include "disk-io.h" #include "ordered-data.h" @@ -3683,7 +3684,11 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx, if (!cache->removed && !cache->ro && cache->reserved == 0 && btrfs_block_group_used(&cache->item) == 0) { spin_unlock(&cache->lock); - btrfs_mark_bg_unused(cache); + if (btrfs_test_opt(fs_info, DISCARD_ASYNC)) + btrfs_add_to_discard_free_list( + &fs_info->discard_ctl, cache); + else + btrfs_mark_bg_unused(cache); } else { spin_unlock(&cache->lock); }
block_group removal is a little tricky. It can race with the extent allocator, the cleaner thread, and balancing. The current path is for a block_group to be added to the unused_bgs list. Then, when the cleaner thread comes around, it starts a transaction and then proceeds with removing the block_group. Extents that are pinned are subsequently removed from the pinned trees and then eventually a discard is issued for the entire block_group. Async discard introduces another player into the game, the discard workqueue. While it has none of the racing issues, the new problem is ensuring we don't leave free space untrimmed prior to forgetting the block_group. This is handled by placing fully free block_groups on a separate discard queue. This is necessary to maintain discarding order as in the future we will slowly trim even fully free block_groups. The ordering helps us make progress on the same block_group rather than say the last fully freed block_group or needing to search through the fully freed block groups at the beginning of a list and insert after. The new order of events is a fully freed block group gets placed on the discard queue first. Once it's processed, it will be placed on the unusued_bgs list and then the original sequence of events will happen, just without the final whole block_group discard. The mount flags can change when processing unused_bgs, so when flipping from DISCARD to DISCARD_ASYNC, the unused_bgs must be punted to the discard_list to be trimmed. If we flip off DISCARD_ASYNC, we punt free block groups on the discard_list to the unused_bg queue which will do the final discard for us. Signed-off-by: Dennis Zhou <dennis@kernel.org> --- fs/btrfs/block-group.c | 39 ++++++++++++++++++--- fs/btrfs/ctree.h | 2 +- fs/btrfs/discard.c | 68 ++++++++++++++++++++++++++++++++++++- fs/btrfs/discard.h | 11 +++++- fs/btrfs/free-space-cache.c | 33 ++++++++++++++++++ fs/btrfs/free-space-cache.h | 1 + fs/btrfs/scrub.c | 7 +++- 7 files changed, 153 insertions(+), 8 deletions(-)