[PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling

[PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling

[Naohiro Aota]

Btrfs clears the content of an extent buffer marked as
EXTENT_BUFFER_ZONED_ZEROOUT before the bio submission. This mechanism is
introduced to prevent a write hole of an extent buffer, which is once
allocated, marked dirty, but turns out unnecessary and cleaned up within
one transaction operation.

Currently, btrfs_clear_buffer_dirty() marks the extent buffer as
EXTENT_BUFFER_ZONED_ZEROOUT, and skip the enteri function. If this call
happens while the buffer is under IO (with the WRITEBACK flag set, without
the DIRTY flag), we can add the ZEROOUT flag and clear the buffer's content
just before a bio submission. As a result, 1) it can lead to adding faulty
delayed reference item which leads to a FS corrupted (EUCLEAN) error, and
2) it writes out cleared tree node on disk

Fix them by skipping a non-dirty extent buffer. Also, the second patch adds
ASSERT and WARN to catch invalid EXTENT_BUFFER_ZONED_ZEROOUT state.

Naohiro Aota (2):
  btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer
  btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT
    handling

 fs/btrfs/extent-tree.c | 8 ++++++++
 fs/btrfs/extent_io.c   | 3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

-- 
Changes:
- Change the fix to address the root cause.

v1: https://lore.kernel.org/linux-btrfs/3f4f2a0ff1a6c818050434288925bdcf3cd719e5.1709124777.git.naohiro.aota@wdc.com/
--
2.44.0

[PATCH v2 1/2] btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer

[Naohiro Aota]

Btrfs clears the content of an extent buffer marked as
EXTENT_BUFFER_ZONED_ZEROOUT before the bio submission. This mechanism is
introduced to prevent a write hole of an extent buffer, which is once
allocated, marked dirty, but turns out unnecessary and cleaned up within
one transaction operation.

Currently, btrfs_clear_buffer_dirty() marks the extent buffer as
EXTENT_BUFFER_ZONED_ZEROOUT, and skip the enteri function. If this call
happens while the buffer is under IO (with the WRITEBACK flag set, without
the DIRTY flag), we can add the ZEROOUT flag and clear the buffer's content
just before a bio submission. As a result, 1) it can lead to adding faulty
delayed reference item which leads to a FS corrupted (EUCLEAN) error, and
2) it writes out cleared tree node on disk

The former issue is previously discussed in [1]. The corruption happens
when it runs a delayed reference update. So, on-disk data is safe.

[1] https://lore.kernel.org/linux-btrfs/3f4f2a0ff1a6c818050434288925bdcf3cd719e5.1709124777.git.naohiro.aota@wdc.com/

The latter one can reach on-disk data. But, as that node is already
btrfs_clear_buffer_dirty()'ed, that will be invalidated in the next
transaction commit anyway. So, the chance of hitting the corruption is
relatively small.

Anyway, we should skip flagging ZEROOUT on a non-DIRTY extent buffer, to
keep the content under IO intact.

Fixes: aa6313e6ff2b ("btrfs: zoned: don't clear dirty flag of extent buffer")
CC: stable@vger.kernel.org # 6.8
Link: https://lore.kernel.org/linux-btrfs/oadvdekkturysgfgi4qzuemd57zudeasynswurjxw3ocdfsef6@sjyufeugh63f/
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 38e689bf148b..bfe7814c818a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4153,7 +4153,7 @@ void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
 	 * The actual zeroout of the buffer will happen later in
 	 * btree_csum_one_bio.
 	 */
-	if (btrfs_is_zoned(fs_info)) {
+	if (btrfs_is_zoned(fs_info) && test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags)) {
 		set_bit(EXTENT_BUFFER_ZONED_ZEROOUT, &eb->bflags);
 		return;
 	}
-- 
2.44.0

[PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT handling

[Naohiro Aota]

Add an ASSERT to catch a faulty delayed reference item resulting from
prematurely cleared extent buffer.

Also, add a WARN to detect if we try to dirty a ZEROOUT buffer again, which
is suspicious as its update will be lost.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/extent-tree.c | 8 ++++++++
 fs/btrfs/extent_io.c   | 1 +
 2 files changed, 9 insertions(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 1a1191efe59e..42525dc8a551 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3464,6 +3464,14 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 	if (root_id != BTRFS_TREE_LOG_OBJECTID) {
 		struct btrfs_ref generic_ref = { 0 };
 
+		/*
+		 * Assert that the extent buffer is not cleared due to
+		 * EXTENT_BUFFER_ZONED_ZEROOUT. Please refer
+		 * btrfs_clear_buffer_dirty() and btree_csum_one_bio() for
+		 * detail.
+		 */
+		ASSERT(btrfs_header_bytenr(buf) != 0);
+
 		btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
 				       buf->start, buf->len, parent,
 				       btrfs_header_owner(buf));
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bfe7814c818a..42bcbe9bd863 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4192,6 +4192,7 @@ void set_extent_buffer_dirty(struct extent_buffer *eb)
 	num_folios = num_extent_folios(eb);
 	WARN_ON(atomic_read(&eb->refs) == 0);
 	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
+	WARN_ON(test_bit(EXTENT_BUFFER_ZONED_ZEROOUT, &eb->bflags));
 
 	if (!was_dirty) {
 		bool subpage = eb->fs_info->nodesize < PAGE_SIZE;
-- 
2.44.0

Re: [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling

[Johannes Thumshirn]

On 26.03.24 06:40, Naohiro Aota wrote:
> Btrfs clears the content of an extent buffer marked as
> EXTENT_BUFFER_ZONED_ZEROOUT before the bio submission. This mechanism is
> introduced to prevent a write hole of an extent buffer, which is once
> allocated, marked dirty, but turns out unnecessary and cleaned up within
> one transaction operation.
> 
> Currently, btrfs_clear_buffer_dirty() marks the extent buffer as
> EXTENT_BUFFER_ZONED_ZEROOUT, and skip the enteri function. If this call
> happens while the buffer is under IO (with the WRITEBACK flag set, without
> the DIRTY flag), we can add the ZEROOUT flag and clear the buffer's content
> just before a bio submission. As a result, 1) it can lead to adding faulty
> delayed reference item which leads to a FS corrupted (EUCLEAN) error, and
> 2) it writes out cleared tree node on disk
> 
> Fix them by skipping a non-dirty extent buffer. Also, the second patch adds
> ASSERT and WARN to catch invalid EXTENT_BUFFER_ZONED_ZEROOUT state.
> 
> Naohiro Aota (2):
>    btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer
>    btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT
>      handling
> 
>   fs/btrfs/extent-tree.c | 8 ++++++++
>   fs/btrfs/extent_io.c   | 3 ++-
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 

Looks good, thanks

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

One minor nit, codespell flagged a typo, but I'm too blind to find it:

johannes@nuc:linux (review)$ b4 shazam 
cover.1711416290.git.naohiro.aota@wdc.com
Grabbing thread from 
lore.kernel.org/all/cover.1711416290.git.naohiro.aota@wdc.com/t.mbox.gz 
 

Checking for newer revisions
Grabbing search results from lore.kernel.org 

Analyzing 3 messages in the thread
Checking attestation on all messages, may take a moment...
---
   ✓ [PATCH v2 1/2] btrfs: zoned: do not flag ZEROOUT on non-dirty 
extent bufffer
   ✓ [PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for 
EXTENT_BUFFER_ZONED_ZEROOUT handling 

   ---
   ✓ Signed: DKIM/wdc.com
---
Total patches: 2
---
Applying: btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer 

Applying: btrfs: zoned: add ASSERT and WARN for 
EXTENT_BUFFER_ZONED_ZEROOUT handling
/home/johannes/src/linux/.git/rebase-apply/final-commit:1: bufffer ==> 
buffer

Re: [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling

[David Sterba]

On Tue, Mar 26, 2024 at 04:50:46PM +0000, Johannes Thumshirn wrote:
> On 26.03.24 06:40, Naohiro Aota wrote:
> > Btrfs clears the content of an extent buffer marked as
> > EXTENT_BUFFER_ZONED_ZEROOUT before the bio submission. This mechanism is
> > introduced to prevent a write hole of an extent buffer, which is once
> > allocated, marked dirty, but turns out unnecessary and cleaned up within
> > one transaction operation.
> > 
> > Currently, btrfs_clear_buffer_dirty() marks the extent buffer as
> > EXTENT_BUFFER_ZONED_ZEROOUT, and skip the enteri function. If this call
                                              ^^^^^^

> One minor nit, codespell flagged a typo, but I'm too blind to find it:

There's the typo.

Re: [PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT handling

[David Sterba]

On Tue, Mar 26, 2024 at 02:39:21PM +0900, Naohiro Aota wrote:
> Add an ASSERT to catch a faulty delayed reference item resulting from
> prematurely cleared extent buffer.
> 
> Also, add a WARN to detect if we try to dirty a ZEROOUT buffer again, which
> is suspicious as its update will be lost.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Reviewed-by: David Sterba <dsterba@suse.com>