Message ID | 28b5064229e24388600f6f776621c6443c3e92b7.1571865775.git.dennis@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: async discard support | expand |
On Wed, Oct 23, 2019 at 06:53:12PM -0400, Dennis Zhou wrote: > As mentioned earlier, discarding data can be done either by issuing an > explicit discard or implicitly by reusing the LBA. Metadata chunks see > much more frequent reuse due to well it being metadata. So instead of > explicitly discarding metadata blocks, just leave them be and let the > latter implicit discarding be done for them. > Hmm now that I look at this, it seems like we won't even discard empty metadata block groups now, right? Or am I missing something? Thanks, Josef
On Fri, Nov 08, 2019 at 02:46:51PM -0500, Josef Bacik wrote: > On Wed, Oct 23, 2019 at 06:53:12PM -0400, Dennis Zhou wrote: > > As mentioned earlier, discarding data can be done either by issuing an > > explicit discard or implicitly by reusing the LBA. Metadata chunks see > > much more frequent reuse due to well it being metadata. So instead of > > explicitly discarding metadata blocks, just leave them be and let the > > latter implicit discarding be done for them. > > > > Hmm now that I look at this, it seems like we won't even discard empty metadata > block groups now, right? Or am I missing something? Thanks, > Empty block groups go through btrfs_add_to_discard_unused_list() which skips that check. So metadata blocks will be discarded here from btrfs_discard_queue_work() which should be called from __btrfs_add_free_space(). We should just skip discarding metadata blocks while they are being used. Thanks, Dennis
On Fri, Nov 08, 2019 at 03:14:58PM -0500, Dennis Zhou wrote: > On Fri, Nov 08, 2019 at 02:46:51PM -0500, Josef Bacik wrote: > > On Wed, Oct 23, 2019 at 06:53:12PM -0400, Dennis Zhou wrote: > > > As mentioned earlier, discarding data can be done either by issuing an > > > explicit discard or implicitly by reusing the LBA. Metadata chunks see > > > much more frequent reuse due to well it being metadata. So instead of > > > explicitly discarding metadata blocks, just leave them be and let the > > > latter implicit discarding be done for them. > > > > > > > Hmm now that I look at this, it seems like we won't even discard empty metadata > > block groups now, right? Or am I missing something? Thanks, > > > > Empty block groups go through btrfs_add_to_discard_unused_list() which > skips that check. So metadata blocks will be discarded here from > btrfs_discard_queue_work() which should be called from > __btrfs_add_free_space(). > > We should just skip discarding metadata blocks while they are being > used. I believe discarding metadata blocks while in use is called corruption :) But I understand what you mean.
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h index 88266cc16c07..6a586b2968ac 100644 --- a/fs/btrfs/block-group.h +++ b/fs/btrfs/block-group.h @@ -181,6 +181,12 @@ static inline u64 btrfs_block_group_end(struct btrfs_block_group_cache *cache) return (cache->key.objectid + cache->key.offset); } +static inline bool btrfs_is_block_group_data( + struct btrfs_block_group_cache *cache) +{ + return (cache->flags & BTRFS_BLOCK_GROUP_DATA); +} + #ifdef CONFIG_BTRFS_DEBUG static inline int btrfs_should_fragment_free_space( struct btrfs_block_group_cache *block_group) diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c index 592a5c7b9dc1..be5a4439ceb0 100644 --- a/fs/btrfs/discard.c +++ b/fs/btrfs/discard.c @@ -51,6 +51,9 @@ static void __btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl, struct btrfs_block_group_cache *cache) { + if (!btrfs_is_block_group_data(cache)) + return; + spin_lock(&discard_ctl->lock); __btrfs_add_to_discard_list(discard_ctl, cache); @@ -161,7 +164,10 @@ static struct btrfs_block_group_cache *peek_discard_list( if (cache && now > cache->discard_eligible_time) { if (cache->discard_index == BTRFS_DISCARD_INDEX_UNUSED && btrfs_block_group_used(&cache->item) != 0) { - __btrfs_add_to_discard_list(discard_ctl, cache); + if (btrfs_is_block_group_data(cache)) + __btrfs_add_to_discard_list(discard_ctl, cache); + else + list_del_init(&cache->discard_list); goto again; } if (cache->discard_state == BTRFS_DISCARD_RESET_CURSOR) { @@ -492,7 +498,8 @@ void btrfs_discard_update_discardable(struct btrfs_block_group_cache *cache, s32 extents_delta; s64 bytes_delta; - if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC)) + if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC) || + !btrfs_is_block_group_data(cache)) return; discard_ctl = &cache->fs_info->discard_ctl;