To prevent concurrent reads for the same extent buffer,
read_extent_buffer_pages() performs these checks:
/* (1) */
if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
return 0;
/* (2) */
if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags))
goto done;
At this point, it seems safe to start the actual read operation. Once
that completes, end_bbio_meta_read() does
/* (3) */
set_extent_buffer_uptodate(eb);
/* (4) */
clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
Normally, this is enough to ensure only one read happens, and all other
callers wait for it to finish before returning. Unfortunately, there is
a racey interleaving:
Thread A | Thread B | Thread C
---------+----------+---------
(1) | |
| (1) |
(2) | |
(3) | |
(4) | |
| (2) |
| | (1)
When this happens, thread B kicks of an unnecessary read. Worse, thread
C will see UPTODATE set and return immediately, while the read from
thread B is still in progress. This race could result in tree-checker
errors like this as the extent buffer is concurrently modified:
BTRFS critical (device dm-0): corrupted node, root=256
block=8550954455682405139 owner mismatch, have 11858205567642294356
expect [256, 18446744073709551360]
Fix it by testing UPTODATE again after setting the READING bit, and if
it's been set, skip the unnecessary read.
Fixes: d7172f52e993 ("btrfs: use per-buffer locking for extent_buffer reading")
Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H2+PS18p+Pzw@mail.gmail.com/
Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9.1706312914.git.wqu@suse.com/
Link: https://lore.kernel.org/linux-btrfs/[email protected]/
Signed-off-by: Tavian Barnes <[email protected]>
---
fs/btrfs/extent_io.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7441245b1ceb..61594eaf1f89 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4333,6 +4333,19 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags))
goto done;
+ /*
+ * Between the initial test_bit(EXTENT_BUFFER_UPTODATE) and the above
+ * test_and_set_bit(EXTENT_BUFFER_READING), someone else could have
+ * started and finished reading the same eb. In this case, UPTODATE
+ * will now be set, and we shouldn't read it in again.
+ */
+ if (unlikely(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))) {
+ clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
+ smp_mb__after_atomic();
+ wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING);
+ return 0;
+ }
+
clear_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags);
eb->read_mirror = 0;
check_buffer_tree_ref(eb);
--
2.44.0
On Fri, Mar 15, 2024 at 09:14:29PM -0400, Tavian Barnes wrote:
> To prevent concurrent reads for the same extent buffer,
> read_extent_buffer_pages() performs these checks:
>
> /* (1) */
> if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
> return 0;
>
> /* (2) */
> if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags))
> goto done;
>
> At this point, it seems safe to start the actual read operation. Once
> that completes, end_bbio_meta_read() does
>
> /* (3) */
> set_extent_buffer_uptodate(eb);
>
> /* (4) */
> clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
>
> Normally, this is enough to ensure only one read happens, and all other
> callers wait for it to finish before returning. Unfortunately, there is
> a racey interleaving:
>
> Thread A | Thread B | Thread C
> ---------+----------+---------
> (1) | |
> | (1) |
> (2) | |
> (3) | |
> (4) | |
> | (2) |
> | | (1)
>
> When this happens, thread B kicks of an unnecessary read. Worse, thread
> C will see UPTODATE set and return immediately, while the read from
> thread B is still in progress. This race could result in tree-checker
> errors like this as the extent buffer is concurrently modified:
>
> BTRFS critical (device dm-0): corrupted node, root=256
> block=8550954455682405139 owner mismatch, have 11858205567642294356
> expect [256, 18446744073709551360]
>
> Fix it by testing UPTODATE again after setting the READING bit, and if
> it's been set, skip the unnecessary read.
>
> Fixes: d7172f52e993 ("btrfs: use per-buffer locking for extent_buffer reading")
> Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H2+PS18p+Pzw@mail.gmail.com/
> Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9.1706312914.git.wqu@suse.com/
> Link: https://lore.kernel.org/linux-btrfs/[email protected]/
> Signed-off-by: Tavian Barnes <[email protected]>
Thank you very much for taking the time to debug the issue and for the
fix. It is a rare occurrence that a tough bug is followed by a fix from
the same person (outside of the developer group) and is certainly
appreciated.
On Fri, Mar 22, 2024 at 3:28 PM David Sterba <[email protected]> wrote:
> On Fri, Mar 15, 2024 at 09:14:29PM -0400, Tavian Barnes wrote:
> > To prevent concurrent reads for the same extent buffer,
> > read_extent_buffer_pages() performs these checks:
> >
> > /* (1) */
> > if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
> > return 0;
> >
> > /* (2) */
> > if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags))
> > goto done;
> >
> > At this point, it seems safe to start the actual read operation. Once
> > that completes, end_bbio_meta_read() does
> >
> > /* (3) */
> > set_extent_buffer_uptodate(eb);
> >
> > /* (4) */
> > clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
> >
> > Normally, this is enough to ensure only one read happens, and all other
> > callers wait for it to finish before returning. Unfortunately, there is
> > a racey interleaving:
> >
> > Thread A | Thread B | Thread C
> > ---------+----------+---------
> > (1) | |
> > | (1) |
> > (2) | |
> > (3) | |
> > (4) | |
> > | (2) |
> > | | (1)
> >
> > When this happens, thread B kicks of an unnecessary read. Worse, thread
> > C will see UPTODATE set and return immediately, while the read from
> > thread B is still in progress. This race could result in tree-checker
> > errors like this as the extent buffer is concurrently modified:
> >
> > BTRFS critical (device dm-0): corrupted node, root=256
> > block=8550954455682405139 owner mismatch, have 11858205567642294356
> > expect [256, 18446744073709551360]
> >
> > Fix it by testing UPTODATE again after setting the READING bit, and if
> > it's been set, skip the unnecessary read.
> >
> > Fixes: d7172f52e993 ("btrfs: use per-buffer locking for extent_buffer reading")
> > Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H2+PS18p+Pzw@mail.gmail.com/
> > Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9.1706312914.git.wqu@suse.com/
> > Link: https://lore.kernel.org/linux-btrfs/[email protected]/
> > Signed-off-by: Tavian Barnes <[email protected]>
>
> Thank you very much for taking the time to debug the issue and for the
> fix. It is a rare occurrence that a tough bug is followed by a fix from
> the same person (outside of the developer group) and is certainly
> appreciated.
Thank you!
Sorry to nitpick, but the paragraph you added to the commit message
[1] has a typo:
> There are reports from tree-checker that detects corrupted nodes,
> without any obvious pattern so possibly an overwrite in memory.
> After some debugging it turns out there's a race when reading an extent
> buffer the uptodate status is can be missed.
s/is can/can/
[1]: https://github.com/btrfs/linux/commit/402887e0e9ad76d72496aefebd37bd729748be79
--
Tavian Barnes
On Fri, Mar 22, 2024 at 06:50:07PM -0400, Tavian Barnes wrote:
> On Fri, Mar 22, 2024 at 3:28 PM David Sterba <[email protected]> wrote:
> > On Fri, Mar 15, 2024 at 09:14:29PM -0400, Tavian Barnes wrote:
> > > To prevent concurrent reads for the same extent buffer,
> > > read_extent_buffer_pages() performs these checks:
> > >
> > > /* (1) */
> > > if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
> > > return 0;
> > >
> > > /* (2) */
> > > if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags))
> > > goto done;
> > >
> > > At this point, it seems safe to start the actual read operation. Once
> > > that completes, end_bbio_meta_read() does
> > >
> > > /* (3) */
> > > set_extent_buffer_uptodate(eb);
> > >
> > > /* (4) */
> > > clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
> > >
> > > Normally, this is enough to ensure only one read happens, and all other
> > > callers wait for it to finish before returning. Unfortunately, there is
> > > a racey interleaving:
> > >
> > > Thread A | Thread B | Thread C
> > > ---------+----------+---------
> > > (1) | |
> > > | (1) |
> > > (2) | |
> > > (3) | |
> > > (4) | |
> > > | (2) |
> > > | | (1)
> > >
> > > When this happens, thread B kicks of an unnecessary read. Worse, thread
> > > C will see UPTODATE set and return immediately, while the read from
> > > thread B is still in progress. This race could result in tree-checker
> > > errors like this as the extent buffer is concurrently modified:
> > >
> > > BTRFS critical (device dm-0): corrupted node, root=256
> > > block=8550954455682405139 owner mismatch, have 11858205567642294356
> > > expect [256, 18446744073709551360]
> > >
> > > Fix it by testing UPTODATE again after setting the READING bit, and if
> > > it's been set, skip the unnecessary read.
> > >
> > > Fixes: d7172f52e993 ("btrfs: use per-buffer locking for extent_buffer reading")
> > > Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H2+PS18p+Pzw@mail.gmail.com/
> > > Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9.1706312914.git.wqu@suse.com/
> > > Link: https://lore.kernel.org/linux-btrfs/[email protected]/
> > > Signed-off-by: Tavian Barnes <[email protected]>
> >
> > Thank you very much for taking the time to debug the issue and for the
> > fix. It is a rare occurrence that a tough bug is followed by a fix from
> > the same person (outside of the developer group) and is certainly
> > appreciated.
>
> Thank you!
>
> Sorry to nitpick, but the paragraph you added to the commit message
> [1] has a typo:
>
> > There are reports from tree-checker that detects corrupted nodes,
> > without any obvious pattern so possibly an overwrite in memory.
> > After some debugging it turns out there's a race when reading an extent
> > buffer the uptodate status is can be missed.
>
> s/is can/can/
I will fix that, thanks.