* [PATCH v2 1/2] btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer
2024-03-26 5:39 [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling Naohiro Aota
@ 2024-03-26 5:39 ` Naohiro Aota
2024-03-26 5:39 ` [PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT handling Naohiro Aota
2024-03-26 16:50 ` [PATCH v2 0/2] btrfs: zoned: fix " Johannes Thumshirn
2 siblings, 0 replies; 6+ messages in thread
From: Naohiro Aota @ 2024-03-26 5:39 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT handling
2024-03-26 5:39 [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling Naohiro Aota
2024-03-26 5:39 ` [PATCH v2 1/2] btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer Naohiro Aota
@ 2024-03-26 5:39 ` Naohiro Aota
2024-03-28 19:50 ` David Sterba
2024-03-26 16:50 ` [PATCH v2 0/2] btrfs: zoned: fix " Johannes Thumshirn
2 siblings, 1 reply; 6+ messages in thread
From: Naohiro Aota @ 2024-03-26 5:39 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling
2024-03-26 5:39 [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling Naohiro Aota
2024-03-26 5:39 ` [PATCH v2 1/2] btrfs: zoned: do not flag ZEROOUT on non-dirty extent bufffer Naohiro Aota
2024-03-26 5:39 ` [PATCH v2 2/2] btrfs: zoned: add ASSERT and WARN for EXTENT_BUFFER_ZONED_ZEROOUT handling Naohiro Aota
@ 2024-03-26 16:50 ` Johannes Thumshirn
2024-03-28 19:49 ` David Sterba
2 siblings, 1 reply; 6+ messages in thread
From: Johannes Thumshirn @ 2024-03-26 16:50 UTC (permalink / raw)
To: Naohiro Aota, linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] btrfs: zoned: fix EXTENT_BUFFER_ZONED_ZEROOUT handling
2024-03-26 16:50 ` [PATCH v2 0/2] btrfs: zoned: fix " Johannes Thumshirn
@ 2024-03-28 19:49 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2024-03-28 19:49 UTC (permalink / raw)
To: Johannes Thumshirn; +Cc: Naohiro Aota, linux-btrfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread