diff mbox series

[18/22] btrfs: only keep track of data extents for async discard

Message ID 28b5064229e24388600f6f776621c6443c3e92b7.1571865775.git.dennis@kernel.org (mailing list archive)
State New, archived
Headers show
Series btrfs: async discard support | expand

Commit Message

Dennis Zhou Oct. 23, 2019, 10:53 p.m. UTC 
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.

Signed-off-by: Dennis Zhou <dennis@kernel.org>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-group.h |  6 ++++++
 fs/btrfs/discard.c     | 11 +++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Josef Bacik Nov. 8, 2019, 7:46 p.m. UTC | #1 
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
Dennis Zhou Nov. 8, 2019, 8:14 p.m. UTC | #2 
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
David Sterba Nov. 15, 2019, 5:31 p.m. UTC | #3 
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 mbox series

Patch

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;