| Message ID | 817581cbc85cfda4c2232fecbfdb6b615b7067ca.1741306938.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: block_group refcounting fixes | expand |
On Fri, Mar 7, 2025 at 12:31 AM Boris Burkov <boris@bur.io> wrote: > > All other users of the bg_list list_head inc the refcount when adding to > a list and dec it when deleting from the list. Just for the sake of > uniformity and to try to avoid refcounting bugs, do it for this list as > well. Please add a note that the reason why it's not ref counted is because the list of new block groups belongs to a transaction handle, which is local and therefore no other tasks can access it. > > Signed-off-by: Boris Burkov <boris@bur.io> > --- > fs/btrfs/block-group.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > index 2db1497b58d9..e4071897c9a8 100644 > --- a/fs/btrfs/block-group.c > +++ b/fs/btrfs/block-group.c > @@ -2801,6 +2801,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans) > spin_lock(&fs_info->unused_bgs_lock); > list_del_init(&block_group->bg_list); > clear_bit(BLOCK_GROUP_FLAG_NEW, &block_group->runtime_flags); > + btrfs_put_block_group(block_group); > spin_unlock(&fs_info->unused_bgs_lock); > > /* > @@ -2939,6 +2940,7 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran > } > #endif > > + btrfs_get_block_group(cache); > list_add_tail(&cache->bg_list, &trans->new_bgs); > btrfs_inc_delayed_refs_rsv_bg_inserts(fs_info); There's a missing btrfs_put_block_group() call at btrfs_cleanup_pending_block_groups(). Thanks. > > -- > 2.48.1 > >
On Fri, Mar 07, 2025 at 02:37:28PM +0000, Filipe Manana wrote: > On Fri, Mar 7, 2025 at 12:31 AM Boris Burkov <boris@bur.io> wrote: > > > > All other users of the bg_list list_head inc the refcount when adding to > > a list and dec it when deleting from the list. Just for the sake of > > uniformity and to try to avoid refcounting bugs, do it for this list as > > well. > > Please add a note that the reason why it's not ref counted is because > the list of new block groups belongs to a transaction handle, which is > local and therefore no other tasks can access it. > Just to make sure, I understand you correctly: you'd like me to add this as a historical note to the commit message? Happy to do so if that's what you mean. Otherwise, I'm confused about your intent. If you are saying that it's better to not refcount it, then we can drop this patch, it's not a fix, just another "try to establish invariants" kinda thing. > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > --- > > fs/btrfs/block-group.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > > index 2db1497b58d9..e4071897c9a8 100644 > > --- a/fs/btrfs/block-group.c > > +++ b/fs/btrfs/block-group.c > > @@ -2801,6 +2801,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans) > > spin_lock(&fs_info->unused_bgs_lock); > > list_del_init(&block_group->bg_list); > > clear_bit(BLOCK_GROUP_FLAG_NEW, &block_group->runtime_flags); > > + btrfs_put_block_group(block_group); > > spin_unlock(&fs_info->unused_bgs_lock); > > > > /* > > @@ -2939,6 +2940,7 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran > > } > > #endif > > > > + btrfs_get_block_group(cache); > > list_add_tail(&cache->bg_list, &trans->new_bgs); > > btrfs_inc_delayed_refs_rsv_bg_inserts(fs_info); > > There's a missing btrfs_put_block_group() call at > btrfs_cleanup_pending_block_groups(). Good catch, thanks. > > Thanks. > > > > > > -- > > 2.48.1 > > > >
On Fri, Mar 07, 2025 at 01:40:40PM -0800, Boris Burkov wrote: > On Fri, Mar 07, 2025 at 02:37:28PM +0000, Filipe Manana wrote: > > On Fri, Mar 7, 2025 at 12:31 AM Boris Burkov <boris@bur.io> wrote: > > > > > > All other users of the bg_list list_head inc the refcount when adding to > > > a list and dec it when deleting from the list. Just for the sake of > > > uniformity and to try to avoid refcounting bugs, do it for this list as > > > well. > > > > Please add a note that the reason why it's not ref counted is because > > the list of new block groups belongs to a transaction handle, which is > > local and therefore no other tasks can access it. > > > > Just to make sure, I understand you correctly: you'd like me to add this > as a historical note to the commit message? Happy to do so if that's what > you mean. > > Otherwise, I'm confused about your intent. If you are saying that it's > better to not refcount it, then we can drop this patch, it's not a fix, > just another "try to establish invariants" kinda thing. > > > > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > > --- > > > fs/btrfs/block-group.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > > > index 2db1497b58d9..e4071897c9a8 100644 > > > --- a/fs/btrfs/block-group.c > > > +++ b/fs/btrfs/block-group.c > > > @@ -2801,6 +2801,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans) > > > spin_lock(&fs_info->unused_bgs_lock); > > > list_del_init(&block_group->bg_list); > > > clear_bit(BLOCK_GROUP_FLAG_NEW, &block_group->runtime_flags); > > > + btrfs_put_block_group(block_group); > > > spin_unlock(&fs_info->unused_bgs_lock); > > > > > > /* > > > @@ -2939,6 +2940,7 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran > > > } > > > #endif > > > > > > + btrfs_get_block_group(cache); > > > list_add_tail(&cache->bg_list, &trans->new_bgs); > > > btrfs_inc_delayed_refs_rsv_bg_inserts(fs_info); > > > > There's a missing btrfs_put_block_group() call at > > btrfs_cleanup_pending_block_groups(). > > Good catch, thanks. > Actually, just for the record, the btrfs_put_block_group *is* there, it just was the mystery btrfs_put_block_group from the other patch that you also noticed. So that put should be in this patch! I think that will make both of them make more sense. Good eye. > > > > Thanks. > > > > > > > > > > -- > > > 2.48.1 > > > > > >
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index 2db1497b58d9..e4071897c9a8 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -2801,6 +2801,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans) spin_lock(&fs_info->unused_bgs_lock); list_del_init(&block_group->bg_list); clear_bit(BLOCK_GROUP_FLAG_NEW, &block_group->runtime_flags); + btrfs_put_block_group(block_group); spin_unlock(&fs_info->unused_bgs_lock); /* @@ -2939,6 +2940,7 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran } #endif + btrfs_get_block_group(cache); list_add_tail(&cache->bg_list, &trans->new_bgs); btrfs_inc_delayed_refs_rsv_bg_inserts(fs_info);
All other users of the bg_list list_head inc the refcount when adding to a list and dec it when deleting from the list. Just for the sake of uniformity and to try to avoid refcounting bugs, do it for this list as well. Signed-off-by: Boris Burkov <boris@bur.io> --- fs/btrfs/block-group.c | 2 ++ 1 file changed, 2 insertions(+)