2024-03-18 13:58:31

by Tavian Barnes

Subject: [PATCH 0/2] extent_buffer read cleanups

This small series refactors some duplicated code introduced by a recent
bugfix (which was intentionally duplicated to make stable backports
easier), and adds a WARN_ON() to make it easier to debug similar issues
in the future.

Link: https://lore.kernel.org/linux-btrfs/[email protected]/T/

Tavian Barnes (2):
btrfs: New helper to clear EXTENT_BUFFER_READING
btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading

fs/btrfs/extent_io.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)

--
2.44.0

2024-03-18 13:59:21

by Tavian Barnes

Subject: [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading

We recently tracked down a race condition that triggered a read for an
extent buffer with EXTENT_BUFFER_UPTODATE already set. While this read
was in progress, other concurrent readers would see the UPTODATE bit and
return early as if the read was already complete, making accesses to the
extent buffer conflict with the read operation that was overwriting it.

Add a WARN_ON() to end_bbio_meta_read() for this situation to make
similar races easier to spot in the future.

Signed-off-by: Tavian Barnes <[email protected]>
---
fs/btrfs/extent_io.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 46173dcfde4f..0024ea20bfc4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4285,6 +4285,13 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio)
struct folio_iter fi;
u32 bio_offset = 0;

+ /*
+ * If the extent buffer is marked UPTODATE before the read operation
+ * completes, other calls to read_extent_buffer_pages() will return
+ * early without waiting for the read to finish, causing data races.
+ */
+ WARN_ON(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags));
+
eb->read_mirror = bbio->mirror_num;

if (uptodate &&
--
2.44.0

2024-03-18 13:59:32

by Tavian Barnes

Subject: [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING

We are clearing the bit and waking up any waiters in two different
places. Factor that code out into a static helper function.

Signed-off-by: Tavian Barnes <[email protected]>
---
fs/btrfs/extent_io.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 61594eaf1f89..46173dcfde4f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4270,6 +4270,13 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
}
}

+static void clear_extent_buffer_reading(struct extent_buffer *eb)
+{
+ clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
+ smp_mb__after_atomic();
+ wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING);
+}
+
static void end_bbio_meta_read(struct btrfs_bio *bbio)
{
struct extent_buffer *eb = bbio->private;
@@ -4304,9 +4311,7 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio)
bio_offset += len;
}

- clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
- smp_mb__after_atomic();
- wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING);
+ clear_extent_buffer_reading(eb);
free_extent_buffer(eb);

bio_put(&bbio->bio);
@@ -4340,9 +4345,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
* 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);
+ clear_extent_buffer_reading(eb);
return 0;
}

--
2.44.0

2024-03-18 20:01:06

by Qu Wenruo

Subject: Re: [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING

在 2024/3/19 00:26, Tavian Barnes 写道:
> We are clearing the bit and waking up any waiters in two different
> places. Factor that code out into a static helper function.
>
> Signed-off-by: Tavian Barnes <[email protected]>

Reviewed-by: Qu Wenruo <[email protected]>

Thanks,
Qu
> ---
> fs/btrfs/extent_io.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 61594eaf1f89..46173dcfde4f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4270,6 +4270,13 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
> }
> }
>
> +static void clear_extent_buffer_reading(struct extent_buffer *eb)
> +{
> + clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
> + smp_mb__after_atomic();
> + wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING);
> +}
> +
> static void end_bbio_meta_read(struct btrfs_bio *bbio)
> {
> struct extent_buffer *eb = bbio->private;
> @@ -4304,9 +4311,7 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio)
> bio_offset += len;
> }
>
> - clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
> - smp_mb__after_atomic();
> - wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING);
> + clear_extent_buffer_reading(eb);
> free_extent_buffer(eb);
>
> bio_put(&bbio->bio);
> @@ -4340,9 +4345,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num,
> * 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);
> + clear_extent_buffer_reading(eb);
> return 0;
> }
>

2024-03-18 20:02:54

by Qu Wenruo

Subject: Re: [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading

在 2024/3/19 00:26, Tavian Barnes 写道:
> We recently tracked down a race condition that triggered a read for an
> extent buffer with EXTENT_BUFFER_UPTODATE already set. While this read
> was in progress, other concurrent readers would see the UPTODATE bit and
> return early as if the read was already complete, making accesses to the
> extent buffer conflict with the read operation that was overwriting it.
>
> Add a WARN_ON() to end_bbio_meta_read() for this situation to make
> similar races easier to spot in the future.
>
> Signed-off-by: Tavian Barnes <[email protected]>

Reviewed-by: Qu Wenruo <[email protected]>

Thanks,
Qu
> ---
> fs/btrfs/extent_io.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 46173dcfde4f..0024ea20bfc4 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4285,6 +4285,13 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio)
> struct folio_iter fi;
> u32 bio_offset = 0;
>
> + /*
> + * If the extent buffer is marked UPTODATE before the read operation
> + * completes, other calls to read_extent_buffer_pages() will return
> + * early without waiting for the read to finish, causing data races.
> + */
> + WARN_ON(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags));
> +
> eb->read_mirror = bbio->mirror_num;
>
> if (uptodate &&

2024-03-22 19:29:37

by David Sterba

Subject: Re: [PATCH 0/2] extent_buffer read cleanups

On Mon, Mar 18, 2024 at 09:56:52AM -0400, Tavian Barnes wrote:
> This small series refactors some duplicated code introduced by a recent
> bugfix (which was intentionally duplicated to make stable backports
> easier), and adds a WARN_ON() to make it easier to debug similar issues
> in the future.
>
> Link: https://lore.kernel.org/linux-btrfs/[email protected]/T/
>
> Tavian Barnes (2):
> btrfs: New helper to clear EXTENT_BUFFER_READING
> btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading

Thanks, patches added to for-next.