Re: [PATCH 8/8] btrfs: add support for multiple global roots

[Josef Bacik <josef@toxicpanda.com>]

On Sat, Nov 06, 2021 at 09:51:20AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/11/6 09:18, Qu Wenruo wrote:
> > 
> > 
> > On 2021/11/6 04:49, Josef Bacik wrote:
> > > With extent tree v2 you will be able to create multiple csum, extent,
> > > and free space trees.  They will be used based on the block group, which
> > > will now use the block_group_item->chunk_objectid to point to the set of
> > > global roots that it will use.  When allocating new block groups we'll
> > > simply mod the gigabyte offset of the block group against the number of
> > > global roots we have and that will be the block groups global id.
> > > 
> > >  From there we can take the bytenr that we're modifying in the respective
> > > tree, look up the block group and get that block groups corresponding
> > > global root id.  From there we can get to the appropriate global root
> > > for that bytenr.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > ---
> > >   fs/btrfs/block-group.c     | 11 +++++++--
> > >   fs/btrfs/block-group.h     |  1 +
> > >   fs/btrfs/ctree.h           |  2 ++
> > >   fs/btrfs/disk-io.c         | 49 +++++++++++++++++++++++++++++++-------
> > >   fs/btrfs/free-space-tree.c |  2 ++
> > >   fs/btrfs/transaction.c     | 15 ++++++++++++
> > >   fs/btrfs/tree-checker.c    | 21 ++++++++++++++--
> > >   7 files changed, 88 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > > index 7eb0a8632a01..85516f2fd5da 100644
> > > --- a/fs/btrfs/block-group.c
> > > +++ b/fs/btrfs/block-group.c
> > > @@ -2002,6 +2002,7 @@ static int read_one_block_group(struct
> > > btrfs_fs_info *info,
> > >       cache->length = key->offset;
> > >       cache->used = btrfs_stack_block_group_used(bgi);
> > >       cache->flags = btrfs_stack_block_group_flags(bgi);
> > > +    cache->global_root_id = btrfs_stack_block_group_chunk_objectid(bgi);
> > > 
> > >       set_free_space_tree_thresholds(cache);
> > > 
> > > @@ -2284,7 +2285,7 @@ static int insert_block_group_item(struct
> > > btrfs_trans_handle *trans,
> > >       spin_lock(&block_group->lock);
> > >       btrfs_set_stack_block_group_used(&bgi, block_group->used);
> > >       btrfs_set_stack_block_group_chunk_objectid(&bgi,
> > > -                BTRFS_FIRST_CHUNK_TREE_OBJECTID);
> > > +                           block_group->global_root_id);
> > >       btrfs_set_stack_block_group_flags(&bgi, block_group->flags);
> > >       key.objectid = block_group->start;
> > >       key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> > > @@ -2460,6 +2461,12 @@ struct btrfs_block_group
> > > *btrfs_make_block_group(struct btrfs_trans_handle *tran
> > >       cache->flags = type;
> > >       cache->last_byte_to_unpin = (u64)-1;
> > >       cache->cached = BTRFS_CACHE_FINISHED;
> > > +    cache->global_root_id = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
> > > +
> > > +    if (btrfs_fs_incompat(fs_info, EXTENT_TREE_V2))
> > > +        cache->global_root_id = div64_u64(cache->start, SZ_1G) %
> > > +            fs_info->nr_global_roots;
> > > +
> > 
> > Any special reason for this complex global_root_id calculation?
> > 
> > My initial assumption for global trees is pretty simple, just something
> > like (CSUM_TREE, ROOT_ITEM, bg bytenr) or (EXTENT_TREE, ROOT_ITEM, bg
> > bytenr) as their root key items.
> > 
> > But this is definitely not the case here.
> > 
> > Thus I'm wondering why we're not using something more simple.
> 

Because we don't have a global tree per-block group.  We have N global roots
that have to be round robined through the block groups.  We could do something
smarter in the future, but for now just round robin'ing them is easy to wrap
your head around, and is a decent default.

> And I also forgot that, for smaller fs, we could have metadata only sized
> several megabytes.
> 
> In that case, several metadata block groups would share the same
> global_root_id, which is definitely not a good idea.
> 

Sure, we can adjust this logic for smaller file systems, I'll change it to scale
down for smaller fs'es.  Thanks,

Josef