[PATCH 0/2] extent_buffer read cleanups

[PATCH 0/2] extent_buffer read cleanups

[Tavian Barnes]

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/20240317203508.GA5975@lst.de/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

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

[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 <tavianator@tavianator.com>
---
 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

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

[Qu Wenruo]

在 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 <tavianator@tavianator.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

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;
>   	}
>   

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

[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 <tavianator@tavianator.com>
---
 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

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

[Qu Wenruo]

在 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 <tavianator@tavianator.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

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 &&

Re: [PATCH 0/2] extent_buffer read cleanups

[David Sterba]

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/20240317203508.GA5975@lst.de/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.