* [PATCH 0/2] extent_buffer read cleanups
@ 2024-03-18 13:56 Tavian Barnes
2024-03-18 13:56 ` [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING Tavian Barnes
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Tavian Barnes @ 2024-03-18 13:56 UTC (permalink / raw)
To: linux-btrfs
Cc: Qu Wenruo, Tavian Barnes, Chris Mason, Josef Bacik, David Sterba,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING 2024-03-18 13:56 [PATCH 0/2] extent_buffer read cleanups Tavian Barnes @ 2024-03-18 13:56 ` Tavian Barnes 2024-03-18 20:00 ` Qu Wenruo 2024-03-18 13:56 ` [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading Tavian Barnes 2024-03-22 19:22 ` [PATCH 0/2] extent_buffer read cleanups David Sterba 2 siblings, 1 reply; 6+ messages in thread From: Tavian Barnes @ 2024-03-18 13:56 UTC (permalink / raw) To: linux-btrfs Cc: Qu Wenruo, Tavian Barnes, Chris Mason, Josef Bacik, David Sterba, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING 2024-03-18 13:56 ` [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING Tavian Barnes @ 2024-03-18 20:00 ` Qu Wenruo 0 siblings, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2024-03-18 20:00 UTC (permalink / raw) To: Tavian Barnes, linux-btrfs Cc: Chris Mason, Josef Bacik, David Sterba, linux-kernel 在 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; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading 2024-03-18 13:56 [PATCH 0/2] extent_buffer read cleanups Tavian Barnes 2024-03-18 13:56 ` [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING Tavian Barnes @ 2024-03-18 13:56 ` Tavian Barnes 2024-03-18 20:02 ` Qu Wenruo 2024-03-22 19:22 ` [PATCH 0/2] extent_buffer read cleanups David Sterba 2 siblings, 1 reply; 6+ messages in thread From: Tavian Barnes @ 2024-03-18 13:56 UTC (permalink / raw) To: linux-btrfs Cc: Qu Wenruo, Tavian Barnes, Chris Mason, Josef Bacik, David Sterba, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading 2024-03-18 13:56 ` [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading Tavian Barnes @ 2024-03-18 20:02 ` Qu Wenruo 0 siblings, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2024-03-18 20:02 UTC (permalink / raw) To: Tavian Barnes, linux-btrfs Cc: Chris Mason, Josef Bacik, David Sterba, linux-kernel 在 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 && ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] extent_buffer read cleanups 2024-03-18 13:56 [PATCH 0/2] extent_buffer read cleanups Tavian Barnes 2024-03-18 13:56 ` [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING Tavian Barnes 2024-03-18 13:56 ` [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading Tavian Barnes @ 2024-03-22 19:22 ` David Sterba 2 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2024-03-22 19:22 UTC (permalink / raw) To: Tavian Barnes Cc: linux-btrfs, Qu Wenruo, Chris Mason, Josef Bacik, David Sterba, linux-kernel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-22 19:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-18 13:56 [PATCH 0/2] extent_buffer read cleanups Tavian Barnes 2024-03-18 13:56 ` [PATCH 1/2] btrfs: New helper to clear EXTENT_BUFFER_READING Tavian Barnes 2024-03-18 20:00 ` Qu Wenruo 2024-03-18 13:56 ` [PATCH 2/2] btrfs: WARN if EXTENT_BUFFER_UPTODATE is set while reading Tavian Barnes 2024-03-18 20:02 ` Qu Wenruo 2024-03-22 19:22 ` [PATCH 0/2] extent_buffer read cleanups David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.