| Message ID | 1c28a05081455379be5d91ee760f9a03e4255e6a.1646690972.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: extent tree v2 support, global roots | expand |
On Mon, Mar 07, 2022 at 05:10:57PM -0500, Josef Bacik wrote: > In order to make sure the file system is consistent we need to record > the number of global roots we should have in the super block. We could > infer this from the number of global roots we find, however this could > lead to interesting fuzzing problems, so add a source of truth to the > super block in order to make it easier to verify the file system is > consistent. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > kernel-shared/ctree.h | 6 +++++- > kernel-shared/disk-io.c | 4 ++++ > mkfs/common.c | 1 + > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h > index b12dbff1..90de7a65 100644 > --- a/kernel-shared/ctree.h > +++ b/kernel-shared/ctree.h > @@ -463,13 +463,15 @@ struct btrfs_super_block { > > u8 metadata_uuid[BTRFS_FSID_SIZE]; > > + __le64 nr_global_roots; > + Shouldn't this be added after the last item? > __le64 block_group_root; > __le64 block_group_root_generation; > u8 block_group_root_level; > > /* future expansion */ > u8 reserved8[7]; > - __le64 reserved[25]; > + __le64 reserved[24]; > u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE]; > struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS]; > /* Padded to 4096 bytes */
On 08/03/2022 17:23, David Sterba wrote: >> u8 metadata_uuid[BTRFS_FSID_SIZE]; >> >> + __le64 nr_global_roots; >> + > > Shouldn't this be added after the last item? > >> __le64 block_group_root; >> __le64 block_group_root_generation; >> u8 block_group_root_level; >> >> /* future expansion */ >> u8 reserved8[7]; >> - __le64 reserved[25]; >> + __le64 reserved[24]; Or at least inside one of these reserved fields.
On Tue, Mar 08, 2022 at 04:41:44PM +0000, Johannes Thumshirn wrote: > On 08/03/2022 17:23, David Sterba wrote: > >> u8 metadata_uuid[BTRFS_FSID_SIZE]; > >> > >> + __le64 nr_global_roots; > >> + > > > > Shouldn't this be added after the last item? > > > >> __le64 block_group_root; > >> __le64 block_group_root_generation; > >> u8 block_group_root_level; > >> > >> /* future expansion */ > >> u8 reserved8[7]; > >> - __le64 reserved[25]; > >> + __le64 reserved[24]; > > Or at least inside one of these reserved fields. OTOH, it's still experimental so we don't expect backward compatibility yet so it should be ok to change for now.
On Wed, Mar 09, 2022 at 06:05:53PM +0100, David Sterba wrote: > On Tue, Mar 08, 2022 at 04:41:44PM +0000, Johannes Thumshirn wrote: > > On 08/03/2022 17:23, David Sterba wrote: > > >> u8 metadata_uuid[BTRFS_FSID_SIZE]; > > >> > > >> + __le64 nr_global_roots; > > >> + > > > > > > Shouldn't this be added after the last item? > > > > > >> __le64 block_group_root; > > >> __le64 block_group_root_generation; > > >> u8 block_group_root_level; > > >> > > >> /* future expansion */ > > >> u8 reserved8[7]; > > >> - __le64 reserved[25]; > > >> + __le64 reserved[24]; > > > > Or at least inside one of these reserved fields. > > OTOH, it's still experimental so we don't expect backward compatibility > yet so it should be ok to change for now. I did it this way because it's all still experimental and it makes more sense for it to be before the new root stuff. Thanks, Josef
On 2022/3/10 05:22, Josef Bacik wrote: > On Wed, Mar 09, 2022 at 06:05:53PM +0100, David Sterba wrote: >> On Tue, Mar 08, 2022 at 04:41:44PM +0000, Johannes Thumshirn wrote: >>> On 08/03/2022 17:23, David Sterba wrote: >>>>> u8 metadata_uuid[BTRFS_FSID_SIZE]; >>>>> >>>>> + __le64 nr_global_roots; >>>>> + >>>> >>>> Shouldn't this be added after the last item? >>>> >>>>> __le64 block_group_root; >>>>> __le64 block_group_root_generation; >>>>> u8 block_group_root_level; >>>>> >>>>> /* future expansion */ >>>>> u8 reserved8[7]; >>>>> - __le64 reserved[25]; >>>>> + __le64 reserved[24]; >>> >>> Or at least inside one of these reserved fields. >> >> OTOH, it's still experimental so we don't expect backward compatibility >> yet so it should be ok to change for now. > > I did it this way because it's all still experimental and it makes more sense > for it to be before the new root stuff. Thanks, I'd say, please don't. It's making anyone who want to add a new member in super block miserable. Everyone is going to add the new member from the reserved members, but such insert into the existing members are destructive. Furthermore, if the new member is going to be merged way before extent tree v2 part, how do we solve the conflicts? (The new member I want to introduce is just to indicate how many bytes we have reserved at the beginning of each device, with a new RO compat flag). Thanks, Qu > > Josef
On 2022/6/14 20:15, Qu Wenruo wrote: > > > On 2022/3/10 05:22, Josef Bacik wrote: >> On Wed, Mar 09, 2022 at 06:05:53PM +0100, David Sterba wrote: >>> On Tue, Mar 08, 2022 at 04:41:44PM +0000, Johannes Thumshirn wrote: >>>> On 08/03/2022 17:23, David Sterba wrote: >>>>>> u8 metadata_uuid[BTRFS_FSID_SIZE]; >>>>>> >>>>>> + __le64 nr_global_roots; >>>>>> + >>>>> >>>>> Shouldn't this be added after the last item? >>>>> >>>>>> __le64 block_group_root; >>>>>> __le64 block_group_root_generation; >>>>>> u8 block_group_root_level; >>>>>> >>>>>> /* future expansion */ >>>>>> u8 reserved8[7]; >>>>>> - __le64 reserved[25]; >>>>>> + __le64 reserved[24]; >>>> >>>> Or at least inside one of these reserved fields. >>> >>> OTOH, it's still experimental so we don't expect backward compatibility >>> yet so it should be ok to change for now. >> >> I did it this way because it's all still experimental and it makes >> more sense >> for it to be before the new root stuff. Thanks, > > I'd say, please don't. > > It's making anyone who want to add a new member in super block miserable. > > Everyone is going to add the new member from the reserved members, but > such insert into the existing members are destructive. > > Furthermore, if the new member is going to be merged way before extent > tree v2 part, how do we solve the conflicts? > > (The new member I want to introduce is just to indicate how many bytes > we have reserved at the beginning of each device, with a new RO compat > flag). My bad, the main problem is not shuffling the members of extent tree v2, but out-of-sync between kernel and btrfs-progs for super block. Anyway, I'd use the padding[] for my new members, to avoid possible out-of-sync problems. Thanks, Qu > > Thanks, > Qu >> >> Josef
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h index b12dbff1..90de7a65 100644 --- a/kernel-shared/ctree.h +++ b/kernel-shared/ctree.h @@ -463,13 +463,15 @@ struct btrfs_super_block { u8 metadata_uuid[BTRFS_FSID_SIZE]; + __le64 nr_global_roots; + __le64 block_group_root; __le64 block_group_root_generation; u8 block_group_root_level; /* future expansion */ u8 reserved8[7]; - __le64 reserved[25]; + __le64 reserved[24]; u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE]; struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS]; /* Padded to 4096 bytes */ @@ -2372,6 +2374,8 @@ BTRFS_SETGET_STACK_FUNCS(super_block_group_root_generation, block_group_root_generation, 64); BTRFS_SETGET_STACK_FUNCS(super_block_group_root_level, struct btrfs_super_block, block_group_root_level, 8); +BTRFS_SETGET_STACK_FUNCS(super_nr_global_roots, struct btrfs_super_block, + nr_global_roots, 64); static inline unsigned long btrfs_leaf_data(struct extent_buffer *l) { diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c index 3d1157ad..fcef6e97 100644 --- a/kernel-shared/disk-io.c +++ b/kernel-shared/disk-io.c @@ -1620,6 +1620,10 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_flags *oc if (ret) goto out_devices; + if (btrfs_fs_incompat(fs_info, EXTENT_TREE_V2)) + fs_info->nr_global_roots = + btrfs_super_nr_global_roots(fs_info->super_copy); + /* * fs_info->zone_size (and zoned) are not known before reading the * chunk tree, so it's 0 at this point. But, fs_info->zoned == 0 diff --git a/mkfs/common.c b/mkfs/common.c index aa65543b..eac8c46c 100644 --- a/mkfs/common.c +++ b/mkfs/common.c @@ -355,6 +355,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg) btrfs_set_super_cache_generation(&super, 0); } if (extent_tree_v2) { + btrfs_set_super_nr_global_roots(&super, 1); btrfs_set_super_block_group_root(&super, cfg->blocks[MKFS_BLOCK_GROUP_TREE]); btrfs_set_super_block_group_root_generation(&super, 1);
In order to make sure the file system is consistent we need to record the number of global roots we should have in the super block. We could infer this from the number of global roots we find, however this could lead to interesting fuzzing problems, so add a source of truth to the super block in order to make it easier to verify the file system is consistent. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- kernel-shared/ctree.h | 6 +++++- kernel-shared/disk-io.c | 4 ++++ mkfs/common.c | 1 + 3 files changed, 10 insertions(+), 1 deletion(-)