在 2024/3/16 11:44, Tavian Barnes 写道:
> 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]>
Reviewed-by: Qu Wenruo <[email protected]>
Thanks,
Qu
> ---
> 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);