buffer redirtying fixes and cleanup

buffer redirtying fixes and cleanup

[Christoph Hellwig]

Hi all,

this series fixed two small bugs in the buffer redirty code for the
zoned mode, and then removed some unnneded code in the same area.

Diffstat:
 disk-io.c     |    9 +--------
 extent_io.c   |    8 ++++----
 extent_io.h   |    3 +--
 transaction.c |    9 ---------
 transaction.h |    3 ---
 zoned.c       |   32 ++++++--------------------------
 zoned.h       |    2 --
 7 files changed, 12 insertions(+), 54 deletions(-)

[PATCH 1/3] btrfs: zero the buffer before marking it dirty in btrfs_redirty_list_add

[Christoph Hellwig]

btrfs_redirty_list_add zeroes the buffer data and sets the
EXTENT_BUFFER_NO_CHECK to make sure writeback is fine with a bogus
header.  But it does that after already marking the buffer dirty, which
means that writeback could already be looking at the buffer.

Switch the order of operations around so that the buffer is only marked
dirty when we're ready to write it.

Fixes: d3575156f662 ("btrfs: zoned: redirty released extent buffers")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/zoned.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index e3fe02aae641f3..7095cfca2fdde1 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1610,11 +1610,11 @@ void btrfs_redirty_list_add(struct btrfs_transaction *trans,
 	    !list_empty(&eb->release_list))
 		return;
 
+	memzero_extent_buffer(eb, 0, eb->len);
+	set_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags);
 	set_extent_buffer_dirty(eb);
 	set_extent_bits_nowait(&trans->dirty_pages, eb->start,
 			       eb->start + eb->len - 1, EXTENT_DIRTY);
-	memzero_extent_buffer(eb, 0, eb->len);
-	set_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags);
 
 	spin_lock(&trans->releasing_ebs_lock);
 	list_add_tail(&eb->release_list, &trans->releasing_ebs);
-- 
2.39.2

[PATCH 2/3] btrfs: fix dirty_metadata_bytes for redirtied buffers

[Christoph Hellwig]

dirty_metadata_bytes is decremented in both places that clear the dirty
bit in a buffer, but only incremented in btrfs_mark_buffer_dirty, which
means that a buffer that is redirtied using btrfs_redirty_list_add won't
be added to dirty_metadata_bytes, but it will be subtracted when written
out, leading an inconsistency in the counter.

Move the dirty_metadata_bytes from btrfs_mark_buffer_dirty into
set_extent_buffer_dirty to also account for the redirty case, and remove
the now unused set_extent_buffer_dirty return value.

Fixes: d3575156f662 ("btrfs: zoned: redirty released extent buffers")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c   | 7 +------
 fs/btrfs/extent_io.c | 7 ++++---
 fs/btrfs/extent_io.h | 2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 96f144094af687..acd8ebf2824d18 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4661,7 +4661,6 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 {
 	struct btrfs_fs_info *fs_info = buf->fs_info;
 	u64 transid = btrfs_header_generation(buf);
-	int was_dirty;
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 	/*
@@ -4676,11 +4675,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 	if (transid != fs_info->generation)
 		WARN(1, KERN_CRIT "btrfs transid mismatch buffer %llu, found %llu running %llu\n",
 			buf->start, transid, fs_info->generation);
-	was_dirty = set_extent_buffer_dirty(buf);
-	if (!was_dirty)
-		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
-					 buf->len,
-					 fs_info->dirty_metadata_batch);
+	set_extent_buffer_dirty(buf);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	/*
 	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a1adadd5d25ddb..a829390632a538 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4148,7 +4148,7 @@ void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
 	WARN_ON(atomic_read(&eb->refs) == 0);
 }
 
-bool set_extent_buffer_dirty(struct extent_buffer *eb)
+void set_extent_buffer_dirty(struct extent_buffer *eb)
 {
 	int i;
 	int num_pages;
@@ -4183,13 +4183,14 @@ bool set_extent_buffer_dirty(struct extent_buffer *eb)
 					     eb->start, eb->len);
 		if (subpage)
 			unlock_page(eb->pages[0]);
+		percpu_counter_add_batch(&eb->fs_info->dirty_metadata_bytes,
+					 eb->len,
+					 eb->fs_info->dirty_metadata_batch);
 	}
 #ifdef CONFIG_BTRFS_DEBUG
 	for (i = 0; i < num_pages; i++)
 		ASSERT(PageDirty(eb->pages[i]));
 #endif
-
-	return was_dirty;
 }
 
 void clear_extent_buffer_uptodate(struct extent_buffer *eb)
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 4341ad978fb8e4..f937654230d3c5 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -262,7 +262,7 @@ void extent_buffer_bitmap_set(const struct extent_buffer *eb, unsigned long star
 void extent_buffer_bitmap_clear(const struct extent_buffer *eb,
 				unsigned long start, unsigned long pos,
 				unsigned long len);
-bool set_extent_buffer_dirty(struct extent_buffer *eb);
+void set_extent_buffer_dirty(struct extent_buffer *eb);
 void set_extent_buffer_uptodate(struct extent_buffer *eb);
 void clear_extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_under_io(const struct extent_buffer *eb);
-- 
2.39.2

[PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[Christoph Hellwig]

When btrfs_redirty_list_add redirties a buffer, it also acquires
an extra reference that is released on transaction commit.  But
this is not required as buffers that are dirty or under writeback
are never freed (look for calls to extent_buffer_under_io())).

Remove the extra reference and the infrastructure used to drop it
again.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/disk-io.c     |  2 --
 fs/btrfs/extent_io.c   |  1 -
 fs/btrfs/extent_io.h   |  1 -
 fs/btrfs/transaction.c |  9 ---------
 fs/btrfs/transaction.h |  3 ---
 fs/btrfs/zoned.c       | 28 ++++------------------------
 fs/btrfs/zoned.h       |  2 --
 7 files changed, 4 insertions(+), 42 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index acd8ebf2824d18..ae81a3b586eaed 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -5106,8 +5106,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
 				     EXTENT_DIRTY);
 	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
 
-	btrfs_free_redirty_list(cur_trans);
-
 	cur_trans->state =TRANS_STATE_COMPLETED;
 	wake_up(&cur_trans->commit_wait);
 }
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a829390632a538..d8becf1cdbc09e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3557,7 +3557,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	init_rwsem(&eb->lock);
 
 	btrfs_leak_debug_add_eb(eb);
-	INIT_LIST_HEAD(&eb->release_list);
 
 	spin_lock_init(&eb->refs_lock);
 	atomic_set(&eb->refs, 1);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index f937654230d3c5..6f3cfadd232c95 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -89,7 +89,6 @@ struct extent_buffer {
 	struct rw_semaphore lock;
 
 	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
-	struct list_head release_list;
 #ifdef CONFIG_BTRFS_DEBUG
 	struct list_head leak_list;
 #endif
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 27c616fdfae274..fe0f00e717a834 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -374,8 +374,6 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	spin_lock_init(&cur_trans->dirty_bgs_lock);
 	INIT_LIST_HEAD(&cur_trans->deleted_bgs);
 	spin_lock_init(&cur_trans->dropped_roots_lock);
-	INIT_LIST_HEAD(&cur_trans->releasing_ebs);
-	spin_lock_init(&cur_trans->releasing_ebs_lock);
 	list_add_tail(&cur_trans->list, &fs_info->trans_list);
 	extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
 			IO_TREE_TRANS_DIRTY_PAGES);
@@ -2482,13 +2480,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 		goto scrub_continue;
 	}
 
-	/*
-	 * At this point, we should have written all the tree blocks allocated
-	 * in this transaction. So it's now safe to free the redirtyied extent
-	 * buffers.
-	 */
-	btrfs_free_redirty_list(cur_trans);
-
 	ret = write_all_supers(fs_info, 0);
 	/*
 	 * the super is written, we can safely allow the tree-loggers
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index fa728ab8082614..8e9fa23bd7fed7 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -94,9 +94,6 @@ struct btrfs_transaction {
 	 */
 	atomic_t pending_ordered;
 	wait_queue_head_t pending_wait;
-
-	spinlock_t releasing_ebs_lock;
-	struct list_head releasing_ebs;
 };
 
 enum {
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 7095cfca2fdde1..5612731fc00d78 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1603,37 +1603,17 @@ void btrfs_calc_zone_unusable(struct btrfs_block_group *cache)
 void btrfs_redirty_list_add(struct btrfs_transaction *trans,
 			    struct extent_buffer *eb)
 {
-	struct btrfs_fs_info *fs_info = eb->fs_info;
-
-	if (!btrfs_is_zoned(fs_info) ||
-	    btrfs_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN) ||
-	    !list_empty(&eb->release_list))
+	if (!btrfs_is_zoned(eb->fs_info) ||
+	    btrfs_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN))
 		return;
 
+	ASSERT(!test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
+
 	memzero_extent_buffer(eb, 0, eb->len);
 	set_bit(EXTENT_BUFFER_NO_CHECK, &eb->bflags);
 	set_extent_buffer_dirty(eb);
 	set_extent_bits_nowait(&trans->dirty_pages, eb->start,
 			       eb->start + eb->len - 1, EXTENT_DIRTY);
-
-	spin_lock(&trans->releasing_ebs_lock);
-	list_add_tail(&eb->release_list, &trans->releasing_ebs);
-	spin_unlock(&trans->releasing_ebs_lock);
-	atomic_inc(&eb->refs);
-}
-
-void btrfs_free_redirty_list(struct btrfs_transaction *trans)
-{
-	spin_lock(&trans->releasing_ebs_lock);
-	while (!list_empty(&trans->releasing_ebs)) {
-		struct extent_buffer *eb;
-
-		eb = list_first_entry(&trans->releasing_ebs,
-				      struct extent_buffer, release_list);
-		list_del_init(&eb->release_list);
-		free_extent_buffer(eb);
-	}
-	spin_unlock(&trans->releasing_ebs_lock);
 }
 
 bool btrfs_use_zone_append(struct btrfs_bio *bbio)
diff --git a/fs/btrfs/zoned.h b/fs/btrfs/zoned.h
index c0570d35fea291..3058ef559c9813 100644
--- a/fs/btrfs/zoned.h
+++ b/fs/btrfs/zoned.h
@@ -54,7 +54,6 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new);
 void btrfs_calc_zone_unusable(struct btrfs_block_group *cache);
 void btrfs_redirty_list_add(struct btrfs_transaction *trans,
 			    struct extent_buffer *eb);
-void btrfs_free_redirty_list(struct btrfs_transaction *trans);
 bool btrfs_use_zone_append(struct btrfs_bio *bbio);
 void btrfs_record_physical_zoned(struct btrfs_bio *bbio);
 void btrfs_rewrite_logical_zoned(struct btrfs_ordered_extent *ordered);
@@ -179,7 +178,6 @@ static inline void btrfs_calc_zone_unusable(struct btrfs_block_group *cache) { }
 
 static inline void btrfs_redirty_list_add(struct btrfs_transaction *trans,
 					  struct extent_buffer *eb) { }
-static inline void btrfs_free_redirty_list(struct btrfs_transaction *trans) { }
 
 static inline bool btrfs_use_zone_append(struct btrfs_bio *bbio)
 {
-- 
2.39.2

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[David Sterba]

On Mon, May 08, 2023 at 07:58:39AM -0700, Christoph Hellwig wrote:
> When btrfs_redirty_list_add redirties a buffer, it also acquires
> an extra reference that is released on transaction commit.  But
> this is not required as buffers that are dirty or under writeback
> are never freed (look for calls to extent_buffer_under_io())).
> 
> Remove the extra reference and the infrastructure used to drop it
> again.

I vaguely remember that the redirty list was need for zoned to avoid
some write pattern that disrupts the ordering, added in d3575156f662
("btrfs: zoned: redirty released extent buffers").

I'd appreciate more eyes on this patch, with the indirections and
writeback involved it's not clear to me that we don't need the list at
all. Pointing to extent_buffer_under_io() is a good start but the state
transitions of eb are complex so a more concrete example how it works
should be in the changelog.

For testing I'll add the series to misc-next, changelog update can be
done later.  Thanks.

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[Christoph Hellwig]

On Wed, May 10, 2023 at 12:57:37AM +0200, David Sterba wrote:
> On Mon, May 08, 2023 at 07:58:39AM -0700, Christoph Hellwig wrote:
> > When btrfs_redirty_list_add redirties a buffer, it also acquires
> > an extra reference that is released on transaction commit.  But
> > this is not required as buffers that are dirty or under writeback
> > are never freed (look for calls to extent_buffer_under_io())).
> > 
> > Remove the extra reference and the infrastructure used to drop it
> > again.
> 
> I vaguely remember that the redirty list was need for zoned to avoid
> some write pattern that disrupts the ordering, added in d3575156f662
> ("btrfs: zoned: redirty released extent buffers").

So the redirting itself is needed for that - without it buffers where
the dirty bit wasn't ever set would never get written, leading to a
write outside of the zone pointer.  But the extra reference can't
influece the write pattern, as we don't make writeback descriptions
based of it.

> I'd appreciate more eyes on this patch, with the indirections and
> writeback involved it's not clear to me that we don't need the list at
> all.

My suspicision is that Aoto-san wanted the extra safety of the extra
reference because he didn't want to trust or hadn't noticed the
extent_buffer_under_io() magic.  Auto-san, can you confirm or deny? :)

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[David Sterba]

On Mon, May 15, 2023 at 11:22:54AM +0200, Christoph Hellwig wrote:
> On Wed, May 10, 2023 at 12:57:37AM +0200, David Sterba wrote:
> > On Mon, May 08, 2023 at 07:58:39AM -0700, Christoph Hellwig wrote:
> > > When btrfs_redirty_list_add redirties a buffer, it also acquires
> > > an extra reference that is released on transaction commit.  But
> > > this is not required as buffers that are dirty or under writeback
> > > are never freed (look for calls to extent_buffer_under_io())).
> > > 
> > > Remove the extra reference and the infrastructure used to drop it
> > > again.
> > 
> > I vaguely remember that the redirty list was need for zoned to avoid
> > some write pattern that disrupts the ordering, added in d3575156f662
> > ("btrfs: zoned: redirty released extent buffers").
> 
> So the redirting itself is needed for that - without it buffers where
> the dirty bit wasn't ever set would never get written, leading to a
> write outside of the zone pointer.  But the extra reference can't
> influece the write pattern, as we don't make writeback descriptions
> based of it.
> 
> > I'd appreciate more eyes on this patch, with the indirections and
> > writeback involved it's not clear to me that we don't need the list at
> > all.
> 
> My suspicision is that Aoto-san wanted the extra safety of the extra
> reference because he didn't want to trust or hadn't noticed the
> extent_buffer_under_io() magic.  Auto-san, can you confirm or deny? :)

The number of patches above this one in the queue is increasing so it
would get harder to remove it. I took another look and agree that
regarding the references it's safe but would still like a confirmation.

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[Christoph Hellwig]

On Tue, May 30, 2023 at 05:56:48PM +0200, David Sterba wrote:
> > > I'd appreciate more eyes on this patch, with the indirections and
> > > writeback involved it's not clear to me that we don't need the list at
> > > all.
> > 
> > My suspicision is that Aoto-san wanted the extra safety of the extra
> > reference because he didn't want to trust or hadn't noticed the
> > extent_buffer_under_io() magic.  Auto-san, can you confirm or deny? :)
> 
> The number of patches above this one in the queue is increasing so it
> would get harder to remove it. I took another look and agree that
> regarding the references it's safe but would still like a confirmation.

As stated, I am very confident that this is safe based on all my
recent work with the extent_buffer code base.  I'd love to hear
from Aota, but there's not much more I can add here myself.

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[Naohiro Aota]

On Wed, May 31, 2023 at 06:16:26AM +0200, Christoph Hellwig wrote:
> On Tue, May 30, 2023 at 05:56:48PM +0200, David Sterba wrote:
> > > > I'd appreciate more eyes on this patch, with the indirections and
> > > > writeback involved it's not clear to me that we don't need the list at
> > > > all.
> > > 
> > > My suspicision is that Aoto-san wanted the extra safety of the extra
> > > reference because he didn't want to trust or hadn't noticed the
> > > extent_buffer_under_io() magic.  Auto-san, can you confirm or deny? :)
> > 
> > The number of patches above this one in the queue is increasing so it
> > would get harder to remove it. I took another look and agree that
> > regarding the references it's safe but would still like a confirmation.
> 
> As stated, I am very confident that this is safe based on all my
> recent work with the extent_buffer code base.  I'd love to hear
> from Aota, but there's not much more I can add here myself.

Sorry. I missed this thread is on-going.

I ran my test runs on misc-next containing this patch, and got no issue
regarding this. So, the patch should be good.

I didn't notice the extent_buffer_under_io() magic. If we can remove it,
let's remove unnecessary variable from extent_buffer.

Also, I dig into the "redirty" history to make it sure. In the first place,
it used releasing_list to hold all the to-be-released extent buffers, and
decided which buffers to re-dirty at the commit time. Then, in a later
version, I change the behavior to re-dirty a necessary buffer and add
re-dirtied one to the list in btrfs_free_tree_block(). In short, the list
was there mostly for the patch series' historical reason.

So, not sure still I can add this but, for the whole series:

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

Re: [PATCH 3/3] btrfs: don't hold an extra reference for redirtied buffers

[David Sterba]

On Wed, May 31, 2023 at 03:04:26PM +0000, Naohiro Aota wrote:
> On Wed, May 31, 2023 at 06:16:26AM +0200, Christoph Hellwig wrote:
> > On Tue, May 30, 2023 at 05:56:48PM +0200, David Sterba wrote:
> > > > > I'd appreciate more eyes on this patch, with the indirections and
> > > > > writeback involved it's not clear to me that we don't need the list at
> > > > > all.
> > > > 
> > > > My suspicision is that Aoto-san wanted the extra safety of the extra
> > > > reference because he didn't want to trust or hadn't noticed the
> > > > extent_buffer_under_io() magic.  Auto-san, can you confirm or deny? :)
> > > 
> > > The number of patches above this one in the queue is increasing so it
> > > would get harder to remove it. I took another look and agree that
> > > regarding the references it's safe but would still like a confirmation.
> > 
> > As stated, I am very confident that this is safe based on all my
> > recent work with the extent_buffer code base.  I'd love to hear
> > from Aota, but there's not much more I can add here myself.
> 
> Sorry. I missed this thread is on-going.
> 
> I ran my test runs on misc-next containing this patch, and got no issue
> regarding this. So, the patch should be good.
> 
> I didn't notice the extent_buffer_under_io() magic. If we can remove it,
> let's remove unnecessary variable from extent_buffer.
> 
> Also, I dig into the "redirty" history to make it sure. In the first place,
> it used releasing_list to hold all the to-be-released extent buffers, and
> decided which buffers to re-dirty at the commit time. Then, in a later
> version, I change the behavior to re-dirty a necessary buffer and add
> re-dirtied one to the list in btrfs_free_tree_block(). In short, the list
> was there mostly for the patch series' historical reason.
> 
> So, not sure still I can add this but, for the whole series:
> 
> Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

Perfect, thanks. Changelog updated and rev-by added.