| Message ID | 20250318095440.436685-1-neelx@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: remove EXTENT_BUFFER_IN_TREE flag | expand |
On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > This flag is set after inserting the eb to the buffer tree and cleared on > it's removal. But it does not bring any added value. Just kill it for good. Would be good to add the reference to commit that added the bit, 34b41acec1ccc0 ("Btrfs: use a bit to track if we're in the radix tree") and wanted to make use of it, faa2dbf004e89e ("Btrfs: add sanity tests for new qgroup accounting code"). And both are 10+ years old. > Signed-off-by: Daniel Vacek <neelx@suse.com> Reviewed-by: David Sterba <dsterba@suse.com>
On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > This flag is set after inserting the eb to the buffer tree and cleared on > it's removal. But it does not bring any added value. Just kill it for good. I na similar way the flag EXTENT_BUFFER_READ_ERR is unused (was removed in eb/io rework in 046b562b20a5cf ("btrfs: use a separate end_io handler for read_extent_buffer").
On Tue, Mar 18, 2025 at 05:00:17PM +0100, David Sterba wrote: > On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > > This flag is set after inserting the eb to the buffer tree and cleared on > > it's removal. But it does not bring any added value. Just kill it for good. > > I na similar way the flag EXTENT_BUFFER_READ_ERR is unused (was removed > in eb/io rework in 046b562b20a5cf ("btrfs: use a separate end_io handler > for read_extent_buffer"). And EXTENT_BUFFER_READAHEAD, removed by f26c9238602856 ("btrfs: remove reada infrastructure").
On Tue, 18 Mar 2025 at 16:45, David Sterba <dsterba@suse.cz> wrote: > > On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > > This flag is set after inserting the eb to the buffer tree and cleared on > > it's removal. But it does not bring any added value. Just kill it for good. > > Would be good to add the reference to commit that added the bit, > 34b41acec1ccc0 ("Btrfs: use a bit to track if we're in the radix tree") > and wanted to make use of it, faa2dbf004e89e ("Btrfs: add sanity tests > for new qgroup accounting code"). And both are 10+ years old. Right, I could have checked the history. Though honestly from the diff of these two commits I don't see any valid usage of this flag either. Must have been somewhere in the context or I'm missing something. > > Signed-off-by: Daniel Vacek <neelx@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com>
On Tue, 18 Mar 2025 at 17:04, David Sterba <dsterba@suse.cz> wrote: > > On Tue, Mar 18, 2025 at 05:00:17PM +0100, David Sterba wrote: > > On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > > > This flag is set after inserting the eb to the buffer tree and cleared on > > > it's removal. But it does not bring any added value. Just kill it for good. > > > > I na similar way the flag EXTENT_BUFFER_READ_ERR is unused (was removed > > in eb/io rework in 046b562b20a5cf ("btrfs: use a separate end_io handler > > for read_extent_buffer"). > > And EXTENT_BUFFER_READAHEAD, removed by f26c9238602856 ("btrfs: remove > reada infrastructure"). I see. I can have a look into further cleanup later. I was not checking any other flags yet as that's unrelated to my issue. The full story is I'm chasing an eb refcount issue on downstream v6.4 running on aarch64 with 64k pages. First I suspected it's due to this flag racing (only setting the flag after inserting a newly allocated eb into the tree but without holding the eb->refs_lock) but after a bit of hammering I managed to reproduce the soft lockup even with this patch applied. That will need to be solved downstream. Still this looks like a worthy cleanup.
On Tue, Mar 18, 2025 at 06:07:20PM +0100, Daniel Vacek wrote: > On Tue, 18 Mar 2025 at 16:45, David Sterba <dsterba@suse.cz> wrote: > > > > On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > > > This flag is set after inserting the eb to the buffer tree and cleared on > > > it's removal. But it does not bring any added value. Just kill it for good. > > > > Would be good to add the reference to commit that added the bit, > > 34b41acec1ccc0 ("Btrfs: use a bit to track if we're in the radix tree") > > and wanted to make use of it, faa2dbf004e89e ("Btrfs: add sanity tests > > for new qgroup accounting code"). And both are 10+ years old. > > Right, I could have checked the history. > > Though honestly from the diff of these two commits I don't see any > valid usage of this flag either. Must have been somewhere in the > context or I'm missing something. Yeah, from the diff it can be seen if the code has any effects. It's recommended to analyze the code also from the historical context because it can be a leftover from a cleanup (and most of the time it is), but there's still a chance the initial intentions of the code are still valid and it was the cleanup that broke it.
On Tue, Mar 18, 2025 at 10:54:38AM +0100, Daniel Vacek wrote: > This flag is set after inserting the eb to the buffer tree and cleared on > it's removal. But it does not bring any added value. Just kill it for good. > > Signed-off-by: Daniel Vacek <neelx@suse.com> Added to for-next with the updated changelog, thanks.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index b168dc354f20b..864bdffaa1123 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3048,7 +3048,6 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, goto again; } check_buffer_tree_ref(eb); - set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags); return eb; free_eb: @@ -3368,7 +3367,6 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, } /* add one reference for the tree */ check_buffer_tree_ref(eb); - set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags); /* * Now it's safe to unlock the pages because any calls to @@ -3432,18 +3430,14 @@ static int release_extent_buffer(struct extent_buffer *eb) WARN_ON(atomic_read(&eb->refs) == 0); if (atomic_dec_and_test(&eb->refs)) { - if (test_and_clear_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags)) { - struct btrfs_fs_info *fs_info = eb->fs_info; + struct btrfs_fs_info *fs_info = eb->fs_info; - spin_unlock(&eb->refs_lock); + spin_unlock(&eb->refs_lock); - spin_lock(&fs_info->buffer_lock); - radix_tree_delete(&fs_info->buffer_radix, - eb->start >> fs_info->sectorsize_bits); - spin_unlock(&fs_info->buffer_lock); - } else { - spin_unlock(&eb->refs_lock); - } + spin_lock(&fs_info->buffer_lock); + radix_tree_delete_item(&fs_info->buffer_radix, + eb->start >> fs_info->sectorsize_bits, eb); + spin_unlock(&fs_info->buffer_lock); btrfs_leak_debug_del_eb(eb); /* Should be safe to release folios at this point. */ diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index 2e261892c7bc7..9cfb1e8f9b42d 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -47,7 +47,6 @@ enum { /* read IO error */ EXTENT_BUFFER_READ_ERR, EXTENT_BUFFER_UNMAPPED, - EXTENT_BUFFER_IN_TREE, /* write IO error */ EXTENT_BUFFER_WRITE_ERR, /* Indicate the extent buffer is written zeroed out (for zoned) */
This flag is set after inserting the eb to the buffer tree and cleared on it's removal. But it does not bring any added value. Just kill it for good. Signed-off-by: Daniel Vacek <neelx@suse.com> --- fs/btrfs/extent_io.c | 18 ++++++------------ fs/btrfs/extent_io.h | 1 - 2 files changed, 6 insertions(+), 13 deletions(-)